This project is archived and is in readonly mode.

#2808 ✓committed
Tjalling van der Wal

AR attribute collides with private method, results in NoMethodError

Reported by Tjalling van der Wal | June 16th, 2009 @ 03:00 PM | in 2.x

Since 2.3 (possibly 2.2) there is a check in activerecord/lib/active_record/attribute_methods.rb to protect against calling private methods through method_missing:

if self.class.private_method_defined?(method_name)
  raise NoMethodError.new("Attempt to call private method", method_name, args)
end

However I've ran into a bug that occurs in particular situations when an attribute and a private method have the same name. Take a look at this: Zipcode is a simple AR::Base object, with a private method 'y'.

>> zip = Zipcode.new
=> #<Zipcode id: nil, zipcode: nil, state: nil, county: nil, city: nil, x: nil, y: nil, z: 0 >
>> zip.y
NoMethodError: Attempt to call private method
    from ../trunk/vendor/rails/activerecord/lib/active_record/attribute_methods.rb:236:in `method_missing'
    from (irb):8
>> zip.x
=> nil
>> zip.y
=> nil

The first call to zip.y raises, but the second one succeeds! This is caused by the call to zip.x, because when the check above does succeed for 'x', all attribute accessors are generated in bulk, including a public 'y' method.

The problem is that the bulk generation does a lot of checking, but does not perform the check against calling private methods, if I understand things correctly.

Comments and changes to this ticket

  • Andy Atkinson

    Andy Atkinson June 18th, 2009 @ 06:36 AM

    I was not able to reproduce your error with the automated tests in the attached app, or in the console. Did I get the test conditions correct?

  • Tjalling van der Wal

    Tjalling van der Wal June 18th, 2009 @ 02:21 PM

    Hi, thanks for your interest.

    To reproduce the error I made the following changes:

    First, instead of defining 'y' in Foo, add it to Object in an initializer (in our project it is some utility function inject by a plugin)

    class Object
      private 
      def y

    &quot;The value of y&quot;
    
    
    
    
    end end

    Second, I rewrote your tests:

    def test_aaaa_raise_error_when_y_is_called_the_first_time
      puts "running: test_raise_error_when_y_is_called_the_first_time"
      assert_raise NoMethodError do

    Foo.new.y
    
    
    
    
    end end
    def test_bbbb_access_the_private_call puts "running: test_access_the_private_call" assert "The value of y", Foo.new.send(:y) end
    def test_cccc_first_call_x_the_y puts "running: test_first_call_x_the_y" f = Foo.new f.x assert_equal "The value of y", f.y # This one should fail as the AR-attribute has no value set. end

    Note that the order of the tests matters because calling 'x' has side effects. Therefore I added explicit output to the tests to be sure they are run in the correct order. I added the aaaa, bbbc, cccc prefixes to the tests names because (apparently) tests are sorted alphabetically.

    The 3rd test you wrote ("fetch the value accessing the private method the second time") should indeed fail. Calling the private y method with send in the 2nd test does not generate the public method.

  • Sam Goldstein

    Sam Goldstein July 1st, 2009 @ 09:16 PM

    I'm having a similar issue with a Model that has a system attribute. Since this is an inherited private method an error is sometimes thrown when the accessor is called (but only if no other attribute accessor has been called.)

    Is anyone working on a patch for this?

  • Tjalling van der Wal

    Tjalling van der Wal July 7th, 2009 @ 08:49 AM

    I've commented the check for calling private methods out in our project. I don't know if the check is really needed, since method_missing performs other checks too.

    The only nice solution to this problem is to resolve the naming conflict.

  • Sam Goldstein

    Sam Goldstein July 8th, 2009 @ 08:01 PM

    • Assigned user set to “CancelProfileIsBroken”

    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.)

  • Sam Goldstein

    Sam Goldstein July 8th, 2009 @ 08:01 PM

    • Tag changed from 2-3-stable, active_record, attributes to 2-3-stable, active_record, attributes, patch
  • CancelProfileIsBroken
  • Repository

    Repository July 9th, 2009 @ 05:47 AM

    • State changed from “new” to “committed”

    (from [d60d7edce462f4602bfc9996689087a235b034c9]) Make it so AR attributes which conflict with object-private methods (e.g. system) don't 'randomly' cause NoMethodErrors

    Previously if you called this attribute before others, you'd get exceptions. But if it was the second-or-subsequent attribute you retrieved you'd get the correct behaviour.

    Signed-off-by: Michael Koziarski michael@koziarski.com
    [#2808 state:committed] http://github.com/rails/rails/commit/d60d7edce462f4602bfc9996689087...

  • Eliot Sykes

    Eliot Sykes January 6th, 2010 @ 07:57 PM

    • Tag changed from 2-3-stable, active_record, attributes, patch to 2, active_record, attributes, patch

    Not sure if this is news, just been looking at the 2.3.5 tag to see if this fix is included and it doesn't seem to be

    http://github.com/rails/rails/blob/v2.3.5/activerecord/lib/active_r...

    Does anybody know if this is scheduled to go into a later version?

  • Eliot Sykes

    Eliot Sykes January 6th, 2010 @ 07:58 PM

    • Tag changed from 2, active_record, attributes, patch to 2-3-stable, active_record, attributes, patch
  • Eliot Sykes

    Eliot Sykes January 6th, 2010 @ 08:50 PM

    • Tag changed from 2-3-stable, active_record, attributes, patch to 2, active_record, attributes, patch

    I'm putting this patch I'm using on Rails 2.2.2 for completeness, in case anyone else hits this, plus I'm not sure if respond_to? has been fixed in line with how method_missing has above.

    Put this in config/initializers/active_record.rb

    
    class ActiveRecord::Base
      def method_missing_with_force_define_attribute_methods(method_id, *args, &block)
        self.class.define_attribute_methods unless self.class.generated_methods?
        method_missing_without_force_define_attribute_methods(method_id, *args, &block)
      end
      alias_method_chain :method_missing, :force_define_attribute_methods
      
      def respond_to_with_force_define_attribute_methods?(method, include_private_methods = false)
        self.class.define_attribute_methods unless self.class.generated_methods?
        respond_to_without_force_define_attribute_methods?(method, include_private_methods)
      end
      alias_method_chain :respond_to?, :force_define_attribute_methods
    end
    
  • Eliot Sykes

    Eliot Sykes January 6th, 2010 @ 08:51 PM

    • Tag changed from 2, active_record, attributes, patch to 2-3-stable, active_record, attributes, patch
  • Marcus Mateus

    Marcus Mateus January 26th, 2010 @ 12:16 AM

    I just ran into this issue as well and wasted a good bit of time tracing the odd behavior before finding this post. Please do incorporate it into the next release... it's sad that a commit has been out there since July.

    Eliot, thanks for the above patch as a quick temporary fix.

  • Paul Barry

    Paul Barry March 26th, 2010 @ 02:42 AM

    I ran into this as well, because I have an attribute called link, but the problem only comes up when you try to call that method in a Rake task, because Rake defines a private link method on Object. I got around it simply by calling define_attribute_methods in the class definition.

  • Jarl Friis

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages