My local build failed too when I tried to run the full test suite. Is master not passing?

lograge/log_subscriber.rb:50:in `<class:RequestLogSubscriber>': uninitialized constant ActionPack (NameError)

CLA should be signed now.

Hello Ryan, thanks for the patch and sorry for such an embarrasing delay! This has slipped me -- I'll find some time to review it in the coming days. Please do ping me if I slip again!

Don't know where to put my two words about STI problem.
My point is that import on parent model and index_document on object produce completely different results.
The Animal.import will add documents to animals index with type animal, but Animal.find_by_type('Dog').__elasticsearch__.index_document will add document to index dogs with type dog.

Your PR should help with this excluding situation when it's needed to keep type while importing. But in that situation it is possible to use different transform function (maybe default function should consider this in activerecord adapter?).

@gsmetal it looks like that's the result of the batch-based import process:
https://github.com/rymohr/elasticsearch-rails/blob/support-inheritance/elasticsearch-model/lib/elasticsearch/model/importing.rb#L122

One option is to eager load the project and then use Animal.descendants to process each subclass in its own set of batches. The project would need to be eager loaded for that to work though.

http://stackoverflow.com/a/19173688/1088743

I think something like this is better and works well
Animal.import transform: ->(m){ { index: { _id: m.id, _type: m.document_type, data: m.__elasticsearch__.as_indexed_json } } }. That's why I wrote about making such transform function default for STI models in activerecord adapter.

I haven't used transforms yet but yes, that does look like a much easier solution. And that will still take advantage of the existing batch import process?

Still haven't found time to properly review the approach and the code... Please ping me if I slip again, @rymohr!

@karmi @rymohr any progress on this?

@Tonkpils nope, I ended up using this as a placeholder for now:

module Search::InheritedIndexing
 extend ActiveSupport::Concern
 included do
 index_name superclass.index_name
 document_type superclass.document_type
 end
end
# ...
class Api::Project < Project
 include Search::InheritedIndexing
 # ...
end

Not sure if it makes more sense to walk the ancestors as I've done or just use base_class like @yuku-t did in #344

As far as base_class vs walking ancestors it depends on the use case. One case supports only ActiveRecord and would get the top parent class, the other case would be to only walk up to the ancestor that defines an index_name/document_type

Ideally, I'd like to avoid having to include a module on each child class as on big codebases there can be many instances.

@Tonkpils yeah, with my approach in the PR you would just opt into the inheritance. You wouldn't include anything in the subclasses. That's just a workaround for the existing behavior.

Elasticsearch::Model.inheritance_enabled = true

👍

Hello, is someone found a workaround for this topic or are you planning to fix this patch?

@karmi

Please do ping me if I slip again!

Ping

Is there any progress on that PR? Would be really useful!

+1

Hi, I've finally merged the patch. Many thanks for the contribution, @rymohr! I've added a Elasticsearch::Model.settings so the inheritance_enabled method is not just floating in the namespace -- it opens the module up for other settings like that. Again, thanks!

UPDATE: And sincere thanks to all bumping the issue with their comments. They do help! :)

I am having an issue using this is Mongoid. On line 80 of model/naming.rb, self is a Elasticsearch::Model::Proxy::ClassMethodsProxy which creates an endless loop.

Still working on figuring out what's up.

Fixed it by changing line model/naming.rb:80 to

next if klass == self || self.respond_to?(:target) && klass == self.target

I don't know enough about the codebase to know why that is being called as a proxy, nor how I could test for that.

this PR seem like problemable for me,

I use Rails 5.0.0, When I add following config to my project.

Gemfile 1576124

gem 'elasticsearch-model', git: 'git://github.com/elasticsearch/elasticsearch-rails.git'
gem 'elasticsearch-rails', git: 'git://github.com/elasticsearch/elasticsearch-rails.git'

models/goods_item.rb, this file is base class for others.

require 'elasticsearch/model'
class GoodsItem < ApplicationRecord
 include Elasticsearch::Model
 include Elasticsearch::Model::Callbacks
 index_name 'goods_items'
 document_type 'goods_items'
 ....
 GoodsItem.import

config/initilaizer/es.rb

Elasticsearch::Model.settings[:inheritance_enabled] = true

This will result in following error:

SystemStackError: stack level too deep

If comment following code:

# Elasticsearch::Model.settings[:inheritance_enabled] = true

SystemStackError: is gone, and return the expected index or document missings error.
because I use STI.

Api::V1::GoodsItemsControllerTest#test_#update:
Elasticsearch::Transport::Transport::Errors::NotFound: [404] {"error":{"root_cause":[{"type":"document_missing_exception","reason":"[free_goods_item][1331029462092192769]: document missing","shard":"0","index":"free_goods_items"}],"type":"document_missing_exception","reason":"[free_goods_item][1331029462092192769]: document missing","shard":"0","index":"free_goods_items"},"status":404}

I've the same issue as @kayakyakr (ActiveRecord though) and arrived at exactly the same fix (should have read the comments more carefully).

I'd submit a PR but I'm afraid I can't get the test suite to run 😬

@rymohr does @kayakyakr's proposal look reasonable?