Don't clear transaction state after manual rollback #32862
Conversation
If an `ActiveRecord::Rollback` error was raised by a persistence method (e.g. in an `after_save` callback), this logic would potentially discard the original state of the record from before the transaction, preventing it from being restored later when the transaction was rolled back.
|
r? @schneems (@rails-bot has picked a reviewer for you, use r? to override) |
|
Thanks! 💯 🥇
I'm not familiar with that part of the code but I noticed that On a side note I think this PR will also fix #32848 |
|
The lighthouse ticket for the patch describes why I'd like to remove I checked the repro scripts from #32847 and #32848 on this branch, and they both still fail. I think I have an idea of what's causing both issues though; I'll triage them later. |
Once rails#32862 is merged, rolling back a record will rollback it's state to match the state before the database changes were applied
Don't clear transaction state after manual rollback
* 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]
* 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]
See #32796 (comment).
If an
ActiveRecord::Rollbackerror was raised by a persistence method (e.g. in anafter_savecallback), this logic would potentially discard the original state of the record from before the transaction, preventing it from being restored later when the transaction was rolled back.This code was originally added in #5535, ironically to address the same problem being fixed here!
The transaction wrapped around a
savecall is itself wrapped withrollback_active_record_state!, which restores the record's original state ifsaveraises an error. RaisingActiveRecord::Rollbackrolls back the transaction without raising an error, sorollback_active_record_state!would incorrectly discard the record's original state instead of restoring it, as if the transaction had been committed.#5535 solved this by decrementing the
levelcounter that ensures only the outermost nested block restores the original state, letting the transaction restore it instead ofrollback_active_record_state!.(Incidentally, a better fix would have been to delete
rollback_active_record_state!; it's vestigial.)The tests added with the original fix used an observer instead of a callback to demonstrate the problem, and were removed in ccecab3 when observers were extracted to a separate gem.
Later when #9068 was merged, the behaviour regressed, because the record's original state is now lazily restored the first time it's accessed after the transaction, instead of immediately when a rollback occurs. So instead of the state being restored when the transaction rolled back, it was discarded by
rollback_active_record_state!before the application had a chance to trigger the restore.