Fix RestJson serialization of sparse null map values #3682

AlexDaines · 2025-02-28T20:21:55Z

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:

Nullable boolean values (bool?)
Nullable number values (int?)
String values
Struct values

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)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

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

dscpinheiro · 2025-02-28T20:25:25Z

I'm not of the reviewers, but I just merged 3 PRs to v4-development. I recommend doing another dry-run to make sure your changes still pass.

peterrsongg · 2025-03-03T17:55:13Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt

@@ -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


@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

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

peterrsongg

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?

boblodgett · 2025-03-07T17:03:15Z

...in/java/software/amazon/smithy/dotnet/codegen/customizations/ProtocolTestCustomizations.java

+            "RestJsonNullAndEmptyHeaders",
+            "NullAndEmptyHeaders"


Why did we add back in two more skips in here? All of the tests should be removed from VNextTests before complete or moved to the main skip section with information why it was skipped.

I was assuming this was because he needs to rebase?

boblodgett · 2025-03-07T19:31:13Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/JsonRPCStructureMarshaller.tt

+                if (this.Structure != null)
+                {
+                    var memberWithMap = this.Structure.Members.FirstOrDefault(m => m.PropertyName == variableName.Split('.').Last());
+                    isNullableMap = (memberWithMap != null && memberWithMap.UseNullable);


Not blocking but another slightly more compact way to do this is: isNullableMap = memberWithMap?.UseNullable == true;

boblodgett · 2025-03-07T19:40:40Z

generator/TestServiceModels/restjson-tests-client/rest-json-protocol-2019-12-16.normal.json

@@ -389,6 +389,17 @@
      "output":{"shape":"JsonMapsInputOutput"},
      "documentation":"<p>The example tests basic map serialization.</p>"
    },
+    "SparseJsonMaps":{


These files come from a repo and can't be changed. If you did this to get the models generated that is fine but you will have to copy the generated code to the partial class and then undo these *.json files.

boblodgett · 2025-03-07T19:41:38Z

sdk/test/Services/RestJsonProtocol/Custom/_netstandard/AmazonRestJsonProtocolClient.cs

@@ -1,76 +0,0 @@
-/*


These custom partials should not be deleted. They are for some of the tests.

boblodgett · 2025-03-07T19:41:56Z

sdk/test/Services/RestJsonProtocol/Custom/_netstandard/IAmazonRestJsonProtocol.cs

@@ -1,56 +0,0 @@
-/*


This file should not be deleted. It is a partial.

boblodgett · 2025-03-07T19:44:19Z

sdk/test/Services/RestJsonProtocol/Generated/_netstandard/AmazonRestJsonProtocolClient.cs

@@ -2095,6 +2095,40 @@ internal virtual SimpleScalarPropertiesResponse SimpleScalarProperties(SimpleSca

        #endregion

+        #region  SparseJsonMaps
+
+        internal virtual SparseJsonMapsResponse SparseJsonMaps(SparseJsonMapsRequest request)


These are only being generated because of the .json files that shouldn't be modified. Remove the JSON files, run the generator again and these will be removed. That will cause you to have to keep the partial files that were deleted.

… PR discussion

AlexDaines requested review from peterrsongg and boblodgett February 28, 2025 20:21

AlexDaines force-pushed the adaines/fix-RestJsonSerializesSparseNullMapValues-protocol-test-v2 branch from d571f0b to b40cb80 Compare February 28, 2025 21:58

peterrsongg requested changes Mar 3, 2025

View reviewed changes

dscpinheiro added the v4 label Mar 3, 2025

AlexDaines force-pushed the adaines/fix-RestJsonSerializesSparseNullMapValues-protocol-test-v2 branch 2 times, most recently from 3627331 to da300d5 Compare March 6, 2025 19:50

peterrsongg approved these changes Mar 7, 2025

View reviewed changes

boblodgett reviewed Mar 7, 2025

View reviewed changes

AlexDaines added 2 commits March 10, 2025 10:53

Fix RestJson serialization of sparse null map values

f9a088d

fix(marshaller): Improve JsonRPCStructureMarshaller template based on…

adce3aa

… PR discussion

AlexDaines force-pushed the adaines/fix-RestJsonSerializesSparseNullMapValues-protocol-test-v2 branch from da300d5 to adce3aa Compare March 10, 2025 14:57

AlexDaines closed this Mar 20, 2025

dscpinheiro deleted the adaines/fix-RestJsonSerializesSparseNullMapValues-protocol-test-v2 branch March 21, 2025 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix RestJson serialization of sparse null map values #3682

Fix RestJson serialization of sparse null map values #3682

Uh oh!

AlexDaines commented Feb 28, 2025

Uh oh!

dscpinheiro commented Feb 28, 2025

Uh oh!

peterrsongg Mar 3, 2025

Uh oh!

boblodgett Mar 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterrsongg left a comment •

edited

Loading

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

peterrsongg Mar 7, 2025

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

boblodgett Mar 7, 2025

Uh oh!

Uh oh!

Fix RestJson serialization of sparse null map values #3682

Fix RestJson serialization of sparse null map values #3682

Uh oh!

Conversation

AlexDaines commented Feb 28, 2025

Description

Motivation and Context

Testing

Types of changes

Checklist

License

Uh oh!

dscpinheiro commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterrsongg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

peterrsongg left a comment •

edited

Loading