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(custom-resources): does not accept strings where numbers are expected, skips recursive types #27112

Merged
merged 11 commits into from
Sep 14, 2023

Conversation

otaviomacedo
Copy link
Contributor

@otaviomacedo otaviomacedo commented Sep 12, 2023

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.

See aws/aws-sdk-js-v3#5198


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 September 12, 2023 09:16
@github-actions github-actions bot added the p2 label Sep 12, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 12, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 12, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, but this PR is incomplete. Doesn't gzipping have better value than unique prefixing?

The consuming code still needs to be changed to account for the new map format.

And we need to make sure that the JSON file is correctly bundled by esbuild.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 12, 2023
@otaviomacedo
Copy link
Contributor Author

Good stuff, but this PR is incomplete. Doesn't gzipping have better value than unique prefixing?

The consuming code still needs to be changed to account for the new map format.

And we need to make sure that the JSON file is correctly bundled by esbuild.

I was planning to do all this in subsequent PRs, to make it easier for reviewers.

@rix0rrr

This comment was marked as resolved.

@otaviomacedo otaviomacedo marked this pull request as draft September 13, 2023 09:59
@otaviomacedo otaviomacedo marked this pull request as ready for review September 13, 2023 15:51
@rix0rrr rix0rrr changed the title chore(scripts): Add a new type coercion map for numeric types chore(scripts): type coercion includes numeric and recursive types Sep 14, 2023
@rix0rrr rix0rrr changed the title chore(scripts): type coercion includes numeric and recursive types fix(custom-resources): type coercion includes numeric and recursive types Sep 14, 2023
@rix0rrr rix0rrr changed the title fix(custom-resources): type coercion includes numeric and recursive types fix(custom-resources): does not accept strings where numbers are expected, skips recursive types Sep 14, 2023
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 14, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@otaviomacedo otaviomacedo removed the pr/do-not-merge This PR should not be merged at this time. label Sep 14, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2023

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 8e21b2c into main Sep 14, 2023
15 checks passed
@mergify mergify bot deleted the otaviom/number-parameters branch September 14, 2023 13:17
@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2023

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

MrArnoldPalmer pushed a commit that referenced this pull request Sep 14, 2023
…cted, skips recursive types (#27112)

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.


See aws/aws-sdk-js-v3#5198


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mikewrighton pushed a commit that referenced this pull request Sep 14, 2023
…cted, skips recursive types (#27112)

AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected.

This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation.

We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists.


See aws/aws-sdk-js-v3#5198


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this pull request Mar 4, 2024
## Description
The following issue reports an error that occurs when calling an API that takes the `Date` type as a parameter, such as `GetMetricData` API, from a Custom Resource Lambda function, where the parameter is passed as `string` type to the AWS SDK.
#27962
https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/cloudwatch/command/GetMetricDataCommand/#:~:text=Description-,EndTime,-Required

To resolve this error, the `string` type must be properly converted to `Date` type when calling the AWS SDK from Lambda.
In this PR, I added the conversion to Date type in the same way as the existing conversion to `number` and `Uint8Array` types.
`Uint8Array`: #27034
`number`: #27112

## Major changes
### `update-sdkv3-parameters-model.ts` script
If the type is `timestamp` in the `smithy` specification, write `d` to the state machine so that it can be converted to a Date type later.
https://smithy.io/2.0/spec/simple-types.html#timestamp

`update-sdkv3-parameters-model.sh` script was not called from anywhere, so I called it manually and updated the JSON file.
Please let me know if there is a problem.

### `sdk-v2-to-v3-adapter` module
I added code to convert value marked `d` in state machine to `Date` type.
If the conversion to `Date` type fails, the `Date` class does not throw an exception, so the error is handled in a slightly tricky way.
Also added a unit test for this process.

### `integ-tests-alpha` module
Added integ-test to verify that errors reported in the related issue have been resolved.
The IAM Policy added internally by the call to `adPolicyStatementFromSdkCall` looks like the following and does not call `GetMetricData` correctly, so the `addToRolePolicy` method was used to explicitly add a new Policy is added explicitly with the `addToRolePolicy` method.
```json
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "monitoring:GetMetricData"
            ],
            "Resource": [
                "*"
            ],
            "Effect": "Allow"
        }
    ]
}
```
https://github.com/aws/aws-cdk/blob/1a9c30e55e58203bd0a61de82711cf10f1e04851/packages/aws-cdk-lib/custom-resources/lib/helpers-internal/sdk-v3-metadata.json#L174


fixes #27962

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants