From ced4e5ccc56a56fdcc753589a54ffdb917f33a98 Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Thu, 11 Jul 2024 14:19:27 -0400
Subject: [PATCH] Better Runtime error detection for bad paths

---
 lib/floe.rb                                   |  2 +
 lib/floe/validation_mixin.rb                  | 27 +++++++---
 lib/floe/workflow/choice_rule/data.rb         | 18 +++++--
 lib/floe/workflow/path.rb                     |  9 +++-
 lib/floe/workflow/state.rb                    | 21 ++++++--
 lib/floe/workflow/states/choice.rb            |  6 ++-
 lib/floe/workflow/states/fail.rb              |  6 +--
 .../workflow/states/input_output_mixin.rb     | 23 ++++----
 lib/floe/workflow/states/succeed.rb           |  4 +-
 lib/floe/workflow/states/task.rb              |  2 +-
 lib/floe/workflow/states/wait.rb              |  6 +--
 spec/workflow/choice_rule_spec.rb             | 52 +++++++++++++++----
 spec/workflow/intrinsic_function_spec.rb      |  4 +-
 spec/workflow/path_spec.rb                    |  8 +--
 spec/workflow/state_spec.rb                   |  4 +-
 spec/workflow/states/choice_spec.rb           |  4 +-
 spec/workflow/states/pass_spec.rb             | 34 ++++++++++++
 17 files changed, 175 insertions(+), 55 deletions(-)

diff --git a/lib/floe.rb b/lib/floe.rb
index 41dd33fb..bef087fe 100644
--- a/lib/floe.rb
+++ b/lib/floe.rb
@@ -43,6 +43,8 @@ module Floe
   class Error < StandardError; end
   class InvalidWorkflowError < Error; end
   class InvalidExecutionInput < Error; end
+  class PathError < Error; end
+  class ExecutionError < Error; end
 
   def self.logger
     @logger ||= NullLogger.new
diff --git a/lib/floe/validation_mixin.rb b/lib/floe/validation_mixin.rb
index 33d5c184..04d7bddd 100644
--- a/lib/floe/validation_mixin.rb
+++ b/lib/floe/validation_mixin.rb
@@ -7,7 +7,7 @@ def self.included(base)
     end
 
     def parser_error!(comment)
-      self.class.parser_error!(name, comment)
+      raise Floe::InvalidWorkflowError, "#{self.class.location_name_str(name)} #{comment}"
     end
 
     def missing_field_error!(field_name)
@@ -18,6 +18,16 @@ def invalid_field_error!(field_name, field_value = nil, comment = nil)
       self.class.invalid_field_error!(name, field_name, field_value, comment)
     end
 
+    def runtime_field_error!(field_name, field_value, comment)
+      raise Floe::ExecutionError, self.class.field_error_text(name, field_name, field_value, comment)
+    end
+
+    def wrap_runtime_error(field_name, field_value)
+      yield
+    rescue Floe::PathError => e
+      runtime_field_error!(field_name, field_value, e.message)
+    end
+
     def workflow_state?(field_value, workflow)
       workflow.payload["States"] ? workflow.payload["States"].include?(field_value) : true
     end
@@ -29,20 +39,25 @@ def wrap_parser_error(field_name, field_value)
     end
 
     module ClassMethods
-      def parser_error!(name, comment)
-        name = name.join(".") if name.kind_of?(Array)
-        raise Floe::InvalidWorkflowError, "#{name} #{comment}"
+      def location_name_str(name)
+        name.kind_of?(Array) ? name.join(".") : name
       end
 
+      # these are here for State.build!
+
       def missing_field_error!(name, field_name)
-        parser_error!(name, "does not have required field \"#{field_name}\"")
+        raise Floe::InvalidWorkflowError, "#{location_name_str(name)} does not have required field \"#{field_name}\""
       end
 
       def invalid_field_error!(name, field_name, field_value, comment)
+        raise Floe::InvalidWorkflowError, field_error_text(name, field_name, field_value, comment)
+      end
+
+      def field_error_text(name, field_name, field_value, comment = nil)
         # instead of displaying a large hash or array, just displaying the word Hash or Array
         field_value = field_value.class if field_value.kind_of?(Hash) || field_value.kind_of?(Array)
 
-        parser_error!(name, "field \"#{field_name}\"#{" value \"#{field_value}\"" unless field_value.nil?} #{comment}")
+        "#{location_name_str(name)} field \"#{field_name}\"#{" value \"#{field_value}\"" unless field_value.nil?} #{comment}"
       end
     end
   end
