diff --git a/Appraisals b/Appraisals index e90c6a8..28f4a33 100644 --- a/Appraisals +++ b/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' diff --git a/Gemfile b/Gemfile index 793030f..57f795e 100644 --- a/Gemfile +++ b/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' \ No newline at end of file diff --git a/README.md b/README.md index 11beb23..6048b71 100644 --- a/README.md +++ b/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) diff --git a/gemfiles/rails_5.0.gemfile b/gemfiles/rails_5.0.gemfile index a9fd7ac..00f6e89 100644 --- a/gemfiles/rails_5.0.gemfile +++ b/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: "../" diff --git a/gemfiles/rails_5.1.gemfile b/gemfiles/rails_5.1.gemfile index de25b9c..165dadd 100644 --- a/gemfiles/rails_5.1.gemfile +++ b/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: "../" diff --git a/gemfiles/rails_5.2.gemfile b/gemfiles/rails_5.2.gemfile index 87a645e..492ddde 100644 --- a/gemfiles/rails_5.2.gemfile +++ b/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: "../" diff --git a/lib/aasm/persistence/active_record_persistence.rb b/lib/aasm/persistence/active_record_persistence.rb index 1199c14..eab0505 100644 --- a/lib/aasm/persistence/active_record_persistence.rb +++ b/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) diff --git a/lib/aasm/persistence/orm.rb b/lib/aasm/persistence/orm.rb index 9b4f949..e6d7191 100644 --- a/lib/aasm/persistence/orm.rb +++ b/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 diff --git a/spec/unit/persistence/active_record_persistence_spec.rb b/spec/unit/persistence/active_record_persistence_spec.rb index 09781f4..db1ddcd 100644 --- a/spec/unit/persistence/active_record_persistence_spec.rb +++ b/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