Browse files

Don't assign the attributes if the list is empty.

  • Loading branch information...
1 parent 7495b26 commit 79c8c4fed46775f30551f6d4b0c50b9ba397bc54 Aurelian Oancea committed Jun 8, 2012
View
2 activerecord/lib/active_record/attribute_assignment.rb
@@ -64,7 +64,7 @@ def attributes=(new_attributes)
# user.name # => "Josh"
# user.is_admin? # => true
def assign_attributes(new_attributes, options = {})
- return unless new_attributes
+ return if new_attributes.blank?
attributes = new_attributes.stringify_keys
multi_parameter_attributes = []
View
4 activerecord/test/cases/mass_assignment_security_test.rb
@@ -98,6 +98,10 @@ def test_mass_assigning_does_not_choke_on_nil
Firm.new.assign_attributes(nil)
end
+ def test_mass_assigning_does_not_choke_on_empty_hash
+ Firm.new.assign_attributes({})
+ end
+
def test_assign_attributes_uses_default_role_when_no_role_is_provided
p = LoosePerson.new
p.assign_attributes(attributes_hash)

8 comments on commit 79c8c4f

@garethrees

@aurelian Does attributes= return the existing attributes if no options are passed? If not, where does the existing attributes method now live (if at all)? http://api.rubyonrails.org/v2.3.11/classes/ActiveRecord/Base.html#M001872

@garethrees

@rafaelfranca cheers. I'd better get started reading the contribution guidelines! Main reason I was after it was because the Hash keys are strings, rather than symbols. I wanted to submit a pull request calling #symbolize_keys on them so that the usage is more common.

My use case was that I was creating a test like the following:

# Update the message with a nil partner_id
put :edit, :id => msg.id, :message => msg.attributes.merge!({ :partner_id => nil })

Took me a while to realise that the symbol key was being merged in as a separate key to the string partner_id key.

What are your thoughts on that?

@rafaelfranca
Ruby on Rails member

I think we should not call #symbolize_keys in the attributes hash. Since symbols are never removed from the memory this can lead a more memory usage.

@tenderlove thoughts?

@garethrees

In any case I think that #attributes should explicitly state that the hash returned has string keys as I think it could catch people out. The only time I've ever really used it is in functional tests so I'd be surprised if my example above is unique. There seems to be extensive use of #symbolize_keys, but if there's a less memory intensive way of achieving the same result it would be an option.

@aurelian

sorry @garethrees been away from internets + don't think I can help you with your question :|

@garethrees

No problem! I just saw you as a recent author of the file. Maybe I should create an issue instead of a comment?

@rafaelfranca
Ruby on Rails member

Please do not create issues to feature requests/proposals. Send an email to the rails core mailing list.

Please sign in to comment.