-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(stepfunctions): cannot use intrinsic functions in Fail state #30210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sakurai-ryo , thank you for working on this! The added validation is great. I left one comment about adding an unhappy path test case if you could address that, otherwise lgtm!
} | ||
|
||
private isIntrinsicString(jsonPath?: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for the supported intrinsic functions here? Currently passing in any string that doesn't start with '$' would still pass, correct? I'd like to see a test case where the value is a plain string (neither an intrinsic function nor Json path) to understand the expected behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct.
I added the validation along with the test because validation is possible for strings.
However, the validation checks only the start of the string.
This is because a parser is needed if we also want to check the syntax of intrinsics.
Thanks for your review @gracelu0 ! |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback promptly! We appreciate your contribution :)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thanks for your work on this, @sakurai-ryo! Looking forward to giving it a try on my application. |
I tried removing my workaround and deploying with cdk version
From what I can see of the build pipeline, 2.144.0 should have the relevant changes. Do you agree? If so, I'll dig deeper. |
in case this is helpful:
|
Thanks @kimyx. import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as sfn from 'aws-cdk-lib/aws-stepfunctions';
export class StepFnStack extends cdk.Stack {
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);
const fail = new sfn.Fail(this, 'Fail', {
errorPath: "States.Format('l1a-step-function FAILED: {}',$.Cause.StatusReason)",
causePath: "States.Format('Hello, my name is {}.', $.name)",
});
new sfn.StateMachine(this, 'StateMachine', {
definitionBody: sfn.DefinitionBody.fromChainable(fail),
});
}
} {
"name": "cdk-test",
"version": "0.1.0",
"bin": {
"kms-import": "bin/cdk-test.js"
},
"scripts": {
"build": "tsc",
"watch": "tsc -w",
"test": "jest",
"cdk": "cdk"
},
"devDependencies": {
"@types/jest": "^29.5.12",
"@types/node": "20.12.7",
"jest": "^29.7.0",
"ts-jest": "^29.1.2",
"aws-cdk": "2.143.0",
"ts-node": "^10.9.2",
"typescript": "~5.4.5"
},
"dependencies": {
"aws-cdk-lib": "2.144.0",
"constructs": "^10.0.0",
"source-map-support": "^0.5.21"
}
} Cloud you please show me with the entire code that can reproduce this error? |
…#30210) ### Issue # (if applicable) Closes aws#30063 ### Reason for this change In the Fail state, we can specify intrinsic functions and json paths as the CausePath and ErrorPath properties. Currently, however, specifying intrinsic functions as a string will result in an error. https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html ```ts export class SampleStack extends cdk.Stack { constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props); const fail = new stepfunctions.Fail(this, "Fail", { errorPath: "$.error", // OK causePath: "States.Format('cause: {}', $.cause)", // Error }); const sm = new stepfunctions.StateMachine(this, "StateMachine", { definitionBody: stepfunctions.DefinitionBody.fromChainable(fail), timeout: cdk.Duration.minutes(5) }); } } ``` ``` Error: Expected JSON path to start with '$', got: States.Format('cause: {}', $.cause) ``` ### Description of changes The value passed to the `renderJsonPath` function is expected to be a string starting with `$` if it is not a token. However, if you pass intrinsic functions as strings to the CausePath and ErrorPath properties, they will never start with `$`. Therefore, I fixed not to call the `renderJsonPath` function if the intrinsic functions are specified as strings. Another change was the addition of validation since error and errorPath, cause and causePath cannot be specified simultaneously. ### Description of how you validated changes I added unit tests to verify that passing intrinsic functions as strings do not cause an error. Tests were also added to verify that errors occur when errors and paths are specified at the same time and when cause and cause paths are specified at the same time. https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Cause https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Error ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The code I provided with my original ticket still shows the error, as well as my full application. Here's the small test case.
Here is some console output. I am not a typescript or node expert, but I think I'm actually using cdk 2.144.0.
|
@kimyx |
Ahh, I apologize. I also needed to upgrade the aws-cdk-lib python package in my virtual environment. With that, it is deploying without error. I'll soon find out if it runs correctly in my full application. 😃 |
It works without the extra Pass state I added as a workaround! Thanks again. |
…#30210) ### Issue # (if applicable) Closes aws#30063 ### Reason for this change In the Fail state, we can specify intrinsic functions and json paths as the CausePath and ErrorPath properties. Currently, however, specifying intrinsic functions as a string will result in an error. https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html ```ts export class SampleStack extends cdk.Stack { constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props); const fail = new stepfunctions.Fail(this, "Fail", { errorPath: "$.error", // OK causePath: "States.Format('cause: {}', $.cause)", // Error }); const sm = new stepfunctions.StateMachine(this, "StateMachine", { definitionBody: stepfunctions.DefinitionBody.fromChainable(fail), timeout: cdk.Duration.minutes(5) }); } } ``` ``` Error: Expected JSON path to start with '$', got: States.Format('cause: {}', $.cause) ``` ### Description of changes The value passed to the `renderJsonPath` function is expected to be a string starting with `$` if it is not a token. However, if you pass intrinsic functions as strings to the CausePath and ErrorPath properties, they will never start with `$`. Therefore, I fixed not to call the `renderJsonPath` function if the intrinsic functions are specified as strings. Another change was the addition of validation since error and errorPath, cause and causePath cannot be specified simultaneously. ### Description of how you validated changes I added unit tests to verify that passing intrinsic functions as strings do not cause an error. Tests were also added to verify that errors occur when errors and paths are specified at the same time and when cause and cause paths are specified at the same time. https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Cause https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Error ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Closes #30063
Reason for this change
In the Fail state, we can specify intrinsic functions and json paths as the CausePath and ErrorPath properties.
Currently, however, specifying intrinsic functions as a string will result in an error.
https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html
Description of changes
The value passed to the
renderJsonPath
function is expected to be a string starting with$
if it is not a token.However, if you pass intrinsic functions as strings to the CausePath and ErrorPath properties, they will never start with
$
.Therefore, I fixed not to call the
renderJsonPath
function if the intrinsic functions are specified as strings.Another change was the addition of validation since error and errorPath, cause and causePath cannot be specified simultaneously.
Description of how you validated changes
I added unit tests to verify that passing intrinsic functions as strings do not cause an error.
Tests were also added to verify that errors occur when errors and paths are specified at the same time and when cause and cause paths are specified at the same time.
https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Cause
https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-fail-state.html#:~:text=%2C%20and%20States.UUID.-,Important,-You%20can%20specify%20either%20Error
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license