create_or_find_by doesn't attempt to find the record when you also have validates_uniqueness_of :field

[jduff]

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

[daveallie]

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.

[itsWill]

note: the create_or_find_by! method is actually implemented by rescuing ActiveRecord::RecordNotUnique:

rails/activerecord/lib/active_record/relation.rb

Lines 209 to 213 in 89b8664

	def create_or_find_by(attributes, &block)
	transaction(requires_new: true) { create(attributes, &block) }
	rescue ActiveRecord::RecordNotUnique
	find_by!(attributes)
	end

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.

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

[ricksuggs]

In my opinion, the validates_uniqueness_of validation should still be run before attempting to create the record. It's been reported that auto-incrementing primary keys (without bigint column) can be exahuasted with usage of create_or_find_by with many RecordNotUnique raised. Adding the validation would be an extra guard against raising this exception, however race conditions could still occur. In any case, if developer adds the validation, the framework should execute the validation as expected.

[swiknaba]

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.

[liamelliott]

@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.

[spaghetticode]

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.

[rusterholz]

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.

[ghiculescu]

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.

[alexcameron89]

I'd love to see a PR with a proposed fix, or an update to the docs that draws attention to this

Yeah, I certainly don't think that would hurt.

I think it would be worth clarifying the statement "Attempts to create a record with the given attributes in a table that has a unique constraint on one or several of its columns." to mean explicitly database constraints on columns as opposed to validations as well as more context on the intent behind why the method was added.

I believe the intent of the method create_or_find_by is to get to get to the INSERT portion as fast as it can, and this is helpful for tables where find_by rarely returns a record (meaning you're going to INSERT much more than you're going to find the record). Specifically the original PR mentions:

This is similar to #find_or_create_by, but avoids the problem of stale reads, as that methods needs
to first query the table, then attempt to insert a row if none is found. That leaves a timing gap
between the SELECT and the INSERT statements that can cause problems in high throughput applications.

I think running the uniqueness validations would get rid of that benefit and put you back to square one of find_or_create_by. I'll also mention that I very rarely use create_or_find_by for these reasons.

[ghiculescu]

Yep, I agree with that. Let's clarify the docs.

[intrip]

Anybody on it? Otherwise I'll make a PR to update the docs

[alexcameron89]

Please do @intrip, that would be great! 👍

[thisismydesign]

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?

[tonywok]

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?

[eserra]

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