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

CF/P AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. #4616

Closed
philipthomas-MSFT opened this issue Jul 31, 2024 · 0 comments · Fixed by #4618
Assignees

Comments

@philipthomas-MSFT
Copy link
Contributor

philipthomas-MSFT commented Jul 31, 2024

We are continuously addressing and improving the SDK, if possible, make sure the problem persist in the latest SDK version.

Stakeholder

  • Any teams that is using CFP-AVAD and CF-AVAD in Preview.

PR: #4618

Describe the bug
Using System.Text.Json custom serializer with CFP-AVAD, GetChangeFeedProcessorBuilderWithAllVersionsAndDeletes, is resulting with deserialization issues for customer when type ChangeFeedItem and ChangeFeedMetadata properties uses internal set properties instead of the default public parameterless constructor.

References Blocking Issue: System.Text.Json and non-public members, Consider extending JsonIncludeAttribute and JsonConstructorAttribute support to internal and private members., and Use immutable types and properties.

We also lack unit test coverage, so I will be improving the SDK by including those in this PR. There were recent discussions on fixing this, but it needs to happen sooner than later, as we have a stakeholder who is blocked.

System.Text.Json does not deserialize the same as Newtonsoft.Json so some changes have to be made to accommodate both.

Current code state prior to this issue and attach branch and PR is the annotation of type ChangeFeedItem and ChangeFeedMetadata properties with JsonProperty obviously only works with Newtonsoft.Json so names must match instead of using alias property names or I would have to dual annotate with both serializers.

This was noticeably a problem with ConflictResolutionTimestamp, IsTimeToLiveExpired, and PreviousLsn with System.Text.Json. The response payload pre-deserialization returns crts, timeToLiveExpired, and previousImageLSN elements respectively as shown below:

[
	{
		"current": {},
		"metadata": {
			"lsn": 17,
			"crts": 1722511591,
			"operationType": "delete",
			"timeToLiveExpired": true,
			"previousImageLSN": 16
		},
		"previous": {
			"id": "1",
			"pk": "1",
			"description": "Testing TTL on CFP.",
			"ttl": 5,
			"_rid": "SnxPAOM2VfMBAAAAAAAAAA==",
			"_self": "dbs/SnxPAA==/colls/SnxPAOM2VfM=/docs/SnxPAOM2VfMBAAAAAAAAAA==/",
			"_etag": "\"00000000-0000-0000-e405-5632b83c01da\"",
			"_attachments": "attachments/",
			"_ts": 1722511453
		}
	}
]

THIS WILL BE A BREAKING CHANGE for those that are using the CFP-AVAD and CF-AVAD in Preview. I am also proposing to remove JsonProperty altogether to avoid the conflict between the two serializers.In addition, ConflictResolutionTimestamp, now crts has changed from DateTime to long because the value pre-deserialization is a long. We can leave it to the user to convert it using their serializer of choice.

There is also some restructuring in the the tests\Microsoft.Azure.Cosmos.EmulatorTests to separate GetChangeFeedProcessorBuilderWithAllVersionsAndDeletes tests without custom serialization from GetChangeFeedProcessorBuilderWithAllVersionsAndDeletes tests with custom serialization.

NOTE: GetChangeFeedProcessorBuilderWithAllVersionsAndDeletes is still in Preview.

To Reproduce
Use a custom serializer with JsonSerializerOptions.

This could be reproduced via CosmosClientOptions.Serializer property

CosmosClientOptions clientOptions = new CosmosClientOptions
{
    Serializer = new Microsoft.Azure.Cosmos.CosmosSystemTextJsonSerializer(new JsonSerializerOptions()
    {
        PropertyNameCaseInsensitive = true,
        Converters = { new JsonStringEnumConverter() }
    })
};

or

CosmosClientOptions.UseSystemTextJsonSerializerWithOptions

CosmosClientOptions clientOptions = new CosmosClientOptions
{
    UseSystemTextJsonSerializerWithOptions = new JsonSerializerOptions()
    {
        PropertyNameCaseInsensitive = true,
        Converters = { new JsonStringEnumConverter() }
    }
};

There is a new UnixDateTimeConverter for System.Text.Json.

Expected behavior
Deserialization of ChangeFeedItem and ChangeFeedMetadata when using Newtonsoft.Json or System.Text.Json.

Actual behavior
Deserialzation of ChangeFeedItem and ChangeFeedMetadata is setting all properties to default values when using System.Text.Json.

Environment summary
SDK Version: Discovered by customer with version 3.43.0-preview.0

OS Version (e.g. Windows, Linux, MacOSX)

Additional context
Add any other context about the problem here (for example, complete stack traces or logs).

@philipthomas-MSFT philipthomas-MSFT self-assigned this Jul 31, 2024
@philipthomas-MSFT philipthomas-MSFT changed the title CFP AVAD: Issues with Serialization/Deserialization with CosmosSystemTextJsonSerializer CFP AVAD: Issues with Serialization/Deserialization with CosmosSystemTextJsonSerializer and custom serializers. Jul 31, 2024
@philipthomas-MSFT philipthomas-MSFT moved this to In Progress in Azure Cosmos SDKs Jul 31, 2024
@philipthomas-MSFT philipthomas-MSFT changed the title CFP AVAD: Issues with Serialization/Deserialization with CosmosSystemTextJsonSerializer and custom serializers. CFP AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializers. Aug 1, 2024
@philipthomas-MSFT philipthomas-MSFT changed the title CFP AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializers. CFP AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. Aug 1, 2024
@philipthomas-MSFT philipthomas-MSFT changed the title CFP AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. ChangeFeed/Processor AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. Aug 1, 2024
@philipthomas-MSFT philipthomas-MSFT moved this from In Progress to Blocked in Azure Cosmos SDKs Aug 1, 2024
@philipthomas-MSFT philipthomas-MSFT changed the title ChangeFeed/Processor AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. CF/P AVAD: Issues with Serialization/Deserialization with JsonSerializerOptions on Custom serializer and UseSystemTextJsonSerializerWithOptions. Aug 1, 2024
@github-project-automation github-project-automation bot moved this from Blocked to Done in Azure Cosmos SDKs Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
1 participant