From 3d50f82e0034d885b10b99299ab4b2f0374ba70b Mon Sep 17 00:00:00 2001 From: JP Camara <48120+jpcamara@users.noreply.github.com> Date: Sat, 8 Oct 2022 22:30:41 -0400 Subject: [PATCH 1/3] `use_transaction` to avoid nesting transactions * `use_transaction` takes advantage of the `in_transaction?` method to determine if the current code is already wrapped in a transaction * If we're in a transaction, just yield the block. If we aren't, wrap the block in a transaction using the provided or default connection * This allows the block to guarantee it is running in a transaction without incurring the downside of rollbacks being swallowed because of nested transaction blocks (see new spec in `after_commit` and spec for `use_transaction` to see the described behavior) --- lib/after_commit_everywhere.rb | 10 ++++++++ spec/after_commit_everywhere_spec.rb | 37 +++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lib/after_commit_everywhere.rb b/lib/after_commit_everywhere.rb index 5d52f29..cef9f43 100644 --- a/lib/after_commit_everywhere.rb +++ b/lib/after_commit_everywhere.rb @@ -132,6 +132,16 @@ def in_transaction?(connection = nil) connection.transaction_open? && connection.current_transaction.joinable? end + def use_transaction(connection = nil) + connection ||= default_connection + + if in_transaction?(connection) + yield + else + connection.transaction { yield } + end + end + private def default_connection diff --git a/spec/after_commit_everywhere_spec.rb b/spec/after_commit_everywhere_spec.rb index eb6b9f1..ced1367 100644 --- a/spec/after_commit_everywhere_spec.rb +++ b/spec/after_commit_everywhere_spec.rb @@ -133,7 +133,7 @@ expect(handler).to have_received(:call) end - it "doesn't execute callback when rollback issued" do + it "doesn't execute callback when rollback issued from :requires_new transaction" do outer_handler = spy("outer") ActiveRecord::Base.transaction do example_class.new.after_commit { outer_handler.call } @@ -145,6 +145,19 @@ expect(outer_handler).to have_received(:call) expect(handler).not_to have_received(:call) end + + it "executes callbacks when rollback issued from default nested transaction" do + outer_handler = spy("outer") + ActiveRecord::Base.transaction do + described_class.after_commit { outer_handler.call } + ActiveRecord::Base.transaction do + raise ActiveRecord::Rollback + end + end + + expect(outer_handler).to have_received(:call) + expect(handler).not_to have_received(:call) + end end context "with transactions to different databases" do @@ -468,6 +481,28 @@ end end + describe "#use_transaction" do + it "rollbacks propogate up to the top level transaction block" do + outer_handler = spy("outer") + ActiveRecord::Base.transaction do + described_class.after_commit { outer_handler.call } + described_class.use_transaction do + raise ActiveRecord::Rollback + end + end + + expect(outer_handler).not_to have_received(:call) + expect(handler).not_to have_received(:call) + end + + it "runs in a new transaction if no wrapping transaction is available" do + expect(ActiveRecord::Base.connection.transaction_open?).to be_falsey + described_class.use_transaction do + expect(ActiveRecord::Base.connection.transaction_open?).to be_truthy + end + end + end + describe ".after_commit" do subject do described_class.after_commit do From 84c793eab9969cd4910c0a684eea09960ab6c0a0 Mon Sep 17 00:00:00 2001 From: JP Camara <48120+jpcamara@users.noreply.github.com> Date: Sun, 9 Oct 2022 13:26:28 -0400 Subject: [PATCH 2/3] Allow use_transaction on instances * Same as `in_transaction?`, delegate `use_transaction` to `AfterCommitEverywhere` so instances can use `use_transaction` directly * Since the specs are identical aside for the receiver, make the spec into a shared example and include it for each scenario --- lib/after_commit_everywhere.rb | 2 +- spec/after_commit_everywhere_spec.rb | 54 ++++++++++++++++------------ 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/lib/after_commit_everywhere.rb b/lib/after_commit_everywhere.rb index cef9f43..bcc08a7 100644 --- a/lib/after_commit_everywhere.rb +++ b/lib/after_commit_everywhere.rb @@ -14,7 +14,7 @@ module AfterCommitEverywhere class NotInTransaction < RuntimeError; end delegate :after_commit, :before_commit, :after_rollback, to: AfterCommitEverywhere - delegate :in_transaction?, to: AfterCommitEverywhere + delegate :in_transaction?, :use_transaction, to: AfterCommitEverywhere # Causes {before_commit} and {after_commit} to raise an exception when # called outside a transaction. diff --git a/spec/after_commit_everywhere_spec.rb b/spec/after_commit_everywhere_spec.rb index ced1367..b722f03 100644 --- a/spec/after_commit_everywhere_spec.rb +++ b/spec/after_commit_everywhere_spec.rb @@ -481,28 +481,6 @@ end end - describe "#use_transaction" do - it "rollbacks propogate up to the top level transaction block" do - outer_handler = spy("outer") - ActiveRecord::Base.transaction do - described_class.after_commit { outer_handler.call } - described_class.use_transaction do - raise ActiveRecord::Rollback - end - end - - expect(outer_handler).not_to have_received(:call) - expect(handler).not_to have_received(:call) - end - - it "runs in a new transaction if no wrapping transaction is available" do - expect(ActiveRecord::Base.connection.transaction_open?).to be_falsey - described_class.use_transaction do - expect(ActiveRecord::Base.connection.transaction_open?).to be_truthy - end - end - end - describe ".after_commit" do subject do described_class.after_commit do @@ -550,4 +528,36 @@ is_expected.to be_falsey end end + + shared_examples "verify use_transaction behavior" do + it "rollbacks propogate up to the top level transaction block" do + outer_handler = spy("outer") + ActiveRecord::Base.transaction do + described_class.after_commit { outer_handler.call } + receiver.use_transaction do + raise ActiveRecord::Rollback + end + end + + expect(outer_handler).not_to have_received(:call) + expect(handler).not_to have_received(:call) + end + + it "runs in a new transaction if no wrapping transaction is available" do + expect(ActiveRecord::Base.connection.transaction_open?).to be_falsey + receiver.use_transaction do + expect(ActiveRecord::Base.connection.transaction_open?).to be_truthy + end + end + end + + describe "#use_transaction" do + let(:receiver) { example_class.new } + include_examples "verify use_transaction behavior" + end + + describe ".use_transaction" do + let(:receiver) { described_class } + include_examples "verify use_transaction behavior" + end end From 9838424b2a043be70abc807cdebd93c7e34bbdbf Mon Sep 17 00:00:00 2001 From: JP Camara <48120+jpcamara@users.noreply.github.com> Date: Thu, 27 Oct 2022 22:16:23 -0400 Subject: [PATCH 3/3] Name change / documentation for `in_transaction` * Changes the name from `use_transaction` to `in_transaction` for better symmetry with the `in_transaction?` method * YARD documentation for the `in_transaction` method * New spec demonstrating `after_rollback` + `ActiveRecord::Rollback` working correctly with nesting when `in_transaction` * README documentation for `in_transaction`, as well as an example for `in_transaction`? --- README.md | 71 +++++++++++++++++++++++++++- lib/after_commit_everywhere.rb | 8 +++- spec/after_commit_everywhere_spec.rb | 33 ++++++++++--- 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 0de7326..1603aa7 100644 --- a/README.md +++ b/README.md @@ -111,13 +111,80 @@ Will be executed right after transaction in which it have been declared was roll If called outside transaction will raise an exception! -Please keep in mind ActiveRecord's [limitations for rolling back nested transactions](http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Nested+transactions). +Please keep in mind ActiveRecord's [limitations for rolling back nested transactions](http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Nested+transactions). See [`in_transaction`](#in_transaction) for a workaround to this limitation. ### Available helper methods +#### `in_transaction` + +Makes sure the provided block is running in a transaction. + +This method aims to provide clearer intention than a typical `ActiveRecord::Base.transaction` block - `in_transaction` only cares that _some_ transaction is present, not that a transaction is nested in any way. + +If a transaction is present, it will yield without taking any action. Note that this means `ActiveRecord::Rollback` errors will not be trapped by `in_transaction` but will propagate up to the nearest parent transaction block. + +If no transaction is present, the provided block will open a new transaction. + +```rb +class ServiceObjectBtw + include AfterCommitEverywhere + + def call + in_transaction do + an_update + another_update + after_commit { puts "We're all done!" } + end + end +end +``` + +Our service object can run its database operations safely when run in isolation. + +```rb +ServiceObjectBtw.new.call # This opens a new #transaction block +``` + +If it is later called from code already wrapped in a transaction, the existing transaction will be utilized without any nesting: + +```rb +ActiveRecord::Base.transaction do + new_update + next_update + # This no longer opens a new #transaction block, because one is already present + ServiceObjectBtw.new.call +end +``` + +This can be called directly on the module as well: + +```rb +AfterCommitEverywhere.in_transaction do + AfterCommitEverywhere.after_commit { puts "We're all done!" } +end +``` + #### `in_transaction?` -Returns `true` when called inside open transaction, `false` otherwise. +Returns `true` when called inside an open transaction, `false` otherwise. + +```rb +def check_for_transaction + if in_transaction? + puts "We're in a transaction!" + else + puts "We're not in a transaction..." + end +end + +check_for_transaction +# => prints "We're not in a transaction..." + +in_transaction do + check_for_transaction +end +# => prints "We're in a transaction!" +``` ### Available callback options diff --git a/lib/after_commit_everywhere.rb b/lib/after_commit_everywhere.rb index bcc08a7..c03bf54 100644 --- a/lib/after_commit_everywhere.rb +++ b/lib/after_commit_everywhere.rb @@ -14,7 +14,7 @@ module AfterCommitEverywhere class NotInTransaction < RuntimeError; end delegate :after_commit, :before_commit, :after_rollback, to: AfterCommitEverywhere - delegate :in_transaction?, :use_transaction, to: AfterCommitEverywhere + delegate :in_transaction?, :in_transaction, to: AfterCommitEverywhere # Causes {before_commit} and {after_commit} to raise an exception when # called outside a transaction. @@ -132,7 +132,11 @@ def in_transaction?(connection = nil) connection.transaction_open? && connection.current_transaction.joinable? end - def use_transaction(connection = nil) + # Makes sure the provided block runs in a transaction. If we are not currently in a transaction, a new transaction is started. + # + # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter] Database connection to operate in. Defaults to +ActiveRecord::Base.connection+ + # @return void + def in_transaction(connection = nil) connection ||= default_connection if in_transaction?(connection) diff --git a/spec/after_commit_everywhere_spec.rb b/spec/after_commit_everywhere_spec.rb index b722f03..29c242b 100644 --- a/spec/after_commit_everywhere_spec.rb +++ b/spec/after_commit_everywhere_spec.rb @@ -529,12 +529,12 @@ end end - shared_examples "verify use_transaction behavior" do + shared_examples "verify in_transaction behavior" do it "rollbacks propogate up to the top level transaction block" do outer_handler = spy("outer") ActiveRecord::Base.transaction do described_class.after_commit { outer_handler.call } - receiver.use_transaction do + receiver.in_transaction do raise ActiveRecord::Rollback end end @@ -545,19 +545,38 @@ it "runs in a new transaction if no wrapping transaction is available" do expect(ActiveRecord::Base.connection.transaction_open?).to be_falsey - receiver.use_transaction do + receiver.in_transaction do expect(ActiveRecord::Base.connection.transaction_open?).to be_truthy end end + + context "when rolling back, the rollback propogates to the parent transaction block" do + subject { receiver.after_rollback { handler.call } } + + it "executes all after_rollback calls, even when raising an ActiveRecord::Rollback" do + outer_handler = spy("outer") + ActiveRecord::Base.transaction do + receiver.after_rollback { outer_handler.call } + described_class.in_transaction do + subject + # ActiveRecord::Rollback works here because `in_transaction` yields without creating a new nested transaction + raise ActiveRecord::Rollback + end + end + + expect(handler).to have_received(:call) + expect(outer_handler).to have_received(:call) + end + end end - describe "#use_transaction" do + describe "#in_transaction" do let(:receiver) { example_class.new } - include_examples "verify use_transaction behavior" + include_examples "verify in_transaction behavior" end - describe ".use_transaction" do + describe ".in_transaction" do let(:receiver) { described_class } - include_examples "verify use_transaction behavior" + include_examples "verify in_transaction behavior" end end