From 707606f36f91171ef11fa22b9b5775c135b91670 Mon Sep 17 00:00:00 2001
From: Wade Tandy <wtandy@bloomberg.net>
Date: Fri, 7 Sep 2018 02:11:35 -0400
Subject: [PATCH] Add `on_rollback` hook to resources

From @jsonapi-suite/jsonapi_compliable#137, this allows an API
maintainer to run a rollback hook on a given resource if an error occurs
_after_ the resource's `before_commit` hook is successful, but _before_
the transaction actually completes.  This will be helpful in situations
where a user would like to rollback something that cannot be
automatically undone in the transaction. The `on_rollback` hook will
receive as its argument the return value of its `before_callback` hook
(or the created/updated/deleted record if no `before_callback` hook is
provided).

```ruby
class EmployeeResource
  has_many :positions

  before_commit only: :create do |record|
    # Makes HTTP request and returns ID of created record
    PayrollSystem::Employee.create(record)
  end

  on_rollback only: :create do |sap_record_id|
    # Delete created record
    PayrollSystem::Employee.find(sap_record_id).destroy
  end
end

class PositionResource
  before_commit do |record|
    raise 'boom'
  end
end
```

If creating an employee and a nested postion, the position will blow up,
and the rollback hook will clean up after itself.
---
 .ruby-version                                |   2 +-
 gemfiles/rails_4.gemfile                     |   6 +-
 gemfiles/rails_5.gemfile                     |   6 +-
 lib/graphiti/resource.rb                     |  11 +-
 lib/graphiti/resource/configuration.rb       |   1 +
 lib/graphiti/resource/dsl.rb                 |   6 +
 lib/graphiti/util/hooks.rb                   |  39 +++-
 lib/graphiti/util/persistence.rb             |  15 +-
 spec/integration/rails/before_commit_spec.rb |   5 +-
 spec/integration/rails/rollback_spec.rb      | 205 +++++++++++++++++++
 spec/spec_helper.rb                          |   2 +
 11 files changed, 274 insertions(+), 24 deletions(-)
 create mode 100644 spec/integration/rails/rollback_spec.rb

diff --git a/.ruby-version b/.ruby-version
index 276cbf9..bb576db 100644
--- a/.ruby-version
+++ b/.ruby-version
@@ -1 +1 @@
-2.3.0
+2.3
diff --git a/gemfiles/rails_4.gemfile b/gemfiles/rails_4.gemfile
index c16ad43..b1b1bcc 100644
--- a/gemfiles/rails_4.gemfile
+++ b/gemfiles/rails_4.gemfile
@@ -3,15 +3,15 @@
 source "https://rubygems.org"
 
 gem "rails", "~> 4.1"
-gem "jsonapi-rails", "~> 0.3.1", :require => "jsonapi/rails"
+gem "jsonapi-rails", "~> 0.3.1", require: "jsonapi/rails"
 gem "rspec-rails"
 
 group :test do
   gem "pry"
-  gem "pry-byebug", :platform => [:mri]
+  gem "pry-byebug", platform: [:mri]
   gem "appraisal"
   gem "guard"
   gem "guard-rspec"
 end
 
-gemspec :path => "../"
+gemspec path: "../"
diff --git a/gemfiles/rails_5.gemfile b/gemfiles/rails_5.gemfile
index 73d22b9..eec02b3 100644
--- a/gemfiles/rails_5.gemfile
+++ b/gemfiles/rails_5.gemfile
@@ -3,15 +3,15 @@
 source "https://rubygems.org"
 
 gem "rails", "~> 5.2"
-gem "jsonapi-rails", "~> 0.3.1", :require => "jsonapi/rails"
+gem "jsonapi-rails", "~> 0.3.1", require: "jsonapi/rails"
 gem "rspec-rails"
 
 group :test do
   gem "pry"
-  gem "pry-byebug", :platform => [:mri]
+  gem "pry-byebug", platform: [:mri]
   gem "appraisal"
   gem "guard"
   gem "guard-rspec"
 end
 
-gemspec :path => "../"
+gemspec path: "../"
diff --git a/lib/graphiti/resource.rb b/lib/graphiti/resource.rb
index b13d2c7..411f35a 100644
--- a/lib/graphiti/resource.rb
+++ b/lib/graphiti/resource.rb
@@ -122,7 +122,16 @@ def resolve(scope)
 
     def before_commit(model, method)
       hook = self.class.config[:before_commit][method]
-      hook.call(model) if hook
+      if hook
+        hook.call(model)
+      else
+        model
+      end
+    end
+
+    def on_rollback(record, method)
+      hook = self.class.config[:on_rollback][method]
+      hook.call(record) if hook
     end
 
     def transaction
diff --git a/lib/graphiti/resource/configuration.rb b/lib/graphiti/resource/configuration.rb
index 9a0be58..a8b6238 100644
--- a/lib/graphiti/resource/configuration.rb
+++ b/lib/graphiti/resource/configuration.rb
@@ -167,6 +167,7 @@ def config
               sorts: {},
               pagination: nil,
               before_commit: {},
