From 2394adb94a0e738155935f78233dabcf61a96aa0 Mon Sep 17 00:00:00 2001
From: Tom Johnson <tomjohnson@ucsb.edu>
Date: Fri, 25 Jan 2019 09:49:45 -0800
Subject: [PATCH] Remove `Hyrax::Actors::TransactionalRequest` from default
 middleware

The actor stack involves writing to several diffrent persistence layers; at
least:

  - ActiveRecord
  - ActiveFedora (Fedora/Solr)
  - ActiveJob

The transactionality provided by `Hyrax::Actors::TransactionalRequest` addresses
only one of these (ActiveRecord). Routinely, failures in the actor stack that
trigger transaction rollback there lead to broken states across the various data
stores.

Additionally, this rollback is not triggered for the main documented failure API
for the stack (returning `false`). Rollback only happens when uncaught errors
are propagated to the top of the stack.

Since this middleware is problematic and doesn't have a clear documented use
case, this proposes its removal.

Fixes #3128.
---
 .../hyrax/default_middleware_stack.rb         |  4 --
 spec/features/actor_stack_spec.rb             | 48 +++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 spec/features/actor_stack_spec.rb

diff --git a/app/services/hyrax/default_middleware_stack.rb b/app/services/hyrax/default_middleware_stack.rb
index 487161d88b..26771cb006 100644
--- a/app/services/hyrax/default_middleware_stack.rb
+++ b/app/services/hyrax/default_middleware_stack.rb
@@ -3,10 +3,6 @@ class DefaultMiddlewareStack
     # rubocop:disable Metrics/MethodLength
     def self.build_stack
       ActionDispatch::MiddlewareStack.new.tap do |middleware|
-        # Wrap everything in a database transaction, if the save of the resource
-        # fails then roll back any database AdminSetChangeSet
-        middleware.use Hyrax::Actors::TransactionalRequest
-
         # Ensure you are mutating the most recent version
         middleware.use Hyrax::Actors::OptimisticLockValidator
 
diff --git a/spec/features/actor_stack_spec.rb b/spec/features/actor_stack_spec.rb
new file mode 100644
index 0000000000..eb8dc99959
--- /dev/null
+++ b/spec/features/actor_stack_spec.rb
@@ -0,0 +1,48 @@
+require 'rails_helper'
+
+# Integration tests for the full midddleware stack
+RSpec.describe Hyrax::DefaultMiddlewareStack, :clean_repo do
+  subject(:actor)  { stack.build(Hyrax::Actors::Terminator.new) }
+  let(:ability)    { ::Ability.new(user) }
+  let(:attributes) { {} }
+  let(:stack)      { described_class.build_stack }
+  let(:terminator) { Hyrax::Actors::Terminator.new }
+  let(:user)       { FactoryBot.create(:user) }
+  let(:work)       { FactoryBot.build(:work) }
+  let(:env)        { Hyrax::Actors::Environment.new(work, ability, attributes) }
+
+  let(:delayed_failure_actor) do
+    Class.new(Hyrax::Actors::AbstractActor) do
+      def create(env)
+        next_actor.create(env) && raise 'ALWAYS RAISE'
+      end
+    end
+  end
+
+  describe '#create' do
+    it 'persists the work' do
+      expect { actor.create(env) }
+        .to change { work.persisted? }
+        .to true
+    end
+
+    context 'when failing on the way back up the actor stack' do
+      before { stack.insert_before(Hyrax::Actors::ModelActor, delayed_failure_actor) }
+
+      before(:context) do
+        Hyrax.config.enable_noids = true
+        # we need to mint once to set the `rand` database column and
+        # make minter behavior predictable
+        ::Noid::Rails.config.minter_class.new.mint
+      end
+
+      after(:context) { Hyrax.config.enable_noids = false }
+
+      it 'leaves a valid minter state', :aggregate_failures do
+        expect { actor.create(env) }.to raise_error 'ALWAYS RAISE'
+
+        expect(GenericWork.new.assign_id).not_to eq work.id
+      end
+    end
+  end
+end