Merge pull request #5 from jobready/AV-3601-initialise-after-lock

[AV-3601] Initialise ivars after lock and do not execute decisions/entries if finished

Author: schlick

lib/decision_tree/version.rb

@@ -1,3 +1,3 @@
 module DecisionTree
-  VERSION = "0.1.0"
+  VERSION = "0.1.1"
 end

lib/decision_tree/workflow.rb

@@ -17,13 +17,18 @@ class MethodNotDefinedError < StandardError; end
   def initialize(store=nil)
     @store = store || DecisionTree::Store.new
     @steps = []
-    initialize_persistent_state
     @proxy = DecisionTree::Proxy.new(self)
 
-    if finished?
-      @steps = store.fetch_steps
-    else
-      execute_workflow
+    store.start_workflow do
+      initialize_persistent_state
+
+      if finished?
+        @steps = store.fetch_steps
+      else
+        execute_workflow
+      end
+
+      persist_state!
     end
   end
 
@@ -46,20 +51,15 @@ def finished?
   # Actually executes the workflow steps, by executing all the steps from
   # either the start, or all previously reached entry points
   def execute_workflow
-    # We're using pessimistic locking here, so this will block until an
-    # exclusive lock can be obtained on the change.
-    store.start_workflow do
-      catch :exit do
-        if @entry_points.empty?
-          send(:__start_workflow)
-        else
-          # TODO: This should fail silently if an entry point is no longer
-          # defined, this will allow for modification of the workflows with
-          # existing changes in the DB.
-          @entry_points.each { |ep| send(ep) }
-        end
+    catch :exit do
+      if @entry_points.empty?
+        send(:__start_workflow)
+      else
+        # TODO: This should fail silently if an entry point is no longer
+        # defined, this will allow for modification of the workflows with
+        # existing changes in the DB.
+        @entry_points.each { |ep| send(ep) }
       end
-      persist_state!
     end
   end
 
@@ -108,6 +108,8 @@ def self.decision(method_name, &block)
     fail YesAndNoRequiredError unless yes_block && no_block
 
     define_method(method_name) do
+      return if finished?
+
       if send(aliased_method_name)
         @steps << DecisionTree::Step.new(method_name, 'YES')
         @proxy.instance_eval(&yes_block)
@@ -128,6 +130,8 @@ def self.entry(method_name, &block)
     aliased_method_name = alias_method_name(method_name)
 
     define_method(method_name) do
+      return self if finished?
+
       @entry_points << method_name.to_s
       @steps << DecisionTree::Step.new('Entry Point', method_name.to_s)
       send(aliased_method_name)
@@ -137,6 +141,7 @@ def self.entry(method_name, &block)
         end
         persist_state!
       end
+
       self
     end
   end

spec/decision_tree/workflow_spec.rb

@@ -49,7 +49,6 @@ def decision_method
             decision_method
           end
         end
-        expect_any_instance_of(TestWorkflow).to receive(:__decision_method).once.and_return(true)
       end
 
       it 'calls only the yes block' do
@@ -75,7 +74,6 @@ def decision_method
             decision_method
           end
         end
-        expect_any_instance_of(TestWorkflow).to receive(:__decision_method).once.and_return(false)
       end
 
       it 'calls only the no block' do
@@ -117,15 +115,47 @@ def always_true
         end
       end
     end
+
+    context 'when the workflow is already finished' do
+      before do
+        class TestWorkflow < DecisionTree::Workflow
+          def decision_method
+          end
+
+          decision :decision_method do
+            yes { }
+            no { }
+          end
+        end
+      end
+
+      it 'does not execute the decision' do
+        allow_any_instance_of(TestWorkflow).to receive(:finished?).and_return(true)
+        expect_any_instance_of(TestWorkflow).not_to receive(:__decision_method)
+        TestWorkflow.new(store).send(:decision_method)
+      end
+    end
   end
 
   describe '.entry' do
     before do
       class TestWorkflow < DecisionTree::Workflow
+        def always_true
+          true
+        end
+
+        def non_idempotent_action!
+        end
+
         def test_entry
         end
 
-        entry(:test_entry) {}
+        decision :always_true do
+          yes { non_idempotent_action! }
+          no { exit }
+        end
+
+        entry(:test_entry) { always_true }
         start {}
       end
     end
