Fix validation on uniqueness #12774

Merged
merged 1 commit into from Nov 22, 2013

Conversation

Projects
None yet
6 participants
Contributor

exAspArk commented Nov 5, 2013

In case if I set validation on uniqueness of relation

class Foo < ActiveRecord::Base
  belongs_to :bar
  validates :bar, uniqueness: true
end

Foo.new.valid?

and it doesn't exist, I have an error:

  NoMethodError - undefined method `attributes' for nil:NilClass:
  activerecord (4.0.1.rc4) lib/active_record/validations/uniqueness.rb:56:in `build_relation'

To fix it I suggest adding the condition

Contributor

dmathieu commented Nov 5, 2013

Could you also add a test?

Contributor

exAspArk commented Nov 5, 2013

@dmathieu sure!

Contributor

exAspArk commented Nov 5, 2013

added tests

@dmathieu dmathieu and 1 other commented on an outdated diff Nov 5, 2013

.../test/cases/validations/uniqueness_validation_test.rb
@@ -376,4 +382,18 @@ def test_validate_uniqueness_with_array_column
assert e2.errors[:nicknames].any?, "Should have errors for nicknames"
assert_equal ["has already been taken"], e2.errors[:nicknames], "Should have uniqueness message for nicknames"
end
+
+ def test_validate_uniqueness_on_existing_relation
+ organization = Organization.create
+ assert MemberDetailWithUniqOrganization.create(organization: organization).valid?
+
+ member_detail = MemberDetailWithUniqOrganization.new(organization: organization)
+ assert !member_detail.valid?
+ assert_equal ['has already been taken'], member_detail.errors[:organization]
+ end
+
+ def test_validate_uniqueness_on_empty_relation
+ member_detail = MemberDetailWithUniqOrganization.new
+ assert member_detail.valid?
@dmathieu

dmathieu Nov 5, 2013

Contributor

How are these tests different from test_validate_uniqueness?

@exAspArk

exAspArk Nov 5, 2013

Contributor

@dmathieu in this case it validates uniqueness of relation. Just to be sure that it doesn't throw any errors (like in pull request's description).

@dmathieu

dmathieu Nov 5, 2013

Contributor

Oh, right, sorry.

@dmathieu dmathieu and 1 other commented on an outdated diff Nov 5, 2013

.../test/cases/validations/uniqueness_validation_test.rb
@@ -35,6 +37,10 @@ class Employee < ActiveRecord::Base
validates_uniqueness_of :nicknames
end
+class MemberDetailWithUniqOrganization < MemberDetail
+ validates :organization, uniqueness: true
+end
@dmathieu

dmathieu Nov 5, 2013

Contributor

Could you use Topic instead of Organization, for consistency with the rest of the tests?

@exAspArk

exAspArk Nov 5, 2013

Contributor

Ok, but there are a lot of other classes in this file like: WarehouseThing, Guid, Event, Employee, Wizard, etc 😃

@exAspArk

exAspArk Nov 5, 2013

Contributor

Done

Contributor

dmathieu commented Nov 5, 2013

👍
One last thing: could you squash your commits?

Contributor

exAspArk commented Nov 5, 2013

@dmathieu done :)

Contributor

egilburg commented Nov 9, 2013

I think in the example you quoted, foo.valid? should do a check whether there are any existing records without bar, and return false if it there are any and and foo has no bar, unless allow_nil: true or allow_blank: true is also set.

If this is too complex, I'd settle for a simpler solution where foo.valid? always return false if foo has no bar, unless allow_nil: true or allow_blank: true is also set. This is because the pattern of allowing exactly one saved nil field, rather than either zero (via also validating presence) or many (via setting allow_nil or allow_blank) is pretty rare.

But in either case I wouldn't want valid? to return true as per current example. It just seems wrong logic.

Contributor

exAspArk commented Nov 9, 2013

@egilburg you're suggesting to change logic of validation on uniqueness, but in this PR I just want to fix this bag.

But in my opinion it is OK that it returns true after executing request:

SELECT 1 AS one FROM "foos" WHERE "foos"."bar_id" IS NULL LIMIT 1

It seems more transparently for me. I'm asking myself: Is it uniq? Yes. Does it exist? No, but this is what presence validation should handle.
Once you saved Foo without Bar it won't be valid next time with an empty association.

Contributor

egilburg commented Nov 9, 2013

Once you saved Foo without Bar it won't be valid next time with an empty association.

If this is indeed the case, this is the right logic. It's just that I haven't seen it from your example.

Contributor

exAspArk commented Nov 20, 2013

@dmathieu Hi, could you please check this PR one more time and merge it?

Contributor

dmathieu commented Nov 20, 2013

