-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: handle S3 error responses with HTTP 200 status code #1117
Conversation
A new generated diff is ready to view. |
import kotlin.test.assertEquals | ||
import kotlin.test.assertFailsWith | ||
|
||
class Handle200ErrorsInterceptorTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also add a test ensuring error payloads are unmodified (tests L27 of your interceptor if (!response.status.isSuccess()) return response
)
assertEquals(expectedMessage, ex.sdkErrorMetadata.errorMessage) | ||
assertEquals(expectedMessage, ex.sdkErrorMetadata.errorMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
Double line.
Double line.
override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = | ||
model.expectShape<ServiceShape>(settings.service).isS3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should this apply to any other S3-adjacent services like S3 Control, Glacier, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question, I actually think several of our customizations may need to apply to both s3 and s3 control. Not 100% on which ones though.
cd63dff
to
d0723dc
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
A new generated diff is ready to view. |
Issue #
fixes #199
Description of changes
Adds an interceptor that attempts to parse the response XML as an error response and resets the HTTP status code accordingly to trigger error handling.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.