-
Notifications
You must be signed in to change notification settings - Fork 80
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 errors in 200 response from S3 #1266
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c20332d
handle errors in 200 response from S3
dayaffe bd7f9a4
try to fix ktlint
dayaffe 6e90ccc
fix lint and move if statement for readability
dayaffe 0741581
replace data! with data and add a guard statement
dayaffe 97baadd
add unit tests for S3ErrorIn200
dayaffe 9783120
Merge branch 'main' into day/s3-200-errors
dayaffe 31744fd
Merge branch 'main' into day/s3-200-errors
dayaffe c9fb9dc
Merge branch 'main' into day/s3-200-errors
dayaffe 1210802
move s3 error test to integration tests folder
dayaffe 79938e7
add codegenerated changes to S3Client and Models
dayaffe 68b793d
move middleware code to AWSClientRuntime
dayaffe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// Code generated by smithy-swift-codegen. DO NOT EDIT! | ||
|
||
import AWSClientRuntime | ||
@testable import AWSS3 | ||
import AwsCommonRuntimeKit | ||
import ClientRuntime | ||
import SmithyTestUtil | ||
import XCTest | ||
|
||
public class MockHttpClientEngine: HttpClientEngine { | ||
|
||
// Public initializer | ||
public init() {} | ||
|
||
func successHttpResponse(request: SdkHttpRequest) -> HttpResponse { | ||
let errorResponsePayload = """ | ||
<Error> | ||
<Code>SlowDown</Code> | ||
<Message>Please reduce your request rate.</Message> | ||
<RequestId>K2H6N7ZGQT6WHCEG</RequestId> | ||
<HostId>WWoZlnK4pTjKCYn6eNV7GgOurabfqLkjbSyqTvDMGBaI9uwzyNhSaDhOCPs8paFGye7S6b/AB3A=</HostId> | ||
</Error> | ||
""" | ||
return HttpResponse(headers: request.headers, body: ByteStream.data(errorResponsePayload.data(using: .utf8)), statusCode: HttpStatusCode.ok) | ||
} | ||
|
||
public func execute(request: SdkHttpRequest) async throws -> HttpResponse { | ||
return successHttpResponse(request: request) | ||
} | ||
} | ||
|
||
class S3ErrorIn200Test: XCTestCase { | ||
|
||
override class func setUp() { | ||
AwsCommonRuntimeKit.CommonRuntimeKit.initialize() | ||
} | ||
|
||
/// S3Client throws expected error in response (200) with <Error> tag | ||
func testFoundExpectedError() async throws { | ||
let config = try await S3Client.S3ClientConfiguration(region: "us-west-2") | ||
config.httpClientEngine = MockHttpClientEngine() | ||
let client = S3Client(config: config) | ||
|
||
do { | ||
// any method on S3Client where the output shape doesnt have a stream | ||
_ = try await client.listBuckets(input: .init()) | ||
XCTFail("Expected an error to be thrown, but it was not.") | ||
} catch let error as UnknownAWSHTTPServiceError { | ||
// check for the error we added in our mock client | ||
XCTAssertEqual("Please reduce your request rate.", error.message) | ||
} catch { | ||
XCTFail("Unexpected error: \(error)") | ||
} | ||
} | ||
} |
93 changes: 93 additions & 0 deletions
93
...tware/amazon/smithy/aws/swift/codegen/customization/s3/S3ErrorWith200StatusIntegration.kt
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
package software.amazon.smithy.aws.swift.codegen.customization.s3 | ||
|
||
import software.amazon.smithy.model.Model | ||
import software.amazon.smithy.model.shapes.OperationShape | ||
import software.amazon.smithy.model.shapes.ServiceShape | ||
import software.amazon.smithy.model.traits.StreamingTrait | ||
import software.amazon.smithy.swift.codegen.ClientRuntimeTypes | ||
import software.amazon.smithy.swift.codegen.SwiftSettings | ||
import software.amazon.smithy.swift.codegen.SwiftWriter | ||
import software.amazon.smithy.swift.codegen.integration.ProtocolGenerator | ||
import software.amazon.smithy.swift.codegen.integration.SwiftIntegration | ||
import software.amazon.smithy.swift.codegen.integration.middlewares.handlers.MiddlewareShapeUtils | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewarePosition | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewareRenderable | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewareStep | ||
import software.amazon.smithy.swift.codegen.middleware.OperationMiddleware | ||
import software.amazon.smithy.swift.codegen.model.expectShape | ||
import software.amazon.smithy.swift.codegen.model.hasTrait | ||
|
||
/** | ||
* Register interceptor to handle S3 error responses returned with an HTTP 200 status code. | ||
* see [aws-sdk-kotlin#199](https://github.com/awslabs/aws-sdk-kotlin/issues/199) | ||
* see [aws-sdk-swift#1113](https://github.com/awslabs/aws-sdk-swift/issues/1113) | ||
*/ | ||
class S3ErrorWith200StatusIntegration : SwiftIntegration { | ||
override fun enabledForService(model: Model, settings: SwiftSettings): Boolean = | ||
model.expectShape<ServiceShape>(settings.service).isS3 | ||
|
||
override fun customizeMiddleware( | ||
ctx: ProtocolGenerator.GenerationContext, | ||
operationShape: OperationShape, | ||
operationMiddleware: OperationMiddleware, | ||
) { | ||
// we don't know for sure what operations S3 does this on. Go customized this for only a select few | ||
// like CopyObject/UploadPartCopy/CompleteMultipartUpload but Rust hit it on additional operations | ||
// (DeleteObjects). | ||
// Instead of playing whack-a-mole broadly apply this interceptor to everything but streaming responses | ||
// which adds a small amount of overhead to response processing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with this. I don't think it will cause any problems where it is not needed. |
||
val output = ctx.model.expectShape(operationShape.output.get()) | ||
val outputIsNotStreaming = output.members().none { | ||
it.hasTrait<StreamingTrait>() || ctx.model.expectShape(it.target).hasTrait<StreamingTrait>() | ||
} | ||
if (outputIsNotStreaming) { | ||
operationMiddleware.appendMiddleware(operationShape, S3HandleError200ResponseMiddleware) | ||
} | ||
} | ||
} | ||
|
||
private object S3HandleError200ResponseMiddleware : MiddlewareRenderable { | ||
override val name = "S3ErrorWith200StatusXMLMiddleware" | ||
|
||
override val middlewareStep = MiddlewareStep.DESERIALIZESTEP | ||
|
||
override val position = MiddlewarePosition.AFTER | ||
|
||
override fun render(ctx: ProtocolGenerator.GenerationContext, writer: SwiftWriter, op: OperationShape, operationStackName: String) { | ||
val outputShape = MiddlewareShapeUtils.outputSymbol(ctx.symbolProvider, ctx.model, op) | ||
writer.openBlock( | ||
"$operationStackName.${middlewareStep.stringValue()}.intercept(position: ${position.stringValue()}, id: \"${name}\") { (context, input, next) -> \$N<${outputShape.name}> in", | ||
"}", | ||
ClientRuntimeTypes.Middleware.OperationOutput, | ||
) { | ||
writer.apply { | ||
// Send the request and get the response | ||
write("let response = try await next.handle(context: context, input: input)") | ||
|
||
// Check if the response status is 200 | ||
write("guard response.httpResponse.statusCode == .ok else {") | ||
write(" return try await next.handle(context: context, input: input)") | ||
write("}") | ||
|
||
// Read the response body | ||
write("guard let data = try await response.httpResponse.body.readData() else {") | ||
write(" return try await next.handle(context: context, input: input)") | ||
write("}") | ||
write("let xmlString = String(data: data, encoding: .utf8) ?? \"\"") | ||
|
||
// Check for <Error> tag in the XML | ||
write("if xmlString.contains(\"<Error>\") {") | ||
write(" // Handle the error as a 500 Internal Server Error") | ||
write(" response.httpResponse.statusCode = .internalServerError") | ||
write(" return response") | ||
write("}") | ||
|
||
write("return try await next.handle(context: context, input: input)") | ||
} | ||
} | ||
} | ||
} |
1 change: 1 addition & 0 deletions
1
...urces/META-INF/services/software.amazon.smithy.swift.codegen.integration.SwiftIntegration
This file contains 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
Oops, something went wrong.
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.
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.
All the files in
Tests/Services/*
get deleted & re-generated every time codegen is performed. I think we need a different place to put this file.