Add configurable deprecation warning for singular associations #45344

HParker · 2022-06-13T19:39:16Z

This removes the singularize from where which runs on all expand_from_hash keys which might be reflections or column names. This saves a lot of time by avoiding singularizing column names.

Previously in #45163 the singularize was removed entirely. after some reflection, I think it is better to at least give a warning for one release since where is a very popular API and the problems you can run into with incorrect relation could be hard to debug.

Configurable with ActiveRecord::Base.allow_deprecated_singular_assocaitions_name = false / config.active_record.allow_deprecated_singular_assocaitions_name = false

cc: @byroot sorry for the flip flop. I was imaging I didn't know about this change and things just started acting different and felt concerned for that rails user.

byroot · 2022-06-13T19:43:39Z

activerecord/lib/active_record/core.rb

@@ -79,6 +79,8 @@ def self.configurations

      class_attribute :strict_loading_by_default, instance_accessor: false, default: false

+      class_attribute :strict_association_name, instance_writer: false, default: false


I think we could go with a single ActiveRecord.allow_deprecated_singular_assocaition_name (or whatever name you think is good, I didn't think much about it).

Surprised the rails spellchecker complains about association, but I like allow_deprecated_singular_assocaitions_name enough I went with it. Thanks!

Sorry it wasn't clear. Aside from the name, I was suggesting to make it an ActiveRecord singleton attribute:

module ActiveRecord singleton_class.attr_accessor :allow_deprecated_singular_assocaitions_name end

This means it's no longer inheritable (but the value here is limited), but you can directly check it where it's needed and don't have to pass it around as much. That should significantly cut down the diff.

byroot · 2022-06-14T17:07:06Z

A couple other things:

The setting need to be documented in guides/source/configuring.md
We need to add it to load_defaults
It need to be added to new_framework_defaults_7_1.rb.tt

HParker · 2022-06-15T17:22:20Z

Thanks @byroot, I have never done configuration variables so I am guessing some of these based on other PRs I was able to find. Please let me know if I got anything wrong.

HParker · 2022-06-15T17:23:29Z

activerecord/CHANGELOG.md

    record within a transaction.

-    When multiple Active Record instances change the same record within a 


gosh, I gotta disable my "cleanup whitespace" config.

byroot · 2022-06-16T07:21:49Z

activerecord/lib/active_record/core.rb

@@ -79,6 +79,8 @@ def self.configurations

      class_attribute :strict_loading_by_default, instance_accessor: false, default: false

+      class_attribute :allow_deprecated_singular_associations_name, instance_writer: false, default: true


I still think we should make this a global on ::ActiveRecord.

I agree configuring this per class seems like a really bad idea. Updated thanks!

byroot · 2022-06-16T07:22:10Z

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -44,3 +44,6 @@
 # For example, it is possible to create an index for a non existing column.
 # See https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted for more details.
 # Rails.application.config.active_record.sqlite3_adapter_strict_strings_by_default = true
+
+# Disable deprecated singular associations names
+# Rails.application.config.active_record.allow_deprecated_singular_associations_name = true


Need a newline :)

byroot · 2022-06-16T07:23:06Z

railties/lib/rails/application/configuration.rb

@@ -288,6 +289,12 @@ def load_defaults(target_version)
          if respond_to?(:active_record)
            active_record.sqlite3_adapter_strict_strings_by_default = true
          end
+        when "7.2"


This should be in 7.1 defaults, since 7.1 isn't out yet.

This removes the singularize from `where` which runs on all `expand_from_hash` keys which might be reflections or column names. This saves a lot of time by avoiding singularizing column names. Previously in rails#45163 the singularize was removed entirely. after some reflection, I think it is better to at least give a warning for one release since `where` is a very popular API and the problems you can run into with incorrect relation could be hard to debug. Configurable with `ActiveRecord::Base.allow_deprecated_singular_assocaitions_name = false` / `config.active_record.allow_deprecated_singular_assocaitions_name = false`

HParker · 2022-06-16T16:15:03Z

phew, thanks so much @byroot I know so much more about how configuration variables work 😅

byroot · 2022-06-16T16:26:02Z

Thanks!

…me [skip ci] The only time rails#45163 and rails#45344 have an effect is when the hash value passed to `where` is a model object. The current sample code does not change behavior between Rails 7.0 and 7.1

Follow-up to rails#45344. This tweaks the description, fleshes out the code example, and fixes the default value listed for 7.1.

Follow-up to rails#45344. When using `config.load_default "7.1"`, the value is `false`, so the value in `new_framework_defaults_7_1.rb` should be `false` as well. This commit also adds configuration tests.

Follow-up to rails#45344. Currently, when `allow_deprecated_singular_associations_name` is true, a deprecation warning like the following is displayed: ```console > Comment.belongs_to :post > Comment.where(posts: 1).count DEPRECATION WARNING: In Rails 7.2, referring to singular associations by their plural name will be deprecated. To continue querying `post` use `posts` instead. You can get the new more performant behavior now by setting config.active_record.allow_deprecated_singular_associations_name = false ``` The warning advises "use `posts` instead", but the user's code should actually use `post`, i.e. `Comment.where(post: 1).count`. The mismatch is likely due to the level at which the deprecation warning is emitted. `associated_with?` does indeed expect `table_name` to be `posts`, but `where` expects `post`. This commit changes the deprecation warning to avoid advising a specific usage, but still be explicit about what is wrong: ```console > Comment.where(posts: 1).count DEPRECATION WARNING: Referring to a singular association (e.g. `post`) by its plural name (e.g. `posts`) is deprecated. To convert this deprecation warning to an error and enable more performant behavior, set config.active_record.allow_deprecated_singular_associations_name = false. ```

rails-bot bot added the activerecord label Jun 13, 2022

HParker force-pushed the add-deprecation-warning-for-association-names branch from 44dbd92 to 944e083 Compare June 13, 2022 19:42

byroot reviewed Jun 13, 2022

View reviewed changes

HParker force-pushed the add-deprecation-warning-for-association-names branch 2 times, most recently from 251b57c to e97cc66 Compare June 13, 2022 20:07

HParker changed the title ~~Add configurable deprecation warning for singular association~~ Add configurable deprecation warning for singular associations Jun 13, 2022

HParker force-pushed the add-deprecation-warning-for-association-names branch from e97cc66 to 147676a Compare June 15, 2022 17:20

HParker commented Jun 15, 2022

View reviewed changes

HParker force-pushed the add-deprecation-warning-for-association-names branch from 147676a to 58937a8 Compare June 15, 2022 17:27

rails-bot bot added docs railties labels Jun 15, 2022

HParker force-pushed the add-deprecation-warning-for-association-names branch 4 times, most recently from 0c5a5e5 to 90b57b6 Compare June 15, 2022 23:38

byroot reviewed Jun 16, 2022

View reviewed changes

HParker force-pushed the add-deprecation-warning-for-association-names branch 4 times, most recently from 75444c5 to c947fb6 Compare June 16, 2022 16:11

HParker force-pushed the add-deprecation-warning-for-association-names branch from c947fb6 to 3a04c7b Compare June 16, 2022 16:14

byroot merged commit 1a37756 into rails:main Jun 16, 2022

HParker deleted the add-deprecation-warning-for-association-names branch June 16, 2022 20:10

willnet mentioned this pull request Jun 23, 2022

Fix wrong sample code about allow_deprecated_singular_associations_name [skip ci] #45438

Merged

jonathanhefner mentioned this pull request Jun 23, 2022

Improve allow_deprecated_singular_associations_name config doc [ci-skip] #45449

Merged

jonathanhefner mentioned this pull request Jun 23, 2022

Fix allow_deprecated_singular_associations_name framework default #45451

Merged

jonathanhefner mentioned this pull request Jun 23, 2022

Clarify allow_deprecated_singular_associations_name deprecation warning #45452

Merged

sambostock mentioned this pull request Jul 19, 2022

Fix BigDecimal (de)serialization for JSON adapters #45618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add configurable deprecation warning for singular associations #45344

Add configurable deprecation warning for singular associations #45344

Uh oh!

HParker commented Jun 13, 2022 •

edited

Loading

Uh oh!

byroot Jun 13, 2022

Uh oh!

HParker Jun 14, 2022

Uh oh!

byroot Jun 14, 2022

Uh oh!

byroot commented Jun 14, 2022

Uh oh!

HParker commented Jun 15, 2022

Uh oh!

HParker Jun 15, 2022

Uh oh!

byroot Jun 16, 2022

Uh oh!

HParker Jun 16, 2022

Uh oh!

byroot Jun 16, 2022

Uh oh!

byroot Jun 16, 2022

Uh oh!

HParker commented Jun 16, 2022

Uh oh!

byroot commented Jun 16, 2022

Uh oh!

Uh oh!

		@@ -79,6 +79,8 @@ def self.configurations

		class_attribute :strict_loading_by_default, instance_accessor: false, default: false

		class_attribute :strict_association_name, instance_writer: false, default: false

		record within a transaction.

		When multiple Active Record instances change the same record within a

		@@ -79,6 +79,8 @@ def self.configurations

		class_attribute :strict_loading_by_default, instance_accessor: false, default: false

		class_attribute :allow_deprecated_singular_associations_name, instance_writer: false, default: true

Add configurable deprecation warning for singular associations #45344

Add configurable deprecation warning for singular associations #45344

Uh oh!

Conversation

HParker commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byroot commented Jun 14, 2022

Uh oh!

HParker commented Jun 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HParker commented Jun 16, 2022

Uh oh!

byroot commented Jun 16, 2022

Uh oh!

Uh oh!

HParker commented Jun 13, 2022 •

edited

Loading