Add allow_nil option to the validate uniquness matcher #182

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@drapergeek
thoughtbot, inc. member

This should fix issue #96.

I'm open to suggestion better ways to handle this. One thing in particular is that I'm not thrilled about having to check for an existing nil in the valid_attribute method. I dont' like the idea of mixing those concerns so any ideas on how to pull that out would be great.

@gabebw gabebw commented on an outdated diff Nov 2, 2012
...tchers/active_model/validate_uniqueness_of_matcher.rb
def existing
@existing ||= first_instance
end
+ def existing=(instance)
@gabebw
gabebw Nov 2, 2012

This doesn't need to be its own method - the overhead is way more than the benefit.

@gabebw gabebw commented on an outdated diff Nov 2, 2012
...tchers/active_model/validate_uniqueness_of_matcher.rb
def existing
@existing ||= first_instance
end
+ def existing=(instance)
+ @existing = instance
+ end
+
+ def existing?
@gabebw
gabebw Nov 2, 2012

This doesn't need to be its own method - the overhead is way more than the benefit.

@gabebw
thoughtbot, inc. member

Generally, let's rename @existing to @existing_record.

@gabebw gabebw and 1 other commented on an outdated diff Nov 2, 2012
...tchers/active_model/validate_uniqueness_of_matcher.rb
@@ -108,9 +141,17 @@ def set_scoped_attributes
end
def validate_attribute?
+ if @options[:allow_nil] && existing_value.nil?
@gabebw
gabebw Nov 2, 2012

I'm confused by these 3 lines of code - it's not immediately obvious what they do.

@drapergeek
drapergeek Nov 2, 2012

We still recommend in the documentation for people to create their own record before using this matcher (to prevent database constraint issues). If someone created a record with a nil value & the validation allows a nil value so the disallows would not fail. If thats the case, we need to create an instance in the database that has something other than the nil value.

That being said, this is the part I would really like to avoid if you have any clever ideas. I couldn't see any that wouldn't screw up people with database constraints.

@gabebw
gabebw Feb 22, 2013

The idea is that here, we check everything except that allows_nil is set correctly, by ensuring we have 1 record with a nil attribute and 1 without. This triggers every validation except the duplicate-nil ones. Elsewhere, in allows_nil?, we check that allows_nil is set correctly.

It's 2 validations, spread across 1 file.

@gabebw
gabebw Feb 22, 2013

Possibly a better name for this method: validate_everything_except_duplicate_nils? or similar.

@gabebw
gabebw Mar 8, 2013

@drapergeek : thoughts on renaming this method, per my comments above?

@gabebw gabebw commented on an outdated diff Nov 2, 2012
...a/active_model/validate_uniqueness_of_matcher_spec.rb
+ it "should allow_nil" do
+ Example.create!(:attr => nil)
+ @model.should validate_uniqueness_of(:attr).allow_nil
+ end
+ end
+
+ it "should create a nil and verify that it is allowed" do
+ @model.should validate_uniqueness_of(:attr).allow_nil
+ end
+ end
+
+ context "when the validation does not allow a nil" do
+ before do
+ @model = define_model(:example, :attr => :integer) do
+ attr_accessible :attr
+ validates_uniqueness_of :attr, :case_sensitive => true
@gabebw
gabebw Nov 2, 2012

Do the tests pass without the :case_sensitive option?

@drapergeek
thoughtbot, inc. member

Updated per request, also removed the case sensitive option, it wasn't necessary and tests still pass fine.

@gabebw
thoughtbot, inc. member

This looks good to me - can you update it for the new spec style? :)

@drapergeek
thoughtbot, inc. member

Will do. Didn't even remember this was still in for review!

@gabebw gabebw commented on the diff Feb 22, 2013
...tchers/active_model/validate_uniqueness_of_matcher.rb
disallows_value_of(existing_value, @expected_message)
end
+ def create_record_without_nil
+ @existing_record = create_record_in_database
@gabebw
gabebw Feb 22, 2013

This is sort of odd, in that it both creates the record and changes an instance variable, which ripples across a lot of the internal state.

I don't have a solution right now.

@gabebw
thoughtbot, inc. member

I'd like to clean up the code before adding functionality (i.e. merging this pull), but cleaning up could take a while (see also #238).

I'm going to punt on whether or not to merge this.

@drapergeek
thoughtbot, inc. member

When you say 'clean up the code', do you mean you want to overhaul this PR or you want to clean up this matcher and remove the inheritance and follow the submatchers pattern?

@gabebw
thoughtbot, inc. member

I'd do it at some point after this PR is dealt with, I don't want to hold it up even more.

