From 3af175599959e59023f91ba4919a9be6e372bcfd Mon Sep 17 00:00:00 2001 From: Till Schulte-Coerne Date: Tue, 30 Dec 2014 14:51:24 +0100 Subject: [PATCH 1/3] Validation was scoped by type Consider a class `Base` and a class `Child < Base` with a uniqueness check in `Base.some_property` The uniqueness check did a `resource.model.first(opts)` where `resource.model` would resolve to `Child`. But nobody said that the uniqueness is scoped by the type (that should be done with `:scope => :type`). This commit adds an (previously failing) spec and a fix --- lib/data_mapper/validation/rule/uniqueness.rb | 2 +- spec/fixtures/corporate_world.rb | 6 ++++++ .../uniqueness_validator_spec.rb | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/data_mapper/validation/rule/uniqueness.rb b/lib/data_mapper/validation/rule/uniqueness.rb index 6ae184d4..b9cd6b4b 100644 --- a/lib/data_mapper/validation/rule/uniqueness.rb +++ b/lib/data_mapper/validation/rule/uniqueness.rb @@ -47,7 +47,7 @@ def valid?(resource) } other_resource = DataMapper.repository(resource.repository.name) do - resource.model.first(opts) + resource.model.base_model.first(opts) end return true if other_resource.nil? diff --git a/spec/fixtures/corporate_world.rb b/spec/fixtures/corporate_world.rb index 37710ed3..b2f68626 100644 --- a/spec/fixtures/corporate_world.rb +++ b/spec/fixtures/corporate_world.rb @@ -17,11 +17,17 @@ class Department include DataMapper::Resource property :id, Serial + + property :type, Discriminator + property :name, String, :unique_index => true validates_uniqueness_of :name end + class SpecialDepartment < Department + end + class User include DataMapper::Resource diff --git a/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb b/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb index b88db31a..ff311d4e 100644 --- a/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb +++ b/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb @@ -28,6 +28,23 @@ end end + describe 'DataMapper::Validations::Fixtures::SpecialDepartment' do + before :all do + DataMapper::Validations::Fixtures::Department.destroy! + DataMapper::Validations::Fixtures::SpecialDepartment.destroy! + + DataMapper::Validations::Fixtures::Department.create(:name => "HR").should be_saved + end + + describe "with a duplicate name" do + before do + @model = DataMapper::Validations::Fixtures::SpecialDepartment.new(:name => "HR") + end + + it_should_behave_like "invalid model" + end + end + describe 'DataMapper::Validations::Fixtures::Organisation' do before :all do DataMapper::Validations::Fixtures::Organisation.destroy! From 6a1f4199aea99f619242b85478c10fbda5262b5d Mon Sep 17 00:00:00 2001 From: Till Schulte-Coerne Date: Tue, 30 Dec 2014 16:24:00 +0100 Subject: [PATCH 2/3] Applied @rykov's better code and added some specs pointing out the STI edge cases addressed by that --- lib/data_mapper/validation/rule/uniqueness.rb | 3 +- spec/fixtures/corporate_world.rb | 7 ++++ .../uniqueness_validator_spec.rb | 33 ++++++++++++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/data_mapper/validation/rule/uniqueness.rb b/lib/data_mapper/validation/rule/uniqueness.rb index b9cd6b4b..942663df 100644 --- a/lib/data_mapper/validation/rule/uniqueness.rb +++ b/lib/data_mapper/validation/rule/uniqueness.rb @@ -46,8 +46,9 @@ def valid?(resource) opts[subject] = resource.__send__(subject) } + property = resource.model.properties[field_name] other_resource = DataMapper.repository(resource.repository.name) do - resource.model.base_model.first(opts) + property.model.first(opts) end return true if other_resource.nil? diff --git a/spec/fixtures/corporate_world.rb b/spec/fixtures/corporate_world.rb index b2f68626..eb495707 100644 --- a/spec/fixtures/corporate_world.rb +++ b/spec/fixtures/corporate_world.rb @@ -26,6 +26,13 @@ class Department end class SpecialDepartment < Department + property :special_id, String + validates_uniqueness_of :special_id + end + + class VerySpecialDepartment < Department + property :special_id, String + validates_uniqueness_of :special_id end class User diff --git a/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb b/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb index ff311d4e..593581bc 100644 --- a/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb +++ b/spec/integration/uniqueness_validator/uniqueness_validator_spec.rb @@ -31,7 +31,6 @@ describe 'DataMapper::Validations::Fixtures::SpecialDepartment' do before :all do DataMapper::Validations::Fixtures::Department.destroy! - DataMapper::Validations::Fixtures::SpecialDepartment.destroy! DataMapper::Validations::Fixtures::Department.create(:name => "HR").should be_saved end @@ -45,6 +44,38 @@ end end + describe 'DataMapper::Validations::Fixtures::SpecialDepartment' do + before :all do + DataMapper::Validations::Fixtures::Department.destroy! + + DataMapper::Validations::Fixtures::SpecialDepartment.create(:name => "HR", :special_id => "1234").should be_saved + end + + describe "with a duplicate special_id" do + before do + @model = DataMapper::Validations::Fixtures::SpecialDepartment.new(:name => "Other Dept.", :special_id => "1234") + end + + it_should_behave_like "invalid model" + end + end + + describe 'DataMapper::Validations::Fixtures::VerySpecialDepartment' do + before :all do + DataMapper::Validations::Fixtures::Department.destroy! + + DataMapper::Validations::Fixtures::SpecialDepartment.create(:name => "HR", :special_id => "1234").should be_saved + end + + describe "with a special_id also used by a SpecialDepartment" do + before do + @model = DataMapper::Validations::Fixtures::VerySpecialDepartment.new(:name => "Some Very Special Other Dept.", :special_id => "1234") + end + + it_should_behave_like "valid model" + end + end + describe 'DataMapper::Validations::Fixtures::Organisation' do before :all do DataMapper::Validations::Fixtures::Organisation.destroy! From 13fce9c08a3dc4b8821b8e78eea549b5106d6f34 Mon Sep 17 00:00:00 2001 From: Till Schulte-Coerne Date: Tue, 30 Dec 2014 16:46:51 +0100 Subject: [PATCH 3/3] Fixed deprecation warnings --- lib/data_mapper/validation/rule/uniqueness.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/data_mapper/validation/rule/uniqueness.rb b/lib/data_mapper/validation/rule/uniqueness.rb index 942663df..d5dd1412 100644 --- a/lib/data_mapper/validation/rule/uniqueness.rb +++ b/lib/data_mapper/validation/rule/uniqueness.rb @@ -46,7 +46,7 @@ def valid?(resource) opts[subject] = resource.__send__(subject) } - property = resource.model.properties[field_name] + property = resource.model.properties[attribute_name] other_resource = DataMapper.repository(resource.repository.name) do property.model.first(opts) end