fix(integ-tests): use `transformToString` on API call response #27122

colifran · 2023-09-13T02:00:10Z

This PR fixes an issue where our assertions handler will fail when logging the response of an API call if the response body is a Blob. Specifically, the following error is thrown:

Converting circular structure to JSON
    --> starting at object with constructor 'TLSSocket'
    |     property 'parser' -> object with constructor 'HTTPParser'
    --- property 'socket' closes the circle (RequestId: 50c1b6cd-47d2-494f-baf1-d22646cd4e5f)

The fix for this issue was to call the transformToString method on the response body. This is mentioned in aws-sdk-js-v3 UPGRADE.md as the solution for Lambda::Invoke. Its unclear why S3::GetObject isn't mentioned as well, but it works for that too.

However, instead of explicitly extracting the Body property from a response, we now recursively traverse the response and detect any such blob types that should be converted to strings - this should protect us against any other APIs that may exhibit this behavior.

transformToString is implemented as part of the SdkStreamMixin interface.

Included in this PR is a fix for aws-s3-deployment/test/integ.bucket-deployment-substitution.ts which was previously failing with the error shown above.

Co-authored by: @iliapolo
Co-authored by: @vinayak-kukreja
Co-authored by: @scanlonp

Closes #27114

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

Signed-off-by: Francis <[email protected]>

packages/@aws-cdk/integ-tests-alpha/lib/assertions/providers/lambda-handler/sdk.ts

iliapolo · 2023-09-13T07:54:19Z

For anyone looking at this, i'll take over the PR now and address my own comments :)

...-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-substitution.ts

mrgrain

Can you add unit tests for coerceResponse?

aws-cdk-automation · 2023-09-13T15:08:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 498ddeb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-09-13T15:38:59Z

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

@iliapolo

…7122) This PR fixes an issue where our assertions handler will fail when logging the response of an API call if the response body is a Blob. Specifically, the following error is thrown: ``` Converting circular structure to JSON --> starting at object with constructor 'TLSSocket' | property 'parser' -> object with constructor 'HTTPParser' --- property 'socket' closes the circle (RequestId: 50c1b6cd-47d2-494f-baf1-d22646cd4e5f) ``` The fix for this issue was to call the `transformToString` method on the response body. This is mentioned in aws-sdk-js-v3 [`UPGRADE.md`](https://github.com/aws/aws-sdk-js-v3/blob/main/UPGRADING.md?plain=1#L573-L576) as the solution for `Lambda::Invoke`. Its unclear why `S3::GetObject` isn't mentioned as well, but it works for that too. However, instead of explicitly extracting the `Body` property from a response, we now recursively traverse the response and detect any such blob types that should be converted to strings - this should protect us against any other APIs that may exhibit this behavior. > `transformToString` is implemented as part of the [`SdkStreamMixin` interface](https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/Interface/SdkStreamMixin/). Included in this PR is a fix for `aws-s3-deployment/test/integ.bucket-deployment-substitution.ts` which was previously failing with the error shown above. Co-authored by: @iliapolo Co-authored by: @vinayak-kukreja Co-authored by: @scanlonp Closes #27114 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

msambol · 2023-09-16T14:48:50Z

@colifran I'm getting the following when running integration tests in aws-lambda-python-alpha. Still investigating but wondering if it's related to this PR.

2023-09-16T13:54:21.340Z	4a446e5c-4fc1-4c97-8cb7-e8316ee45075	INFO	TypeError: Cannot read properties of undefined (reading 'req')
    at AwsApiCallHandler.processEvent (/var/task/index.js:32693:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async AwsApiCallHandler.handle (/var/task/index.js:32360:26)
    at async Runtime.handler (/var/task/index.js:32753:20)

This is directly after the following log line:

2023-09-16T13:54:19.940Z	4a446e5c-4fc1-4c97-8cb7-e8316ee45075	INFO	SDK request to Lambda.invoke with parameters {
    "FunctionName": "integ-lambda-python-poetr-myhandlerpython38withhas-YlEnyP5nCKSl"
}

rix0rrr · 2023-09-18T15:36:45Z

Hey @msambol,

That's worrying. Can you go into a bit of detail what test is triggering this problem?

msambol · 2023-09-18T21:25:28Z

@rix0rrr / @colifran I went back to main to make sure I wasn't missing something. I updated this line to force a change in the integration test (renamed my_handler_inline_with_hashes to my_handler_inline_foo) and got this:

The stack named poetryDefaultTestDeployAssertE9C9CB8F failed to deploy: CREATE_FAILED (The following resource(s) failed to create: [LambdaInvoke631dde0680edf7d2f0eea8d9b9c06c75, LambdaInvokefbdb1fc975f2f372fd64335aa07b46b7, LambdaInvoke26ce5a20d1ffee210d056cd2aa005172, LambdaInvokeead1d1e2cc2d2a843a514ffdc015b2c4, LambdaInvoke26497bb1c12c14ed3deada8d7d76b39e, LambdaInvoke8cbaf98b5be6e4f6f81f5f10b5b8dc89, LambdaInvoke6fd2d93b0d07a2b4220fae7c91786512, LambdaInvoke9a0beb4ea6cc38db92e9ff664c085292]. ): Response object is too long., Response object is too long., Response object is too long., Response object is too long., Response object is too long., Response object is too long.

Different error than I originally got but I think related? Still digging.

msambol · 2023-09-19T19:29:26Z

@rix0rrr / @colifran One issue at a time... In this line, I logged the size of responseBody and it is 13611 bytes for tests in integ.function.poetry.ts. Maximum size is 4096 bytes (see here).

mrgrain · 2023-09-22T10:35:24Z

@msambol FYI you can pass --force in to always run the integration test.

It's possible this has broken one of the existing tests, 'cause you are absolutely right: They don't get automatically re-run for changes to integ-tests-alpha.

If the response body is too large, we will have to set output paths to limit the response.

msambol · 2023-09-22T12:16:39Z

Happy to submit a PR with some guidance.

colifran added 3 commits September 12, 2023 17:05

sdk transformToString

23a89a2

Signed-off-by: Francis <[email protected]>

sdk transformToString

5b32653

Signed-off-by: Francis <[email protected]>

Merge branch 'main' into colifran/log-response

b733acb

github-actions bot added bug This issue is a bug. p2 labels Sep 13, 2023

aws-cdk-automation requested a review from a team September 13, 2023 02:00

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 13, 2023

colifran added 2 commits September 12, 2023 22:14

update logic to handle cases where no response body exists

47cae5f

Signed-off-by: Francis <[email protected]>

remove unused parsePayload

2098c18

Signed-off-by: Francis <[email protected]>

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 13, 2023

iliapolo reviewed Sep 13, 2023

View reviewed changes

packages/@aws-cdk/integ-tests-alpha/lib/assertions/providers/lambda-handler/sdk.ts Outdated Show resolved Hide resolved

iliapolo reviewed Sep 13, 2023

View reviewed changes

packages/@aws-cdk/integ-tests-alpha/lib/assertions/providers/lambda-handler/sdk.ts Outdated Show resolved Hide resolved

iliapolo added 5 commits September 13, 2023 11:11

use transformToString

fe81eae

back to const

0489636

bring back parseJsonPayload

0dac296

recursively coerce response

e1a7c7e

Merge branch 'main' into colifran/log-response

0f98855

mrgrain reviewed Sep 13, 2023

View reviewed changes

...-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-substitution.ts Outdated Show resolved Hide resolved

mrgrain reviewed Sep 13, 2023

View reviewed changes

iliapolo added 4 commits September 13, 2023 17:38

Merge branch 'main' into colifran/log-response

586125b

remove diffAssets

7b06765

add unit tests

7353245

integ test

498ddeb

iliapolo requested a review from mrgrain September 13, 2023 14:51

mrgrain approved these changes Sep 13, 2023

View reviewed changes

mergify bot merged commit b0bbd5e into main Sep 13, 2023
10 checks passed

mergify bot deleted the colifran/log-response branch September 13, 2023 15:39

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 13, 2023

iliapolo changed the title ~~fix(integ-tests): use transformToString on API call response body~~ fix(integ-tests): use transformToString on API call response Sep 13, 2023

iliapolo changed the title ~~fix(integ-tests): use transformToString on API call response~~ fix(integ-tests): use transformToString on API call response Sep 13, 2023

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

antoniordz96 mentioned this pull request Oct 4, 2023

(integ-tests-alpha): invokeFunction is returning Uint8ArrayBlobAdapter each time versus converting to String or JSON object #27395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(integ-tests): use `transformToString` on API call response #27122

fix(integ-tests): use `transformToString` on API call response #27122

colifran commented Sep 13, 2023 •

edited by iliapolo

Loading

iliapolo commented Sep 13, 2023

mrgrain left a comment

aws-cdk-automation commented Sep 13, 2023

mergify bot commented Sep 13, 2023

msambol commented Sep 16, 2023

rix0rrr commented Sep 18, 2023

msambol commented Sep 18, 2023 •

edited

Loading

msambol commented Sep 19, 2023

mrgrain commented Sep 22, 2023

msambol commented Sep 22, 2023

fix(integ-tests): use transformToString on API call response #27122

fix(integ-tests): use transformToString on API call response #27122

Conversation

colifran commented Sep 13, 2023 • edited by iliapolo Loading

iliapolo commented Sep 13, 2023

mrgrain left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 13, 2023

AWS CodeBuild CI Report

mergify bot commented Sep 13, 2023

msambol commented Sep 16, 2023

rix0rrr commented Sep 18, 2023

msambol commented Sep 18, 2023 • edited Loading

msambol commented Sep 19, 2023

mrgrain commented Sep 22, 2023

msambol commented Sep 22, 2023

fix(integ-tests): use `transformToString` on API call response #27122

fix(integ-tests): use `transformToString` on API call response #27122

colifran commented Sep 13, 2023 •

edited by iliapolo

Loading

msambol commented Sep 18, 2023 •

edited

Loading