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

Fixes error in parsing parameters passed to a model with associations #14264

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

Alwahsh commented Mar 3, 2014

When trying to create a record of a model that has_many of another
model and passing the parameters of the second model record as a
hash, it used to raise an exception. This fixes that behaviour and
allows passing of a second model record parameters to the parameters
used to create the first model.
This fixes #14249

[Ahmed AbouElhamayed]

@Alwahsh Alwahsh Fixes error in parsing parameters passed to a model with associations
When trying to create a record of a model that has_many of another
model and passing the parameters of the second model record as a
hash, it used to raise an exception. This fixes that behaviour and
allows passing of a second model record parameters to the parameters
used to create the first model.

[Ahmed AbouElhamayed]
c75d65d
Contributor

al2o3cr commented Mar 4, 2014

The resulting records from this approach are in a really weird state - id won't be set, and new_record? will return true:

[1] pry(main)> Person.new(id: 2, name: 'wat')
=> #<Person id: nil, name: "wat", office_id: nil, email: nil, notes: nil, created_at: nil, updated_at: nil, start_date: nil, end_date: nil, user_id: nil, github: nil, twitter: nil, website: nil, title: nil, bio: nil, avatar_file_name: nil, avatar_content_type: nil, avatar_file_size: nil, avatar_updated_at: nil, deleted_at: nil, role: nil, percent_billable: 100>

You may want to investigate how the other cache stores roundtrip objects (ActiveRecord::Base#init_with seems to be a good place to start) - just pushing in attributes from new doesn't seem like a good solution.

Contributor

Alwahsh commented Mar 4, 2014

@al2o3cr I've tried to do:

Post.new(id: 2, name: "wat")

This returned:

#<Post id: 2, name: "wat">

and shouldn't new_record? return true since you've created that using Person.new ?
This is the behaviour I get using the master branch too.

Contributor

Alwahsh commented Mar 5, 2014

Note: The error(before applying the fix) still exists even if we add accepts_nested_attributes_for. I wonder if we should better raise an exception in case of no accepts_nested_attributes_for telling the developer that he might have missed it and in case of having that set, make it work as this fix suggests.

Contributor

al2o3cr commented Mar 7, 2014

@Alwahsh - I'd forgotten I was using the protected_attributes gem in the Rails 4 app where I was testing. That explains the difference in id behavior.

accepts_nested_attributes_for is not relevant to this, as that expects incoming values in an attribute named after the association with _attributes appended - NOT the actual association name.

Having a record with new_record? true but with an ID will cause errors. For instance, attempting to save the Post above (assuming the corresponding record with id = 2 already exists in the DB) will produce an error like this:

ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "posts_pkey"
DETAIL:  Key (id)=(2) already exists.

I'm -1 on this feature because of this - it produces broken ActiveRecord objects.

Contributor

Alwahsh commented Mar 7, 2014

@al2o3cr yeah, you're right about the accepts_nested_attributes_for.
As for the issue of saving. I noticed that but notice that if the model didn't have any associations, it'd be created with all attributes set correctly and will also fail to save if the ID is not changed. I see this as a bugfix rather than a new feature.
One use of that might be to copy some objects from a DB to another one of the same structure using some JSON API.

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