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
base: master
Are you sure you want to change the base?
SystemTextJsonSerializer: Adds Default STJ Serializer #4332
Conversation
Microsoft.Azure.Cosmos/src/Serializer/CosmosSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/Poco/STJ/Cars.cs
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/Poco/STJ/Cars.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Serializer/CosmosDefaultSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Serializer/CosmosDefaultSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Serializer/CosmosDefaultSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
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.
🕐
Microsoft.Azure.Cosmos/src/Serializer/CosmosDefaultSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Serializer/CosmosSystemTextJsonSerializer.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Outdated
Show resolved
Hide resolved
.../tests/Microsoft.Azure.Cosmos.EmulatorTests/Linq/LinqAggregateCustomSerializationBaseline.cs
Show resolved
Hide resolved
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.
/// 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) |
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'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!
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.
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.
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.
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.
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.
Thank you @eiriktsarpalis !
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.
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
.
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.
How about only make this type public once is fully addressed?
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.
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
…over the property naming policy.
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.
if (stream == null) | ||
throw new ArgumentNullException(nameof(stream)); | ||
|
||
if (stream.CanSeek && stream.Length == 0) |
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.
Is canSeek
is not really required right?
using Utf8JsonWriter writer = new (streamPayload); | ||
|
||
JsonSerializer.Serialize( | ||
writer: writer, |
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.
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
Pull Request Template
Description
This PR adds a Default
System.Text.JSON
Implementation forCosmosLinqSerializer
. This default serializer can be used along with theCosmosClientOptions
to set the runtime serializer. Please take a look at the example below for usage:Note that this is just a default implementation which handles the basic scenarios. Any
JsonSerializerOptions
passed in here may not going to be reflected inSerializeMemberName()
.To handle special cases, please create a custom serializer which inherits from the
CosmosDefaultSystemTextJsonSerializer
and overrides theSerializeMemberName()
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.Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #4330