-
Notifications
You must be signed in to change notification settings - Fork 870
Update CBOR generators to support event streaming #3954
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
Update CBOR generators to support event streaming #3954
Conversation
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.
Pull Request Overview
This PR adds support for CBOR (Concise Binary Object Representation) protocol to event streaming functionality in the AWS SDK for .NET, enabling the generation of event stream publishers and consumers that can handle CBOR-encoded payloads.
- Introduces CBOR-specific marshalling and unmarshalling for event stream events
- Updates code generators to produce CBOR-compatible event stream publishers and output generators
- Adds new utility methods for converting event stream messages to appropriate contexts
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
EventStreamUtils.cs | Adds utility method to convert event stream messages to generic streams |
IEventStreamPublisher.cs | Changes EventStreamRequest access modifier from private to protected |
Member.cs | Updates type unmarshaller instantiation logic for CBOR protocol support |
StructureGenerator.tt | Adds CBOR namespace imports and fixes event interface inheritance |
EventStreamOutputGenerator.tt | Implements CBOR support for event stream output generation |
EventStreamPublisherMarshaller.tt | Adds CBOR marshalling support for event stream publishers |
JsonRPCStructureUnmarshaller.tt | Fixes null reference check for event payload members |
CborStructureUnmarshaller.tt | Fixes event payload member handling and structure cleanup |
CborSimpleTypeUnmarshaller.cs | Adds new DateTime unmarshaller for epoch milliseconds |
CborEventStreamPublisher.cs | New base class for CBOR event stream publishers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -611,7 +611,7 @@ public bool IsEventInputStream | |||
{ | |||
get | |||
{ | |||
if (this.ModelShape.IsEventStream && this.model.Operations.FirstOrDefault(x => string.Equals(this.OwningShape.Name, x.RequestStructure.Name)) != null) | |||
if (this.ModelShape.IsEventStream && this.model.Operations.FirstOrDefault(x => string.Equals(this.OwningShape.Name, x.RequestStructure?.Name)) != null) |
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 null-conditional operator may introduce inconsistent behavior. If RequestStructure is null, the string.Equals will never match, potentially changing the logic from the original code which would have thrown a NullReferenceException.
if (this.ModelShape.IsEventStream && this.model.Operations.FirstOrDefault(x => string.Equals(this.OwningShape.Name, x.RequestStructure?.Name)) != null) | |
if (this.ModelShape.IsEventStream && this.model.Operations.FirstOrDefault(x => string.Equals(this.OwningShape.Name, x.RequestStructure.Name)) != null) |
Copilot uses AI. Check for mistakes.
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.
I did that because I faced a an operation without a request structure in a testing service.
} | ||
reader.ReadEndMap(); |
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.
The reader.ReadEndMap() call has been moved outside the conditional block, but it should only be called when a map was actually started. This could cause issues when handling event structures with explicit payload members.
Copilot uses AI. Check for mistakes.
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.
No it wasn't, the duplicate if conditions was merged in one condition.
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.
looked at the generated branch. looks good.
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.
Looks good at high level. Nice work on modifying T4 files.
Description
EventStreamPublisherMarshaller.tt
to support publishing CBOR payloads.EventStreamOutputGenerator.tt
to support reading CBOR response.Motivation and Context
DOTNET-8164
Testing
CloudWatchLogs
andTranscribeStreaming
services with CBOR protocol and validated the generated code.EventStreamPublisher
generated exampleaws-sdk-net/sdk/src/Services/TranscribeStreaming/Generated/Model/Internal/MarshallTransformations/MedicalScribeInputStreamPublisherMarshaller.cs
Line 60 in f664606
EventStreamOutput
generated example https://github.com/aws/aws-sdk-net/blob/muhamoth/DOTNET-8164-cbor-event-streaming-support-testing/sdk/src/Services/CloudWatchLogs/Generated/Model/StartLiveTailResponseStream.cs#L55DRY_RUN-04e5355c-d6c0-4e41-baa3-ea261d74fb91
.Screenshots (if appropriate)
Types of changes
Checklist
License