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

Fix Input/Output handling for Pass, Choice, Succeed states #225

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 19, 2024

Introduced by https://github.com/ManageIQ/floe/pull/191/files#diff-04416988853398624310337389715edeb1fed6c963a0a2a41ef882df8166dd6e

We're missing proper input processing for the Pass state if there is no Result attribute:

$ aws stepfunctions --endpoint-url http://localhost:8083 create-state-machine --name Pass --role-arn "arn:aws:iam::012345678901:role/DummyRole" --definition "{\"Comment\": \"Using Pass State\", \"StartAt\": \"Pass\", \"States\": {\"Pass\": {\"Type\": \"Pass\", \"End\": true, \"InputPath\": \"$.colors\"}}}"
$ aws stepfunctions --endpoint-url http://localhost:8083 start-execution --state-machine-arn arn:aws:states:us-east-1:123456789012:stateMachine:Pass --input '{"colors": "red"}'
$ aws stepfunctions --endpoint-url http://localhost:8083 describe-execution --execution-arn arn:aws:states:us-east-1:123456789012:execution:Pass:4747e8dd-c3ab-4124-bb45-1d7337018ddb
{
    "executionArn": "arn:aws:states:us-east-1:123456789012:execution:Pass:4747e8dd-c3ab-4124-bb45-1d7337018ddb",
    "stateMachineArn": "arn:aws:states:us-east-1:123456789012:stateMachine:Pass",
    "name": "4747e8dd-c3ab-4124-bb45-1d7337018ddb",
    "status": "SUCCEEDED",
    "startDate": "2024-06-19T12:59:05.702000-04:00",
    "stopDate": "2024-06-19T12:59:05.706000-04:00",
    "input": "{\"colors\": \"red\"}",
    "inputDetails": {
        "included": true
    },
    "output": "\"red\"",
    "outputDetails": {
        "included": true
    }
}

We should be getting "red" as the output, but currently we return the entire input payload:

Failures:

  1) Floe::Workflow::States::Pass#run_nonblock! Without Results Uses input
     Failure/Error: expect(ctx.output).to eq("red")
     
       expected: "red"
            got: {"color"=>"red"}
     
       (compared using ==)
     
       Diff:
       @@ -1 +1 @@
       -"red"
       +"color" => "red",
       
     # ./spec/workflow/states/pass_spec.rb:101:in `block (4 levels) in <top (required)>'

@agrare agrare added the bug Something isn't working label Jun 19, 2024
@agrare agrare requested a review from Fryguy as a code owner June 19, 2024 17:09
@agrare agrare force-pushed the pass_state_input_output_processing branch from 6913e36 to 7ce3028 Compare June 19, 2024 18:38
@agrare agrare changed the title Add spec for Pass using input without Result Fix Pass input processing without Result Jun 19, 2024
@kbrock
Copy link
Member

kbrock commented Jun 19, 2024

I will put together a test with a combination of result, input path, result path, output path.

see how the values go.

I'm wondering if I screwed up and the input path IS processed for the output

Also noticed that Success has input/output path and we do not handle.

@agrare agrare force-pushed the pass_state_input_output_processing branch from 7ce3028 to 223d24c Compare June 20, 2024 14:31
@agrare agrare changed the title Fix Pass input processing without Result Fix Input/Output handling for Pass, Choice, Succeed states Jun 20, 2024
@agrare agrare force-pushed the pass_state_input_output_processing branch from 4f89a3b to c17b774 Compare June 20, 2024 15:02
@kbrock
Copy link
Member

kbrock commented Jun 20, 2024

Note: Merge #214 before merging this

@agrare agrare force-pushed the pass_state_input_output_processing branch from c17b774 to 2b57551 Compare June 21, 2024 11:07
@agrare agrare force-pushed the pass_state_input_output_processing branch from 2b57551 to 7eecf93 Compare June 21, 2024 13:33
@miq-bot
Copy link
Member

miq-bot commented Jun 21, 2024

Checked commits agrare/floe@f7a0d93~...7eecf93 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
6 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy
Copy link
Member

Fryguy commented Jun 21, 2024

Clarifying question - In the OP I see

"output": "\"red\"",

but then the comment right after said

We should be getting "red" as the output, but currently we return the entire input payload:

I think this is supposed to be that we should be getting \"red\" as the output (with the quotations), as that's valid JSON. This overlaps with my concerns in #222 (comment) as well.


it "Uses InputPath to select color" do
workflow.run_nonblock
expect(ctx.output).to eq("red")
Copy link
Member

@Fryguy Fryguy Jun 21, 2024

Choose a reason for hiding this comment

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

So I think this is wrong? I think this should be

Suggested change
expect(ctx.output).to eq("red")
expect(ctx.output).to eq("\"red\"")

or should it be "red" here, and then if that goes through JSON output, then it's "\"red\""?

I'm confused haha

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think you're right and that matches what the asl sim outputs from the description.

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 this is a slightly different issue than the one I'm trying to fix here though. The core issue for this PR is we were completely ignoring InputPath, the issue you describe is around JSON and workflow/state input/output. Think that is probably better solved in #222

Copy link
Member

@kbrock kbrock Jun 21, 2024

Choose a reason for hiding this comment

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

I feel ctx.output is still in ruby, and therefore when asked, it should display a ruby class.

Now if we are printing it as output in the product (e.g. via CLI), then maybe we want the jsonified values..

soap box:
The transitions from the outside world to the inside world are the only models that do any json parsing. So that would be CLI (converting params to input/...), Task (converting command output to our Result), and provider-workflows (handles ruby <-> db).

Copy link
Member Author

Choose a reason for hiding this comment

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

The states-language spec is pretty clear that input/output to the State Machine and in between States is JSON [ref]

When a state machine is started, the caller can provide an initial JSON text as input, which is passed to the machine's start state as input. If no input is provided, the default is an empty JSON object, {}. As each state is executed, it receives a JSON text as input and can produce arbitrary output, which MUST be a JSON text.

We do need to parse that JSON to handle JsonPath.on for Path#value so maybe we JSON.parse() the input and .to_json the output?

Copy link
Member

Choose a reason for hiding this comment

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

Yeha I was conflicted on whether context.output was in Ruby still or should already be converted to a JSON string.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy I think b7377a8#diff-c4b20f6026d80247d4c5645a2811f66f0bd918ebb4763da5fb0d19c8a7d092fa covers your concern here, still very WIP but throw 🍅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought about it a bit more and I agree with @kbrock here, context is "internal" where workflow.output is defining the "user interface". As long as the states get the right input/output and the workflow handles input/output properly everything is up to us.

I changed #228 to have workflow.output as JSON

@kbrock kbrock merged commit 5480473 into ManageIQ:master Jun 21, 2024
5 checks passed
@agrare agrare deleted the pass_state_input_output_processing branch June 21, 2024 21:10
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
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants