-
Notifications
You must be signed in to change notification settings - Fork 863
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
Fix RestJson serialization of sparse null map values #3682
base: v4-development
Are you sure you want to change the base?
Fix RestJson serialization of sparse null map values #3682
Conversation
I'm not of the reviewers, but I just merged 3 PRs to |
d571f0b
to
b40cb80
Compare
@@ -160,8 +165,76 @@ namespace <#= this.Config.Namespace #>.Model.Internal.MarshallTransformat | |||
<#=new string(' ', level * 4)#> context.Writer.WritePropertyName(<#=flatVariableName#>Kvp.Key); | |||
<#=new string(' ', level * 4)#> var <#=flatVariableName#>Value = <#=flatVariableName#>Kvp.Value; | |||
|
|||
<#+ | |||
ProcessStructure(level + 1, flatVariableName + "Value", structure.ValueShape); | |||
<#+ // Check for null values - only null checks for sparse maps as defined in customizations |
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.
@boblodgett I could be convinced either way. Should we also include the logic for sparse lists even though we don't have a test for this? There are tests right now that we are skipping for example: https://github.com/aws/aws-sdk-net/blob/main/generator/ProtocolTestsGenerator/smithy-dotnet-codegen/src/main/java/software/amazon/smithy/dotnet/codegen/customizations/ProtocolTestCustomizations.java#L36
which target SparseStringList
etc. Maybe just backlog it so we are aware
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 am ok with backlog item for it. Nothing uses it right now anyway and C2J doesn't support sparse.
generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt
Outdated
Show resolved
Hide resolved
generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt
Outdated
Show resolved
Hide resolved
generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt
Show resolved
Hide resolved
generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt
Show resolved
Hide resolved
b40cb80
to
3627331
Compare
3627331
to
da300d5
Compare
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, but we have to remember not to overwrite the additions you made in the normal.json
file somehow b/c in the next copy/paste we do from the artifact it will get overwritten. Can we add an additional file like:
rest-json-protocol-partial.json
which has just your additions in it?
Description
Modified the JSON marshaller generation for sparse maps to properly handle null values in accordance with the REST JSON protocol specification.
Motivation and Context
Fixes RestJsonSerializesSparseNullMapValues protocol test. The marshaller was not correctly handling null values in sparse maps, causing an InvalidOperationException when attempting to serialize null values.
Testing
Dry-run succeeded @ Feb 27, 2025, 17:59:48.041 (UTC-05:00)
All tests pass, including RestJsonSerializesSparseNullMapValues. The fix ensures proper serialization of null values for:
Types of changes
Checklist
License