It looks like various places in the code expected the transaction to silently fail, and relied on other checks to detect a problem and throw different exceptions. But when ActiveRecord::Rollback gets propagated, those checks never run, and ActiveRecord::Rollback is swallowed at the top level, resulting in test failures like:
ActiveRecord::RecordNotDestroyed expected but nothing was raised.
This is completely lunatic. Using ActiveRecord::Rollback is NEVER safe, since Rails / ActiveRecord code clearly uses unsafe nested transactions that will just swallow your exception in some situations. If they didn’t, there would be no test failures for that PR.
wow, great research I feel like I should have done that :D. I felt like opening a discussion up again about it at Rails but as other similar bugs have been discarded with “it’s not perfect but this would break too many apps” I sort of gave up on it before I even started :|
Yeah, that’s probably what would happen. I don’t think it’s a huge loss though. I believe all errors should be specific, detailed, with all useful context, so I’d never raise a ridiculously generic exception like ActiveRecord::Rollback. It’s a hokey code ergonomics trick that looks cute at first glance, but isn’t actually useful. Even if it worked properly, I can’t think of any situation where using ActiveRecord::Rollback would improve code quality. Raise an exception that actually means something.
In a case like this where the framework is practically priming a landmine for you to step on, I would say you’d rather fix the framework than read the docs. If your ORM has, for all intents and purposes, completely broken transactions, you’re not allowed to hide behind the docs.
The article says nested transactions don’t exist, but isn’t this what savepoints are about? Savepoint before the “nested transaction”, and jump back to that if you need to roll it back.
Seems pretty straightforward, but I could be missing something
Yes, absolutely. But currently, wrapping in .transaction doesn’t create a savepoint by default, it just wraps the block with a rescue that silently swallows ActiveRecord::Rollback. You have to explicitly say .transaction(requires_new: true) every time you want a savepoint.
This is dumb. And it’s not even for a reason, the original commit that implements nested transactions with savepoints is just buggy. And in the original discussion about the diff no one seemed to notice. Searching around I’ve found numerous issues referencing this problem without the devs acknowledging it exists, until I found this issue that realizes this is a problem. Then the guy that recognized it was an issue opened a PR that tries to fix the bug, but the CI build for the PR had a bunch of failures and he just kinda gave up.
Lame.
It looks like various places in the code expected the transaction to silently fail, and relied on other checks to detect a problem and throw different exceptions. But when
ActiveRecord::Rollbackgets propagated, those checks never run, andActiveRecord::Rollbackis swallowed at the top level, resulting in test failures like:This is completely lunatic. Using
ActiveRecord::Rollbackis NEVER safe, since Rails / ActiveRecord code clearly uses unsafe nested transactions that will just swallow your exception in some situations. If they didn’t, there would be no test failures for that PR.wow, great research I feel like I should have done that :D. I felt like opening a discussion up again about it at Rails but as other similar bugs have been discarded with “it’s not perfect but this would break too many apps” I sort of gave up on it before I even started :|
Maybe it’s worth another try though…
Yeah, that’s probably what would happen. I don’t think it’s a huge loss though. I believe all errors should be specific, detailed, with all useful context, so I’d never raise a ridiculously generic exception like
ActiveRecord::Rollback. It’s a hokey code ergonomics trick that looks cute at first glance, but isn’t actually useful. Even if it worked properly, I can’t think of any situation where usingActiveRecord::Rollbackwould improve code quality. Raise an exception that actually means something.In a case like this where the framework is practically priming a landmine for you to step on, I would say you’d rather fix the framework than read the docs. If your ORM has, for all intents and purposes, completely broken transactions, you’re not allowed to hide behind the docs.
The article says nested transactions don’t exist, but isn’t this what savepoints are about? Savepoint before the “nested transaction”, and jump back to that if you need to roll it back.
Seems pretty straightforward, but I could be missing something
Yes, absolutely. But currently, wrapping in
.transactiondoesn’t create a savepoint by default, it just wraps the block with a rescue that silently swallowsActiveRecord::Rollback. You have to explicitly say.transaction(requires_new: true)every time you want a savepoint.that’s unfortunate. I suppose the initial implementation didn’t have that parameter and for backwards compatibility they needed to keep that default.
Stuff is hard
Yep pretty much. I tracked the lifetime of the behavior in my comment above.