Permalink
Browse files

Optimize for the happy path

Checking respond_to? incurs overhead, and most of the time when
assigning attributes it will return true. So just handle the
NoMethodError instead.
  • Loading branch information...
jonleighton committed Aug 17, 2012
1 parent 2ff47c4 commit 8cbad0293b134d56fb01e5f500010603b4c18cc7
Showing with 14 additions and 11 deletions.
  1. +14 −11 activerecord/lib/active_record/attribute_assignment.rb
@@ -97,22 +97,15 @@ def assign_attributes(new_attributes, options = {})
attributes.each do |k, v|
if k.include?("(")
multi_parameter_attributes << [ k, v ]
- elsif respond_to?("#{k}=")
- if v.is_a?(Hash)
- nested_parameter_attributes << [ k, v ]
- else
- send("#{k}=", v)
- end
+ elsif v.is_a?(Hash)
+ nested_parameter_attributes << [ k, v ]
else
- raise(UnknownAttributeError, "unknown attribute: #{k}")
+ _assign_attribute(k, v)
end
end
# assign any deferred nested attributes after the base attributes have been set
- nested_parameter_attributes.each do |k,v|
- send("#{k}=", v)
- end
-
+ nested_parameter_attributes.each { |k,v| _assign_attribute(k, v) }
assign_multiparameter_attributes(multi_parameter_attributes)
ensure
@mass_assignment_options = previous_options
@@ -130,6 +123,16 @@ def mass_assignment_role
private
+ def _assign_attribute(k, v)
+ public_send("#{k}=", v)
+ rescue NoMethodError
+ if respond_to?("#{k}=")
+ raise
+ else
+ raise UnknownAttributeError, "unknown attribute: #{k}"
+ end
+ end
+
# Instantiates objects for all attribute classes that needs more than one constructor parameter. This is done
# by calling new on the column type or aggregation type (through composed_of) object with these parameters.
# So having the pairs written_on(1) = "2004", written_on(2) = "6", written_on(3) = "24", will instantiate

2 comments on commit 8cbad02

Owner

guilleiguaran replied Sep 2, 2012

@jonleighton I'm having some troubles with this commit in integrate-strong_parameters (#7251) branch, right now I'm getting two failures related to pg schema tests:

  1) Error:
test_classes_with_qualified_schema_name(SchemaTest):
ActiveModel::MissingAttributeError: can't write unknown attribute `'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/write.rb:40:in `write_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:63:in `write_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/primary_key.rb:21:in `id='
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:72:in `public_send'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:72:in `_assign_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:53:in `block in assign_attributes'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:47:in `each'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:47:in `assign_attributes'
    /Users/guille/code/rails/activerecord/lib/active_record/core.rb:188:in `initialize'
    /Users/guille/code/rails/activerecord/lib/active_record/persistence.rb:42:in `new'
    /Users/guille/code/rails/activerecord/lib/active_record/persistence.rb:42:in `create'
    /Users/guille/code/rails/activerecord/test/cases/adapters/postgresql/schema_test.rb:199:in `test_classes_with_qualified_schema_name'

  2) Error:
test_prepared_statements_with_multiple_schemas(SchemaTest):
ActiveModel::MissingAttributeError: can't write unknown attribute `'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/write.rb:40:in `write_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:63:in `write_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_methods/primary_key.rb:21:in `id='
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:72:in `public_send'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:72:in `_assign_attribute'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:53:in `block in assign_attributes'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:47:in `each'
    /Users/guille/code/rails/activerecord/lib/active_record/attribute_assignment.rb:47:in `assign_attributes'
    /Users/guille/code/rails/activerecord/lib/active_record/core.rb:188:in `initialize'
    /Users/guille/code/rails/activerecord/lib/active_record/persistence.rb:42:in `new'
    /Users/guille/code/rails/activerecord/lib/active_record/persistence.rb:42:in `create'
    /Users/guille/code/rails/activerecord/test/cases/adapters/postgresql/schema_test.rb:310:in `test_prepared_statements_with_multiple_schemas'

The tests involved are trying to assign the primary key (id) and fails to determine what's the primary key for model, doing some debugging in AR code I found that ActiveRecord::AttributeMethods::PrimaryKey.get_primary_key is returning nil (the last executed in get_primary_key is this)

I'm not sure about what's happening but changing the definition of _assign_attribute the tests are starting to pass inmediatly:

def _assign_attribute(k, v)
  if respond_to?("#{k}=")
    public_send("#{k}=", v)
  else
    raise UnknownAttributeError, "unknown attribute: #{k}"
  end
end

Can you help me to fix the problem?

Member

jonleighton replied Sep 2, 2012

@guilleiguaran all models respond to #id=, regardless of whether or not they have a primary key, so it looks like that's the issue. so #id= is calling write_attribute(nil, ...). I think the best thing would be to make #id= do nothing if self.class.primary_key is nil.

(please @-mention me in any reply to ensure I see it. thanks.)

Please sign in to comment.