save! and rollbacks

While documenting stuff related to transactions and callbacks I've
realized that if a hook raises precisely AR::Rollback save! silently
returns nil. I've documented that behaviour, but it surprised to me
that save! may not raise an exception on failure. Looks like a side
effect of the implementation in fact more than something intended.

What do you think?

I think this behavior is correct: AR::Rollback is never supposed to be
re-raised. That said, this behavior should be explicitly documented.

What puzzles me is the current usage:

   begin
     if model.save!
       # success
     else
       # faillure due to AR::Rollback
     end
   rescue AR::RecordNotSaved
     # failure due to something else
   end

Isn't that a strange pattern for save!?

Yes imho it is. Since AR::Rollback represents in essence a reason
because record is not saved I can't see what would be the reason to be
treated a different way.

I am not enterily sold on this option but we could manually raise
AR::RecordNotFound on AR::Rollback inside save! and of course document
it. Not sure about the implementation because silencing AR::Rollback
is done deep in database_statements.rb but I am just thinking about
the usage by now. That would give an idiomatic way to cancel +
rollback that in turn results in expected client code.

Another option would be to document the current behaviour (this is
already done) and warn people about the resulting if/else. You can
still deal with the rollback in a rescue clause raising something
different from AR::Rollback

   begin
     model.save!
     # success
   rescue AR::RecordNotSaved, MyApp::SomeError => e
     # failure
   end

AR::RecordNotSaved

Isn't that a strange pattern for save!?

Yeah, it is, but I'm not sure that people should be raising
AR::Rollback themselves anyway, it's intended as a way to abort the
transaction and continue processing, and if you're not using it for
that, then you should be raising some other exception :slight_smile:

Good.

If people raise their own exception to rollback, then I think the
strange pattern is found in non-bang save:

   begin
     if save
       # success
     else
       # validation error
     end
   rescue MyRollBack
     # failure due to rollback
   end

Perhaps the message could be that if you may rollback in the callback
chain you normally would only use the save! method?

In fact, I wonder why halting the chain in before_* callbacks commits
the transaction. Wouldn't you expect a rollback? If AR executed a
rollback automatically, in addition, save and save! would preserve
their usage idioms.

That would still leave room for best practices for rollbacks in
after_* hooks, but I think that would be fine.

In fact, I wonder why halting the chain in before_* callbacks commits
the transaction. Wouldn't you expect a rollback? If AR executed a
rollback automatically, in addition, save and save! would preserve
their usage idioms.

That does sound more ... sensible. Why don't you give it a try in a
branch and see what breaks?

Absolutely! I'll write back with something.

Here we go!

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/891-let-cancels-from-before-filters-issue-a-rollback