Skip to content
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 marshallers to use system text json #3528

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Oct 28, 2024

Description

This is part 1 of a multi-part PR to update the AWS SDK for .NET to use system text json for all json based protocols. This PR focuses only on the marshallers.

Motivation and Context

As described in an internal design document, system text json based serialization is much more efficient in both memory and cpu. Since we're doing a new major version which takes a dependency on system.text.json, we can make the upgrade now.

Testing

I ran the generator for all protocol test services, then ran all the protocol tests which all passed. I had to comment out the JsonSampleGenerator class to get the protocol tests to run. That class will be refactored as part of another PR.

As for formatting of the marshallers I went through each generated marshaller and fixed spacing and formatting iteratively.
Final dry run will be done at the end of the PR.

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)

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

@peterrsongg peterrsongg added the v4 label Oct 28, 2024
{
JsonWriter writer = new JsonWriter(streamWriter);
writer.Validate = false;
#if !NETCOREAPP3_1_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rewrite this part to the following for simplicity? or would this change affect the behavior?

#if NETCOREAPP3_1_OR_GREATER
			ArrayBufferWriter<byte> arrayBufferWriter = new ArrayBufferWriter<byte>();
			using Utf8JsonWriter writer = new Utf8JsonWriter(arrayBufferWriter);
#else
                        using var memoryStream = new MemoryStream();
			using Utf8JsonWriter writer = new Utf8JsonWriter(memoryStream);
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't change the behavior, I will change it for readability, thanks!

}

request.Content = memoryStream.ToArray();
#if !NETCOREAPP3_1_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would prefer if we start with #if NETCOREAPP3_1_OR_GREATER condition instead of using the not version.

@@ -54,18 +54,18 @@ namespace <#= this.Config.Namespace #>.Model.Internal.MarshallTransformat
string memberProperty = variableName + "." + member.PropertyName + (member.IsNullable ? ".Value" : string.Empty);
if(member.IsStructure || member.IsList || member.IsMap)
{
this.ProcessStructure(level, variableName + "." + member.PropertyName, member.Shape);
this.ProcessStructure(level + 1, variableName + "." + member.PropertyName, member.Shape);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change, the generated files looks fine but I want to understand how it used to work before without the +1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually something I just noticed while scanning through the marshallers. It was incorrectly not indented one level so I fixed it

@peterrsongg
Copy link
Contributor Author

verified that the changes don't affect behavior. Merging

@peterrsongg peterrsongg merged commit 14a00fd into petesong/stj Nov 7, 2024
1 check passed
@peterrsongg peterrsongg deleted the petesong/stj-serialization branch January 6, 2025 20:31
peterrsongg added a commit that referenced this pull request Jan 13, 2025
* Serialization changes for System Text Json
peterrsongg added a commit that referenced this pull request Jan 17, 2025
* Serialization changes for System Text Json
muhammad-othman pushed a commit that referenced this pull request Jan 21, 2025
* Serialization changes for System Text Json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants