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

IntegTest Assertions fails while logging response #27114

Closed
mrgrain opened this issue Sep 12, 2023 · 2 comments · Fixed by #27122
Closed

IntegTest Assertions fails while logging response #27114

mrgrain opened this issue Sep 12, 2023 · 2 comments · Fixed by #27122
Assignees
Labels
@aws-cdk/integ-tests bug This issue is a bug. node18-upgrade Any work (bug, feature) related to Node 18 upgrade sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only.

Comments

@mrgrain
Copy link
Contributor

mrgrain commented Sep 12, 2023

See the integ test in
aws-s3-deployment/test/integ.bucket-deployment-substitution.ts for an example.

The assertions handler will attempt to log the response of the made API request. In this case it is S3.getObject which includes a Blob in the Body field.

When logging the response, the handler uses JSON.stringify() which fails with to the following error:

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)

This might also conceal an issue with returning blob data from the ApiCall, but not sure yet.

@mrgrain mrgrain added @aws-cdk/integ-tests sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only. node18-upgrade Any work (bug, feature) related to Node 18 upgrade bug This issue is a bug. labels Sep 12, 2023
@msambol
Copy link
Contributor

msambol commented Sep 12, 2023

@mrgrain If it helps... I got rid of the circular structure with:

    delete response.Body.req.res;
    delete response.Body.socket.parser;
    delete response.Body.client.parser;
    delete response.Body.req.parser;
    delete response.Body.client._httpMessage;
    delete response.Body.socket._httpMessage;

But then get:

Testing equality between undefined and "{\"$StringLike\":\"substitutionStatus: substitution-successful!\\\\nlambdaArn: arn:aws:lambda:us-east-1:453401560325:function:test-s3-deploy-substitution-Hello4A628BD4-Zv966iMnT4st\"}"

I'll dig deeper.

@kaizencc kaizencc self-assigned this Sep 12, 2023
@colifran colifran self-assigned this Sep 12, 2023
@cgarvis cgarvis assigned iliapolo and unassigned kaizencc and colifran Sep 13, 2023
@mergify mergify bot closed this as completed in #27122 Sep 13, 2023
mergify bot pushed a commit that referenced this issue Sep 13, 2023
…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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

mikewrighton pushed a commit that referenced this issue Sep 14, 2023
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/integ-tests bug This issue is a bug. node18-upgrade Any work (bug, feature) related to Node 18 upgrade sdk-v3-upgrade Tag issues that are associated to SDK V3 upgrade. Not limited to CR usage of SDK only.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants