Skip to content

Commit

Permalink
Make uniqueness validations correctly handle nil values
Browse files Browse the repository at this point in the history
Previously, the uniqueness validations did not work correctly for
nil values, since they checked if there were other rows where
(column IS NULL).  However, in SQL, NULL = NULL is not true, and
multiple rows are allowed to have NULL values in a column with a
unique constraint.  So skip the uniqueness check if the value is
nil.

If you have a UNIQUE NOT NULL column, you should have a separate
presence check to check for the nil values.
  • Loading branch information
jeremyevans committed Oct 29, 2012
1 parent 97304e6 commit 23f9c4b
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
=== HEAD

* Make uniqueness validations correctly handle nil values (jeremyevans)

* Add ConnectionPool#pool_type to get the type of connection pool in use (jeremyevans)

* Explicitly mark primary keys as NOT NULL on SQLite (jeremyevans)
Expand Down
1 change: 1 addition & 0 deletions lib/sequel/plugins/validation_class_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ def validates_uniqueness_of(*atts)
error_field = a
a = Array(a)
v = Array(v)
next if v.empty? || !v.all?
ds = o.class.filter(a.zip(v))
num_dups = ds.count
allow = if num_dups == 0
Expand Down
4 changes: 3 additions & 1 deletion lib/sequel/plugins/validation_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ def validates_unique(*atts)
ds = if where
where.call(model.dataset, self, arr)
else
model.where(arr.map{|x| [x, send(x)]})
vals = arr.map{|x| send(x)}
next unless vals.all?
model.where(arr.zip(vals))
end
ds = yield(ds) if block_given?
ds = ds.exclude(pk_hash) unless new?
Expand Down
20 changes: 19 additions & 1 deletion spec/extensions/validation_class_methods_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ def dont_skip; true; end
specify "should validate uniqueness_of with allow_missing => true" do
@c.validates_uniqueness_of :value, :allow_missing => true
@m.should be_valid
@m.value = nil
@m.value = 1
@m.should_not be_valid
end
end
Expand Down Expand Up @@ -858,6 +858,12 @@ class ::User < Sequel::Model
@user = User.new(:username => "0records", :password => "anothertest")
@user.should be_valid
@user.errors.full_messages.should == []

User.db.sqls
@user = User.new(:password => "anothertest")
@user.should be_valid
@user.errors.full_messages.should == []
User.db.sqls.should == []
end

it "should validate the uniqueness of multiple columns" do
Expand Down Expand Up @@ -907,6 +913,18 @@ class ::User < Sequel::Model
@user = User.new(:username => "0records", :password => "anothertest")
@user.should be_valid
@user.errors.full_messages.should == []

User.db.sqls
@user = User.new(:password => "anothertest")
@user.should be_valid
@user.errors.full_messages.should == []
@user = User.new(:username => "0records")
@user.should be_valid
@user.errors.full_messages.should == []
@user = User.new
@user.should be_valid
@user.errors.full_messages.should == []
User.db.sqls.should == []
end

it "should have a validates block that contains multiple validations" do
Expand Down
17 changes: 17 additions & 0 deletions spec/extensions/validation_helpers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,11 @@ def validate
@user = @c.load(:id=>3, :username => "0records", :password => "anothertest")
@user.should be_valid

MODEL_DB.sqls
@user = @c.new(:password => "anothertest")
@user.should be_valid
MODEL_DB.sqls.should == []

@user = @c.new(:username => "1record", :password => "anothertest")
@user.should_not be_valid
@user.errors.full_messages.should == ['username is already taken']
Expand Down Expand Up @@ -369,6 +374,18 @@ def validate
@user = @c.load(:id=>3, :username => "0records", :password => "anothertest")
@user.should be_valid

MODEL_DB.sqls
@user = @c.new(:password => "anothertest")
@user.should be_valid
@user.errors.full_messages.should == []
@user = @c.new(:username => "0records")
@user.should be_valid
@user.errors.full_messages.should == []
@user = @c.new
@user.should be_valid
@user.errors.full_messages.should == []
MODEL_DB.sqls.should == []

@user = @c.new(:username => "1record", :password => "anothertest")
@user.should_not be_valid
@user.errors.full_messages.should == ['username and password is already taken']
Expand Down

0 comments on commit 23f9c4b

Please sign in to comment.