Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Mass-assignment can work with a stringified hash #14

Merged
merged 1 commit into from
Jun 12, 2017
Merged

Mass-assignment can work with a stringified hash #14

merged 1 commit into from
Jun 12, 2017

Conversation

tatey
Copy link
Contributor

@tatey tatey commented Jun 12, 2017

Hello,

We came across an issue where mass assigning a stringified hash of attributes would raise a KeyError if any of those attributes were uninitialized on the object that mixes in ShallowAttributes.

Example:

class MainUser
  include ShallowAttributes
  attributes :age, Integer
end

u = MainUser.new
u.attributes # {}
u.attributes = {"age" => 21} # => KeyError

And this is the type of error you would get:

ShallowAttributes::#attributes=#test_0003_sets attributes which have not been initialized:
RuntimeError: can't add a new key into hash during iteration
    /shallow_attributes/lib/shallow_attributes/class_methods.rb:105:in `age='
    /shallow_attributes/lib/shallow_attributes/instance_methods.rb:207:in `block in define_attributes'
    /shallow_attributes/lib/shallow_attributes/instance_methods.rb:206:in `each'
    /shallow_attributes/lib/shallow_attributes/instance_methods.rb:206:in `define_attributes'
    /shallow_attributes/lib/shallow_attributes/instance_methods.rb:90:in `attributes='
    /shallow_attributes/test/shallow_attributes_test.rb:121:in `block (3 levels) in <top (required)>'

The reason we get this error is because of how we use ShallowAttributes with our form objects.

  1. The form object is built in a before_action and then we update the form object with the POST data.
  2. We perform mass assignment with a stringified hash (It's actually ActionController::StrongParameters) rather than a symbolized hash.

This patch changes the behaviour to make mass assignment work with both stringified and symbolized hashes. Given to_sym on symbols is a no-op there should be minimal overhead to projects using symbolized hashes.

Happy to have any feedback 😄

Cheers,
Tate

@coveralls
Copy link

coveralls commented Jun 12, 2017

Coverage Status

Coverage increased (+0.007%) to 99.26% when pulling de185d3 on fivegoodfriends:fix-mass-assignment-with-uninitialized-attributes into 141489a on davydovanton:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jun 12, 2017

Coverage Status

Coverage increased (+0.007%) to 99.26% when pulling de185d3 on fivegoodfriends:fix-mass-assignment-with-uninitialized-attributes into 141489a on davydovanton:master.

Copy link
Owner

@davydovanton davydovanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect! I'll merge this changes after CI 👍

@davydovanton davydovanton merged commit ac50fbb into davydovanton:master Jun 12, 2017
@davydovanton
Copy link
Owner

thanks!

@tatey
Copy link
Contributor Author

tatey commented Jun 13, 2017

Appreciate you merging @davydovanton. Would you mind cutting a new release? 😀

@tatey
Copy link
Contributor Author

tatey commented Jun 20, 2017

Bump? 😄

I'm happy to help if you need a hand.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants