Change belongs_to foreign key convention to align with ActiveRecord

[rferg]

Currently, the foreign key method name for a belongs_to association is inferred from the associated model name (see here). This differs from the ActiveRecord convention, which is to append _id to the association name.

For example:

belongs_to :client, model: Contact

Currently, ActiveForce would assume that the foreign key field is contact_id. By the ActiveRecord convention, it would be client_id.

It is relatively common in our SF schema to have foreign key fields with names that differ from the tables they reference. In fact, this is required on several tables because they have multiple references to the same table, representing different relationships. The current ActiveForce behavior, however, requires explicitly providing :foreign_key in all of these cases (hence all of the Rails/RedundantForeignKey Rubocop violations).

The scenario where the current convention might be preferable to the ActiveRecord one seems rare or non-existent (but this might be just my limited imagination). This would be a case where the association name and model name differ, but the actual foreign key field is model name + _id. It seems like normally this would just be an unnecessarily confusing association name.

For example, assume that in the :client association above, the real foreign key field is contact_id. This violate expectations when trying to access the foreign key field directly. To get the associated record you would call object.client, but to get the foreign key you would call object.contact_id. Since we can use whatever association name we want, it would make a lot more sense to just call it contact. But this just gives the same results as the ActiveRecord convention.

Required changes

If we don't have to consider backwards compatibility, the change would be straightforward. Without checking thoroughly, I think it would be a simple as changing the current implementation of BelongsToAssociation#default_foreign_key from this:

def default_foreign_key
  infer_foreign_key_from_model relation_model
end

To this (using the included ActiveSupport String#foreign_key method):

def default_foreign_key
  relation_name.to_s.foreign_key.to_sym
end

However, this would break any code that relies on the current convention, like the case where the association name is client but the foreign key is contact_id. Although, I'm not sure if any of those exists.

To help mitigate this, we could check if client_id exists on the model and then fallback to the model-derived foreign key if it doesn't. But this would break if there happened to also be a different field on the model called client_id. This is possible, although seems like a far out case. But I'm not sure of a way to guarantee compatibility with it, so that might be sufficient to make this a breaking change.

[JeffLuckett]

I think it's definitely a good thing to follow AR conventions as closely as possible, so I agree that this change should be implemented. Given that this is a divergence from AR conventions, I think we could almost consider this a bug.

It is indeed a breaking change, as anybody relying on the current functionality will be in for a surprise when we change it, so let's make sure this gets a minor version bump, and a callout in the release notes.