I’m a little excited today because some code I wrote was accepted into the Ruby on Rails core.

It’s a patch I wrote to fix a tricky bug I ran into in ActiveRecord which can cause a call to one of your model’s attribute methods to sometimes throw a NoMethodError.

I came across this bug after upgrading a site I help maintain to Rails 2.3. This site allows users to download demos of software. Users have to specify which operating system they want a demo for, and this is stored in a database column named system.

All of this was working just fine until I upgraded to Rails 2.3. Suddenly the site started throwing errors like this.

NoMethodError: Attempt to call private method
    from /Users/sam/project/vendor/rails/activerecord/lib/active_record/attribute_methods.rb:236:in `method_missing'

I traced the error back to this line in a helper method.

@demo_files.map{|p| p.system}.compact

Why didn’t my tests catch this I was wondering? Here’s where things get esoteric. This test would pass:

it "should have a system" do
  demo = DemoFile.new(:system => "openSUSE")
  demo.system.should == "openSUSE" 
end

This one would fail (with a NoMethodError):

it "should have a system" do
  demo = DemoFile.new
  demo.system.should == nil
end

WTF? ActiveRecord recently started allowing you to mark attribute methods as private, meaning they would raise an error if you try to call them (from outside the object.) Before it would just call the method even if was private. You can see this in the first few line of the (pre-patch) version of ActiveRecord::Base#method_missing.

def method_missing(method_id, *args, &block)
  method_name = method_id.to_s

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

  # If we haven't generated any methods yet, generate them, then
  # see if we've created the method we're looking for.
  if !self.class.generated_methods?
    self.class.define_attribute_methods
    if self.class.generated_methods.include?(method_name)
      return self.send(method_id, *args, &block)
    end
  end

  if self.class.primary_key.to_s == method_name
    id
  elsif md = self.class.match_attribute_method?(method_name)
    attribute_name, method_type = md.pre_match, md.to_s
    if @attributes.include?(attribute_name)
      __send__("attribute#{method_type}", attribute_name, *args, &block)
    else
      super
    end.
  elsif @attributes.include?(method_name)
    read_attribute(method_name)
  else
    super
  end 
end

The problem was that if you had an attribute with the same name as a private method inherited from Object and you called that attribute before any others the thing would blow up.

Why? ActiveRecord only considers an attribute method to be implemented if it is defined in an ActiveRecord::Base descendant. Methods inherited from above ActiveRecord::Base in the inheritance chain are overridden the first time method_missing is hit.

# 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(& :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(& :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

But if there is a protected method inherited from Object which shares the name with an database attribute, it will raise an error before it ever tries to implement the attribute methods. If you call another attribute first it will define your attribute methods (overwriting the inherited private method) and work as expected.

The fix is to make sure define_attribute_methods is called before checking for private methods and raising a NoMethodErrors.

 def method_missing(method_id, *args, &block)
  method_name = method_id.to_s

  # If we haven't generated any methods yet, generate them, then
  # see if we've created the method we're looking for.
  if !self.class.generated_methods?
    self.class.define_attribute_methods
    guard_private_attribute_method!(method_name, args)
    if self.class.generated_methods.include?(method_name)
      return self.send(method_id, *args, &block)
    end
  end

  guard_private_attribute_method!(method_name, args)
  if self.class.primary_key.to_s == method_name
    id
  elsif md = self.class.match_attribute_method?(method_name)
    attribute_name, method_type = md.pre_match, md.to_s
    if @attributes.include?(attribute_name)
      __send__("attribute#{method_type}", attribute_name, *args, &block)
    else
      super
    end
  elsif @attributes.include?(method_name)
    read_attribute(method_name)
  else
    super
  end
end

For anyone else who’s experiencing this issue, the patch is available on the lighthouse ticket.