Add Initial support for CBOR protocol including extension package and Marshaller generators #3879

muhammad-othman · 2025-06-12T23:16:05Z

Description

This PR introduces foundational support for the smithy-rpc-v2-cbor protocol in the AWS SDK for .NET. The changes are structured across three commits:

1c3f3aa Add AWSSDK.Extensions.CborProtocol project which includes the following:
- CborWriterPool: Reduces allocations by pooling CborWriter instances in a thread-safe way.
- CborMarshallerContext: Common context used by marshallers to write CBOR payloads.
- Isolates CBOR-specific dependencies System.Formats.Cbor from the Core SDK and keeps non-CBOR services unaffected.
f61c4f5 Add CBOR marshaller generators
f1a5ae7 Generate sample marshallers for CloudWatch.
- These marshallers can be used as reference for reviewing the generator output.
- Will revert this commit before merging the PR.

Motivation and Context

DOTNET-8158
DOTNET-8159
DOTNET-8160

Testing

Added smithy-rpc-v2-cbor to few service models protocols, generated the marshallers, executed some requests using these marshallers and validated that the serialized content is as expected and that the requests were handled correctly by these services.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

License

I confirm that this pull request can be released under the Apache 2 license

muhammad-othman · 2025-06-12T23:18:46Z

extensions/src/AWSSDK.Extensions.CborProtocol/Internal/CborWriterPool.cs

+        private static readonly ConcurrentBag<CborWriter> _pool = new ConcurrentBag<CborWriter>();
+
+        // Maximum number of CborWriter instances the pool can hold
+        private static int _maxPoolSize = 16;


The current value of _maxPoolSize is temporary, I need evaluate and test it more before merging the feature branch.

peterrsongg

just some minor comments, but also it's kind of hard to tell if it is working correctly or not without the protocol tests for the marshallers. Were you planning on adding that as part of this PR?

peterrsongg · 2025-06-13T19:53:02Z

...nsions/src/AWSSDK.Extensions.CborProtocol/AWSSDK.Extensions.CborProtocol.NetFramework.csproj

+  </Choose>
+
+  <ItemGroup>
+    <PackageReference Include="System.Formats.Cbor" Version="9.0.5" />


this is a general question, but do you have plans to also include these new dependencies in the build system when copying over the dll's? I had to do that when adding System.Text.JSON as an example

Should part of the build system task that we added, will mention that on the task's decription so we don't miss that.

peterrsongg · 2025-06-13T19:54:40Z

extensions/src/AWSSDK.Extensions.CborProtocol/AWSSDK.Extensions.CborProtocol.NetStandard.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>netstandard2.0;netcoreapp3.1;net8.0</TargetFrameworks>


Do we need netcoreapp3.1 in here if cbor is only supported in net8 and netstandard?

It should work on netcoreapp3.1 too, it will just show a warning.

peterrsongg · 2025-06-13T20:01:56Z

extensions/src/AWSSDK.Extensions.CborProtocol/CborWriterExtensions.cs

+                return;
+            }
+
+            if (value % 1 == 0)


is this safe to do? I don't know much about CBOR but it seems that if the user passes in a float value they'd want to keep that extra precision instead of writing it as an int. Am I missing something here? Since it is binary does it not matter?

When if (value % 1 == 0) is true, then we don't have anything after the decimal point to keep. It was part of the SEP to send the numbers as the smallest version possible that doesn't lose any data.

peterrsongg · 2025-06-13T20:06:39Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborRequestMarshaller.tt

+        /// </summary>  
+        /// <param name="input"></param>
+        /// <returns></returns>
+		public IRequest Marshall(AmazonWebServiceRequest input)


whitespace seems to be off here. or is it just the github ui?

Probably, It looks good when generated though which should matter more, will double check if I can do something about it without messing up the generated files.

peterrsongg · 2025-06-13T21:22:49Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborRequestMarshaller.tt

+				throw new System.InvalidOperationException("TargetPrefix is required for CBOR based services.");
+			}
+#>
+			request.ResourcePath = "<#=$"service/{this.Config.ServiceModel.TargetPrefix}/operation/{this.Operation.Name}"#>";


What about the prefix? The SEP states it as a MUST

Every request for the rpcv2Cbor protocol MUST be sent to a URL with the following form: {prefix?}/service/{serviceName}/operation/{operationName}

I couldn't find in the cloudwatch logs service model where to even get this prefix though

