Warn about changing query_constraints: behavior
#51571
Conversation
| ActiveRecord.deprecator.warn <<~MSG.squish | ||
| Setting `query_constraints:` option on `#{active_record}.#{macro} :#{name}` is deprecated | ||
| and its behavior may be modifed in Rails 8. | ||
| To preserve the current behavior please use the `foreign_key` option instead. |
There was a problem hiding this comment.
I'm not sure exactly why but the text here feels confusing. It's not immediately clear that foreign key is a replacement for query constraints. The implication that it "might" be different in the future isn't clear. Maybe something more like:
Setting `query_constraints:` option on `#{active_record}.#{macro} :#{name}` is deprecated.
To maintain current behavior, use the `foreign_key` option instead.There was a problem hiding this comment.
Yeah so I initially came up with a straightforward message like you suggested but then I tried to convey that query_constraints: is not going away, there will just be new behavior which may actually fit your application better but at the same time we can't promise anything until we release the actual new behavior.
So I'll apply your suggestion, it's better to be straightforward as foreign_key being a direct substitution is what we can guarantee at the moment.
Thanks!
| Setting `query_constraints:` option on `Sharded::BlogPost.has_many :qc_deprecated_comments` is deprecated | ||
| and its behavior may be modifed in Rails 8. | ||
| To preserve the current behavior please use the `foreign_key` option instead. | ||
| MSG |
There was a problem hiding this comment.
We don't enforce this style but generally prefer a new line between MSG and the assertion. It makes the test easier to read.
There was a problem hiding this comment.
100% agree. That was an overlook on my side, will change
activerecord/CHANGELOG.md
Outdated
| @@ -1,3 +1,10 @@ | |||
| * Warn about changing `query_constraints:` behavior | |||
There was a problem hiding this comment.
This should explain the change rather than the behavior.
| * Warn about changing `query_constraints:` behavior | |
| * In association option `query_constraints` is deprecated in favor of `foreign_key`. |
activerecord/CHANGELOG.md
Outdated
| * Warn about changing `query_constraints:` behavior | ||
|
|
||
| `query_constraints:` association will change behavior in the future versions of Rails | ||
| and applications are advised to switch to `foreign_key:` to preserve the current behavior. |
There was a problem hiding this comment.
I think it's confusing to say that behavior will change but not to what. Applications just need to know to switch, not that there's some other future change.
There was a problem hiding this comment.
Yeah I agree, since we can't promise anything concrete it's best to just ask applications to migrate to avoid any confusion in the future. I'll drop the "future versions" bit
nvasilevski
force-pushed
the
deprecate-explicit-query-constraints-in-favor-of-foreign-key
branch
2 times, most recently
from
4d6ca09
to
c96b4c7
Compare
This commit adds a deprecation warning for the `query_constraints:` association option. This option will change behavior in the future versions of Rails and applications are encouraged to switch to `foreign_key:` to preserve the current behavior.
nvasilevski
force-pushed
the
deprecate-explicit-query-constraints-in-favor-of-foreign-key
branch
from
c96b4c7
to
ad13455
Compare
This PR adds a deprecation warning for the
query_constraints:association option. This option will change behavior in the future versions of Rails and applications are encouraged to switch toforeign_key:to preserve the current behavior.Related to #49671 (comment)
Deprecation location
Currently Rails will issue a deprecation during boot but there is an option to move it and only warn on runtime when a reflection with
query_constraintsis used.I chose to put it in the
initializejust because it's the simplest available solution and moving it to runtime will require a bit of renaming.Let me know if you think there is a reason to have this deprecation issued in runtime or if we should have both.
Test models
To avoid having this deprecation in tests I migrated all models to
foreign_key:but the long term intention is forSharded::namespace models to use the new behavior ofquery_constrainstsandCpk::to keep usingforeign_keyAlso passing CI is an assurance that
foreign_keyis indeed a safe substitution forquery_constraints: