Patch for Lighthouse Bug #2808

Hi all,

I've submitted a patch for a bug I came across in ActiveRecord. https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2808-ar-attribute-collides-with-private-method-results-in-nomethoderror#ticket-2808-7

This is the first patch I've submitted to the Rails core and I would love to hear any feedback.

I've described the logic behind the fix in the ticket, and also copied it below.

Thanks Sam Goldstein

## From Lighthouse ticket 2808

I've created a patch for this issue.

The root problem here was that ActiveRecord was using two inconsistent methods for determining if an attribute method had been implemented.

When checking if access control allowed an attribute method to be called AR looked to see if a private method with that name was defined, raising an error if it was.

When checking to determine whether an attribute method was already implemented AR looked to see if the method was defined by an ancestor below ActiveRecord in the inheritance chain. Methods that shared their names with attributes but were inherited from ancestors above ActiveRecord (e.g. Object, Module, Kernel) were overridden.

The existing AR method which defines this check is shown below.

      # Checks whether the method is defined in the model or any of its subclasses       # that also derive from Active Record. Raises DangerousAttributeError if the       # method is defined by Active Record though.       def instance_method_already_implemented?(method_name)         method_name = method_name.to_s         return true if method_name =~ /^id(=$|\?$|$)/         @_defined_class_methods ||= ancestors.first (ancestors.index(ActiveRecord::Base)).sum() { |m| m.public_instance_methods(false) | m.private_instance_methods(false) | m.protected_instance_methods(false) }.map {|m| m.to_s }.to_set         @@_defined_activerecord_methods ||= (ActiveRecord::Base.public_instance_methods(false) | ActiveRecord::Base.private_instance_methods(false) | ActiveRecord::Base.protected_instance_methods(false)).map{|m| m.to_s }.to_set         raise DangerousAttributeError, "#{method_name} is defined by ActiveRecord" if @@_defined_activerecord_methods.include?(method_name)         @_defined_class_methods.include?(method_name)       end

I've updated ActiveRecord to respect access control, using this same check. Private methods inherited from Object (for example) will be overridden, while those implemented by an ActiveRecord descendant are respected. This avoids situations where an read attribute method will behave differently depending on whether another attribute was called before it (as detailed in this ticket.)

This is the first patch I've submitted to the Rails core and I would love to hear any feedback.

Appears it was accepted. =r