fix(stepfunctions): item batcher doesn't properly render json paths #29152

noseworthy · 2024-02-17T21:17:17Z

Reason for this change

The ItemBatcher property of DistributedMap supports json path fields, but the new DistributedMap construct doesn't call
FieldUtils.renderObject when rendering the ItemBatcher.

Description of changes

Make sure to call FieldUtils.renderObject() when rendering this property.

Description of how you validated changes

Added a unit test to ensure that the proper cloudformation step function definition is generated. This ensures that the definition includes adding '.$' to the end of keys that contain json path values.
Added an integration test that creates a distributed map based step function that uses the item batcher and includes both a constant value and json path field value in the batch input. After successful execution of the state machine, a lambda checks the that the input to the state machine includes both the constant value and the resolved json path value.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

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.

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

lpizzinidev

Looks good to me 👍
Can you please update the title to describe the bug (not the solution)?

noseworthy · 2024-03-30T15:19:36Z

Looks good to me 👍 Can you please update the title to describe the bug (not the solution)?

Done! Thanks for the review!

noseworthy · 2024-03-31T16:30:04Z

Rebased on main and amended the commit message.

godwingrs22 · 2024-04-18T23:45:01Z

@noseworthy Thank you for this contribution. But I would like to get more context on the issue we are trying to solve.

Could you provide more detail on the issue please?
Do we have any open issue ticket that can be linked to this PR?

godwingrs22 · 2024-04-18T23:48:01Z

packages/aws-cdk-lib/aws-stepfunctions/test/distributed-map.test.ts

@@ -631,6 +634,9 @@ describe('Distributed Map State', () => {
          ToleratedFailurePercentagePath: '$.toleratedFailurePercentage',
          ToleratedFailureCountPath: '$.toleratedFailureCount',
          ItemBatcher: {
+            BatchInput: {
+              'TestBatchInput.$': '$.testBatchInput',


Could you provide detail on why we need to expect '.$' to the key?

👍 to this, a brief comment here explaining why we need the .$ and what it signifies (likely with a link to the relevant docs) would be very helpful

Can do! It might be a little while for a proper response. I'm away for the next week, but will try to get to it when I get back.

The long and short of it is that stepfunctions will parse keys that end in .$ as json paths and will look up the value from previous states using the value of this key rather than just passing the value to the next state.

This is done in almost all other state configuration so that values can be dynamically included from previous states, but this functionality was missed here requiring the user to manually specify '.$'. This is confusing and it isn't clear that the json paths aren't automatically rendered by the cdk construct parsing.

See documentation here: https://docs.aws.amazon.com/step-functions/latest/dg/connect-parameters.html#connect-parameters-path

And here: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_stepfunctions.JsonPath.html

godwingrs22 · 2024-04-18T23:52:44Z

packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map/item-batcher.ts

@@ -71,7 +73,7 @@ export class ItemBatcher {
      ...(this.props.maxItemsPerBatchPath && { MaxItemsPerBatchPath: this.props.maxItemsPerBatchPath }),
      ...(this.props.maxInputBytesPerBatch && { MaxInputBytesPerBatch: this.props.maxInputBytesPerBatch }),
      ...(this.props.maxInputBytesPerBatchPath && { MaxInputBytesPerBatchPath: this.props.maxInputBytesPerBatchPath }),
-      ...(this.props.batchInput && { BatchInput: this.props.batchInput }),
+      ...(this.props.batchInput && { BatchInput: FieldUtils.renderObject(this.props.batchInput) }),


I would suggest to update the PR description with detail specifically for batchInput property in the itemBatcher class. To me from the description its not clear specifcally to which property we are modifying.

I would like to understand more on why do we need to render only this specifc batchInput property since the existing behavious itself will be able to pass Json payload to the batchInput property?

The ItemBatcher property of DistributedMap supports json path fields, but the new DistributedMap construct doesn't call FieldUtils.renderObject when rendering the ItemBatcher. Make sure to call `FieldUtils.renderObject()` when rendering this property. Added an integration test to ensure that the json path field resolves to the actual value from the state in the input by checking the output from the results writer in S3.

aws-cdk-automation · 2024-09-03T13:43:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 27fff5d
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation requested a review from a team February 17, 2024 21:17

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Feb 17, 2024

aws-cdk-automation previously requested changes Feb 17, 2024

View reviewed changes

noseworthy force-pushed the fix-item-batcher-render branch from e96a4cd to 85d4433 Compare February 18, 2024 20:21

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 18, 2024

github-actions bot mentioned this pull request Mar 1, 2024

Monthly PR metrics report - Feb 2023 #29328

Closed

lpizzinidev approved these changes Mar 30, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 30, 2024

noseworthy changed the title ~~fix(stepfunctions): support json path fields for item batcher~~ fix(stepfunctions): item batcher doesn't properly render json paths Mar 30, 2024

noseworthy force-pushed the fix-item-batcher-render branch from e951272 to 03ec32f Compare March 31, 2024 16:29

godwingrs22 self-requested a review April 17, 2024 17:46

godwingrs22 self-assigned this Apr 17, 2024

godwingrs22 reviewed Apr 19, 2024

View reviewed changes

noseworthy force-pushed the fix-item-batcher-render branch 2 times, most recently from 1b75865 to 5954dd8 Compare June 13, 2024 17:26

noseworthy force-pushed the fix-item-batcher-render branch from 5954dd8 to 2b99080 Compare July 4, 2024 13:14

noseworthy force-pushed the fix-item-batcher-render branch from 2b99080 to 27fff5d Compare September 3, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions): item batcher doesn't properly render json paths #29152

fix(stepfunctions): item batcher doesn't properly render json paths #29152

noseworthy commented Feb 17, 2024 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev left a comment

noseworthy commented Mar 30, 2024

noseworthy commented Mar 31, 2024

godwingrs22 commented Apr 18, 2024

godwingrs22 Apr 18, 2024

comcalvi Apr 23, 2024

noseworthy Apr 24, 2024

godwingrs22 Apr 18, 2024

godwingrs22 Apr 19, 2024

aws-cdk-automation commented Sep 3, 2024

fix(stepfunctions): item batcher doesn't properly render json paths #29152

Are you sure you want to change the base?

fix(stepfunctions): item batcher doesn't properly render json paths #29152

Conversation

noseworthy commented Feb 17, 2024 • edited Loading

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

noseworthy commented Mar 30, 2024

noseworthy commented Mar 31, 2024

godwingrs22 commented Apr 18, 2024

godwingrs22 Apr 18, 2024

Choose a reason for hiding this comment

comcalvi Apr 23, 2024

Choose a reason for hiding this comment

noseworthy Apr 24, 2024

Choose a reason for hiding this comment

godwingrs22 Apr 18, 2024

Choose a reason for hiding this comment

godwingrs22 Apr 19, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 3, 2024

AWS CodeBuild CI Report

noseworthy commented Feb 17, 2024 •

edited

Loading