From c16762b2c028199801c7c9b7ea859f184e37eb33 Mon Sep 17 00:00:00 2001 From: Michael MacDonald Date: Thu, 3 Nov 2016 12:28:30 +1100 Subject: [PATCH 1/2] [AV-3601] Initialise instance persistent state after starting workflow and do not execute decisions or entry points after workflow has finished --- lib/decision_tree/workflow.rb | 41 ++++---- spec/decision_tree/workflow_spec.rb | 142 +++++++++++++++++++++++----- 2 files changed, 143 insertions(+), 40 deletions(-) diff --git a/lib/decision_tree/workflow.rb b/lib/decision_tree/workflow.rb index 88ed752..4c46ba5 100644 --- a/lib/decision_tree/workflow.rb +++ b/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 diff --git a/spec/decision_tree/workflow_spec.rb b/spec/decision_tree/workflow_spec.rb index 271e0e9..b89bd6c 100644 --- a/spec/decision_tree/workflow_spec.rb +++ b/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 From 0b0d764add51dc81c0f7d100962133893cab5c3c Mon Sep 17 00:00:00 2001 From: Michael MacDonald Date: Thu, 3 Nov 2016 15:58:11 +1100 Subject: [PATCH 2/2] Bump to v0.1.1 --- lib/decision_tree/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/decision_tree/version.rb b/lib/decision_tree/version.rb index df02d50..8345a5a 100644 --- a/lib/decision_tree/version.rb +++ b/lib/decision_tree/version.rb @@ -1,3 +1,3 @@ module DecisionTree - VERSION = "0.1.0" + VERSION = "0.1.1" end