+              on_rollback: {},
               attributes: {},
               extra_attributes: {},
               sideloads: {}
diff --git a/lib/graphiti/resource/dsl.rb b/lib/graphiti/resource/dsl.rb
index 2be42b5..8f3648e 100644
--- a/lib/graphiti/resource/dsl.rb
+++ b/lib/graphiti/resource/dsl.rb
@@ -86,6 +86,12 @@ def before_commit(only: [:create, :update, :destroy], &blk)
           end
         end
 
+        def on_rollback(only: [:create, :update, :destroy], &blk)
+          Array(only).each do |verb|
+            config[:on_rollback][verb] = blk
+          end
+        end
+
         def attribute(name, type, options = {}, &blk)
           raise Errors::TypeNotFound.new(self, name, type) unless Types[type]
           attribute_option(options, :readable)
diff --git a/lib/graphiti/util/hooks.rb b/lib/graphiti/util/hooks.rb
index 08e834c..403871a 100644
--- a/lib/graphiti/util/hooks.rb
+++ b/lib/graphiti/util/hooks.rb
@@ -2,31 +2,54 @@ module Graphiti
   module Util
     class Hooks
       def self.record
-        self.hooks = []
+        self.reset!
         begin
           yield.tap { run }
         ensure
-          self.hooks = []
+          self.reset!
         end
       end
 
       def self._hooks
-        Thread.current[:_graphiti_hooks] ||= []
+        Thread.current[:_graphiti_hooks] || self.reset!
       end
       private_class_method :_hooks
 
-      def self.hooks=(val)
-        Thread.current[:_graphiti_hooks] = val
+      def self.reset!
+        Thread.current[:_graphiti_hooks] = {
+          before_commit: [],
+          rollback: [],
+          post_process: [],
+          staged_rollbacks: []
+        }
       end
 
       # Because hooks will be added from the outer edges of
       # the graph, working inwards
-      def self.add(prc)
-        _hooks.unshift(prc)
+      def self.add(before_commit, rollback)
+        _hooks[:before_commit].unshift(before_commit)
+        _hooks[:rollback].unshift(rollback)
+      end
+
+      def self.add_post_process(prc)
+        _hooks[:post_process].unshift(prc)
       end
 
       def self.run
-        _hooks.each { |h| h.call }
+        begin
+          _hooks[:before_commit].each_with_index do |before_commit, idx|
+            result = before_commit.call
+            rollback = _hooks[:rollback][idx]
+
+            # Want to run rollbacks in reverse order from before_commit hooks
+            _hooks[:staged_rollbacks].unshift(-> { rollback.call(result) })
+          end
+        rescue => e
+          _hooks[:staged_rollbacks].each {|h| h.call }
+          raise e
+        end
+
+        _hooks[:post_process].each {|h| h.call }
       end
     end
   end
diff --git a/lib/graphiti/util/persistence.rb b/lib/graphiti/util/persistence.rb
index b4ea3c0..09decb5 100644
--- a/lib/graphiti/util/persistence.rb
+++ b/lib/graphiti/util/persistence.rb
@@ -61,15 +61,22 @@ def run
 
     post_process(persisted, parents)
     post_process(persisted, children)
+
     before_commit = -> { @resource.before_commit(persisted, @meta[:method]) }
-    add_hook(before_commit)
+    on_rollback = ->(record) { @resource.on_rollback(record, @meta[:method]) }
+    add_hooks(before_commit, on_rollback)
+
     persisted
   end
 
   private
 
-  def add_hook(prc)
-    ::Graphiti::Util::Hooks.add(prc)
+  def add_hooks(bc, rb)
+    ::Graphiti::Util::Hooks.add(bc, rb)
+  end
+
+  def add_post_process_hook(prc)
+    ::Graphiti::Util::Hooks.add_post_process(prc)
   end
 
   # The child's attributes should be modified to nil-out the
@@ -185,7 +192,7 @@ def post_process(caller_model, processed)
       group.group_by { |g| g[:sideload] }.each_pair do |sideload, members|
         objects = members.map { |x| x[:object] }
         hook = -> { sideload.fire_hooks!(caller_model, objects, method) }
-        add_hook(hook)
+        add_post_process_hook(hook)
       end
     end
   end
diff --git a/spec/integration/rails/before_commit_spec.rb b/spec/integration/rails/before_commit_spec.rb
index 2642444..fabc0a5 100644
--- a/spec/integration/rails/before_commit_spec.rb
+++ b/spec/integration/rails/before_commit_spec.rb
@@ -18,10 +18,7 @@ def self.add(name, object)
     end
 
     before do
-      allow(controller.request.env).to receive(:[])
-        .with(anything).and_call_original
-      allow(controller.request.env).to receive(:[])
-        .with('PATH_INFO') { path }
+      allow(controller.request).to receive(:env).and_return(Rack::MockRequest.env_for(path))
     end
 
     let(:path) { '/integration_hooks/employees' }
