Merge branch 'stokarenko-fix-after-commit-within-nested-transaction-vol2'

Appraisals

@@ -29,6 +29,7 @@ appraise 'rails_5.0' do
   gem 'dynamoid', '~> 1.3', platforms: :ruby
   gem 'aws-sdk', '~> 2', platforms: :ruby
   gem 'redis-objects'
+  gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 end
 
 appraise 'rails_5.0_nobrainer' do
@@ -43,6 +44,7 @@ appraise 'rails_5.1' do
   gem 'dynamoid', '~> 1.3', platforms: :ruby
   gem 'aws-sdk', '~>2', platforms: :ruby
   gem 'redis-objects'
+  gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 end
 
 appraise 'rails_5.2' do
@@ -52,9 +54,11 @@ appraise 'rails_5.2' do
   gem 'dynamoid', '~>2.2', platforms: :ruby
   gem 'aws-sdk', '~>2', platforms: :ruby
   gem 'redis-objects'
+  gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 end
 
 appraise 'norails' do
+  gem 'sqlite3', '~> 1.3', '>= 1.3.5', platforms: :ruby
   gem 'rails', install_if: false
   gem 'sequel'
   gem 'redis-objects'

Gemfile

@@ -3,4 +3,4 @@ source "https://rubygems.org"
 gemspec
 
 gem 'sqlite3', '~> 1.3.5', :platforms => :ruby
-gem 'rails', '5.1.4'
+gem 'rails', '5.1.4'

README.md

@@ -436,7 +436,7 @@ job.aasm.current_state # stage3
 ### Multiple state machines per class
 
 Multiple state machines per class are supported. Be aware though that _AASM_ has been
-built with one state machine per class in mind. Nonetheless, here's how to do it (see below). Please note that you will need to specify database columns for where your pertinent states will be stored - we have specified two columns `move_state` and `work_state` in the example below. See the [Column name & migration](https://github.com/aasm/aasm#column-name--migration) section for further info. 
+built with one state machine per class in mind. Nonetheless, here's how to do it (see below). Please note that you will need to specify database columns for where your pertinent states will be stored - we have specified two columns `move_state` and `work_state` in the example below. See the [Column name & migration](https://github.com/aasm/aasm#column-name--migration) section for further info.
 
 ```ruby
 class SimpleMultipleExample
@@ -981,6 +981,12 @@ job.run
 job.save! #notify_about_running_job is not run
 ```
 
+Please note that `:after_commit` AASM callbacks behaves around custom implementation
+of transaction pattern rather than a real-life DB transaction. This fact still causes
+the race conditions and redundant callback calls within nested transaction. In order
+to fix that it's highly recommended to add `gem 'after_commit_everywhere', '~> 0.1', '>= 0.1.5'`
+to your `Gemfile`.
+
 If you want to encapsulate state changes within an own transaction, the behavior
 of this nested transaction might be confusing. Take a look at
 [ActiveRecord Nested Transactions](http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html)
@@ -1092,7 +1098,7 @@ end
 
 ### Log State Changes
 
