Skip to content

Commit

Permalink
Remove Hyrax::Actors::TransactionalRequest from default middleware
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Tom Johnson committed Jan 25, 2019
1 parent b5c6605 commit 2394adb
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 4 deletions.
4 changes: 0 additions & 4 deletions app/services/hyrax/default_middleware_stack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
48 changes: 48 additions & 0 deletions spec/features/actor_stack_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 2394adb

Please sign in to comment.