Yeah this is something I talked about with the SEP writers in the CBOR channel, it doesn't exist now for CloudWatch but it must be added before it uses CBOR.
Also there is additional details in the SEP few lines after the line you quoted, and it mentions how the serviceName can be found for smithy and that for C2J it should be TargetPrefix, thats why I added this exception to be safe if they forgot to add it for some reason.

peterrsongg · 2025-06-13T21:30:33Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborStructureMarshaller.tt

+using AWSSDK.Extensions.CborProtocol.Internal;
+
+#pragma warning disable CS0612,CS0618
+namespace <#=        this.Config.Namespace #>.Model.Internal.MarshallTransformations


nit: weird spacing here?

Maybe another github UI thing, will double check this one too.

muhammad-othman

@peterrsongg I understand that verifiying that everything work is kinda hard, but this PR is already big enough that I didn't want and increase its size with the Protocol tests which are my next task. Though did some manual testing with the services to be sure.
Also we are targetting a feature branch here, so rest assured that we don't release this without the Protocol tests :D

muhammad-othman · 2025-06-13T21:42:33Z

...nsions/src/AWSSDK.Extensions.CborProtocol/AWSSDK.Extensions.CborProtocol.NetFramework.csproj

+  </Choose>
+
+  <ItemGroup>
+    <PackageReference Include="System.Formats.Cbor" Version="9.0.5" />


Should part of the build system task that we added, will mention that on the task's decription so we don't miss that.

muhammad-othman · 2025-06-13T21:43:19Z

extensions/src/AWSSDK.Extensions.CborProtocol/AWSSDK.Extensions.CborProtocol.NetStandard.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFrameworks>netstandard2.0;netcoreapp3.1;net8.0</TargetFrameworks>


It should work on netcoreapp3.1 too, it will just show a warning.

muhammad-othman · 2025-06-13T21:45:42Z

extensions/src/AWSSDK.Extensions.CborProtocol/CborWriterExtensions.cs

+                return;
+            }
+
+            if (value % 1 == 0)


When if (value % 1 == 0) is true, then we don't have anything after the decimal point to keep. It was part of the SEP to send the numbers as the smallest version possible that doesn't lose any data.

muhammad-othman · 2025-06-13T21:48:45Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborRequestMarshaller.tt

+				throw new System.InvalidOperationException("TargetPrefix is required for CBOR based services.");
+			}
+#>
+			request.ResourcePath = "<#=$"service/{this.Config.ServiceModel.TargetPrefix}/operation/{this.Operation.Name}"#>";


Yeah this is something I talked about with the SEP writers in the CBOR channel, it doesn't exist now for CloudWatch but it must be added before it uses CBOR.
Also there is additional details in the SEP few lines after the line you quoted, and it mentions how the serviceName can be found for smithy and that for C2J it should be TargetPrefix, thats why I added this exception to be safe if they forgot to add it for some reason.

muhammad-othman · 2025-06-13T21:49:52Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborStructureMarshaller.tt

+using AWSSDK.Extensions.CborProtocol.Internal;
+
+#pragma warning disable CS0612,CS0618
+namespace <#=        this.Config.Namespace #>.Model.Internal.MarshallTransformations


Maybe another github UI thing, will double check this one too.

muhammad-othman · 2025-06-13T21:50:56Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/CborRequestMarshaller.tt

+        /// </summary>  
+        /// <param name="input"></param>
+        /// <returns></returns>
+		public IRequest Marshall(AmazonWebServiceRequest input)


Probably, It looks good when generated though which should matter more, will double check if I can do something about it without messing up the generated files.

peterrsongg

Approving, knowing that this may change when the protocol tests are added

muhammad-othman added 3 commits June 12, 2025 18:54

Add CborProtocol extension project

1c3f3aa

Add Cbor request and structure marshallers generators

f61c4f5

Add some of cloudwatch marshallers as an example of Cbor marshallers

f1a5ae7

muhammad-othman commented Jun 12, 2025

View reviewed changes

normj self-requested a review June 13, 2025 07:33

philasmar requested a review from peterrsongg June 13, 2025 17:25

peterrsongg reviewed Jun 13, 2025

View reviewed changes

muhammad-othman commented Jun 13, 2025

View reviewed changes

peterrsongg approved these changes Jun 13, 2025

View reviewed changes

Add Initial support for CBOR protocol including extension package and Marshaller generators #3879

Are you sure you want to change the base?

Add Initial support for CBOR protocol including extension package and Marshaller generators #3879

Uh oh!

Conversation

muhammad-othman commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterrsongg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhammad-othman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterrsongg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

muhammad-othman commented Jun 12, 2025 •

edited

Loading