search

rails / protected_attributes

Protect attributes from mass-assignment in ActiveRecord models.
MIT License
228 stars 92 forks source link

fix broken through_association from addition of build_record method in rails #54

Closed jGRUBBS closed 9 years ago

jGRUBBS commented 9 years ago

fixes #53

@rafaelfranca I noticed when I first commented on #53 that I was running from rails 4.2.0.beta2 locally and that was why there was 119 failures. I updated to Rails 4.2.0 and all tests were passing. I then set my Gemfile to Rails 4-2-stable and saw the test_has_many_through_build_with_attr_accessible_attributes fail with the same error as @dandlezzz: ArgumentError (wrong number of arguments (2 for 1))

I looked back at Rails 3.2 to see how the ThroughAssociation was handling build_record and saw that it was just using the base Association class implementation so I copied that over into a monkey patch for ThroughAssociation. This fixes the failure and all tests now pass again. Let me know if this is good for you, happy to make updates.

dandlezzz commented 9 years ago

@jGRUBBS thanks, I will test it out.

jGRUBBS commented 9 years ago

@rafaelfranca what do you think about this? Also, when do you think a Rails 4.2 compatible release can be made? Especially, considering 1.0.8 definitely breaks with Rails 4.2.

mcfiredrill commented 9 years ago

@jGRUBBS The tests in protected_attributes itself pass without this patch. Perhaps you should add a test that breaks without this PR?

jGRUBBS commented 9 years ago

@mcfiredrill this breaks using the 4.2 Gemfile. To see it fail run this:

BUNDLE_GEMFILE=gemfiles/Gemfile-rails-4.2 bundle
BUNDLE_GEMFILE=gemfiles/Gemfile-rails-4.2 rake

You should get something like this:

Finished in 2.510086s, 69.7187 runs/s, 248.9955 assertions/s.

  1) Error:
MassAssignmentSecurityHasManyRelationsTest#test_has_many_through_build_with_attr_accessible_attributes:
ArgumentError: wrong number of arguments (2 for 1)
    /Users/justin/.rvm/gems/ruby-2.0.0-p247/bundler/gems/rails-62e9e61f2d1b/activerecord/lib/active_record/associations/through_association.rb:95:in `build_record'
    /Users/justin/Sites/PA/lib/active_record/mass_assignment_security/associations.rb:72:in `build_record'
    /Users/justin/Sites/PA/lib/active_record/mass_assignment_security/associations.rb:19:in `build'
    /Users/justin/Sites/PA/lib/active_record/mass_assignment_security/associations.rb:55:in `build'
    /Users/justin/Sites/PA/test/attribute_sanitization_test.rb:687:in `test_has_many_through_build_with_attr_accessible_attributes'

175 runs, 625 assertions, 0 failures, 1 errors, 0 skips
jGRUBBS commented 9 years ago

To clarify this will be an issue whenever Rails 4.2.1 is released.

mcfiredrill commented 9 years ago

Ah ok got it, assumed the default Gemfile was for 4.2. :+1:

jGRUBBS commented 9 years ago

@rafaelfranca any update on a new release of protected_attributes? I'm especially curious as you announced that rails 4.2.1 will be released march 2nd: http://weblog.rubyonrails.org/2015/2/25/Rails-4-2-1-rc2-and-4-1-10-rc2-have-been-released/?utm_source=rubyweekly&utm_medium=email

arthurnn commented 9 years ago

@jGRUBBS thanks for the PR , I decided to merge #64 , as that code is more close to the actual AR-core code. thanks anyways