Fix transaction extensions to handle failures correctly

The `intercepting_failure` method was not handling the block's output
correctly when it was a failure. It wasn't throwing `:halt` with the
failure as the given handlers were raising and therefore the throwing
part was never reached.

We modify the method to pass the failure to the handler and skip the
throwing part, while extensions need to work in two phases:

1 - From within the transaction block, they intercept a failure a first time
and assign it to a `result` variable available from the outer scope.
After that, they raise to rollback the transaction but return the
`result` variable after that.

2 - From outside the transaction block, they throw again the failure
when the `result` is actually a failure.

We also make public the `throw_failure` method so that other extensions
can create similar behavior.

We also add a warning in the ActiveRecord extension to make it clear
that the `:requires_new` option is not yet supported for this approach.
We should find a way to make it work in the future.

Closes #23

lib/dry/operation.rb

@@ -172,35 +172,36 @@ def step(result)
 
     # Invokes a callable in case of block's failure
     #
-    # Throws `:halt` with a {Dry::Monads::Result::Failure} on failure.
-    #
     # This method is useful when you want to perform some side-effect when a
     # failure is encountered. It's meant to be used within the {#steps} block
     # commonly wrapping a sub-set of {#step} calls.
     #
-    # @param handler [#call] a callable that will be called when a failure is encountered
+    # @param handler [#call] a callable that will be called with the encountered failure
     # @yieldreturn [Object]
-    # @return [Object] the block's return value
+    # @return [Object] the block's return value when it's not a failure or the handler's
+    #   return value when the block returns a failure
     def intercepting_failure(handler, &block)
       output = catching_failure(&block)
 
       case output
       when Failure
-        handler.()
-        throw_failure(output)
+        handler.(output)
       else
         output
       end
     end
 
+    # Throws `:halt` with a failure
+    #
+    # @param failure [Dry::Monads::Result::Failure]
+    def throw_failure(failure)
+      throw FAILURE_TAG, failure
+    end
+
     private
 
     def catching_failure(&block)
       catch(FAILURE_TAG, &block)
     end
-
-    def throw_failure(failure)
-      throw FAILURE_TAG, failure
-    end
   end
 end

lib/dry/operation/extensions/active_record.rb

@@ -68,6 +68,8 @@ module Extensions
       #   # ...
       # end
       #
+      # WARNING: Be aware that the `:requires_new` option is not yet supported.
+      #
       # @see https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html
       # @see https://guides.rubyonrails.org/active_record_multiple_databases.html
       module ActiveRecord
@@ -109,8 +111,17 @@ def included(klass)
               #   @see Dry::Operation#steps
               #   @see https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/DatabaseStatements.html#method-i-transaction
               klass.define_method(:transaction) do |connection = default_connection, **opts, &steps|
-                connection.transaction(**options.merge(opts)) do
-                  intercepting_failure(-> { raise ::ActiveRecord::Rollback }, &steps)
+                intercepting_failure(method(:throw_failure)) do
+                  result = nil
+                  connection.transaction(**options.merge(opts)) do
+                    intercepting_failure(->(failure) {
+                                           result = failure
+                                           raise ::ActiveRecord::Rollback
+                                         }) do
+                      result = steps.()
+                    end
+                  end
+                  result
                 end
               end
             end

lib/dry/operation/extensions/rom.rb

@@ -103,10 +103,17 @@ def included(klass)
                   that returns the ROM container
                 MSG
 
-                rom.gateways[gateway].transaction do |t|
-                  intercepting_failure(-> { raise t.rollback! }) do
-                    steps.()
+                intercepting_failure(method(:throw_failure)) do
+                  result = nil
+                  rom.gateways[gateway].transaction do |t|
+                    intercepting_failure(->(failure) {
+                                           result = failure
+                                           t.rollback!
+                                         }) do
+                      result = steps.()
+                    end
                   end
+                  result
                 end
               end
             end

spec/integration/extensions/active_record_spec.rb

@@ -67,7 +67,7 @@ def failure
     expect(model.count).to be(0)
   end
 
-  it "acts transparently for the regular flow" do
+  it "acts transparently for the regular flow for a success" do
     instance = Class.new(base) do
       def initialize(model)
         @model = model
@@ -93,7 +93,60 @@ def count_records
     expect(instance.()).to eql(Success(1))
   end
 
+  it "acts transparently for the regular flow for a failure" do
+    instance = Class.new(base) do
+      def initialize(model)
+        @model = model
+        super()
+      end
+
+      def call
+        transaction do
+          step create_record
+          step count_records
+        end
+      end
+
+      def create_record
+        Success(@model.create(bar: "bar"))
+      end
+
+      def count_records
+        Failure(:failure)
+      end
+    end.new(model)
+
+    expect(
+      instance.()
+    ).to eql(Failure(:failure))
+  end
+
   it "accepts options for ActiveRecord transaction method" do
