Add support for initial-response event streams on servers#4352
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| """, | ||
| *codegenScope, | ||
| ) { | ||
| rustTemplate("let mut headers = #{Vec}::new();", *codegenScope) |
There was a problem hiding this comment.
Worth a with_capacity()?
There was a problem hiding this comment.
If so, renderInitialRequestGenerator above (hidden in collapsible section in diff) may benefit, too.
|
|
||
| ⚠️ **Footgun**: Name collisions mean only one implementation gets generated. | ||
|
|
||
| ## GitHub CLI Integration |
There was a problem hiding this comment.
Feel like since this section isn't project specific it should live somewhere more generic for the agent rather than in the AGENTS file of each project?
ysaito1001
left a comment
ysaito1001
left a comment
There was a problem hiding this comment.
Thanks for fixing the client-side bug
| """, | ||
| *codegenScope, | ||
| ) { | ||
| rustTemplate("let mut headers = #{Vec}::new();", *codegenScope) |
There was a problem hiding this comment.
If so, renderInitialRequestGenerator above (hidden in collapsible section in diff) may benefit, too.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
rcoh
force-pushed
the
send-initial-response
branch
from
bb24c90 to
5f5dd0d
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| events: Events | ||
| } | ||
|
|
||
| structure StreamingOperationWithInitialResponseOutput { |
There was a problem hiding this comment.
Should we add an operation with optional fields to future-proof our tests? Smithy specs state: "Clients and servers MUST NOT fail if an initial-request or initial-response is not received for an initial message that contains only optional members." This would help test our handling of this requirement when we implement client support.
There was a problem hiding this comment.
yeah I'll add that to the model. There are a couple of other small improvements to make to the tests here, I'll open a tickets:
- Make it possible to use a generated client (not just the manually implemented one)
- Make it possible to actually generate these tests to make it easy to run them against multiple protocols.
| } | ||
| } | ||
|
|
||
| fun renderInitialResponseGenerator(contentType: String): RuntimeType { |
There was a problem hiding this comment.
Isn't this functionality server-specific? If so, shouldn't we move it out of the core module and keep it only in server-related modules?
There was a problem hiding this comment.
you could do that for ideological reasons, but all the library functions it needs are in here so its cleaner IMO
| CodegenTest( | ||
| "smithy.protocoltests.rpcv2Cbor#RpcV2CborService", | ||
| "rpcv2Cbor_extras_no_initial_response", | ||
| imports = listOf("$commonModels/rpcv2Cbor-extras.smithy"), |
There was a problem hiding this comment.
Do we have tests for other protocols? or are we only implementing this for rpcv2Cbor for now?
There was a problem hiding this comment.
its implemented for all protocols but currently only tested with cbor.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
Unexpected and contrary to documentation, clients do not currently always accept initial responses. This fixes that issue and updates clients to always accept initial responses for event streams. Because of this, we must also change the default to `false` for the setting to avoid backwards compatibility issues with existing clients.
rcoh
force-pushed
the
send-initial-response
branch
from
50c9b02 to
21656e5
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
4 participants
Motivation and Context
Event Stream operations over RPC protocols (that do not rely on HTTP headers), use
initial-requestandinitial-responseobjects to send data fields that are not bound to the event stream itself.However, there are two major bug in the current implementation:
initial-responseon event streams #4353: Clients error if an initial-response is sent when one is not expectedinitial-responsein all cases #4345: Servers do not send initial responses (ever)Description
This adds two things:
smithy-javaclients MUST enable this setting currently. This is disabled by default because clients in the wild today do not accept an emptyinitial-response.Testing
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.