Support inherited index names and doc types

[rymohr]

This PR adds support for inheriting index_name and document_type on an opt-in basis:

# within an initializer
Elasticsearch::Model.inheritance_enabled = true

class Animal < ActiveRecord::Base
  document_type 'mammal'
  index_name 'mammals'
end

class Dog < Animal
end

Animal.document_type   # 'mammal'
Animal.index_name      # 'mammals'
Dog.document_type      # 'mammal'
Dog.index_name         # 'mammals'

We wrap all our models exposed through our api in an Api:: wrapper that helps us handle versioning issues. Without support for inherited values it's easy to forget to add the explicit values to the subclasses, or update them if the base class ever changed.

Related to #28 and #92. Not sure it's enough for #170.

[rymohr]

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)

[rymohr]

CLA should be signed now.

[karmi]

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!

[gsmetal]

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?).

[rymohr]

@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

[gsmetal]

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.

[rymohr]

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?

[karmi]

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

[Tonkpils]

@karmi @rymohr any progress on this?

[rymohr]

@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

[Tonkpils]

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.

[rymohr]

@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

[nickmerwin]

👍

[kgeoir]

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

[vsizov]

@karmi

Please do ping me if I slip again!

Ping

[tangopium]

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

[phillipoertel]

+1

[karmi]

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! :)

[kayakyakr]

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.

[kayakyakr]

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.

[zw963]

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

[zw963]

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}

[al]

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?