fix: correctly handle all empty HeadObject/HeadBucket responses #1378
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
0marperez
reviewed
Aug 6, 2024
| } | ||
|
|
||
| writer.withBlock("catch (ex: Exception) {", "}") { | ||
| withBlock("throw #T(#S).also {", "}", exceptionBaseSymbol, "Failed to parse response as ${ctx.protocol.name}") { |
There was a problem hiding this comment.
style: I would vote for keeping the part that mentions that we're parsing an error in the exception message. It's kind of useful for determining where the exception happened, i.e. Failed to parse response as '${ctx.protocol.name}' error
There was a problem hiding this comment.
Yeah that was a miss while I was cleaning up the codegen, I'll add error back
HeadObject 403 never contains a payload. Here is a sample codegen: private fun throwHeadObjectError(context: ExecutionContext, call: HttpCall, payload: ByteArray?): kotlin.Nothing {
val wrappedResponse = call.response.withPayload(payload)
val wrappedCall = call.copy(response = wrappedResponse)
val errorDetails = try {
if (payload == null) {
if (call.response.status == HttpStatusCode.NotFound) {
S3ErrorDetails(code = "NotFound")
} else {
S3ErrorDetails(code = call.response.status.toString())
}
} else {
checkNotNull(payload) { "unable to parse error from empty response" }
parseS3ErrorResponse(payload)
}
} catch (ex: Exception) {
throw S3Exception("Failed to parse response as restXml").also {
setS3ErrorMetadata(it, wrappedCall.response, null)
}
}
val ex = when(errorDetails.code) {
"NotFound" -> NotFoundDeserializer().deserialize(context, wrappedCall, payload)
else -> S3Exception(errorDetails.message)
}
setS3ErrorMetadata(ex, wrappedResponse, errorDetails)
throw ex
}I'm not sure how a 403 with a non-null payload will get an |
|
A new generated diff is ready to view. |
ianbotsf
requested changes
Aug 6, 2024
Comment on lines
+70
to
75
| withInlineBlock("if (call.response.status == #T.NotFound) {", "} ", RuntimeTypes.Http.StatusCode) { | ||
| write("#T(code = #S)", s3ErrorDetails, "NotFound") | ||
| } | ||
| withInlineBlock("else {", "}") { | ||
| write("#T(code = #L)", s3ErrorDetails, "call.response.status.toString()") | ||
| } |
There was a problem hiding this comment.
Question: Isn't call.response.status.toString() also correct if the status code is NotFound? Do we need the special case for NotFound in this if block?
There was a problem hiding this comment.
Actually it would be Not Found for a 404 (space in the middle). We map it to "NotFound" which is how S3 models it.
There was a problem hiding this comment.
Does code have to match an S3 error code? Because call.response.status.toString() won't necessarily.
There was a problem hiding this comment.
No the code does not have to match an S3 error code, if it doesn't, it will just be thrown as a high-level S3Exception. For HeadObject this is what the error code mapping looks like:
val ex = when(errorDetails.code) {
"NotFound" -> NotFoundDeserializer().deserialize(context, wrappedCall, payload)
else -> S3Exception(errorDetails.message)
}Only "NotFound" is defined as an error on the operation, so any other error will just get thrown as S3Exception
Comment on lines
78
to
81
| withInlineBlock("else {", "}") { | ||
| write("checkNotNull(payload) { #S }", "unable to parse error from empty response") | ||
| write("#T(payload)", parseS3ErrorResponse) | ||
| } |
There was a problem hiding this comment.
Question: Why does this else block still have a checkNotNull(payload) in it? The if condition was that payload == null so it should be impossible for payload to be null in this else block.
There was a problem hiding this comment.
Yeah, it's probably not necessary, this was carried over from the previous codegen
This comment has been minimized.
This comment has been minimized.
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
ianbotsf
approved these changes
Aug 9, 2024
0marperez
approved these changes
Aug 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we only checked for empty bodies with HTTP 404, but it seems like S3 also returns empty bodies for HTTP 403. This PR expands the handler logic to catch all empty bodies rather than just 404's.
Issue #
Closes #1368
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.