Skip to content

Commit d75997e

Browse files
committed
Fix find_or_create_by! not checking owner persistence:
- Fix rails#54920 - ### Problem In rails#54845, I modified a call to `Record#create` for `Record#new` followed by `Record#save`. I didn't seen that the behaviour between both is different on associations. https://github.com/rails/rails/blob/b97917da8a801bf5360c3a9da913415bc8582735/activerecord/lib/active_record/associations/collection_association.rb#L355-L356 ```ruby car = Car.new car.wheels.create # Check if `car` is persisted before creating a wheel. wheel = car.wheels.new wheel.save # Doesn't check if `car` is persisted ``` ### Solution The original reason why I modified `#create` is because it doesn't allow to get the result of the transaction status within a nested transaction. The documentation explains it and mention to raise a rollback so that the outer transaction sees it but it misses an important detail: How to know whether the inner transaction was sucessfull since the Rollback error is silently swallowed. https://github.com/rails/rails/blob/b97917da8a801bf5360c3a9da913415bc8582735/activerecord/lib/active_record/transactions.rb#L159-L165 A very simple example: ```ruby class Car < ApplicationRecord after_save do raise ActiveRecord::Rollback end end Car.transaction do car = Car.create! puts car.persisted? # Returns true. That's not correct. end ``` The solution in this patch is to provide a flag on the record to know whether the last transaction was succesful.
1 parent b97917d commit d75997e

File tree

3 files changed

+17
-5
lines changed

3 files changed

+17
-5
lines changed

activerecord/lib/active_record/relation.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,8 +274,8 @@ def create_or_find_by(attributes, &block)
274274
with_connection do |connection|
275275
record = nil
276276
transaction(requires_new: true) do
277-
record = new(attributes, &block)
278-
record.save || raise(ActiveRecord::Rollback)
277+
record = create(attributes, &block)
278+
record._last_transaction_return_status || raise(ActiveRecord::Rollback)
279279
end
280280
record
281281
rescue ActiveRecord::RecordNotUnique
@@ -294,8 +294,8 @@ def create_or_find_by!(attributes, &block)
294294
with_connection do |connection|
295295
record = nil
296296
transaction(requires_new: true) do
297-
record = new(attributes, &block)
298-
record.save! || raise(ActiveRecord::Rollback)
297+
record = create!(attributes, &block)
298+
record._last_transaction_return_status || raise(ActiveRecord::Rollback)
299299
end
300300
record
301301
rescue ActiveRecord::RecordNotUnique

activerecord/lib/active_record/transactions.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ module Transactions
1313
scope: [:kind, :name]
1414
end
1515

16-
attr_accessor :_new_record_before_last_commit # :nodoc:
16+
attr_accessor :_new_record_before_last_commit, :_last_transaction_return_status # :nodoc:
1717

1818
# = Active Record \Transactions
1919
#
@@ -436,6 +436,7 @@ def with_transaction_returning_status
436436
status = yield
437437
raise ActiveRecord::Rollback unless status
438438
end
439+
@_last_transaction_return_status = status
439440
status
440441
end
441442
end
@@ -451,6 +452,7 @@ def trigger_transactional_callbacks? # :nodoc:
451452
def init_internals
452453
super
453454
@_start_transaction_state = nil
455+
@_last_transaction_status = nil
454456
@_committed_already_called = nil
455457
@_new_record_before_last_commit = nil
456458
end

activerecord/test/cases/relations_test.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
require "models/category"
2727
require "models/categorization"
2828
require "models/edge"
29+
require "models/wheel"
2930
require "models/subscriber"
3031
require "models/cpk"
3132

@@ -1611,6 +1612,15 @@ def test_create_or_find_by_bang_rollbacks_a_transaction
16111612
end
16121613
end
16131614

1615+
def test_create_or_find_by_on_a_collections_rollbacks_a_transaction_when_owner_is_not_persisted
1616+
car = BrokenCar.create(name: "Civic")
1617+
assert_not_predicate(car, :persisted?)
1618+
1619+
assert_raises(ActiveRecord::RecordNotSaved) do
1620+
car.wheels.create_or_find_by!(size: 1500)
1621+
end
1622+
end
1623+
16141624
def test_create_or_find_by_with_block
16151625
assert_nil Subscriber.find_by(nick: "bob")
16161626

0 commit comments

Comments
 (0)