Accept optional transaction args to #with_lock

[jmileham]

Summary

Make #with_lock as expressive as calling #transaction and #lock!
individually to enable behavior like so:

person.with_lock("FOR UPDATE NOWAIT", requires_new: true) do
  ...
end

Helps teams who prefer #with_lock over #lock! to ensure the lock is
taken out within a transaction, even when advanced transaction control
is required without requiring redundant transaction blocks.

Other Information

This is a nice-to-have in a pretty stable part of ActiveRecord, but it would be helpful for teams who use linters to ensure there's a wrapping transaction before attempting to take out a lock, while still giving them the flexibility to, e.g. require a new nested transaction without double nesting.

Could buy the counterargument that the performance cost of the following is minimal given the default behavior of nested transactions

Person.transaction(requires_new: true) do
  person.with_lock do # no additional transaction is taken out because one's already open
    ...
  end
end

But I wanted to run it up the flagpole anyway since it feels like a coherent addition to the #with_lock method contract, is fully backward compatible, and provides a succinct way to express transaction configuration while advantaging the "safer" block syntax.

Retrospective h/t to @fractaledmind's PR #43077 which I just found as I was about to submit this PR (great minds think alike!). Stephen's PR is patching the exact same issue in maybe a more expressive way. The approach I'm submitting avoids coupling to the options that #transaction takes and passes them through blind, but it does so via some old school options munging as opposed to named arguments; I'm honestly not sure which I prefer, but if his approach was preferred, the testing approach I took would probably work well in his PR.

[rafaelfranca]

This is testing the implementation details. We avoid to do that in Rails. Can you change the test to really test if the transaction was open with the right options?

[jmileham]

Sounds good. I updated this test to introspect the resulting transaction.

[jmileham]

The updated test no longer tests isolation:. Testing isolation is database-specific because they each support different isolation levels (and sqlite3 doesn't even like to have isolation specified at all since it only supports one isolation level). We forward all opts to transaction anyway, so if any work, all should work. I experimented with a test that assert_raises ActiveRecord::TransactionIsolationError when provided a nonexistent isolation type, which would work consistently across DB engines, but it felt weird to test that behavior from here. I am now testing isolation in the adapter-specific tests below since I needed to limit testing to an engine that supports transactions anyway.

[jmileham]

This is a pattern I saw in other tests which feels appropriate here. In order to test transaction behavior I need to do these tests on an engine that supports transactions, but it doesn't feel important to test all database engines' specific behavior at this level, so I stuck with postgresql.

[jmileham]

Thanks, @rafaelfranca! This should be ready for another look. I've got meaningful end to end tests of all the transaction and locking behavior now. Happy to squash commits and rebase the changelog, but want to make sure I'm on the right track here.

[rafaelfranca]

Awesome! Thank you for working on this