Skip to content

Loading…

added failing tests for has_many, has_one and belongs_to associations wi... #4122

Merged
merged 1 commit into from

4 participants

@qoobaa

...th strict mass assignment sanitizer, fixed build_record to not merge creation_attributes, removed failing nested attributes tests (that feature was broken anyway) #4051

@qoobaa qoobaa added failing tests for has_many, has_one and belongs_to associations…
… with strict mass assignment sanitizer, fixed build_record to not merge creation_attributes, removed failing nested attributes tests (that feature was broken anyway) #4051
1ad0a53
@josevalim josevalim merged commit 1634cfa into rails:master
@jonleighton
Ruby on Rails member

@qoobaa the test that you have removed was supposed to fix another regression. please could you find the author and ask discuss with them how to fix that also? (as now the regression is presumably re-introduced)

@qoobaa

Sure, I'll do that. That code was broken with mass assignment protection turned on. It won't work anymore probably.

@akaspick

That test should still be there to make sure that regression keeps working moving forward; it is definitely re-introduced now.

If the new record is being created off of an association, it should have reference to the owning object of the association and then that test should pass. If the creation attributes don't exist, the new object has no reference to the owner.

Shouldn't that test have been left and a solution that worked with the regression be implemented instead? Kind of defeats the purpose of the test in the first place.

@qoobaa

The test should be there, but it should pass. The previous solution doesn't work with mass assignment protection - it's just broken. Do you have any other idea how to fix that?

@akaspick

The test suite passed when I committed the change... you're saying it didn't? Or were there no tests for functionality that should have had them before and now it fails?

I'm not sure how else to allow the reference to the owner object be set on the new record. I was looking for a hook somewhere in the creation process to allow the assignment of the owner, but couldn't find one, so passing in the creation attributes is what seemed to work.

If somebody with more detailed knowledge of the AR API knows of how to make the association between the owner and the new record, then that should be used. The AR API between 3.0 and 3.1 changed considerably and this connection between the owner and new record was lost.

@qoobaa

To break your tests, you can just add:

attr_protected :pirate_id

to test/models/bird.rb. It shouldn't work like that, should it? The bug with merging creation_attributes was hidden, until the "strict" mass assignment sanitizer was introduced. It definitely can't be fixed like that, you can create a separate issue for that (with a reference to the previous one, etc.).

@qoobaa

I'll try to figure it out, maybe we can find another way of fixing it.

@akaspick

To rephrase, the internals of the AR creation process changed, not the API itself.

If the code somewhere in...

reflection.build_association(attributes, options) |record|

... can pass the "record" with the owner pre-assigned, that would be the ideal fix I'd assume, but I don't believe the "reflection" object has the details of the owner either. Perhaps it needs to allow one to be assigned somehow so this all works.

@josevalim
Ruby on Rails member
@akaspick

Well if mass assignment is strict now this regression will have to remain I guess. I don't think there is any other way around it other than making it non-strict (set explicitly) since it depends on the :id field of the nested attributes. Is there a way to turn off the strictness for special cases?

Another alternative is if nested attributes are special cased to allow such values.

@qoobaa

@akaspick could you please create a pull request with your solution?

@akaspick

Nested attributes for existing objects use an :id field, so how does the whole nested attributes feature work now if mass assignment is strict by default? Based on the discussion here, nested attributes would appear to only work for new objects now.

@akaspick

Ok, possible fix here... akaspick@a882790

All tests pass.

The key was letting the attributes in the block be set first. The creation attributes can be safely set without protection, but the block needed to be evaluated first before the normal attributes are set.

I had a few other possible solutions, but a few failing test cases came up that prevented those solutions from moving forward.

I haven't submitted a pull request yet. Comments?

@josevalim
Ruby on Rails member
@jonleighton
Ruby on Rails member

We definitely should not change the position of the yield in initialize...

@akaspick

I'm somewhat out of ideas then as build_association translates directly to klass.new(*options, &block) and that leaves little room for properly assigning any associations. Everything else I had tried had some edge cases that produced failing tests as well.

Another possible solution that looked promising was...

rec = reflection.build_association(creation_attributes, options.merge(:without_protection => true))
rec.assign_attributes(attributes, options)
rec.assign_attributes(create_scope.except(*record.changed), :without_protection => true)

But that produced some failing tests when the association class overrides self.new

If there are no other suggestions on how to accomplish this, I'll just deal with this being a regression now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 21, 2011
  1. @qoobaa

    added failing tests for has_many, has_one and belongs_to associations…

    qoobaa committed
    … with strict mass assignment sanitizer, fixed build_record to not merge creation_attributes, removed failing nested attributes tests (that feature was broken anyway) #4051
