fix(stepfunctions-tasks): batch properties do not accept JsonPaths by peterwoodworth · Pull Request #19550 · aws/aws-cdk

peterwoodworth · 2022-03-24T21:16:58Z

fixes #19546

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use cdk-integ to deploy the infrastructure and generate the snapshot (i.e. cdk-integ without --dry-run)?

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

gitpod-io · 2022-03-24T21:17:05Z

kaizencc · 2022-03-25T18:48:17Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/batch/submit-job.ts

 }
+
+function tokenOrString(value: any) {
+  return Token.isUnresolved(value) ? value : value.toString();


@rix0rrr, I'm a bit confused about the necessity for something like this. Originally we had

Value: `${maybeToken}`

But this does not work when maybeToken is a token. Not sure why this is the case. I looked for prior art for something like this and couldn't find anything. The typical Lazy.number({ produce: () => {}}) isn't applicable because the token is a JsonPath (and there's some fancy footwork that goes on behind the scenes for JsonPaths).

kaizencc · 2022-03-25T19:03:55Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/batch/submit-job.ts

        {
          Type: 'MEMORY',
-          Value: `${containerOverrides.memory.toMebibytes()}`,
+          Value: tokenOrString(containerOverrides.memory.toMebibytes()),


@peterwoodworth I changed this up rather than review because I knew something was off but couldn't put my finger on it until I changed things around. Previously, you had:

Value: Token.isUnresolved(containerOverrides.memory) ? containerOverrides.memory: `${containerOverrides.memory.toMebibytes()}`

This failed when I tested whether the memory property supported tokenized values, which was omitted before.

My thoughts on why this is the case: From the isUnresolved docs:

/** * Returns true if obj represents an unresolved value * * One of these must be true: * * - `obj` is an IResolvable * - `obj` is a string containing at least one encoded `IResolvable` * - `obj` is either an encoded number or list * * This does NOT recurse into lists or objects to see if they * containing resolvables. * * @param obj The object to test. */

Since a user would submit something like cdk.Size(sfn.JsonPath.numberAt('$.whatever')), Token.isUnresolved(containerOverrides.memory) would check for the resolvability of the Size object, and not the properties of the Size object. Instead, we want to see inside cdk.Size, which is why we want to check for: Token.isUnresolved(containerOverrides.memory.toMebibytes()). Hope that makes sense. Maybe @rix0rrr can chime in if what I say is wrong or not the full story.

Related is #19569, which adds the isUnresolved to Size.

Related is also #19578 which fixes this whole class of errors once and for all

kaizencc · 2022-03-25T19:30:09Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/batch/submit-job.ts

    });

    // validate timeout
-    props.timeout !== undefined && withResolved(props.timeout.toSeconds(), (timeout) => {


I notice that we're doing something problematic here as well. props.timeout.toSeconds() is not intended to succeed with Tokens:

function convert(amount: number, fromUnit: TimeUnit, toUnit: TimeUnit, { integral = true }: TimeConversionOptions) { if (fromUnit.inMillis === toUnit.inMillis) { return amount; } if (Token.isUnresolved(amount)) { throw new Error(`Unable to perform time unit conversion on un-resolved token ${amount}.`); } ... }

However it will succeed if the user specified their tokenized timeout in seconds as well. So

timeout: Duration.seconds(JsonPath.numberAt('$.whatever'));

has no error while

timeout: Duration.minutes(JsonPath.numberAt('$.whatever'));

will fail with an unintelligible error message.

It makes more sense to use the duration.isUnresolved() function and only assert on non-tokens.

Now, I understand that timeout must be in seconds so sending Duration.minutes(..) is wrong. However, I think the error message 'unable to perform time unit conversion on un-resolved token' does not help the user understand what the error is in this case.

We can change the error message to something like:

Duration must be specified in 'seconds()' here if its value comes from a token

aws-cdk-automation · 2022-03-25T19:32:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 7437946
Result: FAILED
Build Logs (available for 30 days)

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

Number tokens are encoded as a range of very large negative numbers (for example: -1.888154589709072e+289). When these are naively stringified, the `resolve()` method doesn't recognize and translate them anymore, and these numbers end up in the target template in a confusing way. However, recognizing them is actually not that hard and can be done using a regex. We can then do the token resolution appropriately, making it so that construct authors do not have to call `Tokenization.stringifyNumber()` anymore in order to support stringification of number values. Fixes #19546, closes #19550. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/1.152.0/CHANGELOG.md) For convenience, extracted the relevant CHANGELOG entry: ## [1.152.0](v1.151.0...v1.152.0) (2022-04-06) ### Features * **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9)) * **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64)) * **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667) * **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209)) * **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e)) * **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5)) * **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e)) * add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010)) * **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df)) * **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605) * **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076) * **synthetics:** new puppeteer 3.5 runtime ([#19673](#19673)) ([ce2b91b](ce2b91b)), closes [#19634](#19634) ### Bug Fixes * **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969) * **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160) * **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421) * **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2)) * **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042)) * **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399) * **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad)) * **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550) * **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3)) * **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648) * **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259) * **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751) * **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)

See [CHANGELOG](https://github.com/aws/aws-cdk/blob/bump/2.20.0/CHANGELOG.md) For convenience, extracted the relevant CHANGELOG entry: ## [2.20.0](v2.19.0...v2.20.0) (2022-04-07) ### Features * **cfnspec:** cloudformation spec v63.0.0 ([#19679](#19679)) ([dba96a9](dba96a9)) * **cfnspec:** cloudformation spec v65.0.0 ([#19745](#19745)) ([796fc64](796fc64)) * **cli:** add --build option ([#19663](#19663)) ([eb9b8e2](eb9b8e2)), closes [#19667](#19667) * **cli:** preview of `cdk import` ([#17666](#17666)) ([4f12209](4f12209)) * **core:** throw error when stack name exceeds max length ([#19725](#19725)) ([1ffd45e](1ffd45e)) * **eks:** add k8s v1.22 ([#19756](#19756)) ([9a518c5](9a518c5)) * **opensearch:** Add latest Opensearch Version 1.2 ([#19749](#19749)) ([a2ac36e](a2ac36e)) * add new integration test runner ([#19754](#19754)) ([1b4d010](1b4d010)) * **eks:** alb-controller v2.4.1 ([#19653](#19653)) ([1ec08df](1ec08df)) * **lambda:** add support for ephemeral storage ([#19552](#19552)) ([f1d9b6a](f1d9b6a)), closes [#19605](#19605) * **s3:** EventBridge bucket notifications ([#18614](#18614)) ([d8e602b](d8e602b)), closes [#18076](#18076) ### Bug Fixes * **aws_applicationautoscaling:** Add missing members to PredefinedMetric enum ([#18978](#18978)) ([75a6fa7](75a6fa7)), closes [#18969](#18969) * **cli:** apps with many resources scroll resource output offscreen ([#19742](#19742)) ([053d22c](053d22c)), closes [#19160](#19160) * **cli:** support attributes of DynamoDB Tables for hotswapping ([#19620](#19620)) ([2321ece](2321ece)), closes [#19421](#19421) * **cloudwatch:** automatic metric math label cannot be suppressed ([#17639](#17639)) ([7fa3bf2](7fa3bf2)) * **codedeploy:** add name validation for Application, Deployment Group and Deployment Configuration ([#19473](#19473)) ([9185042](9185042)) * **codedeploy:** the Service Principal is wrong in isolated regions ([#19729](#19729)) ([7e9a43d](7e9a43d)), closes [#19399](#19399) * **core:** `Fn.select` incorrectly short-circuits complex expressions ([#19680](#19680)) ([7f26fad](7f26fad)) * **core:** detect and resolve stringified number tokens ([#19578](#19578)) ([7d9ab2a](7d9ab2a)), closes [#19546](#19546) [#19550](#19550) * **core:** reduce CFN template indent size to save bytes ([#19656](#19656)) ([fd63ca3](fd63ca3)) * **ecs:** 'desiredCount' and 'ephemeralStorageGiB' cannot be tokens ([#19453](#19453)) ([c852239](c852239)), closes [#16648](#16648) * **ecs:** remove unnecessary error when adding volume to external task definition ([#19774](#19774)) ([5446ded](5446ded)), closes [#19259](#19259) * **iam:** policies aren't minimized as far as possible ([#19764](#19764)) ([876ed8a](876ed8a)), closes [#19751](#19751) * **logs:** Faulty Resource Policy Generated ([#19640](#19640)) ([1fdf122](1fdf122)), closes [#17544](#17544)

Number tokens are encoded as a range of very large negative numbers (for example: -1.888154589709072e+289). When these are naively stringified, the `resolve()` method doesn't recognize and translate them anymore, and these numbers end up in the target template in a confusing way. However, recognizing them is actually not that hard and can be done using a regex. We can then do the token resolution appropriately, making it so that construct authors do not have to call `Tokenization.stringifyNumber()` anymore in order to support stringification of number values. Fixes aws#19546, closes aws#19550. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(stepfunctions-tasks): batch properties accept JsonPaths

5690ffd

github-actions bot added the @aws-cdk/aws-stepfunctions-tasks label Mar 24, 2022

github-actions bot assigned kaizencc Mar 24, 2022

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Mar 24, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 24, 2022

jstag711 approved these changes Mar 24, 2022

View reviewed changes

peterwoodworth changed the title ~~fix(stepfunctions-tasks): batch properties accept JsonPaths~~ fix(stepfunctions-tasks): batch properties do not accept JsonPaths Mar 25, 2022

fix memory token issues

d075b81

kaizencc reviewed Mar 25, 2022

View reviewed changes

fix timeout

7437946

kaizencc reviewed Mar 25, 2022

View reviewed changes

kaizencc requested a review from rix0rrr March 25, 2022 19:30

peterwoodworth mentioned this pull request Mar 28, 2022

fix(core): detect and resolve stringified number tokens #19578

Merged

4 tasks

mergify bot closed this in #19578 Apr 1, 2022

This was referenced Apr 7, 2022

chore(release): 1.152.0 #19781

Merged

chore(release): 2.20.0 #19801

Merged

TheRealAmazonKendra deleted the peterwoodworth/stepfunctionsBatchFix branch February 14, 2023 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions-tasks): batch properties do not accept JsonPaths#19550

fix(stepfunctions-tasks): batch properties do not accept JsonPaths#19550
peterwoodworth wants to merge 3 commits intomasterfrom
peterwoodworth/stepfunctionsBatchFix

peterwoodworth commented Mar 24, 2022 •

edited

Loading

Uh oh!

gitpod-io bot commented Mar 24, 2022

Uh oh!

kaizencc Mar 25, 2022

Uh oh!

kaizencc Mar 25, 2022

Uh oh!

kaizencc Mar 25, 2022

Uh oh!

rix0rrr Mar 29, 2022

Uh oh!

kaizencc Mar 25, 2022

Uh oh!

rix0rrr Mar 29, 2022

Uh oh!

aws-cdk-automation commented Mar 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

peterwoodworth commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Mar 24, 2022

Uh oh!

kaizencc Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

kaizencc Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

kaizencc Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

rix0rrr Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

kaizencc Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

rix0rrr Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Mar 25, 2022

AWS CodeBuild CI Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

peterwoodworth commented Mar 24, 2022 •

edited

Loading