@@ -156,6 +186,35 @@ def test_entry
         TestWorkflow.new(store)
       end
     end
+
+    context 'for a store that updates state before yielding to workflow (ie locking)' do
+      subject { TestWorkflow.new(store) }
+      let(:store) { TestStore.new }
+
+      before do
+        class TestStore < DecisionTree::Store
+          def start_workflow(&block)
+            self.state = '__start_workflow:non_idempotent_action!'
+            yield
+          end
+        end
+      end
+
+      it 'does not call the non-idempotent method again' do
+        expect_any_instance_of(TestWorkflow).to_not receive(:non_idempotent_action!)
+        subject.test_entry
+      end
+    end
+
+    context 'when the workflow is already finished' do
+      subject { TestWorkflow.new(store) }
+
+      it 'does not execute the entry point' do
+        allow_any_instance_of(TestWorkflow).to receive(:finished?).and_return(true)
+        expect_any_instance_of(TestWorkflow).not_to receive(:__test_entry)
+        subject.test_entry
+      end
+    end
   end
 
   describe '.start' do
@@ -188,33 +247,72 @@ def decision_method
   end
 
   describe '.initialize' do
+    subject { TestWorkflow.new(store) }
+
+    let(:store) { TestStore.new }
+
     before do
       class TestWorkflow < DecisionTree::Workflow
-      end
+        def always_true
+          true
+        end
+
+        decision :always_true do
+          yes { non_idempotent_action! }
+          no { exit }
+        end
 
-      allow_any_instance_of(TestWorkflow).to receive(:finished?) { finished }
+        start { always_true }
+      end
     end
 
-    subject { TestWorkflow.new(store) }
+    context 'for store that simply yields to the workflow' do
+      before do
+        class TestStore < DecisionTree::Store
+          def start_workflow(&block)
+            yield
+          end
+        end
 
-    context 'when workflow not previously completed' do
-      let(:finished) { false }
-      specify 'executes the workflow' do
-        expect_any_instance_of(TestWorkflow).to receive(:execute_workflow)
-        subject
+        allow_any_instance_of(TestWorkflow).to receive(:finished?) { finished }
       end
-    end
 
-    context 'when workflow previously completed' do
-      let(:finished) { true }
+      context 'when workflow previously completed' do
+        let(:finished) { true }
 
-      specify 'does not execute the workflow' do
-        expect_any_instance_of(TestWorkflow).to_not receive(:execute_workflow)
-        subject
+        it 'does not execute the workflow' do
+          expect_any_instance_of(TestWorkflow).to_not receive(:execute_workflow)
+          subject
+        end
+
+        it 'fetches previously executed steps from the store' do
+          expect(store).to receive(:fetch_steps)
+          subject
+        end
+      end
+
+      context 'when workflow not previously completed' do
+        let(:finished) { false }
+
+        it 'executes the workflow' do
+          expect_any_instance_of(TestWorkflow).to receive(:execute_workflow)
+          subject
+        end
+      end
+    end
+
+    context 'for a store that updates state before yielding to workflow (ie locking)' do
+      before do
+        class TestStore < DecisionTree::Store
+          def start_workflow(&block)
+            self.state = '__start_workflow:non_idempotent_action!'
+            yield
+          end
+        end
       end
 
-      specify 'fetches previously executed steps from the store' do
-        expect(store).to receive(:fetch_steps)
+      it 'does not call the non-idempotent method again' do
+        expect_any_instance_of(TestWorkflow).to_not receive(:non_idempotent_action!)
         subject
       end
     end
@@ -232,7 +330,7 @@ class TestWorkflow < DecisionTree::Workflow
 
     let(:workflow) { TestWorkflow.new(store) }
 
-    specify 'records the finish call' do
+    it 'records the finish call' do
       finish!
       expect(subject).to include('finish!')
     end
@@ -249,7 +347,7 @@ class TestWorkflow < DecisionTree::Workflow
         end
       end
 
-      specify 'calls finish! on the workflow' do
+      it 'calls finish! on the workflow' do
         expect_any_instance_of(TestWorkflow).to receive(:finish!)
         workflow
       end
@@ -271,7 +369,7 @@ def decision_method
         end
       end
 
-      specify 'calls finish! on the workflow' do
+      it 'calls finish! on the workflow' do
         expect_any_instance_of(TestWorkflow).to receive(:finish!)
         workflow
       end