@gabebw gabebw commented on an outdated diff Mar 8, 2013
...a/active_model/validate_uniqueness_of_matcher_spec.rb
+ it "should require a unique value for that attribute" do
+ @model.should validate_uniqueness_of(:attr)
+ end
+
+ it "should pass when the subject is an existing record" do
+ @existing.should validate_uniqueness_of(:attr)
+ end
+
+ it "should fail when a scope is specified" do
+ @model.should_not validate_uniqueness_of(:attr).scoped_to(:other)
+ end
+ end
+
+ context "without an existing value" do
+ before do
+ Example.find(:first).should be_nil
@gabebw
gabebw Mar 8, 2013

I think this can be Example.first

@gabebw gabebw commented on the diff Mar 8, 2013
...a/active_model/validate_uniqueness_of_matcher_spec.rb
+ context "when the validation allows nil" do
+ before do
+ @model = define_model(:example, :attr => :integer) do
+ attr_accessible :attr
+ validates_uniqueness_of :attr, :allow_nil => true
+ end.new
+ end
+
+ context "when there is an existing entry with a nil" do
+ it "should allow_nil" do
+ Example.create!(:attr => nil)
+ @model.should validate_uniqueness_of(:attr).allow_nil
+ end
+ end
+
+ it "should create a nil and verify that it is allowed" do
@gabebw
gabebw Mar 8, 2013

This test isn't actually testing that it created a nil - should there be a new line after 171 like

Example.first.attr.should be_nil
@drapergeek
thoughtbot, inc. member

@gabebw Care to re-review after update please?

@gabebw gabebw commented on an outdated diff Mar 29, 2013
...tchers/active_model/validate_uniqueness_of_matcher.rb
def matches?(subject)
@subject = subject.class.new
@expected_message ||= :taken
set_scoped_attributes &&
- validate_attribute? &&
- validate_after_scope_change?
+ validate_everything_expect_duplicate_nils? &&
@gabebw
gabebw Mar 29, 2013

I assume this is supposed to be except (not expect)?

@drapergeek
thoughtbot, inc. member

Good catch, anything else @gabebw

@gabebw
thoughtbot, inc. member

👍

@mxie
thoughtbot, inc. member

Merged!

@mxie mxie closed this Apr 5, 2013
@mxie mxie deleted the jd-allow-nil branch Apr 5, 2013
@apraditya

So how's the syntax to use this? Neither validate_uniqueness_of(:attr).allow_nil nor validate_uniqueness_of :attr, allow_nil: true works. I'm using 1.5.6. You may wanna add it to the README. Thanks!

@drapergeek
thoughtbot, inc. member

You have the syntax correct but this feature has not been added to a release yet.

It is on the HEAD for this repo so if you run directly from master you can use that but right now it has not been released to a version.

@mattvleming

Do we have a timeline for when this will be released to a version?

@drapergeek
thoughtbot, inc. member

This was added in v2.1.0.

@mattvleming

Yea I just realized this, I was just about to edit my comment. I asked because it wasn't working, even when I was running directly from master. It turns out .allow_nil doesn't work for validate_presence_of, only validate_uniqueness_of, even though in my model I have validates :attribute, presence: true, allow_nil: true.

Should the code be adapted or is there a reason why it is this way?

EDIT: By the way, how can I find what version code is released in?

@drapergeek
thoughtbot, inc. member

@mattvleming Maybe I'm being a bit dense, if you're validating that an attribute is present, how would you also allow a nil? Doesn't that negate the point of requiring it to be present?

@drapergeek
thoughtbot, inc. member

Actually, @mxie verified this to make sure, the allow_nil on your validate presence doesn't actually do anything.

That is one downside to using hash keys as arguments, there is no way for a method to tell you if you passed in something additional that isn't actually doing anything for you. (Not easily and normally supported at least until we use named arguments).

@mattvleming

@drapergeek The documentation for validates_presence_of tells me it uses #blank?. The documentation for #blank? tells me examples of things that are blank include "", " ", nil, [], or {}, so validates_presence_of is not limited to just nil. This is what I want because the attribute can be nil, but if it has a string, the string can't be empty or whitespace.

@drapergeek
thoughtbot, inc. member

Ok, I can see your reasoning. I do not think that your approach is very obvious as to what you're trying to do though. Maybe another validation that handles that case and specifies what you're looking for would be more obvious.

In any case, http://api.rubyonrails.org/classes/ActiveRecord/Validations/ClassMethods.html#method-i-validates_presence_of, the method is not acting like you want it to so you may want to take that into consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment