#1735 after_save callback should not be called if a before_* callback was cancelled - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#1735 ✓committed

after_save callback should not be called if a before_* callback was cancelled

Reported by Michael Lovitt | January 12th, 2009 @ 05:35 AM | in 2.x

Two problems:

When creating an AR object, if a before_create callback returns false, any after_save callbacks will still be invoked.
When updating an AR object, if a before_update callback returns false, any after_save callbacks will still be invoked.

This behavior is incorrect. According to the Rails docs, "if a before_* callback returns false, all the later callbacks and the associated action are canceled."

I've attached a patch with tests and a fix.

after_save_callback_should_not_be_called_when_prior_callbacks_are_cancelled.diff 3.7 KB

Comments and changes to this ticket

You flagged this item as spam.
Pablo Mercado January 15th, 2009 @ 05:33 PM
I have observed the reported behaviour. The scenario I observed is this: A before_create callback 'cancels' the save; the object is not saved. the after_save callback is still called.

The documentation suggests that the after_save callback would not be called in such a scenario.

I have reviewed the patch; the patch is straight-forward and effective. I also reviewed the test cases; I am glad to see that, with the application of the patch, the cancelling of callback chains is actually tested now (I could not find a test cases for this scenario prior to applying the patch).
José Valim January 15th, 2009 @ 09:03 PM
I don't know if this is correct or not.

I think it would break a lot of applications, so if this patch is accepted, at least a deprecation warning should be set.
Michael Koziarski January 15th, 2009 @ 09:09 PM
This change is consistent with the documentation so I'm going to apply it.

However I think we'll just put it into 2.3, not a 2.2 point release. This should limit the risk of surprise breakage.
Repository January 15th, 2009 @ 09:12 PM
- State changed from “new” to “committed”
(from [7a0e7c7270548138a333bc39aab5aec80580174b]) Fixed broken after_save callback; was being called when before_create was canceled or before_update was canceled

Signed-off-by: Michael Koziarski michael@koziarski.com [#1735 state:committed] http://github.com/rails/rails/co...
You flagged this item as spam.
Mathijs Kwik January 18th, 2009 @ 10:12 AM
The description of the bug in the changelog is wrong. Look at the code before the patch, it has: "return false if callback(:before_save) == false" in it. So the case mentioned in the changelog (after save still executing if before_save fails) is wrong.

The problem this patch is trying to fix, is that after_save is being called if a before_create or before_update has cancelled the saving. Not a before_save. That one worked already. This is also the reason why the other before/after callbacks didn't need 'fixing'