Automatically inverse_of with custom foreign keys

[composerinteralia]

Background

Along the same lines as 518b9a3, this commit allows us to automatically detect the inverse_of for associations with custom foreign_keys by searching through the reflections on the target class.

For GitHub, this adds inverse_of to 161 associations that didn't have it before (and a total of nearly 300 associations when combined with automatic_scope_inversing). Of those we had 4 associations that relied on the old behavior and so I added inverse_of: false to those for now.

Approach

With this commit we first try to find the association by name as before, but if that doesn't work we scan through the reflections on the target class and look for one that has matching options. To ensure we've really got a match we have to check just about everything—foreign_key, primary_key, class, macro, etc.

This commit reorders valid_inverse_reflection? a bit to ensure we filter out polymorphic reflections earlier (they can blow up when calling reflection.foreign_key), but other callers of the method shouldn't see any difference in behavior.

Since this is a breaking change (existing code may rely on a missing inverse_of causing fresh copies of a record to be loaded), the behavior is behind the automatic_foreign_key_inversing flag. It is set to true for new applications via framework defaults.

Alternatives Considered

I started off by of chopping off the "_id" part of the foreign key option to try and guess the inverse name, but that worked for fewer cases than this does.

Performance considerations:

Scanning through reflections to find valid ones could be inefficient on
classes with a large number of associations.

GitHub does have a couple classes with > 100 associations, however the vast majority have only a few (4-5 on average). Eagerly loading all of our inverses takes about 160ms of reflection scanning with this change, which I think is an acceptable tradeoff for us given the number of queries these inverses are likely to save us.

[rafaelfranca]

I'm not sure if the cost in complexity and boot time is worth here. Sure, we can create heuristics for all possible cases but do we need to? Now to load a model we would need to run all this complex code while a simple inverse_of: :something in your class would be sufficient and not make all rails applications to pay.

Maybe static analysis of those 161 associations would be better suited?

[composerinteralia]

Thanks for taking a look. If the added complexity is not something we want to take on, I can certainly understand that.

a simple inverse_of: :something in your class would be sufficient

It's simple to add once you know what it does. The problem that led me down this path is that a lot of Rails developers seem confused about when you need the option and what exactly it does. I've seen people writing extra code to do things that inverse_of would have already taken care of. So the goal here was essentially to give the benefit of inverses without requiring people to know much about it.

Maybe static analysis of those 161 associations would be better suited?

There is a RuboCop cop for inverse_of that I've also been exploring.
It's not perfect, but probably still worthwhile if we decide not to move forward with this change.

[bdewater]

FWIW #39506 also was an attempt at this.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.