This project is archived and is in readonly mode.
has_many through transaction rollback
Reported by 2 College Bums | August 28th, 2008 @ 04:33 AM | in 3.x
When saving a model that contains a has_many through association, the entire transaction fails to rollback when the validation fails. Specifically xxx_ids= should not modify the association unless all validations pass.
Example: Let's assume we have a Paper model that contains a title attribute and a has_many through relationship with Categories. We validate to ensure a paper has at least one category and the title is not invalid.
If the title is invalid and the categories are empty and an "update_attributes" occurs even after the validation fails, all associations to categories are lost. The title is not updated as expected. Preferably, the association to categories would not be affected unless a save is successful.
a = Paper.first
a.categories # => [#category1, #category2]
a.update_attributes(:category_ids => [], :title => "invalid") # => false
a.categories # => []
a.title # => "invalid"
a.valid? # => false
a.errors.full_messages # => ["title cannot be invalid", "At least one category must be assigned"]
a.reload
a.title # => "Original"
a.categories # => []
a.valid? # => false
There is a quick fix that doesn't completely solve the problem. In the model where the update_attributes occurs we can force a "rollback" using the following code:
def update_attributes_with_rollback(*args)
old_category_ids = self.category_ids
saved_properly = update_attributes_without_rollback(*args)
self.category_ids = old_category_ids unless saved_properly
saved_properly
end
alias_method_chain :update_attributes, :rollback
Here are some resources which aided us and may help:
Comments and changes to this ticket
-
Xavier Noria August 31st, 2008 @ 09:20 PM
In edge, if any validation fails there's a rollback of the transaction that wraps save (that's since #891).
Could you please test if that solves this issue?
-
2 College Bums September 5th, 2008 @ 10:20 PM
This patch does not solve this issue. We tested it in edge and the use case above still fails. We checked to make sure that save_with_transactions was called, and the rollback still did not completely revert the has_many through association.
-
Xavier Noria September 6th, 2008 @ 02:12 AM
I think I see the problem.
The setter
category_ids=
updates the associated categories in the database right away.On the other hand
update_attributes
is implemented like this:def update_attributes(attributes) self.attributes = attributes save end
and that call to
attributes=
ends up invokingcategory_ids=
:respond_to?(:"#{k}=") ? send(:"#{k}=", v) : raise(UnknownAttributeError, "unknown attribute: #{k}")
Clearly that one is not wrapped by the transaction of
save
. -
Xavier Noria September 6th, 2008 @ 04:31 AM
OK, here's a fix. We wrap the update_attribute* family in transactions.
-
Xavier Noria September 6th, 2008 @ 04:36 AM
Touched a couple of details in the original diff, here's an update.
-
Pratik December 20th, 2008 @ 03:50 PM
- Milestone cleared.
- Assigned user set to Michael Koziarski
-
Michael Koziarski February 1st, 2009 @ 02:11 AM
I think this is good to go, but no longer applies cleanly. Resubmit and I'll apply it
-
Repository February 22nd, 2009 @ 02:42 AM
- State changed from new to committed
(from [fc09ebc669bd58f415f7d3ef932ef02dab821ab5]) Wrap calls to update_attributes in a transaction.
Signed-off-by: Michael Koziarski michael@koziarski.com [#922 state:committed] http://github.com/rails/rails/co...
-
Repository February 22nd, 2009 @ 03:41 AM
(from [06040849b5a680c2a87893699580f9b9b80f72e4]) Revert "Wrap calls to update_attributes in a transaction."
This caused failures on sqlite, sqlite3 and postgresql
This reverts commit fc09ebc669bd58f415f7d3ef932ef02dab821ab5. [#922 state:reopened] http://github.com/rails/rails/co...
-
Michael Koziarski February 22nd, 2009 @ 03:43 AM
- State changed from committed to open
-
DHH March 18th, 2009 @ 04:34 PM
- Milestone set to 2.x
-
Brad Pauly April 25th, 2009 @ 05:22 AM
I think the failures were because the default in delete_records is to nullify and there are db constraints. By changing the audit_logs in Developer to dependent => :destroy gets around that. I am playing with tests to see if I can get them all to pass.
-
Brad Pauly April 25th, 2009 @ 05:49 AM
I'm not sure I completely understand, but I'm wondering if update_attribute should also do this since it doesn't do validation and update_attribute(:foo_ids, []) seems okay to me.
-
Brad Pauly April 25th, 2009 @ 08:08 AM
Posting a patch if anyone is interested. Essentially the same as Xavier posted but without update_attribute and with the change to developer model.
-
Xavier Noria April 25th, 2009 @ 09:29 AM
Hey Brad, thanks for the followup Real Life interrupted this :).
update_attribute does not run validations but it runs callbacks, there's still the chance that something triggers a rollback or has a side-effect in the database.
The reason I included update_attribute is not that one though, because callbacks are run inside a call to save(). Problem is update_attribute is not atomic because the setter itself runs outside the transaction of save().
-
Brad Pauly April 29th, 2009 @ 06:35 AM
Hi Xavier, Thanks for explaining. I went back to your original patch and started over. I have MySQL and Sqlite3 passing, just need to figure out Postgresql.
-
Trey Dempsey July 20th, 2009 @ 11:22 PM
I've started to dig in to this for Postgresql. What I'm seeing in 2.3.2 on the ruby pg-0.80 driver is that validation fails and ActiveRecord::Transactions raises an ActiveRecord::Rollback. However in ActiveRecord::ConnectionAdapters::DatabaseStatements#transaction the rollback_db_transaction is never invoked because the logic for tracking whether or not a transaction is open returns false, causing the rollback to not occur. It looks like transaction_open, requires_new, or open_transactions is not keeping state correctly.
-
Michael Koziarski July 23rd, 2009 @ 05:01 AM
Trey, can you isolate this problem in a stand-alone test for AR?
that'd let us split this ticket up into the has_many problem and the
postgres-transaction-not-working problem -
Amy25 January 15th, 2010 @ 08:25 PM
- Tag cleared.
Buy essay and written essay and be insured you have all facts about this good post*.
-
Neeraj Singh July 5th, 2010 @ 04:41 AM
- Importance changed from to
Attached is patch against rails3 master. All the tests are passing.
All the work has been done by others so they should be given the commit credit.
-
Neeraj Singh July 13th, 2010 @ 05:50 PM
- Importance changed from to High
I am bumping the priority on this one. Fixing this would also fix ticket like #2933.
-
Neeraj Singh July 13th, 2010 @ 08:34 PM
- Assigned user changed from Michael Koziarski to José Valim
- Tag set to rails 3, activerecord, patch
previous patch did not apply cleanly. Attached is updated patch
-
Repository July 13th, 2010 @ 09:05 PM
(from [f4fbc2c1f943ff11776b2c7c34df6bcbe655a4e5]) update_attributes and update_attributes! are now wrapped in a transaction
[#922 state:resovled]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/f4fbc2c1f943ff11776b2c7c34df6b... -
José Valim July 14th, 2010 @ 07:03 AM
- State changed from open to resolved
-
Neeraj Singh July 14th, 2010 @ 03:50 PM
- State changed from resolved to open
Attaching the backported fix for 2-3-stable.
-
Repository July 18th, 2010 @ 10:20 AM
- State changed from open to resolved
(from [99cdea7cbe69f7ea9ef82bdcf9502e47e9e4e07b]) update_attribute and updated_attributes! are now wrapped in a transaction
[#922 state:resolved]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/99cdea7cbe69f7ea9ef82bdcf9502e...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Tags
Referenced by
- 922 has_many through transaction rollback Signed-off-by: Michael Koziarski michael@koziarski.com [#...
- 922 has_many through transaction rollback This reverts commit fc09ebc669bd58f415f7d3ef932ef02dab821...
- 2933 2.3.2 update_attributes save many-to-many associations If the fix for #922 is applied then that would resolve th...
- 922 has_many through transaction rollback [#922 state:resovled]
- 2933 2.3.2 update_attributes save many-to-many associations Neeraj, could you then backport #922 to 2-3-stable so I c...
- 2933 2.3.2 update_attributes save many-to-many associations #922 fix for 2-3-stable has been attached in ticket #922 .
- 2933 2.3.2 update_attributes save many-to-many associations #922 fix for 2-3-stable has been attached in ticket #922 .
- 2933 2.3.2 update_attributes save many-to-many associations Ok. #922 applied.
- 922 has_many through transaction rollback [#922 state:resolved]