active_record_doctor icon indicating copy to clipboard operation
active_record_doctor copied to clipboard

Anonymous subclass of ActiveRecord::Base causes exception

Open owst opened this issue 2 years ago • 5 comments

As part of our test suite we run ActiveRecordDoctor and also create anonymous subclasses of ActiveRecord::Base - the latter causes an exception in the former

Traceback (most recent call last):
	9: from repro.rb:34:in `<main>'
	8: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/runner.rb:14:in `run_one'
	7: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/errors.rb:5:in `handle_exception'
	6: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/runner.rb:15:in `block in run_one'
	5: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/base.rb:11:in `run'
	4: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/base.rb:40:in `run'
	3: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/missing_unique_indexes.rb:32:in `detect'
	2: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/base.rb:145:in `models'
	1: from /Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/base.rb:145:in `reject'
/Users/owenstephens/.rbenv/versions/2.7.4/lib/ruby/gems/2.7.0/gems/active_record_doctor-1.9.0/lib/active_record_doctor/detectors/base.rb:146:in `block in models': undefined method `start_with?' for nil:NilClass (NoMethodError)

here's a simple reproduction script:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails", "= 5.2.6.2"
  gem "active_record_doctor", "= 1.9.0"
end

require "active_record"
require "active_record_doctor"

some_anonymous_model = Class.new(ActiveRecord::Base)

runner = ActiveRecordDoctor::Runner.new(ActiveRecordDoctor.load_config_with_defaults(nil))
runner.run_one(:missing_unique_indexes)

owst avatar Mar 11 '22 14:03 owst

Why not to use

SomeAnonymousModel = Class.new(ActiveRecord::Base)

?

This is not a trivial error to fix in active_record_doctor, since other methods used internally like .table_exists? aren't also working for anonymous models.

fatkodima avatar Mar 11 '22 15:03 fatkodima

Hi @fatkodima,

The classes are defined in a test, and it isn't otherwise necessary to explicitly name the class (we are using RSpec and generating the subclass in a let call, so can refer to the anonymous class directly). As it happens, in the relevant test we explicitly assign self.table_name for the generated subclasses and I checked that model.table_exists? does work.

Is there any other reason to explicitly require model.name be non-nil? Perhaps active_record_doctor could just skip any models where name.nil?

owst avatar Mar 14 '22 15:03 owst

Ah, ok, you're right 👍

fatkodima avatar Mar 14 '22 16:03 fatkodima

Before merging the corresponding PR, I'd like to understand your situation better, @owst. My initial reaction is we shouldn't make active_record_doctor compatible with implementation details of a particular test suite. I understand you're calling active_record_doctor methods directly from your specs which isn't an "officially" recognized way to use active_record_doctor (though, maybe we should make it official).

I'd like to understand your use case better: why do you call active_record_doctor directly instead of using rails active_record_doctor (+ per-detector configuration if customization is needed). Is that to overcome certain limitations imposed by calling directly from the command line? If so, what kind?

gregnavis avatar Mar 30 '22 22:03 gregnavis

@owst, let me know what you think about the above.

gregnavis avatar May 07 '22 08:05 gregnavis

Hi @gregnavis - Apologies for the massive amount of time that's passed on this issue without reply.

Our reason for calling active_record_doctor directly within our test suite is that our app has a somewhat slow startup and therefore rather than pay the startup cost twice (once for test suite loading and then again for active_record_doctor loading) we thought we could get away with only once.

However, upon reflection, since it's not a supported setup and somewhat unusual, I'm happy just close this issue and pay the cost an additional time in our CI pipeline.

Thanks

owst avatar Aug 25 '23 13:08 owst