Allow manual rollbacks in after_save to reset object correctly #5535

Merged
merged 1 commit into from May 16, 2012

Conversation

Projects
None yet
3 participants
@markmcspadden
Contributor

markmcspadden commented Mar 21, 2012

Started digging into #5527 and was at least able to confirm the case was happening in the given situation, find what was causing it, and write some tests to further confirm it and the fix.

Now...I don't really pretend to know if actually fixing this is a good idea or not. The example raises ActiveRecord::Rollback in an after_save hook. I can't find any examples that this is actually a good idea in the first place.

I'm also not keen on where I added the tests or the actual solution....so all around an iffy PR. But all the tests pass...so that's something.

It's my first dive into AR::Transactions, so if this is a bad idea, close the PR and no love lost. If it just needs some cleanup, let me know where to start.

@markmcspadden

This comment has been minimized.

Show comment Hide comment
@markmcspadden

markmcspadden Mar 22, 2012

Contributor

After some clarification on #5527 I think this does need to be addressed.

Looks like @jeremy or @fxn or @wycats might be the best to have a look. Thx.

Contributor

markmcspadden commented Mar 22, 2012

After some clarification on #5527 I think this does need to be addressed.

Looks like @jeremy or @fxn or @wycats might be the best to have a look. Thx.

@markmcspadden

This comment has been minimized.

Show comment Hide comment
@markmcspadden

markmcspadden Mar 26, 2012

Contributor

Would still appreciate some feedback on this whenever someone gets a bit of time. Maybe @tenderlove ?

I know it doesn't have a bounty on it...nor does it address a predicate function...but it may still be worth a look...maybe? ;)

Contributor

markmcspadden commented Mar 26, 2012

Would still appreciate some feedback on this whenever someone gets a bit of time. Maybe @tenderlove ?

I know it doesn't have a bounty on it...nor does it address a predicate function...but it may still be worth a look...maybe? ;)

@markmcspadden

This comment has been minimized.

Show comment Hide comment
@markmcspadden

markmcspadden Apr 2, 2012

Contributor

Friendly weekly bump...

Fixes issue caused by using raise ActiveRecord::Rollback in an after_save hook....

Contributor

markmcspadden commented Apr 2, 2012

Friendly weekly bump...

Fixes issue caused by using raise ActiveRecord::Rollback in an after_save hook....

@markmcspadden

This comment has been minimized.

Show comment Hide comment
@markmcspadden

markmcspadden Apr 13, 2012

Contributor

Anyone out there that would like to take a look at this? Give feedback? Merge?

Proposed fix to issue #5527 caused by using raise ActiveRecord::Rollback in an after_save hook....

/cc @jeremy @fxn @tenderlove (feel free to include anyone you think would be best suited for it)

Contributor

markmcspadden commented Apr 13, 2012

Anyone out there that would like to take a look at this? Give feedback? Merge?

Proposed fix to issue #5527 caused by using raise ActiveRecord::Rollback in an after_save hook....

/cc @jeremy @fxn @tenderlove (feel free to include anyone you think would be best suited for it)

@seanarnold

This comment has been minimized.

Show comment Hide comment
@seanarnold

seanarnold May 15, 2012

I'd use this if it was available. Currently writing a method to update (rollback) a model to it's previous attributes

I'd use this if it was available. Currently writing a method to update (rollback) a model to it's previous attributes

+ status = yield
+ rescue ActiveRecord::Rollback
+ if defined?(@_start_transaction_state)
+ @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove May 16, 2012

Owner

Is it OK for transaction levels to be -1?

@tenderlove

tenderlove May 16, 2012

Owner

Is it OK for transaction levels to be -1?

This comment has been minimized.

Show comment Hide comment
@markmcspadden

markmcspadden May 16, 2012

Contributor

I think possibly so. It has potential to enter that state in #clear_transaction_record_state

https://github.com/markmcspadden/rails/blob/c8792c7b2ea4f5fe7a5610225433ea8dd8d0f83e/activerecord/lib/active_record/transactions.rb#L325

Also please note this from the PR description: It's my first dive into AR::Transactions...

@markmcspadden

markmcspadden May 16, 2012

Contributor

I think possibly so. It has potential to enter that state in #clear_transaction_record_state

https://github.com/markmcspadden/rails/blob/c8792c7b2ea4f5fe7a5610225433ea8dd8d0f83e/activerecord/lib/active_record/transactions.rb#L325

Also please note this from the PR description: It's my first dive into AR::Transactions...

tenderlove added a commit that referenced this pull request May 16, 2012

Merge pull request #5535 from markmcspadden/issue_5527_rollbacks
Allow manual rollbacks in after_save to reset object correctly

@tenderlove tenderlove merged commit 8e5f07d into rails:master May 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment