elastic / elasticsearch-rails

Elasticsearch integrations for ActiveModel/Record and Ruby on Rails
Apache License 2.0
3.07k stars 793 forks source link

ElasticSearch::Model does not register a subclass #1072

Open martinstreicher opened 6 months ago

martinstreicher commented 6 months ago

A subclass of a class that includes Elasticsearch::Model is not registered. This yields a crash when searching on the subclass.

Specifically, this method in lib/elasticsearch/model/adapters/multiple.rb returns nil because detect cannot find the subclass in the registry.

          # Returns the class of the model corresponding to a specific `hit` in Elasticsearch results
          #
          # @see Elasticsearch::Model::Registry
          #
          # @api private
          #
          def __type_for_hit(hit)
            @@__types ||= {}

            key = "#{hit[:_index]}::#{hit[:_type]}" if hit[:_type] && hit[:_type] != '_doc'
            key = hit[:_index] unless key

            @@__types[key] ||= begin
              Registry.all.detect do |model|
                (model.index_name == hit[:_index] && __no_type?(hit)) ||
                    (model.index_name == hit[:_index] && model.document_type == hit[:_type])
              end
            end
          end

This is not working code, but it is illustrative of the problem.

# test.rb
module Searchable
  extend ActiveSupport::Concern

  included do
    include Elasticsearch::Model
  end
end

class Example
  include Searchable
end

class ExampleSubclass < Example
  include Searchable
end

puts Elasticsearch::Model::Registry.all
$ rails runner test.rb
Example

This is the code in lib/elasticsearch/model.rb that fails to add a class to the registry when it is a subclass...

   def self.included(base)
      base.class_eval do
        include Elasticsearch::Model::Proxy

        # Delegate common methods to the `__elasticsearch__` ClassMethodsProxy, unless they are defined already
        class << self
          METHODS.each do |method|
            delegate method, to: :__elasticsearch__ unless self.public_instance_methods.include?(method)
          end
        end
      end

      # Add to the model to the registry if it's a class (and not in intermediate module)
      Registry.add(base) if base.is_a?(Class)
    end

The line Registry.add(base) if base.is_a?(Class) should likely be copied to a def inherited(subclass) method like so...

def inherited(subclass)
  Registry.add(subclass) if subclass.is_a?(Class)
end 

... to account for the subclassing. As is, only the first class to include Elasticsearch::Model is instrumented.

### Tasks
- [ ] Create a failing test case to demonstrate the issue.  The subclass fails to appear in the registry.
- [ ] Repair the bug
- [ ] Have the test case pass.
martinstreicher commented 6 months ago

I will note that if you explicitly include Elasticsearch::Model in both classes, both classes are added to the registry.

martinstreicher commented 6 months ago

I do see this in the README:

Versions < 7.0.0 of this gem supported inheritance-- more specifically, Single Table Inheritance. With this feature, elasticsearch settings (index mappings, etc) on a parent model could be inherited by a child model leading to different model documents being indexed into the same Elasticsearch index. This feature depended on the ability to set a type for a document in Elasticsearch. The Elasticsearch team has deprecated support for types, as is described here. This gem will also remove support for types and Single Table Inheritance in version 7.0 as it enables an anti-pattern. Please save different model documents in separate indices. If you want to use STI, you can include an artificial type field manually in each document and use it in other operations.

Does this apply to subclasses outside of STI? I suppose it might.

martinstreicher commented 6 months ago

A proposed PR is here: https://github.com/elastic/elasticsearch-rails/pull/1073/files -- if this is a valid issue and repair, I will add tests to the PR. I signed the contributor's agreement.