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