Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rollback parent transaction when children fails to update #32796

Conversation

@gmalette
Copy link
Contributor

@gmalette gmalette commented May 2, 2018

Summary

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. This PR starts making it rollback.

Details

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 persistence of the Company already starts a transaction that nests
it's associated record updates. However, it didn't track the success or failure of it's
associations 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.

@rails-bot
Copy link

@rails-bot rails-bot commented May 2, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Loading

@@ -401,7 +401,10 @@ def save_collection_association(reflection)
if autosave
saved = association.insert_record(record, false)
else
association.insert_record(record) unless reflection.nested?
association_saved = association.insert_record(record) unless reflection.nested?
if reflection.options.fetch(:validate, true)
Copy link
Contributor Author

@gmalette gmalette May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is validate: nil considered true?

Loading

Copy link
Member

@eugeneius eugeneius May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is:

# == Validation
#
# Child records are validated unless <tt>:validate</tt> is +false+.

We should be calling reflection.validate? here though, as define_autosave_validation_callbacks does:

if reflection.validate? && !method_defined?(validation_method)

Loading

assert_predicate client, :valid?
assert_predicate new_firm, :valid?
assert_not new_firm.save
assert new_firm, :persisted?
Copy link
Contributor Author

@gmalette gmalette May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not thrilled that new_firm is persisted?, but this is a side-effect of the insert of new_firm succeeding and the database providing an ID, even if the transaction is rolled back later. Thoughts?

Loading

Copy link
Member

@Edouard-chin Edouard-chin May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be prone to obscure bug inside application

new_firm.persisted? => true
new_firm.new_record? => false
Firm.exists?(new_firm.id) => false

Should we instead reset the @new_record ivar when the transaction is rolled back?

Loading

Copy link
Contributor Author

@gmalette gmalette May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If everyone agrees that's the solution, I'm all for it and I can do it, I just didn't want to change the existing behaviour

Loading

Copy link
Contributor Author

@gmalette gmalette May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the ID and other database-provided attributes set on creation, like created_at, updated_at?

Loading

Copy link
Contributor Author

@gmalette gmalette May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More on this: it looks like there is already code that handles rolling back the attributes

def rollback_active_record_state!
remember_transaction_record_state
yield
rescue Exception
restore_transaction_record_state

However, it looks like ActiveRecord::Rollback is swallowed before it reaches that point.

def transaction(requires_new: nil, isolation: nil, joinable: true)
if !requires_new && current_transaction.joinable?
if isolation
raise ActiveRecord::TransactionIsolationError, "cannot set isolation when joining a transaction"
end
yield
else
transaction_manager.within_new_transaction(isolation: isolation, joinable: joinable) { yield }
end
rescue ActiveRecord::Rollback
# rollbacks are silently swallowed

I'm absolutely not familiar with this code so I'd really appreciate pointers

Loading

Copy link
Member

@eugeneius eugeneius May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#32862 should fix the rollback behaviour so that new_firm.persisted? is false.

Incidentally, the assert here will never fail, because new_firm will always be truthy; it looks like it should be assert_predicate instead.

Loading

Copy link
Contributor Author

@gmalette gmalette May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#32862 should fix the rollback behaviour so that new_firm.persisted? is false.

Awesome! I'll rebase and make the changes when it gets merged.

Incidentally, the assert here will never fail

😂 that takes me back to #20527

Loading

Copy link
Member

@eugeneius eugeneius May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

I've also bumped into this problem before: cypriss/mutations#110

Loading

gmalette added 2 commits May 14, 2018
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.
Once rails#32862 is merged, rolling back a record will rollback it's state to match
the state before the database changes were applied
@gmalette gmalette force-pushed the rollback-outer-transaction-when-persisting-parent-and-children branch from a88675b to b8ed1b8 May 14, 2018
@gmalette
Copy link
Contributor Author

@gmalette gmalette commented May 14, 2018

Now that #32862 is merged I rebased, and the tests are passing

Loading

@@ -401,7 +401,10 @@ def save_collection_association(reflection)
if autosave
saved = association.insert_record(record, false)
else
association.insert_record(record) unless reflection.nested?
association_saved = association.insert_record(record) unless reflection.nested?
Copy link
Member

@eugeneius eugeneius May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If reflection.nested? is true, we should probably skip reassigning saved.

Loading

@@ -401,7 +401,10 @@ def save_collection_association(reflection)
if autosave
saved = association.insert_record(record, false)
else
association.insert_record(record) unless reflection.nested?
association_saved = association.insert_record(record) unless reflection.nested?
if reflection.options.fetch(:validate, true)
Copy link
Member

@eugeneius eugeneius May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is:

# == Validation
#
# Child records are validated unless <tt>:validate</tt> is +false+.

We should be calling reflection.validate? here though, as define_autosave_validation_callbacks does:

if reflection.validate? && !method_defined?(validation_method)

Loading

def test_adding_unsavable_association
new_firm = Firm.new("name" => "A New Firm, Inc")
client = new_firm.clients.new("name" => "Apple")
# Stub the save method of the client to simulate aborting the insert
Copy link
Member

@eugeneius eugeneius May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this simulating throw :abort in a before callback? Could we make that happen for real? Rails' tests are generally written end-to-end, using only the public API.

Loading

Copy link
Contributor Author

@gmalette gmalette May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually came across the bug addressed in this PR trying to fix Shopify/activerecord-rescue_from_duplicate#13, and I just replicated it's behaviour, but I think it will work with throw :abort in a before_save just as well.

Loading

assert_predicate new_firm, :valid?
assert_not new_firm.save
assert_not_predicate new_firm, :persisted?
assert_not Firm.exists?(new_firm.id)
Copy link
Member

@eugeneius eugeneius May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that persisted? works correctly, this feels redundant.

Loading

@gmalette gmalette force-pushed the rollback-outer-transaction-when-persisting-parent-and-children branch from 6fac40b to 3bb3008 May 21, 2018
@gmalette
Copy link
Contributor Author

@gmalette gmalette commented May 21, 2018

@eugeneius I refactored to use the public API with throw :abort, along with your other suggestions

Loading

Copy link
Member

@eugeneius eugeneius left a comment

This now looks good to me! 👍

Loading

@rafaelfranca rafaelfranca merged commit 976ef40 into rails:master May 22, 2018
2 checks passed
Loading
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 22, 2018

Thanks!

Loading

rafaelfranca added a commit that referenced this issue May 22, 2018
* 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]
@gmalette gmalette deleted the rollback-outer-transaction-when-persisting-parent-and-children branch Aug 8, 2018
@ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Jan 21, 2019

Hi @gmalette just wondering if you would consider #35009 to be a regression?

Loading

@gmalette
Copy link
Contributor Author

@gmalette gmalette commented Jan 22, 2019

I’m not familiar enough with the contract of AR with regards to validate: false to know for sure, but to me it looks like a regression

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants