index_name STI inheritance problem

[felixbuenemann]

The behavior of Class.index_name is not as expected when using concerns and STI:

module Searchable
  extend ActiveSupport::Concern
  included do
    include Elasticsearch::Model
    index_name "app_name-#{Rails.env}-#{table_name}"
  end
end

class Post < ActiveRecord::Base
  include Searchable
end

class Post::News < Post; end

puts Post.index_name
=> "appname-development-posts" [OK]
puts Post::News.index_name
=> "post-news" [Expected: "appname-development-posts"]

[karmi]

Yeah, I guess there are problems with STI-based models, there is one or two issues already opened for that.

I'm personally not a big fan of supporting it, I'm afraid -- we would need to do some acrobatics to inspect ancestors, etc.

I understand that setting it up in generic way as you do is convenient, but what about simply configuring index_name and document_type in each model? Either directly or in the included hook (of course, then you would have to not only subclass but also include, and it might not work as I imagine)?

[felixbuenemann]

Hmm, setting it on each model seems error prone to me. I also tried to simply override the method in the included block:

def self.index_name; … ; end

This worked fine in the console when calling MyClass.index_name, but on indexing it was ignored.

[karmi]

Yeah, I do agree it's convoluted, as I said. The proper solution to this would really be to traverse ancestors, see whether they have defined index_name, use it if they have and it's not overloaded in the descendant class. Nothing out of the ordinary, but also something I can't invest time into right now.

[felixbuenemann]

I'll try to come up with a PR for this. I'm working on lots of different stuff ATM, so please keep the issue open for some time.

[karmi]

Sure thing, we can leave it open, take your time!

[felixbuenemann]

Btw, wouldn't it make sense to use the base STI class to derive the index_name in the default implementation? Eg. self.base_class.model_name.collection.gsub(/\//, '-') instead of self.model_name.collection.gsub(/\//, '-'). Or would that be problematic regarding the mapping?

[karmi]

Is base_class available on all ActiveModel-compatible models? I don't think so. Note the implementation must work for any ActiveModel-like class (eg. Mongoid), as for ActiveRecord models, STI or no STI.

[felixbuenemann]

Good point, base_class is ActiveRecord specific.

[karmi]

Yeah, and it would be cool to extract the functionality into the ActiveRecord adapter, so it's properly isolated.

[felixbuenemann]

I'm currently experimenting on what is the most useful implementation. Eg. in my case I have an index per market and per locale, but it includes the different document types in the same index, so it's very different from the default approach.

[karmi]

Again, yeah, default features are nice, but world is very colourful :)

We can actually approach it differently, forget about STU, and basically consider any subclassing meaningful: if the subclass doesn't define index_name, it would check ancestors and so on.

I bet there will be people confused about that again, so it's hard to make a good decision in this case.

The STI case is simpler, since you assume that the index name will be the same, much as the table name for these models will be the same.

[lorgio]

Have you guys had any progress on this?

[felixbuenemann]

No, I had to switch over to another project. If things go as planned I'll be working on search in August.

[karmi]

@felixbuenemann Can you please coordinate any effort with @AaronRustad, who submitted #170.

I'm leaning towards closing this ticket for now, in favour of opening a new pull request / issue when we have some code, if that's OK?