fix(custom-resources): empty Lambda response payload causes deployment failure #27000
Conversation
The payload response of a Lambda used to be a `string`, and could occasionally be `""`, which we detected and special-case parsed to an empty object. In SDKv3, the payload response of a Lambda changed to type `Uint8Array`, but a `Uint8Array(0)` doesn't check as *falsey*, so we'd decode it to `""` and then the `JSON.parse()` of that would fail. First decode, then check the string for emptyness. Fixes #26429.
github-actions
bot
added
bug
There was a problem hiding this comment.
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.
rix0rrr
changed the title
In fact I don't know how to trigger the empty Payload response condition. It seems to be an unusual case |
| } catch { | ||
| return payload; | ||
| throw new Error(`return values from user-handlers must be JSON objects. got: "${text}"`); |
There was a problem hiding this comment.
Can a Lambda Handler not return plain text? Or is it always converted to a JSON parsable string, i.e. "foobar"?
There was a problem hiding this comment.
Looks to be always a JSON representation of a JavaScript object.
There was a problem hiding this comment.
I.e. a plain text is returned as "Hi I'm a plain string".
There was a problem hiding this comment.
I'm still concerned we are going to fail on valid values or return unexpected values.
In the assertion handler parseJsonPayload is naively called on a potentially existing payload property of the response object. That's different than the provider framework where we know we are dealing with a lambda response.
In hindsight, maybe we should do the assertions handler on a different PR.
There was a problem hiding this comment.
I agree that the code is making too many assumptions, and might be subtly broken.
I would posit that it is subtly broken already, and that nothing we're doing is making it worse than what it currently is.
But sure, I'll revert!
rix0rrr
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…t failure (#27000) The payload response of a Lambda used to be a `string`, and could occasionally be `""`, which we detected and special-case parsed to an empty object. The Payload should never be empty, and will only be that under exceptional circumstances which we haven't been able to pin down yet, but we shouldn't fail in any case. In SDKv3, the payload response of a Lambda changed to type `Uint8Array`, but a `Uint8Array(0)` doesn't check as *falsey*, so we'd decode it to `""` and then the `JSON.parse()` of that would fail. First decode, then check the string for emptyness. Fixes #26429.
|
This seems to break the release. |
The payload response of a Lambda used to be a
string, and could occasionally be"", which we detected and special-case parsed to an empty object. The Payload should never be empty, and will only be that under exceptional circumstances which we haven't been able to pin down yet, but we shouldn't fail in any case.In SDKv3, the payload response of a Lambda changed to type
Uint8Array, but aUint8Array(0)doesn't check as falsey, so we'd decode it to""and then theJSON.parse()of that would fail.First decode, then check the string for emptyness.
Fixes #26429.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license