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

SystemTextJsonSerializer: Adds Default STJ Serializer #4332

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

kundadebdatta
Copy link
Member

@kundadebdatta kundadebdatta commented Feb 28, 2024

Pull Request Template

Description

This PR adds a Default System.Text.JSON Implementation for CosmosLinqSerializer. This default serializer can be used along with the CosmosClientOptions to set the runtime serializer. Please take a look at the example below for usage:

        CosmosClientOptions clientOptions = new CosmosClientOptions()
        {
            Serializer = new CosmosDefaultSystemTextJsonSerializer(
                new System.Text.Json.JsonSerializerOptions())
        };

Note that this is just a default implementation which handles the basic scenarios. Any JsonSerializerOptions passed in here may not going to be reflected in SerializeMemberName().

To handle special cases, please create a custom serializer which inherits from the CosmosDefaultSystemTextJsonSerializer and overrides the SerializeMemberName() method to add any special handling.

Below is an example of a custom serializer, which is inherits the CosmosDefaultSystemTextJsonSerializer to cover the special handling for camel case naming property.

    public class CustomSystemTextJsonSerializer : CosmosDefaultSystemTextJsonSerializer
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="CustomSystemTextJsonSerializer "/> class.
        /// </summary>
        /// <param name="jsonSerializerOptions"></param>
        public CustomSystemTextJsonSerializer (JsonSerializerOptions jsonSerializerOptions)
            : base(jsonSerializerOptions)
        {
        }

        /// <inheritdoc/>
        public override string SerializeMemberName(MemberInfo memberInfo)
        {
            System.Text.Json.Serialization.JsonExtensionDataAttribute jsonExtensionDataAttribute =
                memberInfo.GetCustomAttribute<System.Text.Json.Serialization.JsonExtensionDataAttribute>(true);
            if (jsonExtensionDataAttribute != null)
            {
                return null;
            }

            JsonPropertyNameAttribute jsonPropertyNameAttribute = memberInfo.GetCustomAttribute<JsonPropertyNameAttribute>(true);
            if (!string.IsNullOrEmpty(jsonPropertyNameAttribute?.Name))
            {
                return jsonPropertyNameAttribute.Name;
            }

            return System.Text.Json.JsonNamingPolicy.CamelCase.ConvertName(memberInfo.Name);
        }
    }

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #4330

@kundadebdatta kundadebdatta requested a review from a team as a code owner March 11, 2024 19:07
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

🕐

@kundadebdatta kundadebdatta self-assigned this Mar 19, 2024
@adityasa
Copy link
Contributor

                "Select number -> Min w/ mapping", b => getQuery(b)

Nit - please include serializer name here too.


Refers to: Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs:154 in 683ca13. [](commit_id = 683ca13, deletion_comment = False)

adityasa
adityasa previously approved these changes Apr 15, 2024
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

:shipit:

/// To handle such scenarios, please create a custom serializer which inherits from the <see cref="CosmosSystemTextJsonSerializer"/> and overrides the
/// SerializeMemberName to add any special handling.
/// </remarks>
public override string SerializeMemberName(MemberInfo memberInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated the STJ sample code and made some improvements to this function in this PR #4420. Could you update this function to look like this - https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos.Samples/Usage/SystemTextJson/CosmosSystemTextJsonSerializer.cs ? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Maya-Painter. We are trying to gather some feedback from the .NET team on this. I will take your changes as well, and update the PR.

Choose a reason for hiding this comment

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

The JsonTypeInfo/JsonPropertyInfo types contain authoritative information on the JSON contract used by a particular type/property. See https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/custom-contracts for more details.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @eiriktsarpalis !

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your suggestion @eiriktsarpalis! Looks like, these (JsonSerializerOptions.TryGetTypeInfo, custom contracts) are applicable for the latest versions of the System.Text.JSON package, and it's not available in 4.6.0.

In order to use the System.Text.Json (STJ) serializer, we currently depend on the Azure.Core nuget package (specifically for this class JsonObjectSerializer). The current version of Azure.Core library, referred in our v3 .NET SDK is 1.19.0, which is quite old. This internally uses System.Text.Json version 4.6.0.

I am planning to keep this suggestion as follow-up task 4479 to upgrade the Azure.Core library to the latest. Even the latest version of Azure.Core 1.39.0 library uses the STJ version 4.7.2.

image

Copy link
Member

Choose a reason for hiding this comment

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

How about only make this type public once is fully addressed?

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that Azure.Core 'uses' (gets by with) an old version does not of itself mean that this lib can't take a dependency on a higher version. I'd ask, what version would simplify and/or significantly improve perf here? Then validate the answer by lookign at package download counts at https://www.nuget.org/packages/system.text.json.
For instance 6.0.1 has a net6.0 variant and seems to be in popular use, and is only 18 months newer - it's not like STJ is known to be a pain to upgrade.
But of course, I'm just being a devil's advocate; there's lots of silent votes for the status quo!
Hopefully there will be a security reason along to trigger a forced upgrade in due course ;)
The other benefit of taking a higher dep is that perf in general has been on an upward trajectory, and the perceived experience of the SDK would hence also improve for people that don't explicitly pin latest in their apps

@kundadebdatta kundadebdatta added the auto-merge Enables automation to merge PRs label May 10, 2024
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

:shipit:

if (stream == null)
throw new ArgumentNullException(nameof(stream));

if (stream.CanSeek && stream.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is canSeek is not really required right?

using Utf8JsonWriter writer = new (streamPayload);

JsonSerializer.Serialize(
writer: writer,
Copy link
Contributor

Choose a reason for hiding this comment

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

its great that I don't need to care as this will be an impl detail deep in a library but this formatting/named params is way overkill and makes it look like some really exciting stuff is going on, which there isnt
is the intention to disambiguate from some adjacent overload?
in general I'd be nice if this code was scannable so people can compare their impls quickly before deleting them in favor of this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enables automation to merge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Default System.Text.JSON Implementation for CosmosLinqSerializer
8 participants