diff --git a/spec/integration/rails/rollback_spec.rb b/spec/integration/rails/rollback_spec.rb
new file mode 100644
index 0000000..27ab60e
--- /dev/null
+++ b/spec/integration/rails/rollback_spec.rb
@@ -0,0 +1,205 @@
+if ENV["APPRAISAL_INITIALIZED"]
+  RSpec.describe 'rollback hooks', type: :controller do
+    class Callbacks
+      class << self
+        attr_accessor :rollbacks, :commits
+      end
+
+      def self.add_rollback(object)
+        self.rollbacks << object
+      end
+
+      def self.add_commit(object)
+        self.commits << object
+      end
+    end
+
+    before do
+      Callbacks.rollbacks = []
+      Callbacks.commits = []
+      $raise_on_before_commit = { }
+    end
+
+    before do
+      allow(controller.request).to receive(:env).and_return(Rack::MockRequest.env_for(path))
+    end
+
+    let(:path) { '/integration_hooks/employees' }
+
+    module IntegrationHooks
+      class ApplicationResource < Graphiti::Resource
+        self.adapter = Graphiti::Adapters::ActiveRecord
+        before_commit do |record|
+          Callbacks.add_commit(record)
+
+          if $raise_on_before_commit[record.class.name]
+            raise 'rollitback'
+          end
+
+          record
+        end
+      end
+
+      class DepartmentResource < ApplicationResource
+        self.model = ::Department
+
+        on_rollback do |record|
+          Callbacks.add_rollback(record)
+        end
+      end
+
+      class PositionResource < ApplicationResource
+        self.model = ::Position
+
+        attribute :employee_id, :integer, only: [:writable]
+
+        on_rollback do |record|
+          Callbacks.add_rollback(record)
+        end
+
+        belongs_to :department
+      end
+
+      class EmployeeResource < ApplicationResource
+        self.model = ::Employee
+
+        attribute :first_name, :string
+
+        on_rollback only: [:create] do |record|
+          Callbacks.add_rollback(record)
+        end
+
+        has_many :positions
+      end
+    end
+
+    controller(ApplicationController) do
+      def create
+        employee = IntegrationHooks::EmployeeResource.build(params)
+
+        if employee.save
+          render jsonapi: employee
+        else
+          raise 'whoops'
+        end
+      end
+
+      private
+
+      def params
+        @params ||= begin
+          hash = super.to_unsafe_h.with_indifferent_access
+          hash = hash[:params] if hash.has_key?(:params)
+          hash
+        end
+      end
+    end
+
+    before do
+      @request.headers['Accept'] = Mime[:json]
+      @request.headers['Content-Type'] = Mime[:json].to_s
+
+      routes.draw {
+        post "create" => "anonymous#create"
+        put "update" => "anonymous#update"
+        delete "destroy" => "anonymous#destroy"
+      }
+    end
+
+    def json
+      JSON.parse(response.body)
+    end
+
+    context 'on_rollback' do
+      context 'when creating a single resource' do
+        let(:payload) do
+          {
+            data: {
+              type: 'employees',
+              attributes: { first_name: 'Jane' }
+            }
+          }
+        end
+
+        context 'when the creation is successful' do
+          it "does not call rollback hook" do
+            post :create, params: payload
+
+            expect(Callbacks.rollbacks).to eq []
+          end
+        end
+
+        context 'when the resource raises an error in before_commit' do
+          before do
+            $raise_on_before_commit = { 'Employee' => true }
+          end
+
+          it "does not call rollback hook" do
+            expect {
+              post :create, params: payload
+            }.to raise_error('rollitback')
+
+            expect(Callbacks.rollbacks).to eq []
+          end
+        end
+      end
+
+      context 'creating nested resources' do
+        let(:payload) do
+          {
+            data: {
+              type: 'employees',
+              attributes: { first_name: 'joe' },
+              relationships: {
+                positions: {
+                  data: [
+                    { :'temp-id' => 'a', type: 'positions', method: 'create' }
+                  ]
+                }
+              }
+            },
+            included: [
+              {
+                type: 'positions',
+                :'temp-id' => 'a',
+                relationships: {
+                  department: {
+                    data: {
+                      :'temp-id' => 'b', type: 'departments', method: 'create'
+                    }
+                  }
+                }
+              },
+              {
+                type: 'departments',
+                :'temp-id' => 'b'
+              }
+            ]
+          }
+        end
+
+        context 'when creation is successful' do
+          it 'does not call any rollback hooks' do
+            post :create, params: payload
+
+            expect(Callbacks.rollbacks).to eq []
+          end
+        end
+
+        context 'when one of the resources throws an error in a before_commit hook' do
+          before do
+            $raise_on_before_commit = { 'Department' => true }
+          end
+
+          it 'runs rollback hook for any previously commited resources in reverse order' do
+            expect {
+              post :create, params: payload
+            }.to raise_error('rollitback')
+
+            expect(Callbacks.rollbacks).to eq [Callbacks.commits[1], Callbacks.commits[0]]
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 7622be6..c50b7db 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -11,6 +11,8 @@
 require 'fixtures/poro'
 
 RSpec.configure do |config|
+  config.filter_run_when_matching :focus
+
   config.include GraphitiSpecHelpers::RSpec
   config.include GraphitiSpecHelpers::Sugar