`in_transaction` helper to avoid nesting transaction blocks

[jpcamara]

Nesting transactions is perilous: https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls

use_transaction attempts to simplify that a bit by utilizing in_transaction? to determine if it's safe to assume code is already operating in a transaction and avoid issues with active record swallowing ActiveRecord::Rollback errors.

The reason I think it makes sense for this to live inside of after_commit_everywhere is that the biggest benefits of using after_commit come from being in a transaction. But if you're adding after_commit callbacks in custom code, it can be very difficult to tell if it's safe to wrap in a transaction or if you might coincidentally get wrapped inside one at a higher level of execution. By utilizing use_transaction, you don't have to worry about this situation.

Hopefully that all makes sense, and i'm not missing something!

[jpcamara]

Realizing now that this will need a doc update as well. I'll wait for some feedback, but am happy to write up docs if this gets considered for the gem

[Envek]

Hey, thanks for the pull request!

But if you're adding after_commit callbacks in custom code, it can be very difficult to tell if it's safe to wrap in a transaction or if you might coincidentally get wrapped inside one at a higher level of execution.

That is the main point of after_commit_everywhere to just put after_commit anywhere without worrying whether it is in transaction or not. You don't even need to wrap after_commit call with transaction block if you don't need a transaction in that place (it will be just executed immediately if called outside of transaction).

to assume code is already operating in a transaction and avoid issues with active record swallowing ActiveRecord::Rollback errors.

That is known pitfall, yeah, but I didn't face it myself (hadn't to manually raise callback exception by hand), so kind of surprised that it is a real pain for someone. Can you please tell a bit more about your use case?

[jpcamara]

Hi @Envek, thanks for the response! Also, thanks for the gem - it's been a great asset in my projects!

That is the main point of after_commit_everywhere to just put after_commit anywhere without worrying whether it is in transaction or not. You don't even need to wrap after_commit call with transaction block if you don't need a transaction in that place (it will be just executed immediately if called outside of transaction).

Yep, definitely get that 👍🏼 . I probably wasn't clear enough when I said "the biggest benefits of using after_commit come from being in a transaction". I know it can be used without a transaction, but it's more beneficial to use it in one.

I think it's arguable whether running it outside a transaction would ever really be intentional - but I can see times where specifically wrapping it doesn't make sense - like in the sidekiq transaction_aware_client - wrapping it there would provide no benefit. Library code like that puts the responsibility on the developer - and in the case they forget to wrap it you would still expect the callback to run.

I think the most important points of use_transaction are:

1. Works with ActiveRecord::Rollback
2. Works with after_rollback
3. Possibly most important of all, it specifies intention

When you encounter ActiveRecord::Base.transaction, it is unclear what the goal of that block is:

• Is this a nested transaction? Should it actually be requires_new?
• Is this just a safeguard to make sure the current database code is atomic?

With use_transaction, you are indicating that whatever transaction is available is fine. If no transaction is available, make sure the following block of database operations are run atomically. The intention is clear - I don't want a nested transaction I just want to run in some transaction.

If I have a service object with the following code:

class ServiceObjectBtw
  include AfterCommitEverywhere

  def call
    an_update
    another_update
    after_commit { puts "We're all done!" }
  end
end

The most logical thing for me to do is wrap it in a transaction (which is what almost all the examples in the documentation do):

class ServiceObjectBtw
  include AfterCommitEverywhere

  def call
    ActiveRecord::Base.transaction do
      an_update
      another_update
      after_commit { puts "We're all done!" }
    end
  end
end

But now by doing that - i've muddled what my actual intention is. When I look at this code - is it intended to run in isolation? Is it ok to run this in other code running in a transaction? Is it meant to be a true nested transaction that can rollback itself and not affect the parent (requires_new)?

If I modify that example:

class ServiceObjectBtw
  include AfterCommitEverywhere

  def call
    use_transaction do
      an_update
      another_update
      after_commit { puts "We're all done!" }
    end
  end
end

use_transaction indicates: "please make sure i'm in a transaction. whichever is in the current context is fine - if there isn't one, please start one".

That is known pitfall, yeah, but I didn't face it myself (hadn't to manually raise callback exception by hand), so kind of surprised that it is a real pain for someone. Can you please tell a bit more about your use case?

There have been many times where i've seen people manually raise an ActiveRecord::Rollback. When you want to have more granular control over rolling back, it's the most logical thing to do. But if you are nested in another transaction, you'll accidentally commit anyways. I don't have a more specific use-case than developers in codebases knowing it's an available option and using it.

[Envek]

Thanks a lot for the explanation! It makes a lot of sense, so I think it worth to be included to this gem.

Please add YARD documentation for the method and README entry.

By the way, what do you think about changing method name to in_transaction (to be consistent with in_transaction? predicate):

class ServiceObjectBtw
  include AfterCommitEverywhere

  def call
    in_transaction do
      an_update
      another_update
      after_commit { puts "We're all done!" }
    end
  end
end

[jpcamara]

Thanks a lot for the explanation! It makes a lot of sense, so I think it worth to be included to this gem.

Please add YARD documentation for the method and README entry.

By the way, what do you think about changing method name to in_transaction (to be consistent with in_transaction? predicate):

class ServiceObjectBtw
  include AfterCommitEverywhere

  def call
    in_transaction do
      an_update
      another_update
      after_commit { puts "We're all done!" }
    end
  end
end

Awesome! I'm glad I explained it clearly enough to make my case 😃

I think in_transaction sounds great. Nice symmetry with the predicate 👍🏼

I'll get the README and YARD changes updated within the next couple days. Thanks!

[jpcamara]

@Envek I've updated the README, added an additional after_rollback spec, and added YARD documentation. I'm not super familiar with YARD syntax, so let me know if there's any better way of specifying it, or changes to the README you'd like.

[Envek]

Thank you very much for such a well done pull request!

Added an ability to pass arguments through it and released in 1.3.0

[jpcamara]

Thank you very much for such a well done pull request!

Added an ability to pass arguments through it and released in 1.3.0

Awesome! Thanks for the discussion - the argument addition makes alot of sense 🎉