search

composite-primary-keys / composite_primary_keys

Composite Primary Keys support for Active Record
1.03k stars 350 forks source link

Fix setting parent foreign key on child associations #556

Closed sholden closed 3 years ago

sholden commented 3 years ago

In Rails 6.1, AR calls _has_attribute? on a record when checking to see if it can set an inverse relationship on save. This method doesn't convert to string before checking, so #foreign_key needs to return a string (or in the case of CPK, an array of strings). I've implemented the tests for postgres, but would need someone who has access to the other databases to check against them (and add the small table migration).

codeodor commented 3 years ago

This may have been related to one issue I remember when originally working on the 6.1 branch, where I was looking for it not converting to a string, and couldn't seem to find it.

That said, it feels a little much. Is there a way to actually change how the keys are stored as strings, rather than change all this other code?

For example, if calling self.primary_keys = [:key1, :key2] on our models just converted them to strings, would this work without as much code?

sholden commented 3 years ago

self.primary_keys= already converts the keys to strings.

https://github.com/composite-primary-keys/composite_primary_keys/blob/master/lib/composite_primary_keys/base.rb#L17

The problem occurs when you explicitly specify the foreign key on an association as a symbol or array of symbols. If the foreign key is derived, it will be a string. Before the CPK patch, options[:foreign_key] would be converted to string before memoization as well. This patch just brings CPK behavior to be consistent with default.

codeodor commented 3 years ago

Seems good to me, but can you remove the new Email model/table in the tests and use one of the existing ones so we can get all the CI DBs working?

Or, is there a reason one of the existing relationships wouldn't work?

sholden commented 3 years ago

I think an existing one should work, but I added this one to recreate the specific bug I was seeing while upgrading to 6.1 (my table was specifying a single foreign key). I didn't dig too deep into the existing schema, but I think the test should fail before and pass after with composite keys as well. Might not be a bad idea to add a test for both. Do you have a model/association in mind?

I had added the other db changes last night as well, but forgot to push. Whoops.

cfis commented 3 years ago

@sholden - Thanks for the pull request, really nice job with including tests. These look like good changes.

I do agree with @codeodor that I'd rather not add another table to the test suite - I have in fact been trying to remove them.

Since I couldn't find a clean way to merge this pull request, instead I took the relevant pieces and created a new commit (with some editorializations) which I just pushed and credited you in the commit notes. I then added tests for the has_attribute and _has_atttribute methods. I tried adding tests for inverse, but I couldn't replicate a failing example. However, that code is called plenty of times by all the other tests so it is tested indirectly.