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?

+1 to the question above and also to provide some more details in the description.

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-10-25T20:46:38Z

AWS CodeBuild CI Report

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

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

paulhcsun

Hi @noseworthy, just wondering if you're still available to work on this. Theres just the one pending question + an integ test snapshot that needs to be updated. Thanks!

paulhcsun · 2024-10-28T22:49:34Z

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) }),


+1 to the question above and also to provide some more details in the description.

noseworthy · 2024-10-31T20:57:49Z

Sorry @paulhcsun, I've been swamped and haven't been able to get back to this yet.

paulhcsun · 2024-11-01T18:41:05Z

No worries @noseworthy, just wanted to confirm this PR was not abandoned. Though with our automation this will be closed after 3 weeks due to the Changes Requested status.

aws-cdk-automation · 2024-12-06T00:11:38Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-12-14T00:11:16Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2024-12-14T00:11:36Z

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

Merge branch 'main' into fix-item-batcher-render

ec2c608

paulhcsun self-assigned this Oct 28, 2024

paulhcsun requested changes Oct 28, 2024

View reviewed changes

gracelu0 removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2024

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 14, 2024

aws-cdk-automation closed this Dec 14, 2024

github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2024

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

paulhcsun Oct 28, 2024

aws-cdk-automation commented Oct 25, 2024

paulhcsun left a comment

paulhcsun Oct 28, 2024

noseworthy commented Oct 31, 2024

paulhcsun commented Nov 1, 2024

aws-cdk-automation commented Dec 6, 2024

aws-cdk-automation commented Dec 14, 2024

github-actions bot commented Dec 14, 2024

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

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

paulhcsun Oct 28, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 25, 2024

AWS CodeBuild CI Report

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun Oct 28, 2024

Choose a reason for hiding this comment

noseworthy commented Oct 31, 2024

paulhcsun commented Nov 1, 2024

aws-cdk-automation commented Dec 6, 2024

aws-cdk-automation commented Dec 14, 2024

github-actions bot commented Dec 14, 2024

noseworthy commented Feb 17, 2024 •

edited

Loading