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

Add support for JSON.net JsonSerializerSettings #551

Closed
j82w opened this issue Jul 16, 2019 · 14 comments · Fixed by #571
Closed

Add support for JSON.net JsonSerializerSettings #551

j82w opened this issue Jul 16, 2019 · 14 comments · Fixed by #571
Labels
feature-request New feature or request

Comments

@j82w
Copy link
Contributor

j82w commented Jul 16, 2019

To override a single JsonSerializerSettings requires implementing an entire serializer class. There should be a way to override the default serializer settings to avoid forcing users to create a entire new class just for one setting.

  1. Make CosmosSerializerCore public sealed with a constructor to take in the settings.
  2. Add an option to CosmosClientOptions. Might be confusing with the ability to override serializer
@j82w j82w added the feature-request New feature or request label Jul 16, 2019
@nheinbaugh
Copy link

It would also be nice to have a sample showing a simple change to the default serializer.

Without official documentation or a sample I don't even know what I don't know. Having to dig through source code and tests is painful

@j82w
Copy link
Contributor Author

j82w commented Jul 16, 2019

@nheinbaugh thanks for the suggestion. I created an issue to track it #559

@richstokoe
Copy link

Until Microsoft exposes the serializer settings, you could just take the code of the default CosmosJsonSerializer and pass in a JsonSerializerSettings. Example gist: https://gist.github.com/richstokoe/c82fc831c6b4020926f5b5f772f7cd70

@j82w
Copy link
Contributor Author

j82w commented Jul 16, 2019

@richstokoe that is the current plan. That is why this issue was created.

@richstokoe
Copy link

Sure, and I didn't mean to hinder that, just wanted to show that open source means we don't always have to wait for the samples. Keep going, v3 SDK is awesome.

@kirankumarkolli kirankumarkolli changed the title Add support for JsonSerializerSettings on default serializer Add support for JSON.net JsonSerializerSettings on default serializer Jul 17, 2019
@kirankumarkolli kirankumarkolli changed the title Add support for JSON.net JsonSerializerSettings on default serializer Add support for JSON.net JsonSerializerSettings Jul 17, 2019
@j82w
Copy link
Contributor Author

j82w commented Jul 30, 2019

@richstokoe 3.1.0 is now released with the new serializer

@j82w
Copy link
Contributor Author

j82w commented Aug 7, 2019

@richstokoe and @nheinbaugh can you please take a look at this PR #650? We need to remove the CosmosJsonDotNetSerializer. The Azure central SDK team has guideline that we need to move to the new system.text.json, and exposing the JSON settings directly conflicts with that. This PR add supports for some common JSON settings. Does this support all of your scenarios?

@richstokoe
Copy link

@j82w Thanks for the engagement!
I may have missed it in the PR but I’d personally prefer to disambiguate property casing, i.e.
CosmosPropertyNamingPolicy.Default
CosmosPropertyNamingPolicy.UpperCamelCase
CosmosPropertyNamingPolicy.LowerCamelCase
That would solve my original issue (POCO “Id” properties not serialising to “id” without having to add a JsonPropertyAttribute) and would be explicit for consumers.

@j82w
Copy link
Contributor Author

j82w commented Aug 7, 2019

@richstokoe camel case refers to the first character being lower case. Pascal case is where the first character is capitalized. Check out Camel Case vs Pascal Case. I don't see an option in Newtonsoft or system.text.JSON that support pascal case by default. They both would require a custom converter. Do you have any requirements to support pascal case?

@j82w j82w reopened this Aug 7, 2019
@j82w
Copy link
Contributor Author

j82w commented Aug 15, 2019

The final solution with cosmos serialization settings it merged.

@j82w j82w closed this as completed Aug 15, 2019
@adstep
Copy link

adstep commented Sep 12, 2019

Is there support for customer converts with the current setup? I am looking to add a stringtoenumconverter.

@j82w
Copy link
Contributor Author

j82w commented Sep 12, 2019

Hi @adstep ,

To support a custom converter you will need to implement a CosmosSerializer that is set on the CosmosClientOptions. You can take a look @richstokoe implementation to see an example.

@thomaslevesque
Copy link
Contributor

To be honest, I'm not very happy with the current solution. CosmosSerializationOptions only lets us control a very small subset of JSON.NET's JsonSerializerSettings. For instance, I really need to be able to add converters, and the only way to do that now is by reimplementing a complete serializer (which isn't very hard, since I can just copy and modify CosmosJsonDotNetSerializer, but it's not ideal).

I understand that you don't want to explicitly depend on JSON.NET, but since there's a package reference, you depend on it anyway.

I think a good approach would be the following:

  • implement a default serializer using whatever you want (JSON.NET, System.Text.Json or whatever)
  • expose a CosmosJsonDotNetSerializer in a separate NuGet package, so users can have more control over the serialization settings

@bartelink
Copy link
Contributor

@thomaslevesque 's suggestion will also lead to a thriving STJ implementation sitting alongside it in a matter of days, methinks.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants