Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remove ActiveRecord::AttributeAssignment#assign_nested_parameter_attributes #49678

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Oct 17, 2023

Motivation / Background

The private #assign_nested_parameter_attributes was introduced in 774ff18 (Dec 15, 2011), moved to ActiveRecord::AttributeAssignment in ceb33f8 (also Dec 15, 2011), then most recently modified in 2606fb3 (Jan 23, 2015) when ActiveModel::AttributeAssignment was extracted.

Support for accepts_nested_attributes_for (introduced in ec8f045 Feb 1, 2009) pre-dates those commits, and has evolved enough to cover this behavior in other ways.

Detail

With its removal, Active Record's test suite still passes, so if it's a crucial piece of code to retain, we should expand the suite to exercise it.

Additional information

The fact that removing these methods doesn't fail the test suite came out of #49675. If that PR isn't deemed to be viable, removing the code in this PR might still have value. If that PR is deemed to be viable, we might want to merge this PR ahead of it, then make similar changes to remove the calls from https://github.com/rails/rails/pull/49675/files#diff-bd47ea9eef412ec5aca24908af217592cef01d24fd54eeec976bd37bd9a18151R80.

@p8
Copy link
Member

p8 commented Oct 21, 2023

The following test was added in 774ff18 to ensure the assignment was working as expected:

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

This test was removed in #4122 as it was failing and probably broken because of mass assignment protection.
There is a discussion in the PR.

Enabling the test again, will cause it to fail in 6-0-stable and main.
So I think we can remove assign_nested_parameter_attributes.

@p8
Copy link
Member

p8 commented Oct 22, 2023

It also seems to me that assign_nested_parameter_attributes calls _assign_attribute(key, v), which is the same as _assign_attributes does in the else statement. It's only done after all other key/value pairs have been set.

@seanpdoyle
Copy link
Contributor Author

@rafaelfranca is the historical context provided in #49678 (comment) sufficient to consider this code dead and safe to remove?

…ributes`

The private `#assign_nested_parameter_attributes` was introduced in
[774ff18][] (Dec 15, 2011), moved to `ActiveRecord::AttributeAssignment`
in [ceb33f8][] (also Dec 15, 2011), then most recently modified in
[2606fb3][] (Jan 23, 2015) when `ActiveModel::AttributeAssignment` was
extracted.

Support for `accepts_nested_attributes_for` (introduced in [ec8f045][]
Feb 1, 2009) pre-dates those commits, and has evolved enough to cover
this behavior in other ways.

With its removal, Active Record's test suite still passes, so if it's a
crucial piece of code to retain, we should expand the suite to exercise
it.

[774ff18]: rails@774ff18
[ceb33f8]: rails@ceb33f8
[2606fb3]: rails@2606fb3
[ec8f045]: rails@ec8f045
@seanpdoyle seanpdoyle force-pushed the remove-unnecessary-method-from-attribute-assignment branch from d10dc38 to ded026b Compare January 10, 2024 12:36
@rafaelfranca rafaelfranca merged commit 75ba520 into rails:main Jan 10, 2024
4 checks passed
@seanpdoyle seanpdoyle deleted the remove-unnecessary-method-from-attribute-assignment branch January 10, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants