fix: rpcv2Cbor event stream requests sending incorrect :content-type
#1180
Conversation
This comment has been minimized.
This comment has been minimized.
Affected ArtifactsNo artifacts changed size |
| writer.write("builder.headers.setMissing(\"Content-Type\", #S)", resolver.determineRequestContentType(op)) | ||
| val contentTypeHeader = when { | ||
| op.isInputEventStream(ctx.model) -> "application/vnd.amazon.eventstream" | ||
| else -> "application/cbor" | ||
| } | ||
|
|
||
| writer.write("builder.headers.setMissing(\"Content-Type\", #S)", contentTypeHeader) |
There was a problem hiding this comment.
Question: I don't see how this code materially differs from what we had before in determineRequestContentType. How does this fix the bug of incorrectly setting the content type for event stream inputs?
There was a problem hiding this comment.
The error we had before was in setting the :content-type during event stream message serialization. The value of that content type is set by determineRequestContentType which now only returns application/cbor.
However the overall request Content-Type still needs to be application/vnd.amazon.eventstream for event streams, which is what this renderContentTypeHeader function does
There was a problem hiding this comment.
I think the tests I wrote do a good job explaining the requirements but let me know if there's clarification to be done
| // Model taken from https://smithy.io/2.0/spec/streaming.html#event-streams | ||
| @streaming |
There was a problem hiding this comment.
Question: Is this comment still accurate given the additional shapes we've added?
There was a problem hiding this comment.
Yes, PublishEvents / Message / LeaveEvent are in that Smithy docs page
There was a problem hiding this comment.
Nit: I think the comment is a little unnecessary
There was a problem hiding this comment.
I disagree, more context is better than less
|
|
||
| // Event stream requests should have Content-Type=application/vnd.amazon.eventstream | ||
| val serializeBody = serializer.lines(" override suspend fun serialize(context: ExecutionContext, input: PutFooStreamingRequest): HttpRequestBuilder {", "}") | ||
| serializeBody.shouldContainOnlyOnceWithDiff("""builder.headers.setMissing("Content-Type", "application/vnd.amazon.eventstream")""") |
There was a problem hiding this comment.
Question: This is coming from line 252 of HttpBindingProtocolGenerator.kt right?
| if (op.isInputEventStream(ctx.model)) { | ||
| val eventStreamSerializeFn = eventStreamRequestHandler(ctx, op) | ||
| writer.write("builder.body = #T(context, input)", eventStreamSerializeFn) | ||
| renderContentTypeHeader(ctx, op, writer, resolver) |
There was a problem hiding this comment.
Question: This doesn't break anything for the other protocols?
There was a problem hiding this comment.
not that I've seen. we have e2e tests for event streams in aws-sdk-kotlin
| // Model taken from https://smithy.io/2.0/spec/streaming.html#event-streams | ||
| @streaming |
There was a problem hiding this comment.
Nit: I think the comment is a little unnecessary
3 participants
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.