find_or_create_by! behavior raises RecordNotFound in Rails 7.1

[tubaxenor]

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", branch: "7-1-stable"
  # Switch to below works
  # gem "rails", github: "rails/rails", branch: "7-0-stable"
  gem "mysql2"
end

require "active_record"
require "minitest/autorun"
require "logger"

opts = {
  database: "unique_keys_test",
  adapter: "mysql2",
  username: "root"
}
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.establish_connection(opts.except(:database))
ActiveRecord::Base.connection.drop_database(opts[:database])
ActiveRecord::Base.connection.create_database(opts[:database])
ActiveRecord::Base.establish_connection(opts)

ActiveRecord::Schema.define do
  create_table :tags, force: true do |t|
    t.string :foo
    t.bigint :foo_id
    t.string :name
    t.index [:foo, :foo_id], unique: true
  end
end

class Tag < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_find_or_create_by
    2.times do |t|
      begin
        Tag.find_or_create_by!(foo: 'Foo', foo_id: 1, name: "foo_#{t}")
      rescue ActiveRecord::RecordNotUnique
        # some app logic that handles RecordNotUnique
      end
    end

    assert_equal 1, Tag.count
  end
end

Expected behavior

This is almost the same as #48035 but without concurrency involved.

In Rails 7.0, find_or_create_by! with duplicated uniqueness constraints raises ActiveRecord::RecordNotUnique error

Actual behavior

In Rails 7.1, the above example raises ActiveRecord::RecordNotFound with following sql queries:

D, [2024-02-21T09:55:06.652671 #36179] DEBUG -- :   Tag Load (0.7ms)  SELECT `tags`.* FROM `tags` WHERE `tags`.`foo` = 'Foo' AND `tags`.`foo_id` = 1 AND `tags`.`name` = 'foo_0' LIMIT 1
D, [2024-02-21T09:55:06.653057 #36179] DEBUG -- :   TRANSACTION (0.1ms)  BEGIN
D, [2024-02-21T09:55:06.665723 #36179] DEBUG -- :   Tag Create (0.3ms)  INSERT INTO `tags` (`foo`, `foo_id`, `name`) VALUES ('Foo', 1, 'foo_0')
D, [2024-02-21T09:55:06.666365 #36179] DEBUG -- :   TRANSACTION (0.6ms)  COMMIT
D, [2024-02-21T09:55:06.666758 #36179] DEBUG -- :   Tag Load (0.2ms)  SELECT `tags`.* FROM `tags` WHERE `tags`.`foo` = 'Foo' AND `tags`.`foo_id` = 1 AND `tags`.`name` = 'foo_1' LIMIT 1
D, [2024-02-21T09:55:06.666985 #36179] DEBUG -- :   TRANSACTION (0.1ms)  BEGIN
D, [2024-02-21T09:55:06.667332 #36179] DEBUG -- :   Tag Create (0.4ms)  INSERT INTO `tags` (`foo`, `foo_id`, `name`) VALUES ('Foo', 1, 'foo_1')
D, [2024-02-21T09:55:06.667620 #36179] DEBUG -- :   TRANSACTION (0.2ms)  ROLLBACK
D, [2024-02-21T09:55:06.667940 #36179] DEBUG -- :   Tag Load (0.2ms)  SELECT `tags`.* FROM `tags` WHERE `tags`.`foo` = 'Foo' AND `tags`.`foo_id` = 1 AND `tags`.`name` = 'foo_1' LIMIT 1

System configuration

Rails version: 7.1.3

Ruby version: 3.3.0

[ngan]

cc @fatkodima @casperisfine -- since you guys will have the most context having fixed the other related issue.

[fatkodima]

This was changed in #45720.

I think, the correct use was like the following (even before that change):

2.times do |t|
  begin
    Tag.find_or_create_by!(foo: 'Foo', foo_id: 1) do |tag|
      tag.name = "foo_#{t}"
    end
  rescue ActiveRecord::RecordNotUnique
    # some app logic that handles RecordNotUnique
  end
end

[casperisfine]

Yeah, I agree with @fatkodima here, only the unique keys should be passed as keyword arguments, it's evidenced by the performed queries on both versions:

Tag Load (0.2ms)  SELECT `tags`.* FROM `tags` WHERE `tags`.`foo` = 'Foo' AND `tags`.`foo_id` = 1 AND `tags`.`name` = 'foo_1' LIMIT 1

Note the:

AND `tags`.`name` = 'foo_1'

I understand that you were relying on that behavior, but that never was the intended use.

Maybe you could extend a bit more on what your use case is? The reduced test case is nice for showing the change in behavior, but doesn't help figuring out if there is a legitimate reason to expose such behavior or if you were better served by using it differently.

[ngan]

This makes sense to me. We can close out this issue. We'll address the code on our end. Thanks!