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

Already on GitHub? Sign in to your account

Make nested attributes act as create_or_update #10399

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

chadmoone commented May 1, 2013

After some more exploration into #10344, I felt that I understood how things were set up a bit better, and this seemed the best solution (of course, let me know if you see a better one 😉).

Since :without_protection has been removed from ActiveRecord in an effort to move security away from the model, there is now no way to create nested objects while specifying the id/primary key. Since strong_parameters works with nested attributes, it should still support this ability.

Also, since all other behavior with nested attributes generally works as create_or_update, we should follow that pattern here and allow creation of nested attributes, both when an ID is omitted and when it is present. Currently, passing a hash which includes a (currently) non-existant id will throw a RecordNotFound error (this behavior was tested, but not documented anywhere). Rather than adding a :without_protection-type parameter back in, this seemed like the best option because it was more or less what the documentation and API seemed to already be implying.

The clearest case where this is an issue is the case of UUID, where the primary key may be generated elsewhere and should be respected by the Rails service.

I would welcome any feedback or suggestions.

Note: this pull request is closely related to #10271, and the merge may be slightly messy, but they seemed like separate issues, so I created a separate pull request.

Make nested attributes act as create_or_update
Since :without_protection has been removed from Rails in an effort to move security away from the model, we should allow creation of nested attributes, both when an ID is omitted and when it is present.
Contributor

JonRowe commented Aug 18, 2013

Hey has anyone had time to review this? @steveklabnik @pixeltrix

Member

steveklabnik commented Aug 21, 2013

I'm not familiar enough with the details, but it doesn't merge cleanly any more.

Contributor

JonRowe commented Aug 21, 2013

@chadmoone could you rebase this?

Contributor

chadmoone commented Aug 21, 2013

Hey, sorry I haven't had a chance to get to this today. I would love to get this in. I should be able to get it rebased and retested tomorrow.

As this is a 3 months old issue, is this still a valid concern or should this be closed?

Contributor

pftg commented Nov 16, 2013

@chadmoone any updates? Also please add Changelog entry.

Contributor

chadmoone commented Nov 18, 2013

Sorry, guys, just got back from some long travel.

I still see this as a valid concern. It was functionality that seems to be unintentionally removed with the change to strong_parameters, and there is no alternative way to enable this without either disabling strong_params or patching ActiveRecord. Frankly, I'm surprised it hasn't come up more (though I haven't done a recent search to check for similar issues).

Will get this rebased & merging cleanly and add a changelog update this week.

Owner

pixeltrix commented Nov 19, 2013

@chadmoone I'm 👎 on allowing rows with non-existing primary keys being created - seems like something that could have unintentional side effects. For example if due to network issues a destroy action and update action happen out of order, currently an error will be raised whereas if we were to change it then the row would be recreated.

Also there are security implications of allowing users to essentially pick their primary keys.

Contributor

chadmoone commented Nov 19, 2013

To me, this seems more like an argument against user-generated primary keys than an argument against supporting it in this specific case. As I mentioned above, the case of UUID often requires primary keys to be generated client-side.

Since UUID and client-generated primary keys are specifically supported in Active Record (see the relatively new UUID column type), it seems inconsistent to not support it here. It effectively makes using nested resources with UUID impossible without patching AR.

If it's felt that there are too many concerns to have this enabled by default, what about adding a flag to enable this behavior? We have an update_only flag already; we could add a create_or_update or some other flag to allow this. Previously, we could use the without_protection flag, but with the switch to strong_parameters, this option no longer exists.

It's been a while, but last I checked, to fully support nested UUID, #10271 would probably also need to be added. What little feedback I got there did seem to be positive, but both were created right around the 4.0 RC sprints, so probably got lost in the fray. I created them separately because they seemed like separate but related issues, but if it's easier to get them in, I can certainly combine them into a single PR. I would love to get the hacks out of our apps.

Thoughts?

Owner

pixeltrix commented Nov 19, 2013

@chadmoone I could live with an option that's disabled by default if you want to work up a PR on that basis. As for client side UUIDs being explicitly supported I don't now without digging into the commit history for the UUID column type - do you have a reference handy?

Member

sgrif commented Oct 30, 2015

Closing due to inactivity. I agree that this change should not be made by default. If you're interested in working on this feature as an opt-in option, let me know and I'll re-open.

@sgrif sgrif closed this Oct 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment