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

Invalid single-table inheritance type: SubClass is not a subclass of Class #848

Closed
PhilCoggins opened this issue Oct 9, 2018 · 18 comments · May be fixed by #1064
Closed

Invalid single-table inheritance type: SubClass is not a subclass of Class #848

PhilCoggins opened this issue Oct 9, 2018 · 18 comments · May be fixed by #1064

Comments

@PhilCoggins
Copy link

I encountered some serious headaches while working with Elasticsearch-Model in development.

My problem is that the Invalid single-table inheritance type: X is not a subclass of Y exception was being thrown when I tried to instantiate records with the Multiple adapter after my code had been reloaded for the first time. I would have to restart my Rails server every time I made any changes to my code.

What I found was that Elasticsearch::Model::Registry.models would endlessly accumulate reloaded classes each time my code was being reloaded. If I had five classes that included Elasticsearch::Model, it would increase by 5 each time my code reloaded. I have monkey-patched the Registry class with:

  module Elasticsearch
    module Model
      class Registry
        def add(klass)
+         existing_model = @models.detect { |model| model.name == klass.name }
+         @models.delete(existing_model)
          @models << klass
        end
      end
    end
  end

in an initializer and only for development environment to fix this particular issue to ensure this Registry only contains the newly-reloaded classes.

After this change, I continued to encounter the same error. What I found was that Elasticsearch::Model::Adapter::Multiple::Records was caching the result of __type_for_hit in a class variable @@__types, and after my code was reloaded, the stale classes were being returned instead of the newly reloaded classes. I unset this class variable by hooking into Active Support's reloader:

Rails.application.reloader.before_class_unload do
  Elasticsearch::Model::Adapter::Multiple::Records.remove_class_variable(:@@__types) }
end

This is in the same environment block that the above monkey-patch is in to avoid affecting production environment. This allowed my records to be instantiated even after my code had been reloaded and fixed my issue.

This issue took a ton of time and effort (and frustration) to debug and fix, it would be nice to see the team provide better support for STI since this is a core feature in Rails. Hopefully this report will help you guys improve on some of these weak spots. I'm happy to provide any other information you might need.

@estolfo
Copy link
Contributor

estolfo commented Oct 9, 2018

Hi @PhilCoggins Thanks for raising this issue. Would you mind providing an example so we can reproduce it?

@PhilCoggins
Copy link
Author

Thanks @estolfo, here is a sample app with instructions:

https://github.com/PhilCoggins/elasticsearch-sti-fail

LMK if you have any issues running. I tried to do this in a single script file but ActiveSupport::Reloader.reload would not trigger the error for some reason.

@darkhelmet
Copy link

I'm also hitting this, will use the initializer patches for now.

@darkhelmet
Copy link

Looking at the code, I wonder if it makes sense to use the before_class_unload hook to empty the @models array as well.

@PhilCoggins
Copy link
Author

@darkhelmet You could, though that's already occurring in the Elasticsearch::Model::Registry#add monkey patch (we remove a model if there's already one there with the same constant name).

Also want to point out that the initializer is wrapped in an environment check since I only need this on dev, not sure how it would behave in prod.

@darkhelmet
Copy link

Yeah, this is what I ended up with in my initializer.

if Rails.env.development?
  Rails.application.reloader.before_class_unload do
    Elasticsearch::Model::Registry.remove_instance_variable(:@instance)
    Elasticsearch::Model::Adapter::Multiple::Records.remove_class_variable(:@@__types)
  end
end

@surjay
Copy link

surjay commented Mar 22, 2019

Is there any movement on a gem fix for this? I just ran into this and its a major issue. It only happens for me in Rails5, though which we are currently upgrading to. I've never encountered this in Rails4.

@estolfo
Copy link
Contributor

estolfo commented Mar 25, 2019

@surjay I will look into it and have an update soon!

@estolfo estolfo added the bug label Jul 12, 2019
@estolfo
Copy link
Contributor

estolfo commented Jul 16, 2019

@PhilCoggins I'm having trouble reproducing this using the example app you provided. I followed the steps in the README. Is there anything I'm missing below?

2.5.1 :001 > require_dependency 'animal'
 => true 
2.5.1 :002 > Elasticsearch::Model.search("Fluffy")
 => #<Elasticsearch::Model::Response::Response:0x00007f8018591588 @klass=#<Elasticsearch::Model::Multimodel:0x00007f80185932c0 @models=[Animal (call 'Animal.connection' to establish a connection)]>, @search=#<Elasticsearch::Model::Searching::SearchRequest:0x00007f80185930b8 @klass=#<Elasticsearch::Model::Multimodel:0x00007f80185932c0 @models=[Animal (call 'Animal.connection' to establish a connection)]>, @options={}, @definition={:index=>["animals"], :type=>["_doc"], :q=>"Fluffy"}>> 
2.5.1 :004 > reload!
Reloading...
 => true 
2.5.1 :005 > Elasticsearch::Model.search("Fluffy")
 => #<Elasticsearch::Model::Response::Response:0x00007f801da14380 @klass=#<Elasticsearch::Model::Multimodel:0x00007f801da146f0 @models=[Animal (call 'Animal.connection' to establish a connection)]>, @search=#<Elasticsearch::Model::Searching::SearchRequest:0x00007f801da14600 @klass=#<Elasticsearch::Model::Multimodel:0x00007f801da146f0 @models=[Animal (call 'Animal.connection' to establish a connection)]>, @options={}, @definition={:index=>["animals"], :type=>["_doc"], :q=>"Fluffy"}>> 
