has_many through transaction rollback

I've just been bitten from this too.

In edge, if any validation fails there's a rollback of the transaction that wraps save (that's since #891 let cancels from before filters issue a ROLLBACK).

Could you please test if that solves this issue?

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.

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 invoking category_ids=:

respond_to?(:"#{k}=") ? send(:"#{k}=", v) : raise(UnknownAttributeError, "unknown attribute: #{k}")

Clearly that one is not wrapped by the transaction of save.

OK, here's a fix. We wrap the update_attribute* family in transactions.

Touched a couple of details in the original diff, here's an update.

And yet another revision.

+1 good one

+1 works

I think this is good to go, but no longer applies cleanly. Resubmit and I'll apply it

Sure!

(from [06040849b5a680c2a87893699580f9b9b80f72e4]) Revert "Wrap calls to update_attributes in a transaction."

This caused failures on sqlite, sqlite3 and postgresql

This reverts commit fc09ebc669bd58f415f7d3ef932ef02dab821ab5. [#922 has_many through transaction rollback state:reopened] http://github.com/rails/rails/co...

I'll have a look at those failures.

Indeed in my machine tests fail even for MySQL now.

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.

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.

Posting a patch if anyone is interested. Essentially the same as Xavier posted but without update_attribute and with the change to developer model.

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().

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.

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.

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

(from [f4fbc2c1f943ff11776b2c7c34df6bcbe655a4e5]) update_attributes and update_attributes! are now wrapped in a transaction

[#922 has_many through transaction rollback state:resovled]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/f4fbc2c1f943ff11776b2c7c34df6b...