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

Validate that state machine input is valid JSON #227

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 21, 2024

When running a workflow validate that the state-machine input is valid JSON. We were a little lax with specs passing Hashes in for convenience but it isn't hard to ensure we're passing JSON in to context input.

When running with ASL passing invalid input:

$ aws stepfunctions --endpoint-url http://localhost:8083 start-execution --state-machine-arn arn:aws:states:us-east-1:123456789012:stateMachine:Pass --input 'foo'

An error occurred (InvalidExecutionInput) when calling the StartExecution operation: Invalid State Machine Execution Input: 'Unrecognized token 'foo': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (String)"foo"; line: 1, column: 4]'

With this PR:

$ exe/floe examples/workflow.asl 'foo'
Invalid State Machine Execution Input: unexpected token at 'foo': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')

@agrare agrare requested a review from Fryguy as a code owner June 21, 2024 16:50
@agrare agrare force-pushed the ensure_context_input_must_be_valid_json branch 3 times, most recently from 189eacb to 3d900e6 Compare June 21, 2024 16:57
@Fryguy
Copy link
Member

Fryguy commented Jun 21, 2024

I like how it matches the simulator 👍

Comment on lines -13 to +12
input ||= {}
input = JSON.parse(input) if input.kind_of?(String)
input = JSON.parse(input || "{}")
Copy link
Member

Choose a reason for hiding this comment

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

Like how the if input... has been removed.

Would prefer to change the default input: "{}" and drop the input || "{}"

Copy link
Member Author

@agrare agrare Jun 21, 2024

Choose a reason for hiding this comment

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

the input ||= {} was if someone explicitly passed :input => nil it would still default to {} and that still applies

Copy link
Member

Choose a reason for hiding this comment

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

Heh. Yea.

I guess in general I want to keep the data closer to what the user provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the better question is "what does step functions do?", if I do

aws stepfunctions start-execution --input "null"

I get "null" output

{
    "executionArn": "arn:aws:states:us-east-1:123456789012:execution:Succeed:6cae58d1-1aa6-415f-a010-ddc730d6b81f",
    "stateMachineArn": "arn:aws:states:us-east-1:123456789012:stateMachine:Succeed",
    "name": "6cae58d1-1aa6-415f-a010-ddc730d6b81f",
    "status": "SUCCEEDED",
    "startDate": "2024-06-24T14:50:47.849000-04:00",
    "stopDate": "2024-06-24T14:50:47.876000-04:00",
    "input": "null",
    "inputDetails": {
        "included": true
    },
    "output": "null",
    "outputDetails": {
        "included": true
    }
}

So I think actually {} if not provided, but nil if explicitly passed "null" is actually correct.

Copy link
Member

Choose a reason for hiding this comment

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

I think we are in total agreement here.

(I was just squabbling over who converted a nil => {} / "null" => nil, and was thinking it was the cli. But not my hill)

Copy link
Member Author

Choose a reason for hiding this comment

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

(I was just squabbling over who converted a nil => {} / "null" => nil, and was thinking it was the cli. But not my hill)

Hey I'm open to why we shouldn't do it Context, but if we did it in cli then the same logic wouldn't apply to anyone using Context/Workflow as ruby classes right?

Copy link
Member

Choose a reason for hiding this comment

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

Hey I'm open to why we shouldn't do it Context, but if we did it in cli then the same logic wouldn't apply to anyone using Context/Workflow as ruby classes right?

Totally.
I double talk here. I feel that workflow is the public interface, but I want people to pass in a context.

Maybe we need a public interface for creating a context, like we have Workflow#load

Ok, the only thing I care about is when you are creating a Map, do you have a context for each parallel branch? Are you able to handle this conversion?

If so, lets :shipit:

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, the only thing I care about is when you are creating a Map, do you have a context for each parallel branch? Are you able to handle this conversion?

Yes, each ItemProcessor iteration has its own ItemProcessorContext which lives under the top-level Context["State"] for the Map state.

@agrare agrare force-pushed the ensure_context_input_must_be_valid_json branch 3 times, most recently from fa1da69 to be26ebc Compare June 24, 2024 19:33
@Fryguy Fryguy added the enhancement New feature or request label Jun 26, 2024
@agrare agrare force-pushed the ensure_context_input_must_be_valid_json branch from be26ebc to c9d9155 Compare June 28, 2024 15:39
@agrare agrare mentioned this pull request Jul 1, 2024
9 tasks
@agrare agrare force-pushed the ensure_context_input_must_be_valid_json branch 2 times, most recently from 0e48002 to fc09e6c Compare July 3, 2024 15:42
@agrare agrare force-pushed the ensure_context_input_must_be_valid_json branch from fc09e6c to 0698fcf Compare July 8, 2024 16:48
@agrare
Copy link
Member Author

agrare commented Jul 8, 2024

@Fryguy what do you think of this, something that we want to do?

@miq-bot
Copy link
Member

miq-bot commented Jul 8, 2024

Checked commit agrare@0698fcf with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
15 files checked, 0 offenses detected
Everything looks fine. 👍

@Fryguy
Copy link
Member

Fryguy commented Jul 8, 2024

Yes I think this is something we want to do. Actually had a few questions of we can chat about it.

@Fryguy Fryguy merged commit 279fd55 into ManageIQ:master Jul 8, 2024
5 checks passed
@agrare agrare deleted the ensure_context_input_must_be_valid_json branch July 8, 2024 19:23
agrare added a commit that referenced this pull request Aug 1, 2024
Added
- Set Floe.logger.level if DEBUG env var set (#234)
- Add InstrinsicFunction foundation + States.Array, States.UUID (#194)
- Evaluate IntrinsicFunctions from PayloadTemplate (#236)
- Add more intrinsic functions (#242)
- Add State#cause_path, error_path (#249)
- Validate Catcher Next (#250)
- Add spec/supports (#248)

Fixed
- Handle non-hash input/output values (#214)
- Fix Input/Output handling for Pass, Choice, Succeed states (#225)
- Wrap Parslet::ParseFailed errors as Floe::InvalidWorkflowError (#235)
- Fix edge cases with States.Array (#237)
- Fix sporadic test failure with Wait state (#243)
- Fix invalid container names after normalizing (#252)

Changed
- Extract ErrorMatcherMixin from Catch and Retry (#186)
- Output should be JSON (#230)
- Normalize functions to all take arguments (#238)
- Pass full path name to State.new for better errors (#229)
- Validate that state machine input is valid JSON (#227)
- Move the Parslet parse into initialize so that invalid function definition will fail on workflow load (#245)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants