Fix transaction extensions to handle failures correctly

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