-Logging state change can be done using [paper_trail](https://github.com/paper-trail-gem/paper_trail) gem 
+Logging state change can be done using [paper_trail](https://github.com/paper-trail-gem/paper_trail) gem
 
 Example of implementation can be found here [https://github.com/nitsujri/aasm-papertrail-example](https://github.com/nitsujri/aasm-papertrail-example)
 

gemfiles/rails_5.0.gemfile

@@ -9,5 +9,6 @@ gem "sequel"
 gem "dynamoid", "~> 1.3", platforms: :ruby
 gem "aws-sdk", "~> 2", platforms: :ruby
 gem "redis-objects"
+gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 
 gemspec path: "../"

gemfiles/rails_5.1.gemfile

@@ -9,5 +9,6 @@ gem "sequel"
 gem "dynamoid", "~> 1.3", platforms: :ruby
 gem "aws-sdk", "~>2", platforms: :ruby
 gem "redis-objects"
+gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 
 gemspec path: "../"

gemfiles/rails_5.2.gemfile

@@ -9,5 +9,6 @@ gem "sequel"
 gem "dynamoid", "~>2.2", platforms: :ruby
 gem "aws-sdk", "~>2", platforms: :ruby
 gem "redis-objects"
+gem "after_commit_everywhere", "~> 0.1", ">= 0.1.5"
 
 gemspec path: "../"

lib/aasm/persistence/active_record_persistence.rb

@@ -28,6 +28,19 @@ module AASM
       #   end
       #
       def self.included(base)
+        begin
+          require 'after_commit_everywhere'
+          raise LoadError unless Gem::Version.new(::AfterCommitEverywhere::VERSION) >= Gem::Version.new('0.1.5')
+
+          base.send(:include, ::AfterCommitEverywhere) unless base.include?(::AfterCommitEverywhere)
+          base.send(:alias_method, :aasm_execute_after_commit, :after_commit)
+        rescue LoadError
+          warn <<-MSG
+[DEPRECATION] :after_commit AASM callback is not safe in terms of race conditions and redundant calls.
+              Please add `gem 'after_commit_everywhere', '~> 0.1', '>= 0.1.5'` to your Gemfile in order to fix that.
+          MSG
+        end
+
         base.send(:include, AASM::Persistence::Base)
         base.send(:include, AASM::Persistence::ORM)
         base.send(:include, AASM::Persistence::ActiveRecordPersistence::InstanceMethods)

lib/aasm/persistence/orm.rb

@@ -81,6 +81,10 @@ module AASM
         true
       end
 
+      def aasm_execute_after_commit
+        yield
+      end
+
       def aasm_write_state_attribute(state, name=:default)
         aasm_write_attribute(self.class.aasm(name).attribute_name, aasm_raw_attribute_value(state, name))
       end
@@ -116,32 +120,32 @@ module AASM
 
       # Returns true if event was fired successfully and transaction completed.
       def aasm_fire_event(state_machine_name, name, options, *args, &block)
-        if aasm_supports_transactions? && options[:persist]
-          event = self.class.aasm(state_machine_name).state_machine.events[name]
-          event.fire_callbacks(:before_transaction, self, *args)
-          event.fire_global_callbacks(:before_all_transactions, self, *args)
+        return super unless aasm_supports_transactions? && options[:persist]
 
-          begin
-            success = if options[:persist] && use_transactions?(state_machine_name)
-              aasm_transaction(requires_new?(state_machine_name), requires_lock?(state_machine_name)) do
-                super
-              end
-            else
+        event = self.class.aasm(state_machine_name).state_machine.events[name]
+        event.fire_callbacks(:before_transaction, self, *args)
+        event.fire_global_callbacks(:before_all_transactions, self, *args)
+
+        begin
+          success = if options[:persist] && use_transactions?(state_machine_name)
+            aasm_transaction(requires_new?(state_machine_name), requires_lock?(state_machine_name)) do
               super
             end
+          else
+            super
+          end
 
-            if success
+          if success
+            aasm_execute_after_commit do
               event.fire_callbacks(:after_commit, self, *args)
               event.fire_global_callbacks(:after_all_commits, self, *args)
             end
-
-            success
-          ensure
-            event.fire_callbacks(:after_transaction, self, *args)
-            event.fire_global_callbacks(:after_all_transactions, self, *args)
           end
-        else
-          super
+
+          success
+        ensure
+          event.fire_callbacks(:after_transaction, self, *args)
+          event.fire_global_callbacks(:after_all_transactions, self, *args)
         end
       end
 

spec/unit/persistence/active_record_persistence_spec.rb

@@ -613,6 +613,38 @@ if defined?(ActiveRecord)
           expect(validator).to be_running
           expect(validator.name).to eq("name")
         end
+
+        context "nested transaction" do
+          it "should fire :after_commit if root transaction was successful" do
+            validator = Validator.create(:name => 'name')
+            expect(validator).to be_sleeping
+
+            validator.transaction do
+              validator.run!
+              expect(validator.name).to eq("name")
+              expect(validator).to be_running
+            end
+
+            expect(validator.name).to eq("name changed")
+            expect(validator.reload).to be_running
+          end
+
+          it "should not fire :after_commit if root transaction failed" do
+            validator = Validator.create(:name => 'name')
+            expect(validator).to be_sleeping
+
+            validator.transaction do
+              validator.run!
+              expect(validator.name).to eq("name")
+              expect(validator).to be_running
+
+              raise ActiveRecord::Rollback, "failed on purpose"
+            end
+
+            expect(validator.name).to eq("name")
+            expect(validator.reload).to be_sleeping
+          end
+        end
       end
 
       describe 'before and after transaction callbacks' do