fix(custom-resources): empty Lambda response payload causes deployment failure#27000
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.
aws-cdk-automation
left a comment
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.
iliapolo
left a comment
There was a problem hiding this comment.
I think a test is warranted...if not an integ that at least a unit test. though i suspect integ would actually be easier to write :)
mrgrain
left a comment
There was a problem hiding this comment.
The same issue will be present here, and we probably should align on a single implementation for this.
In fact I don't know how to trigger the empty Payload response condition. It seems to be an unusual case |
| return JSON.parse(text); | ||
| } 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!
✅ 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