Skip to content

Commit 474a417

Browse files
Edouard-chinbyroot
authored andcommitted
Fix create_or_find_by not rolling back a transaction:
- Fix rails#54830 (This indirectly fix the issue) - ### Problem The `create_or_find_by` method doesn't allow rolling back a transaction. This is a different behaviour than its counterpart method `find_or_create_by`. The reason is because we are in the double transaction edge case and the outer transaction doesn't see the rollback. ### Details ```ruby class User < ApplicationRecord after_save do raise ActiveRecord::Rollback end end User.find_or_create_by(name: "John") => Correctly rolled back User.create_or_find_by(name: "John") => Does not roll back ``` ### Solution We need to be able to know whether the inner transaction succeeded, which is not possible using `.create`, as this method always returns the record (rather than the status of the transaction). https://github.com/rails/rails/blob/1eac7f27748ffa43c5ce91c036c928fc22d4c597/activerecord/lib/active_record/persistence.rb#L39 Using `new` followed by a save is equivalent to `create`, but with `save` we get the transaction return status and we can properly rollback the outer transaction.
1 parent 7645f01 commit 474a417

File tree

3 files changed

+36
-2
lines changed

3 files changed

+36
-2
lines changed

activerecord/lib/active_record/relation.rb

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,12 @@ def find_or_create_by!(attributes, &block)
272272
# such situation.
273273
def create_or_find_by(attributes, &block)
274274
with_connection do |connection|
275-
transaction(requires_new: true) { create(attributes, &block) }
275+
record = nil
276+
transaction(requires_new: true) do
277+
record = new(attributes, &block)
278+
record.save || raise(ActiveRecord::Rollback)
279+
end
280+
record
276281
rescue ActiveRecord::RecordNotUnique
277282
if connection.transaction_open?
278283
where(attributes).lock.find_by!(attributes)
@@ -287,7 +292,12 @@ def create_or_find_by(attributes, &block)
287292
# is raised if the created record is invalid.
288293
def create_or_find_by!(attributes, &block)
289294
with_connection do |connection|
290-
transaction(requires_new: true) { create!(attributes, &block) }
295+
record = nil
296+
transaction(requires_new: true) do
297+
record = new(attributes, &block)
298+
record.save! || raise(ActiveRecord::Rollback)
299+
end
300+
record
291301
rescue ActiveRecord::RecordNotUnique
292302
if connection.transaction_open?
293303
where(attributes).lock.find_by!(attributes)

activerecord/test/cases/relations_test.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1593,6 +1593,24 @@ def test_create_or_find_by
15931593
assert_not_equal subscriber, Subscriber.create_or_find_by(nick: "cat")
15941594
end
15951595

1596+
def test_create_or_find_by_rollbacks_a_transaction
1597+
assert_no_difference(-> { Car.count }) do
1598+
car = BrokenCar.create_or_find_by(name: "Civic")
1599+
1600+
assert_instance_of(BrokenCar, car)
1601+
assert_not_predicate(car, :persisted?)
1602+
end
1603+
end
1604+
1605+
def test_create_or_find_by_bang_rollbacks_a_transaction
1606+
assert_no_difference(-> { Car.count }) do
1607+
car = BrokenCar.create_or_find_by!(name: "Civic")
1608+
1609+
assert_instance_of(BrokenCar, car)
1610+
assert_not_predicate(car, :persisted?)
1611+
end
1612+
end
1613+
15961614
def test_create_or_find_by_with_block
15971615
assert_nil Subscriber.find_by(nick: "bob")
15981616

activerecord/test/models/car.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,9 @@ class CoolCar < Car
3535
class FastCar < Car
3636
default_scope { order("name desc") }
3737
end
38+
39+
class BrokenCar < Car
40+
after_save do
41+
raise ActiveRecord::Rollback
42+
end
43+
end

0 commit comments

Comments
 (0)