I can't merge pull requests, sorry.
ping @carlosantoniodasilva @rafaelfranca

@senny senny and 2 others commented on an outdated diff Nov 22, 2013

activerecord/lib/active_record/validations/uniqueness.rb
@@ -48,7 +48,7 @@ def find_finder_class_for(record) #:nodoc:
def build_relation(klass, table, attribute, value) #:nodoc:
if reflection = klass.reflect_on_association(attribute)
attribute = reflection.foreign_key
- value = value.attributes[reflection.primary_key_column.name]
+ value = value.attributes[reflection.primary_key_column.name] if value.present?
@senny

senny Nov 22, 2013

Member

do we really need present? wouldn't it be enough to use the same conditional as below: unless value.nil?.

@igor04

igor04 Nov 22, 2013

Contributor

what about unless value.nil? isn't it better if value ?

@exAspArk

exAspArk Nov 22, 2013

Contributor

Just prefer if to unless in equal cases. It's not so important I think, but if value.present? looks more like a native language :)

@senny

senny Nov 22, 2013

Member

I also prefer to use if but in this case you actually changed the code. if present? also accounts for "" while unless nil? clearly expresses that the only case we care about is nil.

@senny senny commented on an outdated diff Nov 22, 2013

activerecord/lib/active_record/validations/uniqueness.rb
@@ -59,7 +59,7 @@ def build_relation(klass, table, attribute, value) #:nodoc:
# will use SQL LOWER function before comparison, unless it detects a case insensitive collation
klass.connection.case_insensitive_comparison(table, attribute, column, value)
else
- value = klass.connection.case_sensitive_modifier(value) unless value.nil?
+ value = klass.connection.case_sensitive_modifier(value) if value.present?
@senny

senny Nov 22, 2013

Member

do we need to switch this?

@senny senny commented on an outdated diff Nov 22, 2013

.../test/cases/validations/uniqueness_validation_test.rb
@@ -376,4 +381,18 @@ def test_validate_uniqueness_with_array_column
assert e2.errors[:nicknames].any?, "Should have errors for nicknames"
assert_equal ["has already been taken"], e2.errors[:nicknames], "Should have uniqueness message for nicknames"
end
+
+ def test_validate_uniqueness_on_existing_relation
+ event = Event.create
+ assert TopicWithUniqEvent.create(event: event).valid?
+
+ topic = TopicWithUniqEvent.new(event: event)
+ assert !topic.valid?
@senny

senny Nov 22, 2013

Member

please use assert_not

Member

senny commented Nov 22, 2013

I agree with @exAspArk the presence validation will make sure that it really exists. Another possibility are foreign keys.

I added a few minor comments. Can you also include a changelog entry?

Contributor

exAspArk commented Nov 22, 2013

@senny yep, done

Member

senny commented Nov 22, 2013

@exAspArk thank you 💛

@senny senny added a commit that referenced this pull request Nov 22, 2013

@senny senny Merge pull request #12774 from exAspArk/fix_uniqueness_on_relation
Fix validation on uniqueness
818b362

@senny senny merged commit 818b362 into rails:master Nov 22, 2013

1 check passed

default The Travis CI build passed
Details

@senny
I am experiencing this bug using activerecord 4.0.3
I was able to fix the issue only after manually going into my activerecord/lib/active_record/validations/uniqueness.rb file and editing the line as per this commit
`value = value.attributes[reflection.primary_key_column.name] unless value.nil?``

Am I missing something? Or is this not yet released via rubygems?
Thank you for your help and great contribution

@senny senny added a commit that referenced this pull request Feb 23, 2014

@senny senny Merge pull request #12774 from exAspArk/fix_uniqueness_on_relation
Fix validation on uniqueness
Conflicts:
	activerecord/CHANGELOG.md
e8458d0
Member

senny commented Feb 23, 2014

@stefanosc 4.0.3 was a security release and did not contain any bug fixes:

$ git log v4.0.2..v4.0.3
1f6113c Preparing for 4.0.3 release [Rafael Mendonça França] [5 days ago]
3eaea65 Correctly escape PostgreSQL arrays. [Aaron Patterson] [5 days ago]
9e2d63d Escape format, negative_format and units options of number helpers [Rafael Mendonça França] [5 days ago]

However I noticed that I forgot to backport the patch to 4-0-stable. The backport is now in place (e8458d0) and will be released with the next 4.0.x bugfix release. (probably 4.0.4).

@senny thank you very much for taking the time to clarify, I am a junior rails dev and still getting used to git. Great to know this will be included in 4.0.4 😄
Thank you again for your help.

Member

senny commented Feb 23, 2014

@stefanosc glad to help.

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