fix: S3 200 error customization should only not apply to streaming + blob shapes #1633
Conversation
|
so event streams DO get the s3/200 customization applied to them? |
...lientRuntime/Sources/AWSClientRuntime/Middlewares/AWSS3ErrorWith200StatusXMLMiddleware.swift
Outdated
Show resolved
Hide resolved
...lientRuntime/Sources/AWSClientRuntime/Middlewares/AWSS3ErrorWith200StatusXMLMiddleware.swift
Show resolved
Hide resolved
...lientRuntime/Sources/AWSClientRuntime/Middlewares/AWSS3ErrorWith200StatusXMLMiddleware.swift
Outdated
Show resolved
Hide resolved
...lientRuntime/Sources/AWSClientRuntime/Middlewares/AWSS3ErrorWith200StatusXMLMiddleware.swift
Outdated
Show resolved
Hide resolved
...software/amazon/smithy/aws/swift/codegen/customization/s3/S3ErrorWith200StatusIntegration.kt
Outdated
Show resolved
Hide resolved
...lientRuntime/Sources/AWSClientRuntime/Middlewares/AWSS3ErrorWith200StatusXMLMiddleware.swift
Show resolved
Hide resolved
| return | ||
| } | ||
|
|
||
| let statusCode = try isRootErrorElement(data: data) ? errorStatusCode : response.statusCode |
There was a problem hiding this comment.
How do we know we will have enough data here to determine if the body returns an error?
readData() could return zero bytes if the data is not yet available, and the stream could be filled with error data after this step is complete.
There was a problem hiding this comment.
we don't get all the messages back all at once, each response is a separate action so it should be checking each streamed message for Error at root
| extension ByteStream { | ||
|
|
||
| // Copy an existing ByteStream, optionally with new data | ||
| public func copy(data: Data?) -> ByteStream { |
There was a problem hiding this comment.
Does this need to be public? Is it used anywhere other than in the middleware above?
There was a problem hiding this comment.
Also, can this param be non-nil Data? In the code above, data is safe-unwrapped before this function is called
There was a problem hiding this comment.
sure I can make it private and non-nil unless we want to make it generally available and move it to a place like SmithyStreams or ClientRuntime. Only issue with that is neither has both ByteStream and the various types of streams
| case .data(let existingData): | ||
| return .data(data ?? existingData) | ||
| case .stream(let existingStream): | ||
| return .stream(data != nil ? BufferedStream(data: data, isClosed: true) : existingStream) |
There was a problem hiding this comment.
Shouldn't the copied stream be created un-closed? At this point we don't know if there will be more data in the stream in the future.
There was a problem hiding this comment.
If its created unclosed then the AsyncThrowingStream will never complete when looping through it. It yields each event until an end event and then hangs forever.
Issue #
#1629
Description of changes
selectObjectContentwhich didn't have the customization applied but now does. The response of this method contains an event stream.New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.