plentz / lol_dba

lol_dba is a small package of rake tasks that scan your application models and displays a list of columns that probably should be indexed. Also, it can generate .sql migration scripts.
1.59k stars 69 forks source link

Check if obj has ActiveRecord::Base as ancestor #91

Closed epergo closed 6 years ago

epergo commented 7 years ago

Hi!

I recently discovered this gem and I decided to try it out in a test application, so I created a brand new Rails 5 web app, added the gem acts_as_votable to check if the indexes that the gem creates are correct but I got the following error:

rake aborted!
NoMethodError: undefined method `descends_from_active_record?' for #<Class:XPath::HTML>
/home/epergo/.asdf/installs/ruby/2.4.1/bin/bundle:22:in `load'
/home/epergo/.asdf/installs/ruby/2.4.1/bin/bundle:22:in `<main>'
Tasks: TOP => db:find_indexes
(See full trace by running task with --trace)

The exact line is the following one:

lol_dba-2.1.4/lib/lol_dba.rb:94:in `block in check_for_indexes'
Class == obj.class && obj != ActiveRecord::Base && obj.ancestors.include?(ActiveRecord::Base) && (!defined?(ActiveRecord::SessionStore::Session) || obj != ActiveRecord::SessionStore::Session)

Specifically this condition:

obj.ancestors.include?(ActiveRecord::Base)

includes? use operator == to check if the object passed as parameter is the same as any of the objects in the array.

XPath gem redefines operator :== and creates an XPath::Expression which evaluates as true in a condition, that means that for the object Class:XPath::HTML that has the following ancestors:

[3] pry(LolDba)> obj.ancestors                                                                                                                                 
=> [#<Class:XPath::HTML>,                                                                                                                                      
 XPath::HTML,                                                                                                                                                  
 XPath::DSL,                                                                                                                                                   
 Module,                                                                                                                                                       
 ActiveSupport::Dependencies::ModuleConstMissing,                                                                                                              
 Module::Concerning,                                                                                                                                           
 ActiveSupport::ToJsonWithActiveSupportEncoder,                                                                                                                
 Object,                                                                                                                                                       
 PP::ObjectMixin,                                                                                                                                              
 ActiveSupport::Dependencies::Loadable,                                                                                                                        
 JSON::Ext::Generator::GeneratorMethods::Object,                                                                                                               
 ActiveSupport::Tryable,                                                                                                                                       
 Kernel,                                                                                                                                                       
 BasicObject] 

The ancestor XPath::HTML evaluates as a True value when comparing to ActiveRecord::Base:

[4] pry(LolDba)> XPath::HTML == ActiveRecord::Base
=> #<XPath::Expression:0x0055a0ee565560
 @arguments=[:"=", #<XPath::Expression:0x0055a0ee5655d8 @arguments=[], @expression=:this_node>, ActiveRecord::Base],
 @expression=:binary_operator>
[5] pry(LolDba)> !!(XPath::HTML == ActiveRecord::Base)
=> true

In this Pull Request I change the way is checked if the object has ActiveRecord::Base as ancestor, using eql? instead of == because eql? is not usually redefined.

I think no extra tests are needed.

plentz commented 6 years ago

I've changed a few things here, can you give 2.1.5 a try? sorry for taking so long to reply.

epergo commented 6 years ago

It seems to work now :+1:

plentz commented 6 years ago

awesome, thanks!