Skip to content

Commit

Permalink
Raise error for bad paths
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kbrock committed Aug 1, 2024
1 parent 29f923a commit dceeab9
Show file tree
Hide file tree
Showing 8 changed files with 177 additions and 25 deletions.
1 change: 1 addition & 0 deletions lib/floe.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 21 additions & 7 deletions lib/floe/workflow/choice_rule/data.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -47,16 +46,31 @@ 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
value.nil?
end

def is_present?(value) # rubocop:disable Naming/PredicateName
!value.nil?
# If it got here (and value was fetched), then it is present.
# This probably will never be called, but keeping just in case.
def is_present?(_value) # rubocop:disable Naming/PredicateName
true
end

def is_numeric?(value) # rubocop:disable Naming/PredicateName
Expand Down
13 changes: 12 additions & 1 deletion lib/floe/workflow/path.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 43 additions & 9 deletions spec/workflow/choice_rule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions spec/workflow/intrinsic_function_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 4 additions & 4 deletions spec/workflow/path_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
10 changes: 8 additions & 2 deletions spec/workflow/states/choice_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
86 changes: 86 additions & 0 deletions spec/workflow/states/pass_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"}} }
Expand Down

0 comments on commit dceeab9

Please sign in to comment.