aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authorGuillaume Malette <gmalette@gmail.com>2018-05-22 15:58:22 -0400
committerRafael França <rafaelmfranca@gmail.com>2018-05-22 15:58:22 -0400
commit976ef40aefc92a30ca44a1b0322e5b46e32c0d91 (patch)
treeef6be483cb67e3f9c3804d267f9914d1eedc54c1 /activerecord
parent9385a5b5854567a059997d59001ddee0d35c7ed2 (diff)
downloadrails-976ef40aefc92a30ca44a1b0322e5b46e32c0d91.tar.gz
rails-976ef40aefc92a30ca44a1b0322e5b46e32c0d91.tar.bz2
rails-976ef40aefc92a30ca44a1b0322e5b46e32c0d91.zip
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]
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/lib/active_record/autosave_association.rb7
-rw-r--r--activerecord/test/cases/autosave_association_test.rb12
-rw-r--r--activerecord/test/models/company.rb5
3 files changed, 22 insertions, 2 deletions
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index a1250c3835..9575cc24c8 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -400,8 +400,11 @@ module ActiveRecord
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)
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 0c212ecc22..7355f4cd62 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -517,6 +517,18 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa
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
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index fc6488f729..6219f57fa1 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -145,6 +145,11 @@ class Client < Company
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