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(stepfunctions): cannot use intrinsic functions in Fail state #30210

Merged
merged 7 commits into from
May 30, 2024

Conversation

sakurai-ryo
Copy link
Contributor

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

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


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team May 15, 2024 14:00
@github-actions github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels May 15, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@sakurai-ryo
Copy link
Contributor Author

Exemption Request
I think that unit tests cover the scope of this change.

Copy link
Contributor

@gracelu0 gracelu0 left a 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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@sakurai-ryo
Copy link
Contributor Author

Thanks for your review @gracelu0 !
I added validations and tests to verify.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 60c1bba
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@gracelu0 gracelu0 added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label May 30, 2024
Copy link
Contributor

@gracelu0 gracelu0 left a 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 :)

@aws-cdk-automation aws-cdk-automation dismissed their stale review May 30, 2024 17:39

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Contributor

mergify bot commented May 30, 2024

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).

@mergify mergify bot merged commit 81a558f into aws:main May 30, 2024
14 of 15 checks passed
@kimyx
Copy link

kimyx commented Jun 2, 2024

Thanks for your work on this, @sakurai-ryo! Looking forward to giving it a try on my application.

@kimyx
Copy link

kimyx commented Jun 2, 2024

I tried removing my workaround and deploying with cdk version 2.144.0 (build 5fb15bc) and see the same error as before.

RuntimeError: Expected JSON path to start with '$', got: States.Format('l1a-step-function FAILED: {}',$.Cause.StatusReason)

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.

@kimyx
Copy link

kimyx commented Jun 2, 2024

in case this is helpful:

  Error: Expected JSON path to start with '$', got: States.Format('l1a-step-function FAILED: {}',$.Cause.StatusReason)
      at renderJsonPath (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-2dUphk/node_modules/aws-cdk-lib/aws-stepfunctions/lib/states/state.js:1:9755)
      at Fail.toStateJson (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-2dUphk/node_modules/aws-cdk-lib/aws-stepfunctions/lib/states/fail.js:1:978)
      at StateGraph.toGraphJson (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-2dUphk/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-graph.js:1:2159)
      at ChainDefinitionBody.bind (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-2dUphk/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-machine.js:1:12008)
      at new StateMachine (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-2dUphk/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-machine.js:1:6368)
      at Kernel._Kernel_create (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmp0zm93wtb/lib/program.js:10108:25)
      at Kernel.create (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmp0zm93wtb/lib/program.js:9779:93)
      at KernelHost.processRequest (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmp0zm93wtb/lib/program.js:11696:36)
      at KernelHost.run (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmp0zm93wtb/lib/program.js:11656:22)
      at Immediate._onImmediate (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmp0zm93wtb/lib/program.js:11657:46)

@sakurai-ryo
Copy link
Contributor Author

Thanks @kimyx.
I was able to deploy the following code correctly on my laptop with v2.144.0 using TypeScript.

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?

atanaspam pushed a commit to atanaspam/aws-cdk that referenced this pull request Jun 3, 2024
…#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*
@kimyx
Copy link

kimyx commented Jun 3, 2024

The code I provided with my original ticket still shows the error, as well as my full application. Here's the small test case.

# adapted from https://github.com/aws-samples/aws-cdk-examples/blob/main/python/stepfunctions/stepfunctions/stepfunctions_stack.py
# used to report cdk issue https://github.com/aws/aws-cdk/issues/30063

from aws_cdk import (
    aws_stepfunctions as _aws_stepfunctions,
    App, Duration, Stack
)

class JobPollerStack(Stack):
    def __init__(self, app: App, id: str, **kwargs) -> None:
        super().__init__(app, id, **kwargs)

        # Step functions Definition

        submit_job = _aws_stepfunctions.Wait(
            self, "Submit Job",
            time=_aws_stepfunctions.WaitTime.duration(
                Duration.seconds(30))
        )

        wait_job = _aws_stepfunctions.Wait(
            self, "Wait 30 Seconds",
            time=_aws_stepfunctions.WaitTime.duration(
                Duration.seconds(30))
        )

        status_job = _aws_stepfunctions.Wait(
            self, "Get Status",
            time=_aws_stepfunctions.WaitTime.duration(
                Duration.seconds(30))
        )

        error_path = "$.Cause.Attempts[0].StatusReason"
        cause_path = "States.Format('LogStreamName: {}', $.Cause.Container.LogStreamName)"
        fail_job = _aws_stepfunctions.Fail(
            self, "Fail",
            error_path=error_path,  # works
            cause_path=cause_path  # fails
        )

        succeed_job = _aws_stepfunctions.Succeed(
            self, "Succeeded",
            comment='AWS Batch Job succeeded'
        )

        # Create Chain

        chain = submit_job.next(wait_job) \
            .next(status_job) \
            .next(_aws_stepfunctions.Choice(self, 'Job Complete?')
                  .when(_aws_stepfunctions.Condition.string_equals('$.status', 'FAILED'), fail_job)
                  .when(_aws_stepfunctions.Condition.string_equals('$.status', 'SUCCEEDED'), succeed_job)
                  .otherwise(wait_job))

        # Create state machine
        sm = _aws_stepfunctions.StateMachine(
            self, "StateMachine",
            definition_body=_aws_stepfunctions.DefinitionBody.from_chainable(chain),
            timeout=Duration.minutes(5),
        )

Here is some console output. I am not a typescript or node expert, but I think I'm actually using cdk 2.144.0.

csds/src reorder_algs_convert_microradians +2 -1 $! venv_311 default ❯ cdk --version
2.144.0 (build 5fb15bc)
csds/src reorder_algs_convert_microradians +2 -1 $! venv_311 default ❯ npm list -g --depth=0 | grep cdk
├── aws-cdk@2.144.0
csds/src reorder_algs_convert_microradians +2 -1 $! venv_311 default ❯ npx cdk --version
2.144.0 (build 5fb15bc)
csds/src reorder_algs_convert_microradians +2 -1 $! venv_311 default 1 ❯ cdk deploy --context hosc_ip_addr=111.222.333.44 --exclusively JobPollerStack
jsii.errors.JavaScriptError:
  Error: Expected JSON path to start with '$', got: States.Format('LogStreamName: {}', $.Cause.Container.LogStreamName)
      at renderJsonPath (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-zoVzKr/node_modules/aws-cdk-lib/aws-stepfunctions/lib/states/state.js:1:9755)
      at Fail.toStateJson (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-zoVzKr/node_modules/aws-cdk-lib/aws-stepfunctions/lib/states/fail.js:1:1050)
      at StateGraph.toGraphJson (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-zoVzKr/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-graph.js:1:2159)
      at ChainDefinitionBody.bind (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-zoVzKr/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-machine.js:1:12008)
      at new StateMachine (/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/jsii-kernel-zoVzKr/node_modules/aws-cdk-lib/aws-stepfunctions/lib/state-machine.js:1:6368)
      at Kernel._Kernel_create (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmpvt3ulni4/lib/program.js:10108:25)
      at Kernel.create (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmpvt3ulni4/lib/program.js:9779:93)
      at KernelHost.processRequest (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmpvt3ulni4/lib/program.js:11696:36)
      at KernelHost.run (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmpvt3ulni4/lib/program.js:11656:22)
      at Immediate._onImmediate (/private/var/folders/_b/2b2x6djs2q77mc8d8pt49r0m007nfk/T/tmpvt3ulni4/lib/program.js:11657:46)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/kiko1739/code/csds/src/csds_app.py", line 562, in <module>
    job_poller_stack = JobPollerStack(app, 'BadStack')
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/src/stacks/step_bad_stack.py", line 56, in __init__
    sm = _aws_stepfunctions.StateMachine(
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/aws_cdk/aws_stepfunctions/__init__.py", line 10097, in __init__
    jsii.create(self.__class__, self, [scope, id, props])
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/jsii/_kernel/__init__.py", line 334, in create
    response = self.provider.create(
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/jsii/_kernel/providers/process.py", line 365, in create
    return self._process.send(request, CreateResponse)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kiko1739/code/csds/venv_311/lib/python3.11/site-packages/jsii/_kernel/providers/process.py", line 342, in send
    raise RuntimeError(resp.error) from JavaScriptError(resp.stack)
RuntimeError: Expected JSON path to start with '$', got: States.Format('LogStreamName: {}', $.Cause.Container.LogStreamName)

@sakurai-ryo
Copy link
Contributor Author

@kimyx
Could you please make sure that you are also using the latest version of the aws-cdk-lib listed in requirements.txt file?
https://pypi.org/project/aws-cdk-lib/

@kimyx
Copy link

kimyx commented Jun 3, 2024

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. 😃

@kimyx
Copy link

kimyx commented Jun 3, 2024

It works without the extra Pass state I added as a workaround! Thanks again.

vdahlberg pushed a commit to vdahlberg/aws-cdk that referenced this pull request Jun 10, 2024
…#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*
@aws-cdk-automation
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

step function: Fail state cause_path generates error using States.Format
4 participants