rails: create_or_find_by doesn't attempt to find the record when you also have validates_uniqueness_of :field
Steps to reproduce
# frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", github: "rails/rails"
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
require "logger"
# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :posts, force: true do |t|
t.string :email
t.index :email, unique: true
end
create_table :comments, force: true do |t|
t.string :email
t.index :email, unique: true
end
end
class Post < ActiveRecord::Base
end
class Comment < ActiveRecord::Base
validates :email, uniqueness: true
end
class BugTest < Minitest::Test
def test_without_validation
count = Post.count
post = Post.create_or_find_by!(email: 'test@example.com')
assert_equal count+1, Post.count
post2 = Post.create_or_find_by!(email: 'test@example.com')
assert_equal count+1, Post.count # count didn't change
assert_equal post.id, post2.id
end
def test_with_validation
count = Comment.count
comment = Comment.create_or_find_by!(email: 'test@example.com')
assert_equal count+1, Comment.count
# raises ActiveRecord::RecordInvalid: Validation failed: Email has already been taken
comment2 = Comment.create_or_find_by!(email: 'test@example.com')
assert_equal count+1, Comment.count
assert_equal comment.id, comment2.id
end
end
Expected behavior
create_or_find_by
should find the existing record when there is a uniqueness validation on the field.
Actual behavior
An ActiveRecord::RecordInvalid
exception is raised because the record is validated before it is created, which checks the database and finds an existing record. create_or_find_by
doesn’t get the ActiveRecord::RecordNotUnique
exception it is expecting so it doesn’t attempt to find
the record.
System configuration
Rails version: master
Ruby version: ruby 2.5.3p105
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 4
- Comments: 16 (8 by maintainers)
Commits related to this issue
- Adjust docs of create_or_find_by - Mention explicitly in the docs that `create_of_find_by` requires a DB constraint. Closes #36027 Co-authored-by: Eileen M. Uchitelle <eileencodes@users.noreply.git... — committed to intrip/rails by intrip 3 years ago
- need to remove uniqueness validation for create_or_find_by to work https://github.com/rails/rails/issues/36027 — committed to sciencehistory/scihist_digicoll by jrochkind 7 months ago
create_or_find_by
literally says “try to create the record, if that failed due to uniqueness constraints, find it instead”. To me, it makes perfect sense to have avalidates_uniqueness_of
validation on the model and still be able to usecreate_or_find_by
, which I would expect to run that validation, if it fails, well, fetch the record. I might still want the Rails uniqueness validation for the case where I do a regularcreate!
. If you are worried about your auto-incremented pkeys: AFAIK, Rails uses bigint by default.Can this be reopened? the issue was closed because stale, no opinion from a Rails team member has been provided yet, and the issue is still there.
It’s helpful that the docs now read that this method requires uniqueness constraints in the database. Should we also mention that it also seems to require we explicitly not use the active uniqueness validation?
Just ran into this myself. It’s beyond confusing that Rails’ own uniqueness validation would break a Rails method intended to help enforce uniqueness. +1 to reopen.
I think that this should be addressed differently than removing the uniqueness constraint from the model as there are many part of the rails ecosystem that rely on ActiveRecord validation.
I just came across this issue and patched locally by writing my own create_or_find_by as follows. The goal is to treat the subset of ActiveRecord::RecordNotValid errors due to uniqueness constraint the same as the ActiveRecord::RecordNotUnique
Another solution would be for ActiveRecord::RecordInvalid to be an ancestor of ActiveRecord::RecordNotUnique and raising the latter when a uniqueness constraint fails in validation. This class hierarchy would give us backward compatibility
Was running into this today. This is still super confusing.
Obviously you want to
validates_uniqueness_of
unique fields. In this case, however, this is not at all similar tofind_or_create_by
because it won’t work when the record exists…What’s even worse is that I can unknowingly interact with the faulty record which will create a duplicate
Can this be reopened and addressed properly either in docs or behavior?
@swiknaba Totally! To me, the whole point of activerecord validations is as a backup to the database constraints and to get richer error feedback. Of course there is also a performance benefit to validating in memory. IMO they should always work in tandem, not one or the other. We should never have to rely 100% on the application for ensuring database validity.
Please do @intrip, that would be great! 👍
I think there’s a reasonable argument to be made that this is surprising, so reopening the issue. I’d love to see a PR with a proposed fix, or an update to the docs that draws attention to this.
note: the
create_or_find_by!
method is actually implemented by rescuingActiveRecord::RecordNotUnique
:https://github.com/rails/rails/blob/89b86640901be4f423cef4059d970c0aa0adb648/activerecord/lib/active_record/relation.rb#L209-L213
so an easy work around for this problem is to remove the uniqueness validation.
I’m of the same mind as @daveallie that this seems like expected behaviour and not treating this validation in any special way.
To me, this reads as expected behaviour. If there was an email regex validator and it failed the record, then I’d expect it to throw and not create or find a record, so why wouldn’t other validators also throw?
That said, the specific case is a little more complicated given that there is a uniqueness validator with a unique constraint. At the time the create is run, there is no guarantee that the database has a constraint that matches with the uniqueness validator.
Even if the presence and validity of the constraint could be verified, I would still expect your scenario to throw. Say that
Comment
also had amessage
column. The following is hopefully not that controversial:So in my mind, the following all follow the same pattern:
I don’t think there’s a reliable way to know the full context of the query, the layout of the tables and their constraints in order to selectively ignore validators.
My suggestion would be to override
create_or_find_by!
andcreate_or_find_by
in model classes where you have specific validator you want to ignore and explicitly disable those validators before calling back to the super method.