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

Most upvoted comments

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 a validates_uniqueness_of validation on the model and still be able to use create_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 regular create!. 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

 def self.create_or_find_by_with_validation!(attributes, &block)
     transaction(requires_new: true) { create!(attributes, &block) }
   rescue ActiveRecord::RecordInvalid => exception
     if attributes
          .map do |key, _value|
            if column_names.include?(key.to_s)
              key
            else
              reflections.fetch(key.to_s).foreign_key.to_sym
            end
          end
          .map { |key| exception.record.errors.of_kind?(key, :taken) }
          .any?
       find_by!(attributes)
     else
       raise exception
     end
   rescue ActiveRecord::RecordNotUnique
     find_by!(attributes)
   end

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.

This is similar to #find_or_create_by, but avoids the problem of stale reads

Obviously you want to validates_uniqueness_of unique fields. In this case, however, this is not at all similar to find_or_create_by because it won’t work when the record exists…

class Model < ApplicationRecord
  validates :name, uniqueness: true
end

record = Model.create_or_find_by(name: 'test')
record.id # 1

record2 = Model.create_or_find_by(name: 'test')
record2.id # nil
record2.errors # #<ActiveModel::Errors [#<ActiveModel::Error attribute=name, type=taken, options={:value=>"test"}>]>

What’s even worse is that I can unknowingly interact with the faulty record which will create a duplicate

record2.update!(name: 'something else')
record2.id # 2

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 rescuing ActiveRecord::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 a message column. The following is hopefully not that controversial:

Comment.where(email: 'test@example.com').create_or_find_by!(message: 'Hi There')
 => #<Comment id: 1, message: "Hi There", email: "test@example.com">

Comment.where(email: 'test@example.com').create_or_find_by!(message: 'Bye')
   (0.1ms)  begin transaction
  Comment Exists? (0.2ms)  SELECT 1 AS one FROM "comments" WHERE "comments"."email" = ? LIMIT ?  [["email", "test@example.com"], ["LIMIT", 1]]
   (0.1ms)  rollback transaction
Traceback (most recent call last):
        1: from (irb):1
ActiveRecord::RecordInvalid (Validation failed: Email has already been taken)

So in my mind, the following all follow the same pattern:

Comment.where(email: 'test@example.com').create_or_find_by!(message: 'Hi There')
 => #<Comment id: 1, message: "Hi There", email: "test@example.com">

Comment.where(email: 'test@example.com').create_or_find_by!(message: 'Hi There')
   (0.1ms)  begin transaction
  Comment Exists? (0.2ms)  SELECT 1 AS one FROM "comments" WHERE "comments"."email" = ? LIMIT ?  [["email", "test@example.com"], ["LIMIT", 1]]
   (0.1ms)  rollback transaction
Traceback (most recent call last):
        1: from (irb):1
ActiveRecord::RecordInvalid (Validation failed: Email has already been taken)

# and

Comment.create_or_find_by!(email: 'test@example.com')

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! and create_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.