Emit deprecation warning about inverse_of inference only for valid reflections #51442
Conversation
…flections That is, if the reflection found is not valid, we don't want to emit the warning because it's misleading. It says the inverse association could have been automatically inferred but wasn't because `automatically_invert_plural_associations` is disabled. However, this is not true, because later on, when we check whether the reflection is valid, we see it's not, and end up returning `nil`.
| @@ -708,7 +708,7 @@ def automatic_inverse_of | |||
| plural_inverse_name = ActiveSupport::Inflector.pluralize(inverse_name) | |||
| reflection = klass._reflect_on_association(plural_inverse_name) | |||
|
|
|||
| if reflection && !active_record.automatically_invert_plural_associations | |||
| if valid_inverse_reflection?(reflection) && !active_record.automatically_invert_plural_associations | |||
There was a problem hiding this comment.
valid_inverse_reflection calls .name on its argument, but given this branch had a truthiness check, I assume that could blow up if reflection is falsey.
We should probably either keep the truthiness check first, or use &.name in valid_inverse_reflection?.
It would also be good to have test coverage for this, to be sure it behaves as expected.
There was a problem hiding this comment.
Thanks for reviewing! 🙏🏻
valid_inverse_reflectioncalls.nameon its argument, but given this branch had a truthiness check, I assume that could blow up if reflection is falsey.We should probably either keep the truthiness check first, or use
&.nameinvalid_inverse_reflection?.
valid_inverse_reflection? has a truthiness check first thing:
rails/activerecord/lib/active_record/reflection.rb
Lines 741 to 742 in 8e46af8
That's why I just replaced the existing truthiness check, as I mentioned in the PR description, in the Detail section.
It would also be good to have test coverage for this, to be sure it behaves as expected.
The original PR that introduced this new configuration flag and deprecation warning didn't add any tests, and this is a fairly tiny change over it 🤔
Motivation / Background
This is a small follow-up to #50883.
As we went through all deprecation warnings while we run Rails edge with
we realised that we had warnings for an inferred reflection that would be wrong to infer. Digging deeper into whether this meant that enabling
automatically_invert_plural_associationswould actually mean we'd have completely wrong values in the collection of the inferred association, we realised that no, the reflection was correctly not inferred. However, the warning was emitted regardless.This happens because the deprecation warning is emitted without checking whether the found
reflectionis valid. The warning says that the inverse association could have been automatically inferred but wasn't becauseautomatically_invert_plural_associationsis disabled. However, this is not true, because later on, when we check whether the reflection is valid, we see it's not, and end up returningnil.Detail
This pull request just adds the
valid_inverse_reflection?(reflection)condition to determine whether the deprecation warning needs to happen. That already checks thatreflectionis truthy.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]