Respect custom primary keys for nested attributes

[chadmoone]

ActiveRecord allows us to specify custom primary keys, but does not properly accept them for nested attributes.

For example, with the models:

class Deck < ActiveRecord::Base
  has_many :cards, autosave: true
  accepts_nested_attributes_for :cards, allow_destroy: true
end

class Card < ActiveRecord::Base
  self.primary_key = "uuid"
  belongs_to :deck
end

The primary key stored in the database (and presumably other related applications) as uuid, and the object is serialized as such:

my_deck.cards.inspect
# => [{"uuid":"abcd","content":"Hello","deck_id":14,"created_at":"2013-04-17T22:05:09.623Z","updated_at":"2013-04-17T22:05:09.679Z"},{"uuid":"efgh","content":"World!","deck_id":14,"created_at":"2013-04-17T22:05:09.626Z","updated_at":"2013-04-17T22:05:09.681Z"}]

my_deck.cards.to_json
#=> [{"uuid":"abcd","content":"Hello","deck_id":14,"created_at":"2013-04-17T22:05:09.623Z","updated_at":"2013-04-17T22:05:09.679Z"},{"uuid":"efgh","content":"World!","deck_id":14,"created_at":"2013-04-17T22:05:09.626Z","updated_at":"2013-04-17T22:05:09.681Z"}]

To create a new instance of the model itself:

my_deck.cards.create(uuid:"abcdefg", content: "Sup?")

However, using the correct primary key in an update to nested attributes will fail in multiple ways:

# Can't destroy nested objects
my_deck.update([{uuid:"abcd",content:"Hello", _destroy: true}},
                {uuid:"efgh",content:"World!", _destroy: true}])
my_deck.cards.inspect
#=> [{"uuid":"abcd","content":"Hello","deck_id":14,"created_at":"2013-04-17T22:05:09.623Z","updated_at":"2013-04-17T22:05:09.679Z"},{"uuid":"efgh","content":"World!","deck_id":14,"created_at":"2013-04-17T22:05:09.626Z","updated_at":"2013-04-17T22:05:09.681Z"}]

# Attempting to modify an object creates a new one,
# with the same primary key value (which will fail)
my_deck.update([{uuid:"abcd",content:"Something"}},
                {uuid:"efgh",content:"Else!"}])
#=> Throws ActiveRecord::RecordNotUnique error

Instead, we currently need to set up a hash like so:

my_deck.update([{id:"abcd",content:"All alone :("}},
                {id:"efgh",content:"Else!", _destroy: true}])
my_deck.cards.inspect
#=> [{"uuid":"abcd","content":"All alone :(","deck_id":14,"created_at":"2013-04-17T22:05:09.623Z","updated_at":"2013-04-17T22:05:09.679Z"}]

Forcing the use of the arbitrary key id doesn't seem to make a lot of sense, and it forces anything interacting with Rails (and sometimes even Rails itself) to specially modify the object hash to use an otherwise unknown key.

The only tests that I needed to modify were two that specifically deal with custom primary keys in nested attributes (as these were verifying the old behavior), and all other tests pass.

Please let me know if you have any issues or if you think there is a better way to go about this.

[steveklabnik]

lol, sorry about that. re-opening.

[codeodor]

👍 Nice find + fix. Not that I have any say in whether this gets accepted, but I'll offer a couple of changes I would make:

1. The contribution guidelines mention something about not having whitespace on blank lines, so you might remove it from https://github.com/rails/rails/pull/10271/files#L0R411
2. Instead of changing the tests to use pet_id, I'd keep them using id and add tests like test_nonstandard_primary_key_not_named_id or something to that effect.

For item 2, I'm just assuming the fixture has the column named id, but is a nonstandard data type? If that's true, I'd (personally) like to keep a test in place for that case as well, to catch regressions. (Even though the current test as you've changed it currently does, it might not be that way forever).

Like I said though, I don't get to decide these things, it's just what I'd like to see. 😄

Great work!

[egilburg]

wouldn't record.id also become record.<primary_key>?

[chadmoone]

We could do that, but it's always safe to use id on the record, because ActiveRecord::AttributeMethods::PrimaryKey defines the id method to always read/write the primary key, and then actually aliases a method with the primary key name to the id method. Obviously, this means that if we use a custom primary key name, we can't use an id column name at all, but that's a separate issue.

[chadmoone]

@codeodor

1. Thanks for pointing that out, I must have missed that in the contributions guide. I've removed the whitespace and amended the commit.
2. Actually, these two tests are using the Pet model, which uses a standard integer data type. They were testing the old behavior, which always expects the primary key for nested attributes hashes to be id—regardless of what it actually is in the model. I modified it to now test that Rails actually checks the model for the primary key name and looks for that key in the hash. I used the uuid example in the description, because to me it seemed like a very common use case (also what led me to the issue). The examples above do work with this change, but I didn't add any tests here because I assumed that non-integer data types for primary keys was already covered somewhere else (looks like it's here), and that it probably doesn't belong in the nested attributes test suite. That said, I can easily add a case for that if people think we need it here.

Thanks for the feedback, guys, hope my responses make sense 😄

[pixeltrix]

Do we still need this to_sym - I think the attributes hash is pretty much guaranteed to be using string keys

[pixeltrix]

Sorry, we do - it's the user supplied hash. Got confused there for a minute 😄

[pixeltrix]

@chadmoone why doesn't the id= method handle setting the correct primary key column?

[chadmoone]

@pixeltrix id= does set the correct primary key column, but the problem here was that it was hard-coded to look for an existing record based on a key with the value'id'.

So if the nested attribute hash does not include a key id, but instead includes a key uuid (or whatever the primary key actually is), no attempt is made to see if that record already exists—it assumes that it must be a new record. But when it tries to set that uuid value and save the record, it will fail, because the record does exist. What I'm trying to do here is make sure we check the hash for a key based on the model's primary key, rather than always assuming id.

I think that answers your question, but let me know if I'm missing it.

[pixeltrix]

@chadmoone thanks, understand now.

[chadmoone]

Last update there was just clarifying a bit in the documentation, but went ahead and squashed.