v4.5.0 breaks Composite Primary Keys

[natematykiewicz]

We use composite primary keys. I believe this PR has caused an incompatibility #1376.

Spec

it do
  is_expected.to belong_to(:vehicle).
    with_foreign_key(%i[company_id vehicle_id]).
    inverse_of(:vehicles_options)
end

Model

class VehiclesOption
  belongs_to :vehicle,
    foreign_key: %i[company_id vehicle_id],
    inverse_of: :vehicles_options
end

With v4.4.1 this spec would succeed.
With v4.5.0 it fails with:

Expected VehiclesOption to have a belongs_to association called vehicle (VehiclesOption does not have a [:company_id, :vehicle_id] foreign key.)

As a test of v4.4.1 I tried removing the foreign key from the model, and also changing one of the values of the array:
Ex:

belongs_to :vehicle,
  inverse_of: :vehicles_options

belongs_to :vehicle,
  foreign_key: %i[company_id foo_id],
  inverse_of: :vehicles_options

Both of these fail with:

Expected VehiclesOption to have a belongs_to association called vehicle ()

This is the error message that #1376 aimed to fix. This also shows me that the foreign key check was indeed working (I was worried I had a false positive before).

Looking at the diff, I'm not sure why this isn't working anymore.

[vsppedro]

Thank you for all the details. I'll take a look at that as soon as possible.

[natematykiewicz]

@vsppedro I think I see the problem. With v4.4.1, if we were providing a :foreign_key option, it would only run the option_verifier.correct_for_string?. The result of that determined whether the test passes.

With v4.5.0 it runs options.key?(:foreign_key) && !foreign_key_correct?. Since we have a options.key?(:foreign_key), it then checks foreign_key_correct?. Since foreign_key_correct? returns true (all is well), it then carries on to the elsif.

The column_names_for(klass).include?(foreign_key) part never ran in v4.4.1 when options.key?(:foreign_key) was provided.

In my case foreign_key is actually an array. It sort of needs to do:

Array(foreign_key).all? { |f| column_names_for(klass).include?(f) }

This is weird for everyone who doesn't use Composite Primary Keys, since foreign_key typically is a symbol, but I think it'd fix the problem.

I'm unsure what @missing = foreign_key_failure_message(klass, foreign_key) does when foreign_key is an Array, though.

[vsppedro]

@natematykiewicz, nice!

We need to add tests to prevent something like this from happening again.

I'm unsure what @missing = foreign_key_failure_message(klass, foreign_key) does when foreign_key is an Array, though.

I think we only need to adapt the error message when it fails because more than one foreing_key is missing or incorrect.

This is weird for everyone who doesn't use Composite Primary Keys, since foreign_key typically is a symbol, but I think it'd fix the problem.

I agree and to be honest, I'm not sure if the support for this gem was added before, I couldn't find any tests for it or even any reference in the Changelog. But as it was working and I ended up removing this feature I intend to add some tests and return the behavior.

Thank you very much for your help!

[vsppedro]

I think we can close this.

Thanks again for your contribution!

[natematykiewicz]

Sounds good to me. No problem!