+    instance = Class.new(base) do
+      def initialize(model)
+        @model = model
+        super()
+      end
+
+      def call
+        transaction(requires_new: :false) do
+          step create_record
+        end
+      end
+
+      def create_record
+        Success(@model.create(bar: "bar"))
+      end
+    end.new(model)
+
+    expect(ActiveRecord::Base).to receive(:transaction).with(requires_new: :false).and_call_original
+
+    instance.()
+
+    expect(model.count).to be(1)
+  end
+
+  xit "works with `requires_new` for nested transactions" do
     instance = Class.new(base) do
       def initialize(model)
         @model = model
@@ -114,12 +167,12 @@ def create_record
       end
 
       def failure
-        @model.create(bar: "bar")
         Failure(:failure)
       end
     end.new(model)
 
     instance.()
+
     expect(model.count).to be(1)
   end
 end

spec/integration/extensions/rom_spec.rb

@@ -51,7 +51,7 @@ def failure
     expect(rom.relations[:foo].count).to be(0)
   end
 
-  it "acts transparently for the regular flow" do
+  it "acts transparently for the regular flow for a success" do
     instance = Class.new(base) do
       def call
         transaction do
@@ -73,4 +73,27 @@ def count_records
       instance.()
     ).to eql(Success(1))
   end
+
+  it "acts transparently for the regular flow for a failure" do
+    instance = Class.new(base) do
+      def call
+        transaction do
+          step create_record
+          step count_records
+        end
+      end
+
+      def create_record
+        Success(rom.relations[:foo].command(:create).(bar: "bar"))
+      end
+
+      def count_records
+        Failure(:failure)
+      end
+    end.new(rom: rom)
+
+    expect(
+      instance.()
+    ).to eql(Failure(:failure))
+  end
 end

spec/unit/extensions/active_record_spec.rb

@@ -5,22 +5,22 @@
 RSpec.describe Dry::Operation::Extensions::ActiveRecord do
   describe "#transaction" do
     it "forwards options to ActiveRecord transaction call" do
-      instance = Class.new.include(Dry::Operation::Extensions::ActiveRecord).new
+      instance = Class.new(Dry::Operation).include(Dry::Operation::Extensions::ActiveRecord).new
 
       expect(ActiveRecord::Base).to receive(:transaction).with(requires_new: true)
       instance.transaction(requires_new: true) {}
     end
 
     it "accepts custom initiator and options" do
-      instance = Class.new.include(Dry::Operation::Extensions::ActiveRecord).new
+      instance = Class.new(Dry::Operation).include(Dry::Operation::Extensions::ActiveRecord).new
       record = double(:transaction)
 
       expect(record).to receive(:transaction)
       instance.transaction(record) {}
     end
 
     it "merges options with default options" do
-      instance = Class.new.include(Dry::Operation::Extensions::ActiveRecord[requires_new: true]).new
+      instance = Class.new(Dry::Operation).include(Dry::Operation::Extensions::ActiveRecord[requires_new: true]).new
 
       expect(ActiveRecord::Base).to receive(:transaction).with(requires_new: true, isolation: :serializable)
       instance.transaction(isolation: :serializable) {}

spec/unit/operation_spec.rb

@@ -71,34 +71,38 @@ def foo(value)
   describe "#intercepting_failure" do
     it "forwards the block's output when it's not a failure" do
       expect(
-        described_class.new.intercepting_failure(-> {}) { :foo }
+        described_class.new.intercepting_failure(->(_failure) {}) { :foo }
       ).to be(:foo)
     end
 
     it "doesn't call the handler when the block doesn't return a failure" do
       called = false
 
       catch(:halt) {
-        described_class.new.intercepting_failure(-> { called = true }) { :foo }
+        described_class.new.intercepting_failure(->(_failure) { called = true }) { :foo }
       }
 
       expect(called).to be(false)
     end
 
-    it "throws :halt with the result when the block returns a failure" do
-      expect {
-        described_class.new.intercepting_failure(-> {}) { Failure(:foo) }
-      }.to throw_symbol(:halt, Failure(:foo))
-    end
-
-    it "calls the handler when the block returns a failure" do
-      called = false
+    it "calls the handler with the failure when the block returns a failure" do
+      failure = nil
 
       catch(:halt) {
-        described_class.new.intercepting_failure(-> { called = true }) { Failure(:foo) }
+        described_class.new.intercepting_failure(->(intercepted_failure) { failure = intercepted_failure }) { Failure(:foo) }
       }
 
-      expect(called).to be(true)
+      expect(failure).to eq(Failure(:foo))
+    end
+  end
+
+  describe "#throw_failure" do
+    it "throws :halt with the failure" do
+      failure = Failure(:foo)
+
+      expect {
+        described_class.new.throw_failure(failure)
+      }.to throw_symbol(:halt, failure)
     end
   end
 end