From 29f923a6bc2e5a7c231e16072d2f8f990b912fb7 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 1 Aug 2024 12:32:15 -0400 Subject: [PATCH 1/2] Ensure States are completed if an error is raised --- lib/floe/workflow/state.rb | 24 +++++++++++++++++++++--- spec/workflow/state_spec.rb | 4 ++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/floe/workflow/state.rb b/lib/floe/workflow/state.rb index 7ba66470..477241eb 100644 --- a/lib/floe/workflow/state.rb +++ b/lib/floe/workflow/state.rb @@ -48,17 +48,27 @@ def run_nonblock!(context) return Errno::EAGAIN unless ready?(context) finish(context) + rescue Floe::Error => e + mark_error(context, e) end def start(context) + mark_started(context) + end + + def finish(context) + mark_finished(context) + end + + def mark_started(context) context.state["EnteredTime"] = Time.now.utc.iso8601 logger.info("Running state: [#{long_name}] with input [#{context.json_input}]...") end - def finish(context) - finished_time = Time.now.utc - entered_time = Time.parse(context.state["EnteredTime"]) + def mark_finished(context) + finished_time = Time.now.utc + entered_time = Time.parse(context.state["EnteredTime"]) context.state["FinishedTime"] ||= finished_time.iso8601 context.state["Duration"] = finished_time - entered_time @@ -69,6 +79,14 @@ def finish(context) 0 end + def mark_error(context, exception) + # InputPath or OutputPath were bad. + context.next_state = nil + context.output = {"Error" => "States.Runtime", "Cause" => exception.message} + # Since finish threw an exception, super was never called. Calling that now. + mark_finished(context) + end + def ready?(context) !context.state_started? || !running?(context) end diff --git a/spec/workflow/state_spec.rb b/spec/workflow/state_spec.rb index 850a3986..2c4943a9 100644 --- a/spec/workflow/state_spec.rb +++ b/spec/workflow/state_spec.rb @@ -45,7 +45,7 @@ it "is finished" do state.start(ctx) - state.finish(ctx) + state.mark_finished(ctx) expect(ctx.state_started?).to eq(true) end @@ -63,7 +63,7 @@ it "is finished" do state.start(ctx) - state.finish(ctx) + state.mark_finished(ctx) expect(ctx.state_finished?).to eq(true) end From 22c08f96ea923398aeb01f6288080b3c39031215 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 1 Aug 2024 12:30:53 -0400 Subject: [PATCH 2/2] Raise error for bad paths Before: If the path referenced a missing value in the input, treat it as a nil. After: If the path references a missing value in the input, raise an error. This makes it necessary to catch errors and ensure that the context is marked appropriately. Since a path lookup is so stringent, introduce the presence_check. --- lib/floe.rb | 1 + lib/floe/workflow/choice_rule/data.rb | 22 ++++-- lib/floe/workflow/path.rb | 13 +++- spec/workflow/choice_rule_spec.rb | 52 +++++++++++--- spec/workflow/intrinsic_function_spec.rb | 4 +- spec/workflow/path_spec.rb | 8 +-- spec/workflow/states/choice_spec.rb | 10 ++- spec/workflow/states/pass_spec.rb | 86 ++++++++++++++++++++++++ 8 files changed, 173 insertions(+), 23 deletions(-) diff --git a/lib/floe.rb b/lib/floe.rb index 41dd33fb..7165b6be 100644 --- a/lib/floe.rb +++ b/lib/floe.rb @@ -43,6 +43,7 @@ module Floe class Error < StandardError; end class InvalidWorkflowError < Error; end class InvalidExecutionInput < Error; end + class PathError < Error; end def self.logger @logger ||= NullLogger.new diff --git a/lib/floe/workflow/choice_rule/data.rb b/lib/floe/workflow/choice_rule/data.rb index fb626fd5..d644655e 100644 --- a/lib/floe/workflow/choice_rule/data.rb +++ b/lib/floe/workflow/choice_rule/data.rb @@ -5,14 +5,13 @@ class Workflow class ChoiceRule class Data < Floe::Workflow::ChoiceRule def true?(context, input) + return presence_check(context, input) if compare_key == "IsPresent" + lhs = variable_value(context, input) rhs = compare_value(context, input) - validate!(lhs) - case compare_key when "IsNull" then is_null?(lhs) - when "IsPresent" then is_present?(lhs) when "IsNumeric" then is_numeric?(lhs) when "IsString" then is_string?(lhs) when "IsBoolean" then is_boolean?(lhs) @@ -47,8 +46,21 @@ def true?(context, input) private - def validate!(value) - raise "No such variable [#{variable}]" if value.nil? && !%w[IsNull IsPresent].include?(compare_key) + def presence_check(context, input) + # Get the right hand side for {"Variable": "$.foo", "IsPresent": true} i.e.: true + # If true then return true when present. + # If false then return true when not present. + rhs = compare_value(context, input) + # Don't need the variable_value, just need to see if the path finds the value. + variable_value(context, input) + + # The variable_value is present + # If rhs is true, then presence check was successful, return true. + rhs + rescue Floe::PathError + # variable_value is not present. (the path lookup threw an error) + # If rhs is false, then it successfully wasn't present, return true. + !rhs end def is_null?(value) # rubocop:disable Naming/PredicateName diff --git a/lib/floe/workflow/path.rb b/lib/floe/workflow/path.rb index 51f3eba7..c41a514b 100644 --- a/lib/floe/workflow/path.rb +++ b/lib/floe/workflow/path.rb @@ -34,7 +34,18 @@ def value(context, input = {}) return obj if path == "$" results = JsonPath.on(obj, path) - results.count < 2 ? results.first : results + case results.count + when 0 + raise Floe::PathError, "Path [#{payload}] references an invalid value" + when 1 + results.first + else + results + end + end + + def to_s + payload end end end diff --git a/spec/workflow/choice_rule_spec.rb b/spec/workflow/choice_rule_spec.rb index a2a9371f..f2afca3e 100644 --- a/spec/workflow/choice_rule_spec.rb +++ b/spec/workflow/choice_rule_spec.rb @@ -93,7 +93,7 @@ let(:input) { {} } it "raises an exception" do - expect { subject }.to raise_exception(RuntimeError, "No such variable [$.foo]") + expect { subject }.to raise_exception(Floe::PathError, "Path [$.foo] references an invalid value") end end @@ -118,22 +118,51 @@ end context "with IsPresent" do - let(:payload) { {"Variable" => "$.foo", "IsPresent" => true, "Next" => "FirstMatchState"} } + let(:positive) { true } + let(:payload) { {"Variable" => "$.foo", "IsPresent" => positive, "Next" => "FirstMatchState"} } context "with null" do let(:input) { {"foo" => nil} } + it { expect(subject).to eq(true) } + end - it "returns false" do - expect(subject).to eq(false) - end + context "with false" do + let(:input) { {"foo" => "bar"} } + it { expect(subject).to eq(true) } end - context "with non-null" do + context "with string" do + let(:input) { {"foo" => false} } + it { expect(subject).to eq(true) } + end + + context "with missing value" do + let(:input) { {} } + it { expect(subject).to eq(false) } + end + + context "with null" do + let(:positive) { false } + let(:input) { {"foo" => nil} } + it { expect(subject).to eq(false) } + end + + context "with false" do + let(:positive) { false } let(:input) { {"foo" => "bar"} } + it { expect(subject).to eq(false) } + end - it "returns true" do - expect(subject).to eq(true) - end + context "with string" do + let(:positive) { false } + let(:input) { {"foo" => false} } + it { expect(subject).to eq(false) } + end + + context "with missing value" do + let(:positive) { false } + let(:input) { {} } + it { expect(subject).to eq(true) } end end @@ -279,6 +308,11 @@ expect(subject).to eq(false) end end + + context "with path not found" do + let(:input) { {"foo" => 2} } + it { expect { subject }.to raise_error(Floe::PathError, "Path [$.bar] references an invalid value") } + end end context "with a NumericLessThan" do diff --git a/spec/workflow/intrinsic_function_spec.rb b/spec/workflow/intrinsic_function_spec.rb index 17510038..513bf421 100644 --- a/spec/workflow/intrinsic_function_spec.rb +++ b/spec/workflow/intrinsic_function_spec.rb @@ -655,8 +655,8 @@ end it "handles invalid path references" do - result = described_class.value("States.Array($.xxx)", {"context" => {"baz" => "qux"}}, {"input" => {"foo" => "bar"}}) - expect(result).to eq([nil]) + ctx = {"context" => {"baz" => "qux"}}, {"input" => {"foo" => "bar"}} + expect { described_class.value("States.Array($.xxx)", ctx) }.to raise_error(Floe::PathError, "Path [$.xxx] references an invalid value") end end diff --git a/spec/workflow/path_spec.rb b/spec/workflow/path_spec.rb index d2c52d8d..a77d7e9a 100644 --- a/spec/workflow/path_spec.rb +++ b/spec/workflow/path_spec.rb @@ -11,15 +11,15 @@ describe "#value" do context "referencing the global context" do it "with a missing value" do - expect(described_class.new("$$.foo").value({})).to be_nil + expect { described_class.new("$$.foo").value({}, {"foo" => "bar"}) }.to raise_error(Floe::PathError, "Path [$$.foo] references an invalid value") end it "with a single value" do - expect(described_class.new("$$.foo").value({"foo" => "bar"})).to eq("bar") + expect(described_class.new("$$.foo").value({"foo" => "bar"}, {})).to eq("bar") end it "with a nested hash" do - expect(described_class.new("$$.foo.bar").value({"foo" => {"bar" => "baz"}})).to eq("baz") + expect(described_class.new("$$.foo.bar").value({"foo" => {"bar" => "baz"}}, {})).to eq("baz") end it "with an array" do @@ -33,7 +33,7 @@ context "referencing the inputs" do it "with a missing value" do - expect(described_class.new("$.foo").value({"foo" => "bar"})).to be_nil + expect { described_class.new("$.foo").value({"foo" => "bar"}, {}) }.to raise_error(Floe::PathError, "Path [$.foo] references an invalid value") end it "with a single value" do diff --git a/spec/workflow/states/choice_spec.rb b/spec/workflow/states/choice_spec.rb index 754fe392..40cdfb02 100644 --- a/spec/workflow/states/choice_spec.rb +++ b/spec/workflow/states/choice_spec.rb @@ -49,8 +49,14 @@ describe "#run_nonblock!" do context "with a missing variable" do - it "raises an exception" do - expect { state.run_nonblock!(ctx) }.to raise_error(RuntimeError, "No such variable [$.foo]") + it "shows error" do + workflow.run_nonblock + expect(ctx.output).to eq( + { + "Cause" => "Path [$.foo] references an invalid value", + "Error" => "States.Runtime" + } + ) end end diff --git a/spec/workflow/states/pass_spec.rb b/spec/workflow/states/pass_spec.rb index 75704ff7..aba7b571 100644 --- a/spec/workflow/states/pass_spec.rb +++ b/spec/workflow/states/pass_spec.rb @@ -86,6 +86,34 @@ # it { expect { workflow }.to raise_error(Floe::InvalidWorkflowError, "States.PassState error") } # end + + context "With an invalid InputPath" do + let(:payload) do + { + "PassState" => { + "Type" => "Pass", + "InputPath" => "bad", + "End" => true + } + } + end + + it { expect { workflow }.to raise_error(Floe::InvalidWorkflowError, "Path [bad] must start with \"$\"") } + end + + context "With an invalid OutputPath" do + let(:payload) do + { + "PassState" => { + "Type" => "Pass", + "OutputPath" => "bad", + "End" => true + } + } + end + + it { expect { workflow }.to raise_error(Floe::InvalidWorkflowError, "Path [bad] must start with \"$\"") } + end end describe "#end?" do @@ -124,6 +152,52 @@ end end + context "With a missing InputPath" do + let(:payload) do + { + "PassState" => { + "Type" => "Pass", + "End" => true, + "InputPath" => "$.missing" + } + } + end + + it "completes with an error" do + workflow.run_nonblock + expect(ctx.state_finished?).to eq(true) + expect(ctx.output).to eq( + { + "Cause" => "Path [$.missing] references an invalid value", + "Error" => "States.Runtime" + } + ) + end + end + + context "With a missing OutputPath" do + let(:payload) do + { + "PassState" => { + "Type" => "Pass", + "End" => true, + "OutputPath" => "$.missing.spot" + } + } + end + + it "completes with an error" do + workflow.run_nonblock + expect(ctx.state_finished?).to eq(true) + expect(ctx.output).to eq( + { + "Cause" => "Path [$.missing.spot] references an invalid value", + "Error" => "States.Runtime" + } + ) + end + end + # https://states-language.net/#inputoutput-processing-examples context "with 2 blocks" do let(:payload) do @@ -182,6 +256,18 @@ end end + context "with Invalid InputPath" do + let(:input) { {} } + let(:payload) do + {"Pass" => {"Type" => "Pass", "End" => true, "InputPath" => "$.color"}} + end + + it "detects missing value" do + workflow.run_nonblock + expect(ctx.output).to eq({"Cause" => "Path [$.color] references an invalid value", "Error" => "States.Runtime"}) + end + end + context "with OutputPath" do let(:input) { {"color" => "red", "garbage" => nil} } let(:payload) { {"Pass" => {"Type" => "Pass", "End" => true, "OutputPath" => "$.color"}} }