View
7 activerecord/lib/active_record/associations/association.rb
@@ -230,13 +230,8 @@ def association_class
end
def build_record(attributes, options)
- attributes = (attributes || {}).reverse_merge(creation_attributes)
-
reflection.build_association(attributes, options) do |record|
- record.assign_attributes(
- create_scope.except(*record.changed),
- :without_protection => true
- )
+ record.assign_attributes(create_scope.except(*record.changed), :without_protection => true)
end
end
end
View
63 activerecord/test/cases/mass_assignment_security_test.rb
@@ -50,6 +50,13 @@ def assert_all_attributes(person)
assert_equal 'm', person.gender
assert_equal 'rides a sweet bike', person.comments
end
+
+ def with_strict_sanitizer
+ ActiveRecord::Base.mass_assignment_sanitizer = :strict
+ yield
+ ensure
+ ActiveRecord::Base.mass_assignment_sanitizer = :logger
+ end
end
module MassAssignmentRelationTestHelpers
@@ -323,6 +330,13 @@ def test_has_one_build_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_one_build_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.build_best_friend(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
# create
def test_has_one_create_with_attr_protected_attributes
@@ -350,6 +364,13 @@ def test_has_one_create_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_one_create_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.create_best_friend(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
# create!
def test_has_one_create_with_bang_with_attr_protected_attributes
@@ -377,6 +398,13 @@ def test_has_one_create_with_bang_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_one_create_with_bang_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.create_best_friend!(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
end
@@ -438,6 +466,13 @@ def test_belongs_to_create_without_protection
assert_all_attributes(best_friend)
end
+ def test_belongs_to_create_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.create_best_friend_of(attributes_hash.except(:id, :comments))
+ assert_equal best_friend.id, @person.best_friend_of_id
+ end
+ end
+
# create!
def test_belongs_to_create_with_bang_with_attr_protected_attributes
@@ -465,6 +500,13 @@ def test_belongs_to_create_with_bang_without_protection
assert_all_attributes(best_friend)
end
+ def test_belongs_to_create_with_bang_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.create_best_friend_of!(attributes_hash.except(:id, :comments))
+ assert_equal best_friend.id, @person.best_friend_of_id
+ end
+ end
+
end
@@ -499,6 +541,13 @@ def test_has_many_build_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_many_build_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.best_friends.build(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
# create
def test_has_many_create_with_attr_protected_attributes
@@ -526,6 +575,13 @@ def test_has_many_create_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_many_create_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.best_friends.create(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
# create!
def test_has_many_create_with_bang_with_attr_protected_attributes
@@ -553,6 +609,13 @@ def test_has_many_create_with_bang_without_protection
assert_all_attributes(best_friend)
end
+ def test_has_many_create_with_bang_with_strict_sanitizer
+ with_strict_sanitizer do
+ best_friend = @person.best_friends.create!(attributes_hash.except(:id, :comments))
+ assert_equal @person.id, best_friend.best_friend_id
+ end
+ end
+
end
View
5 activerecord/test/cases/nested_attributes_test.rb
@@ -617,11 +617,6 @@ def test_should_take_a_hash_with_composite_id_keys_and_assign_the_attributes_to_
assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.name, @child_2.name]
end
- def test_should_take_a_hash_with_owner_attributes_and_assign_the_attributes_to_the_associated_model
- @pirate.birds.create :name => 'bird', :pirate_attributes => {:id => @pirate.id.to_s, :catchphrase => 'Holla!'}
- assert_equal 'Holla!', @pirate.reload.catchphrase
- end
-
def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find #{@child_1.class.name} with ID=1234567890 for Pirate with ID=#{@pirate.id}" do
@pirate.attributes = { association_getter => [{ :id => 1234567890 }] }
View
4 activerecord/test/models/person.rb
@@ -54,7 +54,7 @@ class LoosePerson < ActiveRecord::Base
self.table_name = 'people'
self.abstract_class = true
- attr_protected :comments
+ attr_protected :comments, :best_friend_id, :best_friend_of_id
attr_protected :as => :admin
has_one :best_friend, :class_name => 'LoosePerson', :foreign_key => :best_friend_id
@@ -81,4 +81,4 @@ class TightPerson < ActiveRecord::Base
accepts_nested_attributes_for :best_friend, :best_friend_of, :best_friends
end
-class TightDescendant < TightPerson; end
+class TightDescendant < TightPerson; end
Something went wrong with that request. Please try again.