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

Deprecate mismatched collation comparison for uniquness validator #35350

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented Feb 21, 2019

In MySQL, the default collation is case insensitive. Since the
uniqueness validator enforces case sensitive comparison by default, it
frequently causes mismatched collation issues (performance, weird
behavior, etc) to MySQL users.

https://grosser.it/2009/12/11/validates_uniqness_of-mysql-slow/
#1399
#13465
gitlabhq/gitlabhq@c1dddf8
huginn/huginn#1330 (comment)

I'd like to deprecate the implicit default enforcing since I frequently
experienced the problems in code reviews.

e.g.

Note that this change has no effect to sqlite3, postgresql, and
oracle-enhanced adapters which are implemented as case sensitive by
default, only affect to mysql2 adapter (I can take a work if sqlserver
adapter will support Rails 6.0).

As far as I can see, Basecamp, Shopify, and GitHub are using MySQL (or MariaDB).
Can we make this deprecated in Rails 6.0?

cc @jeremy @rafaelfranca @tenderlove

In MySQL, the default collation is case insensitive. Since the
uniqueness validator enforces case sensitive comparison by default, it
frequently causes mismatched collation issues (performance, weird
behavior, etc) to MySQL users.

https://grosser.it/2009/12/11/validates_uniqness_of-mysql-slow/
rails#1399
rails#13465
gitlabhq/gitlabhq@c1dddf8
huginn/huginn#1330 (comment)

I'd like to deprecate the implicit default enforcing since I frequently
experienced the problems in code reviews.

Note that this change has no effect to sqlite3, postgresql, and
oracle-enhanced adapters which are implemented as case sensitive by
default, only affect to mysql2 adapter (I can take a work if sqlserver
adapter will support Rails 6.0).
@kamipo kamipo force-pushed the deprecate_validate_uniqueness_mismatched_collation branch from 4c5a18d to 9def053 Mar 4, 2019

if current_adapter?(:Mysql2Adapter)
assert_equal 2, Topic.where(author_email_address: "david@loudthinking.com").count
assert_equal 2, Topic.where(author_email_address: "David@loudthinking.com").count
Copy link
Member Author

@kamipo kamipo Mar 4, 2019

That happening this (break the uniqueness contract) easily without race condition is what I want to prevent, which frequently experienced to me as MySQL DBA.

@kamipo
Copy link
Member Author

@kamipo kamipo commented Mar 4, 2019

I'm going to merge this, since the case sensitive comparison on the case insensitive column is potentially problematic, the enforcing implicitly would causes any problems silently without being noticed by MySQL users.

If the deprecation message would be seen on the apps using MySQL, I strongly recommend to confirm the query execution plan and data integrity on the database, and to reconsider that the case sensitive comparison on the case insensitive column is whether or not what you want.

@kamipo kamipo merged commit 467ae9c into rails:master Mar 4, 2019
2 of 3 checks passed
@kamipo kamipo deleted the deprecate_validate_uniqueness_mismatched_collation branch Mar 4, 2019
suketa added a commit to suketa/rails_sandbox that referenced this issue Sep 1, 2019
Deprecate mismatched collation comparison for uniquness validator
rails/rails#35350
suketa added a commit to suketa/rails_sandbox that referenced this issue Sep 1, 2019
Deprecate mismatched collation comparison for uniquness validator
rails/rails#35350
joevandyk
Copy link
Contributor

joevandyk commented on 9def053 Sep 10, 2019

I'm upgrading a mysql Rails app from 5.2 to 6.0 now. I'm using the utf8mb4_unicode_ci collation.

I'm using the uniqueness validator in places, like this, and I'm getting a lot of deprecation warnings when records are saved.

  validates :sid, uniqueness: true

To avoid the deprecation messages and to ensure the app works in 6.1, will I need to change all those to this?

  validates :sid, uniqueness: true, case_sensitive: false
drujensen
Copy link

drujensen commented on 9def053 Sep 26, 2019

@joevandyk you need to pass the parameter to the uniqueness validator. validates :sid, uniqueness: {case_sensitive: false} seems to work.

flanger001
Copy link
Contributor

flanger001 commented on 9def053 Jun 2, 2020

@drujensen Thank you for this. I was running into problems with this and your comment helped a lot!

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

Successfully merging this pull request may close these issues.

None yet

4 participants