Add composite primary key validity check on belongs_to, has_one, and has_many associations.

[gmcgibbon]

Motivation / Background

This Pull Request has been created because belongs_to associations with composite primary keys are broken.

Detail

Raises ActiveRecord::CompositePrimaryKeyMismatchError when belongs_to foreign key and primary key don't have the same length.

Additional information

This fixes an issue where autosave fails to update foreign key attributes here:

rails/activerecord/lib/active_record/autosave_association.rb

Lines 511 to 515 in ced5e77

	primary_key_foreign_key_pairs = primary_key.zip(foreign_key)
	primary_key_foreign_key_pairs.each do |primary_key, foreign_key|
	association_id = record._read_attribute(primary_key)
	self[foreign_key] = association_id unless self[foreign_key] == association_id
	end

I'm not sure if my fix is the best way to fix this problem, there might be an easier way around this than introducing an error and additional validation check. From my perspective, if a model has a composite primary key, its associations need query constraints to spell out the foreign key columns Active Record should use. Let me know what you think!

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[nvasilevski]

This is going to be very handy to avoid ambiguous error messages when defining associations from/to a model with a composite primary key. Thanks!

I have a suspicion that the validation check can eventually be moved to the parent class so we check the condition for any type of the association. However I'm okay if we start from belongs_to and consider expanding it once we have another failing test-case

[nvasilevski]

The validation is true for any type of the model so klass.composite_primary_key? check is not necessary from the design point of view. However we can kind of use it to prevent unnecessary arrays allocation for non-cpk models. So it's up to you too keep it

Suggested change

	if !polymorphic? && klass.composite_primary_key? && Array(association_primary_key).length != Array(foreign_key).length
	if !polymorphic? && Array(association_primary_key).length != Array(foreign_key).length

If we are going to keep it I think we designed association_primary_key and foreign_key to be an array if the klass is a cpk model, so potentially we should be able to do:

Suggested change

	if !polymorphic? && klass.composite_primary_key? && Array(association_primary_key).length != Array(foreign_key).length
	if !polymorphic? && klass.composite_primary_key? && association_primary_key.length != foreign_key.length

though I could be missing something

[gmcgibbon]

I believe removing the composite PK check makes the tests for query_constraints that use models under Sharded:: fail. In this case, query_constraints don't necessarily map to FKs AFAICT.

[nvasilevski]

This is going to be very handy to avoid ambiguous error messages when defining associations from/to a model with a composite primary key. Thanks!

I have a suspicion that the validation check can eventually be moved to the parent class so we check the condition for any type of the association. However I'm okay if we start from belongs_to and consider expanding it once we have another failing test-case

[eileencodes]

This doesn't appear to be necessary, where is this used?

[gmcgibbon]

It isn't, but a lot of inverse and association validity errors seem to have that accessor and ivar for some reason. I'll remove it 👍