Permalink
Browse files

Rollback parent transaction when children fails to update (#32796)

* Rollback parent transaction when children fails to update

Rails supports autosave associations on the owner of a `has_many`
relationship. In certain situation, if the children of the association
fail to save, the parent is not rolled back.

```ruby
class Employee < ActiveRecord::Base
end

class Company < ActiveRecord::Base
  has_many(:employees)
end

company = Company.new
employee = company.employees.new
company.save
```

In the previous example, if the Employee failed to save, the Company
will not be rolled back. It will remain in the database with no
associated Employee.

I expect the `company.save` call to be atomic, and either create all or
none of the records.

The persistance of the Company already starts a transaction that nests
it's children. However, it didn't track the success or failure of it's
children in this very situation, and the outermost transaction is not
rolled back.

This PR makes the change to track the success of the child insertion and
rollback the parent if any of the children fail.

* Change the test to reflect what we expect

Once #32862 is merged, rolling back a record will rollback it's state to match
the state before the database changes were applied

* Use only the public API to express the tests

* Refactor to avoid reassigning saved for nested reflections

[Guillaume Malette + Rafael Mendonça França]
  • Loading branch information...
gmalette authored and rafaelfranca committed May 22, 2018
1 parent 75a4c7f commit c87b3346ca6e1d21a6bccb29ccedf0b95fda7abc
@@ -400,8 +400,11 @@ def save_collection_association(reflection)
if autosave != false && (@new_record_before_save || record.new_record?)
if autosave
saved = association.insert_record(record, false)
else
association.insert_record(record) unless reflection.nested?
elsif !reflection.nested?
association_saved = association.insert_record(record)
if reflection.validate?
saved = association_saved
end
end
elsif autosave
saved = record.save(validate: false)
@@ -517,6 +517,18 @@ def test_invalid_adding_before_save
assert_not_predicate new_firm, :persisted?
end

def test_adding_unsavable_association
new_firm = Firm.new("name" => "A New Firm, Inc")
client = new_firm.clients.new("name" => "Apple")
client.throw_on_save = true

assert_predicate client, :valid?
assert_predicate new_firm, :valid?
assert_not new_firm.save
assert_not_predicate new_firm, :persisted?
assert_not_predicate client, :persisted?
end

def test_invalid_adding_with_validate_false
firm = Firm.first
client = Client.new
@@ -145,6 +145,11 @@ class RaisedOnSave < RuntimeError; end
raise RaisedOnSave if raise_on_save
end

attr_accessor :throw_on_save
before_save do
throw :abort if throw_on_save
end

class RaisedOnDestroy < RuntimeError; end
attr_accessor :raise_on_destroy
before_destroy do

0 comments on commit c87b334

Please sign in to comment.