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

Explicitly specify email uniqueness validator as case insensitive #889

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

unixmonkey
Copy link
Contributor

Fixes deprecation warning from Rails 6.0.x versions:

DEPRECATION WARNING: Uniqueness validator will no longer enforce case
sensitive comparison in Rails 6.1. To continue case sensitive comparison
on the :email attribute in User model, pass case_sensitive: true
option explicitly to the uniqueness validator.

Since the case_sensitive option has been supported on Rails since 1.x, this should be completely safe, while also being explicit enough for Rails 6+ to not throw any deprecation warnings.

Fixes deprecation warning from Rails 6.0.x versions:

> DEPRECATION WARNING: Uniqueness validator will no longer enforce case
sensitive comparison in Rails 6.1. To continue case sensitive comparison
on the :email attribute in User model, pass `case_sensitive: true`
option explicitly to the uniqueness validator.
@mjankowski mjankowski merged commit 566cebf into thoughtbot:master Jun 19, 2020
@@ -143,7 +143,7 @@ module Validations
validates :email,
email: { strict_mode: true },
presence: true,
uniqueness: { allow_blank: true },
uniqueness: { allow_blank: true, case_sensitive: false },
Copy link
Contributor

Choose a reason for hiding this comment

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

@unixmonkey @mjankowski I believe this was supposed to be case_sensitive: true.

This actually changes the logic quite a bit, since it builds the queries on LOWER(email) instead of email. This change broke our app because we didn't have an index on lower(email), so our queries started timing out 😞

This was originally changed in 56f0f04.

Also, the deprecation warning said we should set this to true if want to keep the behavior:

To continue case sensitive comparison
on the :email attribute in User model, pass case_sensitive: true
option explicitly to the uniqueness validator.

This can make a big difference in performance and requires everyone to change their indexes, so it's not really a backwards compatible change. I'd suggest updating to case_sensitive: true and doing a bugfix release.

sidonath added a commit to 80pctsols/clearance that referenced this pull request Feb 3, 2021
In thoughtbot#889, `case_sensitive` setting of the `uniqueness` validator was set
to `false` to prevent deprecation warnings from Rails.

However, the setting should have been set to `true` to preserve
backwards-compatibility.

The deprecation warning said:

> To **continue** case sensitive comparison
> on the :email attribute in User model, pass case_sensitive: true
> option explicitly to the uniqueness validator.

Setting it to `false`, made ActiveRecord generate User queries on
`LOWER(email)` instead of on `email`. The apps that didn't have an index
on `LOWER(email)` would have faced performance issues.

This was mentioned in an old commit that originally made the switch from
`false` to the implicit `true`:
thoughtbot@56f0f04
MottiniMauro pushed a commit that referenced this pull request Feb 26, 2021
In #889, `case_sensitive` setting of the `uniqueness` validator was set
to `false` to prevent deprecation warnings from Rails.

However, the setting should have been set to `true` to preserve
backwards-compatibility.

The deprecation warning said:

> To **continue** case sensitive comparison
> on the :email attribute in User model, pass case_sensitive: true
> option explicitly to the uniqueness validator.

Setting it to `false`, made ActiveRecord generate User queries on
`LOWER(email)` instead of on `email`. The apps that didn't have an index
on `LOWER(email)` would have faced performance issues.

This was mentioned in an old commit that originally made the switch from
`false` to the implicit `true`:
56f0f04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants