Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify allow_deprecated_singular_associations_name deprecation warning #45452

Conversation

jonathanhefner
Copy link
Member

Follow-up to #45344.

Currently, when allow_deprecated_singular_associations_name is true, a deprecation warning like the following is displayed:

> 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:

> 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.

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.
```
@byroot byroot requested a review from HParker June 25, 2022 07:06
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I cc-ed @HParker just in case.

@HParker
Copy link
Contributor

HParker commented Jun 25, 2022

Great catch thank you! 🙇
Reflection name should actually be more correct in more cases too like when the reflection and table name do not match. I like this change a lot 👍

@byroot byroot merged commit 4106da5 into rails:main Jun 25, 2022
In Rails 7.2, referring to singular associations by their plural name will be deprecated.
To continue querying `#{table_name.singularize}` use `#{table_name}` instead.
You can get the new more performant behavior now by setting config.active_record.allow_deprecated_singular_associations_name = false
Referring to a singular association (e.g. `#{reflection.name}`) by its plural name (e.g. `#{reflection.plural_name}`) is deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the reflection.plural_name here be the table name since that is what they passed in?

I am worried about a case where you pass in the plural table name and we instead say you can't pass the plural reflection name which seems confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think if you did that and we found the reflection from the singular name, it should be the same unless it is a inflection difference, which I don't think we should need to worry about here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants