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

[vNext] System.Text.Json as default serialization mechanism #1119

Merged
merged 72 commits into from
Jan 7, 2020

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Dec 27, 2019

This PR works on replacing Json.NET as the default serialization mechanism in the SDK. While Json.NET can still be used through an implementation of CosmosSerializer (like the current one for V3) all REST API contracts, and item operations (CRUD and Queries) will use System.Text.Json by default.

In this PR:

This PR closes #73 and closes #1111

@ealsur ealsur changed the title [vNext] Draft PR for System.Text.Json test [vNext] System.Text.Json as default serialization mechanism Dec 31, 2019
@ealsur ealsur self-assigned this Dec 31, 2019
@ealsur ealsur added feature-request New feature or request VNext Next Major version of SDK labels Dec 31, 2019
throw new JsonException(string.Format(CultureInfo.CurrentCulture, RMResources.JsonUnexpectedToken));
}

using JsonDocument json = JsonDocument.ParseValue(ref reader);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use CosmosElements instead? That way we could support hybrid row or other encoding types in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a System.Text.Json converter, and it works through the System.Text.Json.JsonConverter decorator, plus the input classes (reader) is System.Text.Json, why would I add CosmosElements? I'd argue that CosmosElements actually is very very similar to JsonDocument, and I'm not sure if it should be replaced

Copy link
Contributor

Choose a reason for hiding this comment

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

The agruements I have for switching to CosmosElements are:

  1. It's a clean interface that abstracts away the serializer implementation. This will prevent all the pain if we need to convert to a different serializer in the future.
  2. It supports the different content format like hybrid row.

For the actual implementation for CosmosElements it would probably be best to just have methods on the classes and avoid the JsonConverters altogether removing all dependencies from System.Text.Json.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't CosmosElement depend on Newtonsoft.Json? https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/CosmosElements/CosmosElement.cs, https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/CosmosElements/CosmosElementSerializer.cs

The ask was to actually add the dependnecy of System.Text.Json, if I use CosmosElement instead and remove the dependency, doesn't that go against the ask?

And how does CosmosElement work for write scenarios that require support for different types? Aren't we kind of re-inventing the wheel? I get the reason for CosmosElement in a scenario where we are using Newtonsoft.Json, but since JsonDocument has the same benefit and is maintained by a dedicated team, why would we need to have our own custom thing? Will we evolve at the same speed as a dedicated team? Will we work on performance and produce the same results?

Copy link
Contributor

Choose a reason for hiding this comment

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

CosmosElement does currently depend on Newtonsoft, but it should be pretty easy to replace it with System.Text.Json. It will also need to be converted eventually since Newtonsoft needs to be removed.

It's more like adding an abstraction to JsonDocument. This way it's a clean interface that is easy to swap. They both have the pros and cons. It's just the amount of energy it is taking to convert the current code base makes me wonder if having the abstraction is worth it to avoid this pain in the future.

@kirankumarkolli
Copy link
Member

Thanks alot @ealsur

@ealsur ealsur merged commit d69794a into v4 Jan 7, 2020
@ealsur ealsur deleted the users/ealsur/textjson branch January 7, 2020 19:06
@KrzysztofCwalina
Copy link
Member

@ahsonkhan
@ericstj

@ealsur ealsur mentioned this pull request Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request VNext Next Major version of SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants