-
Notifications
You must be signed in to change notification settings - Fork 50
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
S3 404/NotFound isn't modeled correctly #638
Comments
Looks like this broke starting in The model is fine, the response that S3 returns isn't though for how it's modeled. The "com.amazonaws.s3#HeadObject": {
"type": "operation",
"input": {
"target": "com.amazonaws.s3#HeadObjectRequest"
},
"output": {
"target": "com.amazonaws.s3#HeadObjectOutput"
},
"errors": [
{
"target": "com.amazonaws.s3#NotFound"
}
], The issue is We implement a customization for s3 to deal with this. The rendered version looks like this: private suspend fun throwHeadObjectError(context: ExecutionContext, response: HttpResponse): kotlin.Nothing {
val payload = response.body.readAll()
val wrappedResponse = response.withPayload(payload)
val errorDetails = try {
if (payload == null && response.status == HttpStatusCode.NotFound) {
S3ErrorDetails(code = "NotFound")
} else {
checkNotNull(payload){ "unable to parse error from empty response" }
parseS3ErrorResponse(payload)
}
} catch (ex: Exception) {
throw S3Exception("Failed to parse response as 'restXml' error", ex).also {
setS3ErrorMetadata(it, wrappedResponse, null)
}
}
val ex = when(errorDetails.code) {
"NotFound" -> NotFoundDeserializer().deserialize(context, wrappedResponse)
else -> S3Exception(errorDetails.message)
}
setS3ErrorMetadata(ex, wrappedResponse, errorDetails)
throw ex
} There are even custom protocol tests for this. The issue seems to be that we implemented the customization against a particular HTTP engine behavior to model an empty response as We likely need to extend HTTP test suite to codify this behavior and update the customization as necessary. That or make the customization a bit more robust to check for empty payloads in addition to null ones. |
Interestingly we fixed this with the direct binding to okhttp. Whereas our wrapped ktor engine always sets a body. Should be able to rectify that. |
This will be fixed in the next release due to the direct okhttp binding. I've also added expanded test coverage to ensure all HTTP engines we support behave consistently w.r.t empty bodies. |
Verified issue does not occur with OkHttp engine available in 0.16.4-SNAPSHOT...should've synced my sources before filing the bug! 😅 |
Verified latest release throws
|
|
Describe the bug
Calls to S3 APIs that return a 404/
NotFound
error aren't properly modeled and fail to deserialize, causing anS3Exception
instead of a more accurate error.Expected behavior
Any error should be deserialized correctly and thrown to calling code as a proper exception type.
Current behavior
An incorrect error is thrown because of failures in deserialization:
From examining wire logs, it appears the response itself has no body:
Steps to Reproduce
Call S3
HeadObject
for a bucket that exists but a key that does not exist:Possible Solution
No response
Context
No response
AWS Kotlin SDK version used
0.16.3-SNAPSHOT
Platform (JVM/JS/Native)
OpenJDK Runtime Environment Corretto-17.0.3.6.1 (build 17.0.3+6-LTS)
Operating System and version
Amazon Linux 2
The text was updated successfully, but these errors were encountered: