New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #23625 #23628

Merged
merged 1 commit into from Feb 23, 2016

Conversation

Projects
None yet
3 participants
@maclover7
Member

maclover7 commented Feb 12, 2016

This resolves a bug where if the primary key used is not id (ex:
uuid), and has a validates_uniqueness_of in the model, a uniqueness error
would be raised. This is a partial revert of commit 119b918, which introduced this behavior.

@diego-silva

This comment has been minimized.

Contributor

diego-silva commented Feb 12, 2016

I was looking into this issue and was about to commit and propose a different solution, so I would like to discuss a few things:

  1. This issue happens in every database, the test doesn't need to be only for postgres. You actually don't need to use the uuid type, the regular id field can be used. It happens on every model that has a validates_uniqueness_of on the primary key. So I thought would be better to go with a more generic test.
  2. This solution will cause the same (kinda of weird) behaviour that we can see on current Rails versions when updating a record, where it would query for something like this:
    ValidatingItem Exists (0.7ms) SELECT 1 AS one FROM "items" WHERE ("items"."id" = 1 AND "items"."id" != '1') LIMIT 1

The solution I was going for was changing the should_validate? method in activemodel/lib/active_model/validator.rb, so that it would also receive the attribute and call record.changed.include?(attribute.to_s) instead of just record.changed?. (this method is called at the beginning of absence, length, presence and uniqueness validators)

This would prevent the above query from being executed, and would also prevent validations from running on attributes that weren't changed. It might increase performance by not having to hit the DB to check those attributes, but I'm not sure it would be a desirable behaviour...
Btw, I ran all the tests on activerecord and activemodel and they all passed with this solution.

@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 12, 2016

It sounds like your change might a little more involved... cc @sgrif

@sgrif

View changes

activerecord/test/cases/validations/uniqueness_validation_test.rb Outdated
error = assert_raises ActiveRecord::RecordInvalid do
CoolTopic.create!(id: item.id, title: 'MyItem2')
end
assert_includes('Validation failed: Id has already been taken', error.message)

This comment has been minimized.

@sgrif

sgrif Feb 18, 2016

Member

Can you call .valid? and .errors instead of relying on exceptions here?

This comment has been minimized.

@maclover7

maclover7 Feb 18, 2016

Member

👍 updated per this comment.

Fix issue #23625
This resolves a bug where if the primary key used is not `id` (ex:
`uuid`), and has a `validates_uniqueness_of` in the model, a uniqueness error
would be raised. This is a partial revert of commit `119b9181ece399c67213543fb5227b82688b536f`, which introduced this behavior.

@maclover7 maclover7 force-pushed the maclover7:fix-23625 branch to 55385c8 Feb 18, 2016

sgrif added a commit that referenced this pull request Feb 23, 2016

@sgrif sgrif merged commit 96c1ada into rails:master Feb 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maclover7 maclover7 deleted the maclover7:fix-23625 branch Feb 23, 2016

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