2.5.1 :006 > Elasticsearch::Model::Registry.all
 => [Animal (call 'Animal.connection' to establish a connection)] 
2.5.1 :007 > reload!
Reloading...
 => true 
2.5.1 :008 > Elasticsearch::Model::Registry.all
 => [Animal (call 'Animal.connection' to establish a connection)] 

@PhilCoggins
Copy link
Author

@estolfo thanks for reaching back out.

I have updated the instructions in the repo. Also make sure you run the setup instructions at the top (rails db:migrate and rails runner script.rb)

@estolfo
Copy link
Contributor

estolfo commented Jul 16, 2019

@PhilCoggins I've opened this pull request but I get a (ActiveRecord::SubclassNotFound) error when I test it with your example app. Maybe there's something else that needs to change in the example app to fully test the changes. I'll work on adding tests but I wanted to get the PR up asap so you could take a look and let me know what you think.

@PhilCoggins
Copy link
Author

I would consider the ActiveRecord::SubclassNotFound a derivative of the core issue.

Somehow, when a klass is first added to the @models array, it initially contains no descendants, but subsequently performing a search the descendant is added.

When code is reloaded, however, the klass is added to the @models array, and a search is performed, the descendants don't get added.

Using your branch of elasticsearch-model on my test project:

irb(main):001:0> require_dependency 'animal'
=> true
# Open connection
irb(main):002:0> Animal.connection
# Animal is in registry
irb(main):003:0> Elasticsearch::Model::Registry.all
=> [Animal(id: integer, name: text, type: text)]
# The model contains no descendants (ActiveRecord has not yet loaded this class)
irb(main):004:0> Elasticsearch::Model::Registry.all[0].descendants
=> []
irb(main):005:0> Elasticsearch::Model.search("Fluffy").records.to_a
=> [#<Dog id: 4, name: "Fluffy", type: "Dog">]
# Descendants now show up
irb(main):006:0> Elasticsearch::Model::Registry.all[0].descendants
=> [Dog(id: integer, name: text, type: text)]
irb(main):007:0> reload!
Reloading...
=> true
# Code reloaded, registry updated with new class, no descendants
irb(main):008:0> Elasticsearch::Model::Registry.all[0].descendants
=> []
# Blows up
irb(main):009:0> Elasticsearch::Model.search("Fluffy").records.to_a
Traceback (most recent call last):
        1: from (irb):9
ActiveRecord::SubclassNotFound (The single-table inheritance mechanism failed to locate the subclass: 'Dog'. This error is raised because the column 'type' is reserved for storing the class in case of inheritance. Please rename this column if you didn't intend it to be used for storing the inheritance class or overwrite Animal.inheritance_column to use another column for that information.)

I hope this helps.

@estolfo
Copy link
Contributor

estolfo commented Jul 17, 2019

@PhilCoggins thanks for the extra info, I'll keep investigating.

@estolfo
Copy link
Contributor

estolfo commented Jul 18, 2019

I want to give an update on this: I've been looking into the cause of the inherited class not being autoloaded after reload! is called and in the process, I've realized a big issue with using STI and Elasticsearch 7. Essentially, the feature was making use of the document type in the index but types are deprecated as of Elasticsearch 7. See here for more info on that.

That means we'll have to figure out another mechanism for supporting STI with Elasticsearch 7, as we can't rely on the automatic population of _type field based on the model name.

I'll have an update soon, thanks again for your patience.

@estolfo
Copy link
Contributor

estolfo commented Jul 19, 2019

@PhilCoggins I think one of the ways to support STI going forward with the removal of types in Elasticsearch would be to add an artificial type field to each document if inheritance is detected. I did this in the past with Mongoid. What do you think about that approach? For example:

class Animal < ApplicationRecord
  include Elasticasearch::Model
end
class Dog < Animal
end

The name, _inheritance_type is inspired by rail's inheritance_column

d = Dog.create(name: 'Fluffy')
# => { name: 'Fluffy', _inheritance_type: 'dog' }

Mongoid does something similiar in automatically adding a _type field in the document. It detects when a class is inherited and adds the _type field to a query if necessary.

One drawback of this approach is that an extra field is added to the document and that field has to be in the mapping and added to queries.

There are other ways to implement this but they are more manual and require user intervention.

@rabinpoudyal
Copy link

Not only in elasticsearch, I am also getting this error when using with active admin. Attached the screenshot.
This should be fixed.
image

@stale
Copy link

stale bot commented Aug 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 31, 2020
@stale stale bot closed this as completed Sep 7, 2020
@vanboom
Copy link

vanboom commented Nov 9, 2023

Update: still seeing this in 7.2.1 Rails 5.2. After reloading the Elasticsearch::Model::Registry.all[0].descendants is blank, so the ActiveRecord::SubclassNotFound is being thrown.

What is interesting is that I am putting my STI subclasses in the same index so should it not get instantiated using the base class?

Following the Dog < Animal example from above...

If all Dogs are indexed in the animals index, should it not instantiate them with Animal.where(id: [...]). It seems like Rails should take care of this with nothing else required here but yet the SubclasNotFound occurs.

@stalebot, please reopen - closing active issues does not make them go away.

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

Successfully merging a pull request may close this issue.

6 participants