Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Nil values uniquines validation #2325

Merged
merged 1 commit into from

3 participants

@pyromaniac

It casts all nil values to blank string. I think, it is a bug, because rails < 3.1 didn't do it. Here is solution. Thanks.

@josevalim
Owner

Please rebase and then ping me once you do?

@pyromaniac

Done. Ping.

@josevalim josevalim merged commit 958d25d into from
@josevalim
Owner

Reverted as it broke the build for mysql isolated tests:

http://travis-ci.org/#!/rails/rails/jobs/484547

@pyromaniac

Ups, i'll try to fix this issue.

@josevalim
Owner
@josevalim josevalim referenced this pull request from a commit
@josevalim josevalim Revert "Merge pull request #2325 from pyromaniac/master"
It breaks the build for mysql.

This reverts commit 958d25d, reversing
changes made to 8f309e3.
705b29b
@pyromaniac

I'm confused. My solution works, but arel generates for all dbs something like this for existance checking:

SELECT 1 FROM "topics" WHERE "topics"."title" IS NULL LIMIT 1

And for mysql it uses:

SELECT 1 FROM "topics" WHERE "topics"."title" = BINARY NULL LIMIT 1

Is it a feature? I think, the first is more proper, no?

@pyromaniac

Done. Works now. Ping.

#4346

Also, i think adapter shoul be fixed here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L523 like this:

Arel::Nodes::Bin.new(node) unless node.nil?

@josevalim
Owner

/cc @jonleighton @tenderlove bros, please check the adapter comment above ^^

@tenderlove
Owner

I'm not sure what we should do about this. The case_sensitive_modifier does exactly what it says: wraps the node for doing case sensitive searches. How can you do a case sensitive search against NULL? I guess we either need to avoid calling this method when the value is nil, or have the method return the original node in the nil case.

@josevalim
Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 6, 2012
  1. @pyromaniac
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/validations/uniqueness.rb
@@ -54,7 +54,7 @@ def find_finder_class_for(record) #:nodoc:
def build_relation(klass, table, attribute, value) #:nodoc:
column = klass.columns_hash[attribute.to_s]
- value = column.limit ? value.to_s[0, column.limit] : value.to_s if column.text?
+ value = column.limit ? value.to_s[0, column.limit] : value.to_s if value && column.text?
if !options[:case_sensitive] && value && column.text?
# will use SQL LOWER function before comparison, unless it detects a case insensitive collation
View
12 activerecord/test/cases/validations/uniqueness_validation_test.rb
@@ -45,6 +45,18 @@ def test_validate_uniqueness
assert t2.save, "Should now save t2 as unique"
end
+ def test_validates_uniqueness_with_nil_value
+ Topic.validates_uniqueness_of(:title)
+
+ t = Topic.new("title" => nil)
+ assert t.save, "Should save t as unique"
+
+ t2 = Topic.new("title" => nil)
+ assert !t2.valid?, "Shouldn't be valid"
+ assert !t2.save, "Shouldn't save t2 as unique"
+ assert_equal ["has already been taken"], t2.errors[:title]
+ end
+
def test_validates_uniqueness_with_validates
Topic.validates :title, :uniqueness => true
Topic.create!('title' => 'abc')
Something went wrong with that request. Please try again.