diff --git a/lib/floe/workflow/choice_rule/data.rb b/lib/floe/workflow/choice_rule/data.rb
index fb626fd5..1407218e 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,17 @@ 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)
+          rhs = compare_value(context, input)
+          # don't need the value, just need to see if the path finds the value
+          variable_value(context, input)
+
+          # path found the variable_value, (so if they said true, return true)
+          rhs
+        rescue Floe::ExecutionError
+          # variable_value (path) threw an error
+          # it was not found (so if they said false, 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 fe8a3cf7..6ae06b21 100644
--- a/lib/floe/workflow/path.rb
+++ b/lib/floe/workflow/path.rb
@@ -34,7 +34,14 @@ 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, "references an invalid value"
+        when 1
+          results.first
+        else
+          results
+        end
       end
 
       def to_s
diff --git a/lib/floe/workflow/state.rb b/lib/floe/workflow/state.rb
index be096f03..0ab743b8 100644
--- a/lib/floe/workflow/state.rb
+++ b/lib/floe/workflow/state.rb
@@ -45,20 +45,35 @@ def wait(context, timeout: nil)
       # @return for incomplete Errno::EAGAIN, for completed 0
       def run_nonblock!(context)
         start(context) unless context.state_started?
+
         return Errno::EAGAIN unless ready?(context)
 
         finish(context)
+      rescue Floe::ExecutionError => e
+        # input / output paths were bad
+        context.next_state = nil
+        context.output     = {"Error" => "States.RuntimeError", "Cause" => e.message}
+        # finish.super was never called
+        mark_finished(context)
       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
diff --git a/lib/floe/workflow/states/choice.rb b/lib/floe/workflow/states/choice.rb
index d2487e5f..d8037cac 100644
--- a/lib/floe/workflow/states/choice.rb
+++ b/lib/floe/workflow/states/choice.rb
@@ -19,12 +19,14 @@ def initialize(workflow, name, payload)
         end
 
         def finish(context)
-          input      = input_path.value(context, context.input)
-          output     = output_path.value(context, input)
+          input      = wrap_runtime_error("InputPath", input_path.to_s) { input_path.value(context, context.input) }
+          output     = wrap_runtime_error("OutputPath", output_path.to_s) { output_path.value(context, input) }
+          # For a bad path, throws an ExecutionError
           next_state = choices.detect { |choice| choice.true?(context, output) }&.next || default
 
           context.next_state = next_state
           context.output     = output
+
           super
         end
 
diff --git a/lib/floe/workflow/states/fail.rb b/lib/floe/workflow/states/fail.rb
index ae0f54f8..56e28160 100644
--- a/lib/floe/workflow/states/fail.rb
+++ b/lib/floe/workflow/states/fail.rb
@@ -4,7 +4,7 @@ module Floe
   class Workflow
     module States
       class Fail < Floe::Workflow::State
-        attr_reader :cause, :error
+        attr_reader :cause, :error, :cause_path, :error_path
 
         def initialize(workflow, name, payload)
           super
@@ -21,8 +21,8 @@ def finish(context)
           # see https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html
           #     https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-intrinsic-functions.html#asl-intrsc-func-generic
           context.output     = {
-            "Error" => @error_path ? @error_path.value(context, context.input) : error,
-            "Cause" => @cause_path ? @cause_path.value(context, context.input) : cause
+            "Error" => error_path ? wrap_runtime_error("ErrorPath", error_path.to_s) { @error_path.value(context, context.input) } : error,
+            "Cause" => cause_path ? wrap_runtime_error("CausePath", cause_path.to_s) { @cause_path.value(context, context.input) } : cause
           }.compact
           super
         end
diff --git a/lib/floe/workflow/states/input_output_mixin.rb b/lib/floe/workflow/states/input_output_mixin.rb
index 59b3a732..b0149baa 100644
--- a/lib/floe/workflow/states/input_output_mixin.rb
+++ b/lib/floe/workflow/states/input_output_mixin.rb
@@ -5,25 +5,30 @@ class Workflow
     module States
       module InputOutputMixin
         def process_input(context)
-          input = input_path.value(context, context.input)
-          input = parameters.value(context, input) if parameters
+          input = wrap_runtime_error("InputPath", input_path.to_s) { input_path.value(context, context.input) }
+          input = wrap_runtime_error("Parameters", parameters.to_s) { parameters.value(context, input) } if parameters
           input
         end
 
         def process_output(context, results)
-          return context.input.dup if results.nil?
-          return if output_path.nil?
+          results = context.input.dup if results.nil?
 
-          results = result_selector.value(context, results) if @result_selector
-          if result_path.payload.start_with?("$.Credentials")
-            credentials = result_path.set(context.credentials, results)["Credentials"]
+          results = wrap_runtime_error("ResultSelector", @result_selector.to_s) { result_selector.value(context, results) } if @result_selector
+          if result_path.nil?
+            output = results
+          elsif result_path.payload.start_with?("$.Credentials")
+            credentials = wrap_runtime_error("ResultPath", result_path.to_s) { result_path.set(context.credentials, results)["Credentials"] }
             context.credentials.merge!(credentials)
             output = context.input.dup
           else
-            output = result_path.set(context.input.dup, results)
+            output = wrap_runtime_error("ResultPath", result_path.to_s) { result_path.set(context.input.dup, results) }
           end
 
-          output_path.value(context, output)
+          if output_path
+            wrap_runtime_error("OutputPath", output_path.to_s) { output_path.value(context, output) }
+          else
+            output
+          end
         end
       end
     end
diff --git a/lib/floe/workflow/states/succeed.rb b/lib/floe/workflow/states/succeed.rb
index bbbd814b..47a51d53 100644
--- a/lib/floe/workflow/states/succeed.rb
+++ b/lib/floe/workflow/states/succeed.rb
@@ -14,8 +14,8 @@ def initialize(workflow, name, payload)
         end
 
         def finish(context)
-          input              = input_path.value(context, context.input)
-          context.output     = output_path.value(context, input)
+          input              = wrap_runtime_error("InputPath", input_path.to_s) { input_path.value(context, context.input) }
+          context.output     = wrap_runtime_error("OutputPath", output_path.to_s) { output_path.value(context, input) }
           context.next_state = nil
 
           super
diff --git a/lib/floe/workflow/states/task.rb b/lib/floe/workflow/states/task.rb
index 414b8e0c..b5e94626 100644
--- a/lib/floe/workflow/states/task.rb
+++ b/lib/floe/workflow/states/task.rb
@@ -116,7 +116,7 @@ def catch_error!(context, error)
           return if catcher.nil?
 
           context.next_state = catcher.next
-          context.output     = catcher.result_path.set(context.input, error)
+          context.output     = wrap_runtime_error("ResultPath", result_path.to_s) { catcher.result_path.set(context.input, error) }
           logger.info("Running state: [#{long_name}] with input [#{context.json_input}]...CatchError - next state: [#{context.next_state}] output: [#{context.json_output}]")
 
           true
diff --git a/lib/floe/workflow/states/wait.rb b/lib/floe/workflow/states/wait.rb
index e22ee5d2..99d3b464 100644
--- a/lib/floe/workflow/states/wait.rb
+++ b/lib/floe/workflow/states/wait.rb
@@ -28,7 +28,7 @@ def initialize(workflow, name, payload)
         def start(context)
           super
 
-          input = input_path.value(context, context.input)
+          input = wrap_runtime_error("InputPath", input_path.to_s) { input_path.value(context, context.input) }
 
           wait_until!(
             context,
@@ -38,8 +38,8 @@ def start(context)
         end
 
         def finish(context)
-          input          = input_path.value(context, context.input)
-          context.output = output_path.value(context, input)
+          input          = wrap_runtime_error("InputPath", input_path.to_s) { input_path.value(context, context.input) }
+          context.output = wrap_runtime_error("OutputPath", input_path.to_s) { output_path.value(context, input) }
           super
         end
 
diff --git a/spec/workflow/choice_rule_spec.rb b/spec/workflow/choice_rule_spec.rb
index a2a9371f..c9dbde2c 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, "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 raise_error(Floe::PathError, "references an invalid value") }
+        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 raise_error(Floe::PathError, "references an invalid value") }
         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, "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..14a31fb4 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, "references an invalid value")
       end
     end
 
diff --git a/spec/workflow/path_spec.rb b/spec/workflow/path_spec.rb
index d2c52d8d..f405fa4b 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, "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, "references an invalid value")
       end
 
       it "with a single value" do
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
diff --git a/spec/workflow/states/choice_spec.rb b/spec/workflow/states/choice_spec.rb
index 754fe392..b62bd2a7 100644
--- a/spec/workflow/states/choice_spec.rb
+++ b/spec/workflow/states/choice_spec.rb
@@ -49,9 +49,7 @@
 
   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]")
-      end
+      it { expect { workflow.run_nonblock }.to raise_error(Floe::PathError, "references an invalid value") }
     end
 
     context "with an input value matching a condition" do
diff --git a/spec/workflow/states/pass_spec.rb b/spec/workflow/states/pass_spec.rb
index 112fa9ae..5eacadd5 100644
--- a/spec/workflow/states/pass_spec.rb
+++ b/spec/workflow/states/pass_spec.rb
@@ -138,6 +138,28 @@
       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" => "States.PassState field \"InputPath\" value \"$.missing\" references an invalid value",
+            "Error" => "States.RuntimeError"
+          }
+        )
+      end
+    end
+
     # https://states-language.net/#inputoutput-processing-examples
     context "with 2 blocks" do
       let(:payload) do
@@ -196,6 +218,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" => "States.Pass field \"InputPath\" value \"$.color\" references an invalid value", "Error" => "States.RuntimeError"})
+        end
+      end
+
       context "with OutputPath" do
         let(:input)   { {"color" => "red", "garbage" => nil} }
         let(:payload) { {"Pass" => {"Type" => "Pass", "End" => true, "OutputPath" => "$.color"}} }