Skip to content

Commit

Permalink
Deprecate mismatched collation comparison for uniquness validator
Browse files Browse the repository at this point in the history
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.

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).
  • Loading branch information
kamipo committed Mar 4, 2019
1 parent cbedbde commit 9def053
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 12 deletions.
8 changes: 8 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Original file line Diff line number Diff line change
@@ -1,3 +1,11 @@
* Deprecate mismatched collation comparison for uniquness validator.

Uniqueness validator will no longer enforce case sensitive comparison in Rails 6.1.
To continue case sensitive comparison on the case insensitive column,
pass `case_sensitive: true` option explicitly to the uniqueness validator.

*Ryuta Kamizono*

* Add `reselect` method. This is a short-hand for `unscope(:select).select(fields)`. * Add `reselect` method. This is a short-hand for `unscope(:select).select(fields)`.


Fixes #27340. Fixes #27340.
Expand Down
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ def raw_connection
@connection @connection
end end


def default_uniqueness_comparison(attribute, value) # :nodoc: def default_uniqueness_comparison(attribute, value, klass) # :nodoc:
case_sensitive_comparison(attribute, value) case_sensitive_comparison(attribute, value)
end end


Expand Down
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -453,6 +453,20 @@ def primary_keys(table_name) # :nodoc:
SQL SQL
end end


def default_uniqueness_comparison(attribute, value, klass) # :nodoc:
column = column_for_attribute(attribute)

if column.collation && !column.case_sensitive?
ActiveSupport::Deprecation.warn(<<~MSG.squish)
Uniqueness validator will no longer enforce case sensitive comparison in Rails 6.1.
To continue case sensitive comparison on the :#{attribute.name} attribute in #{klass} model,
pass `case_sensitive: true` option explicitly to the uniqueness validator.
MSG
end

super
end

def case_sensitive_comparison(attribute, value) # :nodoc: def case_sensitive_comparison(attribute, value) # :nodoc:
column = column_for_attribute(attribute) column = column_for_attribute(attribute)


Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/validations/uniqueness.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def build_relation(klass, attribute, value)
if bind.nil? if bind.nil?
attr.eq(bind) attr.eq(bind)
elsif !options.key?(:case_sensitive) elsif !options.key?(:case_sensitive)
klass.connection.default_uniqueness_comparison(attr, bind) klass.connection.default_uniqueness_comparison(attr, bind, klass)
elsif options[:case_sensitive] elsif options[:case_sensitive]
klass.connection.case_sensitive_comparison(attr, bind) klass.connection.case_sensitive_comparison(attr, bind)
else else
Expand Down
45 changes: 45 additions & 0 deletions activerecord/test/cases/validations/uniqueness_validation_test.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -314,6 +314,51 @@ def test_validate_case_insensitive_uniqueness_with_special_sql_like_chars
assert t3.save, "Should save t3 as unique" assert t3.save, "Should save t3 as unique"
end end


if current_adapter?(:Mysql2Adapter)
def test_deprecate_validate_uniqueness_mismatched_collation
Topic.validates_uniqueness_of(:author_email_address)

topic1 = Topic.new(author_email_address: "david@loudthinking.com")
topic2 = Topic.new(author_email_address: "David@loudthinking.com")

assert_equal 1, Topic.where(author_email_address: "david@loudthinking.com").count

assert_deprecated do
assert_not topic1.valid?
assert_not topic1.save
assert topic2.valid?
assert topic2.save
end

assert_equal 2, Topic.where(author_email_address: "david@loudthinking.com").count
assert_equal 2, Topic.where(author_email_address: "David@loudthinking.com").count
end
end

def test_validate_case_sensitive_uniqueness_by_default
Topic.validates_uniqueness_of(:author_email_address)

topic1 = Topic.new(author_email_address: "david@loudthinking.com")
topic2 = Topic.new(author_email_address: "David@loudthinking.com")

assert_equal 1, Topic.where(author_email_address: "david@loudthinking.com").count

ActiveSupport::Deprecation.silence do
assert_not topic1.valid?
assert_not topic1.save
assert topic2.valid?
assert topic2.save
end

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
else
assert_equal 1, Topic.where(author_email_address: "david@loudthinking.com").count
assert_equal 1, Topic.where(author_email_address: "David@loudthinking.com").count
end
end

def test_validate_case_sensitive_uniqueness def test_validate_case_sensitive_uniqueness
Topic.validates_uniqueness_of(:title, case_sensitive: true, allow_nil: true) Topic.validates_uniqueness_of(:title, case_sensitive: true, allow_nil: true)


Expand Down
27 changes: 17 additions & 10 deletions activerecord/test/schema/schema.rb
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
# # # #
# ------------------------------------------------------------------- # # ------------------------------------------------------------------- #


case_sensitive_options =
if current_adapter?(:Mysql2Adapter)
{ collation: "utf8mb4_bin" }
else
{}
end

create_table :accounts, force: true do |t| create_table :accounts, force: true do |t|
t.references :firm, index: false t.references :firm, index: false
t.string :firm_name t.string :firm_name
Expand Down Expand Up @@ -266,7 +273,7 @@
end end


create_table :dashboards, force: true, id: false do |t| create_table :dashboards, force: true, id: false do |t|
t.string :dashboard_id t.string :dashboard_id, **case_sensitive_options
t.string :name t.string :name
end end


Expand Down Expand Up @@ -330,15 +337,15 @@
end end


create_table :essays, force: true do |t| create_table :essays, force: true do |t|
t.string :name t.string :name, **case_sensitive_options
t.string :writer_id t.string :writer_id
t.string :writer_type t.string :writer_type
t.string :category_id t.string :category_id
t.string :author_id t.string :author_id
end end


create_table :events, force: true do |t| create_table :events, force: true do |t|
t.string :title, limit: 5 t.string :title, limit: 5, **case_sensitive_options
end end


create_table :eyes, force: true do |t| create_table :eyes, force: true do |t|
Expand Down Expand Up @@ -380,16 +387,16 @@
end end


create_table :guids, force: true do |t| create_table :guids, force: true do |t|
t.column :key, :string t.column :key, :string, **case_sensitive_options
end end


create_table :guitars, force: true do |t| create_table :guitars, force: true do |t|
t.string :color t.string :color
end end


create_table :inept_wizards, force: true do |t| create_table :inept_wizards, force: true do |t|
t.column :name, :string, null: false t.column :name, :string, null: false, **case_sensitive_options
t.column :city, :string, null: false t.column :city, :string, null: false, **case_sensitive_options
t.column :type, :string t.column :type, :string
end end


Expand Down Expand Up @@ -876,8 +883,8 @@
end end


create_table :topics, force: true do |t| create_table :topics, force: true do |t|
t.string :title, limit: 250 t.string :title, limit: 250, **case_sensitive_options
t.string :author_name t.string :author_name, **case_sensitive_options
t.string :author_email_address t.string :author_email_address
if subsecond_precision_supported? if subsecond_precision_supported?
t.datetime :written_on, precision: 6 t.datetime :written_on, precision: 6
Expand All @@ -889,10 +896,10 @@
# use VARCHAR2(4000) instead of CLOB datatype as CLOB data type has many limitations in # use VARCHAR2(4000) instead of CLOB datatype as CLOB data type has many limitations in
# Oracle SELECT WHERE clause which causes many unit test failures # Oracle SELECT WHERE clause which causes many unit test failures
if current_adapter?(:OracleAdapter) if current_adapter?(:OracleAdapter)
t.string :content, limit: 4000 t.string :content, limit: 4000, **case_sensitive_options
t.string :important, limit: 4000 t.string :important, limit: 4000
else else
t.text :content t.text :content, **case_sensitive_options
t.text :important t.text :important
end end
t.boolean :approved, default: true t.boolean :approved, default: true
Expand Down

3 comments on commit 9def053

@joevandyk
Copy link
Contributor

@joevandyk joevandyk commented on 9def053 Sep 10, 2019

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@flanger001
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Please sign in to comment.