Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ElasticSearch::Model does not register a subclass #1072

Open
3 tasks
martinstreicher opened this issue Mar 4, 2024 · 3 comments
Open
3 tasks

ElasticSearch::Model does not register a subclass #1072

martinstreicher opened this issue Mar 4, 2024 · 3 comments

Comments

@martinstreicher
Copy link
Contributor

martinstreicher commented Mar 4, 2024

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

@martinstreicher
Copy link
Contributor Author

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

@martinstreicher
Copy link
Contributor Author

martinstreicher commented Mar 4, 2024

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
Copy link
Contributor Author

martinstreicher commented Mar 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant