Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Choice rule payload validation path #253

Merged
merged 2 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
22 changes: 17 additions & 5 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,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
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
24 changes: 21 additions & 3 deletions lib/floe/workflow/state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the essence of the issue

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