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

Api proposal: Change JsonSerializerOptions default settings #31094

Open
andre-ss6 opened this issue Oct 7, 2019 · 79 comments
Open

Api proposal: Change JsonSerializerOptions default settings #31094

andre-ss6 opened this issue Oct 7, 2019 · 79 comments
Labels
api-suggestion area-System.Text.Json
Milestone

Comments

@andre-ss6
Copy link

@andre-ss6 andre-ss6 commented Oct 7, 2019

New API:

public static class JsonSerializerOptions
{
    // Can only be set until the first (de)serialization occurs (like other properties here)
    public static Func<JsonSerializerOptions> Default { get; set; }
}

Newtonsoft \ JSON.NET does it this way:

public static class JsonConvert
{
    public static Func<JsonSerializerSettings> DefaultSettings { get; set;}
}

The upside is that you can have different settings for different situations, including different options per assembly, even though that would require the developer be aware of the potential pitfalls of that. The fact that it has also worked for many years in JSON.NET I think is a plus as well.

Performance could suffer a little bit, would have to test. Either way, one could always get over that by explicitly passing their options.


Original text:

Provide a way to change the default settings for the static JsonSerializer class. In Json.NET, you could do:

JsonConvert.DefaultSettings = () => new JsonSerializerSettings();

Currently you have to keep passing the options around and remembering to pass it to each call to [De]Serialize

@ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Oct 7, 2019

Currently you have to keep passing the options around and remembering to pass it to each call to [De]Serialize

Can you expand on why that is a concern for you and what scenario is blocked by the current design (preferably with some sample code)?

Given the JsonSerializer is a static class containing purely static methods, it doesn't contain any state atm. Providing rationale for why this needs to change would help motivate it (other than the fact that Json.NET has this behavior) since it does introduce some complexity in the design.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 8, 2019

Can you expand on why that is a concern for you

JSON properties are conventionally camelCase. .NET type's members, on the other hand, are conventionally PascalCase. I don't really want to stray away from any of these two conventions.

The solutions today, as far as I'm aware, all involve passing things around:

  1. You can annotate every single property in your project with JsonPropertyNameAttribute. This is not nice, convenient or even desirable. [JsonPropertyName("foo")] int Foo { get; set; } adds unnecessary verbosity and feels and looks completely redundant. The only thing changing between the two lines is the first character casing. It is also error prone.
  2. Pass options around everywhere. I think this one speaks for itself. If you're passing options around everywhere, you should consider instead doing it in one place only. This is the path we're following with our project, which I talk more about in the end.
  3. Pass a serializer around everywhere as well. Although this one seems to make some sense at first (oh, you need JSON serialization? Ask the IoC for a serializer!), it does make you wonder: why have a static helper class in the first place then? As far as I'm aware, this requires writing code considerably more involved than simply calling JsonSerializer.Desearilize<T>("..."). So you won't be writing conventional code just because you need different options. And again, error prone.

Now, of course one could argue that the correct way is indeed to have case-sensitivity, since you can always have JSON like { "foo": 1, "Foo": 2}. However, being honest, this is not a common case -- and not only that, but I don't really see people writing this kind of JSON and expecting it to work 100% in the wild. Of course it's on the spec, but realistically it will simply break lots of APIs out there. Finally, who governs the names of accepted properties is the API, thus, our API obviously do not do such things. In fact, creating classes like that would go against the .NET design guidelines.

A better option, perhaps, would be to inform the serializer that by default all properties are to be assumed camelCase (so, they would remain case-sensitive, but by default be assumed JSON camelCase even though being written PascalCase in C#). However, the problem here is the "by default" bit, which again boils down to the same problem exposed in this issue.

All of our projects currently expect this behavior. Changing this wouldn't be a deal breaker for adopting System.Text.Json, but it is surely requiring some unexpected changes. The first change we did was to make our REST internal library now accept a JsonSerializerOptions in its constructor so that it can be set during DI registration. However, there are a considerable number of other places in the project where we use JSON serialization for other purposes. For those other scenarios, the only idea we currently have is to pass options around.

If there was an instance, non-static version of JsonSerializer we could also make extension methods. This unfortunately still wouldn't change the behavior of the entire running program, but would at least change the behavior for our assembly.

@Symbai
Copy link

@Symbai Symbai commented Oct 9, 2019

Why don't you declare the default setting in a static class and then just do

static class DefaultJsonSettings {
	public static readonly JsonSerializerSettings Settings = new JsonSerializerSettings {
		NullValueHandling = NullValueHandling.Ignore,
		// [...]
	};
}

 JsonConvert.SerializeObject(myObject, DefaultJsonSettings.Settings);

That's what I did to avoid passing settings around but making sure it's getting serialized the same everywhere in project. If your wish is to set the settings ONCE at start you already know how the settings should be so static is fine. Otherwise if you load the settings, so they are unknown before application start, it cannot be static itself but you could do the same but with a static reference to an instance:

public sealed class DefaultJsonSettings  {
	static DefaultJsonSettings instance;

	public static DefaultJsonSettings Instance => instance ??= new DefaultJsonSettings();

	public DefaultJsonSettings() {
		//Load JSON settings here
		Settings = new JsonSerializerSettings {
			NullValueHandling = NullValueHandling.Ignore,
			// [...]
		};
	}

	public static JsonSerializerSettings Settings;
}
 JsonConvert.SerializeObject(myObject, DefaultJsonSettings.Instance.Settings);

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 9, 2019

Sure, but that still kind of have the same issues as passing it around everywhere, except they're not a method parameter.

You still have to remember to do it, and thus it's still error prone, and it still feels quite redundant to be passing the "default" options to every single method call. If it's supposed to be the default, why do I need to specify it every single time? It's like having C# optional parameters, but still having to specify them in every call.

@Symbai
Copy link

@Symbai Symbai commented Oct 9, 2019

why do I need to specify it every single time?

Because of what ahsonkhan said. Its a static class with static methods for performance reasons. You want add performance decreases by making the method not static only to avoid remembering to add "DefaultJsonSettings.Settings" every time you use it. This seems like a really bad trade, especially for the great majority where the current way it works isn't a big deal.

If this is such a big deal for you, why don't you just write DefaultJsonSettings.Settings once and put that inside a static method and call this instead, problem solved.

static class Bla {
	public string SerializeObject(object myObject)
	{
		return JsonConvert.SerializeObject(myObject, DefaultJsonSettings.Settings);
	}
}

Bla.SerializeObject(myObject);

And please do not say you don't want this because you have to remember using Bla.SerializeObject instead of JsonConvert.SerializeObject

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 9, 2019

And please do not say you don't want this because you have to remember using Bla.SerializeObject instead of JsonConvert.SerializeObject

How so "please do not say"? Am I not allowed to express my opinion? By saying so, you show that you do understand the issues I'm raising and you do understand that they're present in the ""solution"" you suggest. That's dishonest. Remember that different people have different needs, works in different projects and under different scenarios. Your opinion is not golden. It's just as valid as mine. If my suggestion doesn't prove to be much useful, don't worry, it simply won't get traction and won't get implemented. Don't think there's a need for you to harass anyone.

You want add performance decreases

I want no such thing. Never said it. I'm merely asking for a way to have default settings. The implementation is up for discussion.

only to avoid remembering to add "DefaultJsonSettings.Settings" every time you use it

It's not "only" to avoid remembering. When you argue like that, you can make any argument look pointless. As I said in previous replies, this is simply a point of great coding practices. Anyone who owns a codebase and see a thing such as having a method being called all around the solution every time with the same argument would do something about it. And again, this is because it is simply a point of great coding practices. It's easy to see the point.

This seems like a really bad trade

Only if it has to be that way. I too agree that if a non-insignificant performance impact is required in order to do this, it wouldn't pay itself, and I wouldn't support it.


@ahsonkhan Can't we just create a new overload that doesn't have the JsonSerializerSettings optional parameter and that just redirects the call, passing the DefaultSettings as parameter? Sure, that would introduce state in the class, but I don't understand why is that a problem per se. And AFAIK that wouldn't be a breaking change either if the DefaultSettings property is default initialized with null. I'm sorry if there's other complexity involved that I'm not aware, I haven't looked at the code yet for that class.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 9, 2019

Just as a quick note, I think it's also worth remembering that another benefit of having a static property is that you're able to change the serializer behavior for all assemblies in the running program. I don't depend on this behavior currently though, so I can't say how much of a benefit that would be. I guess it could be useful in scenarios where you have code over which you have no control that uses JsonSerializer internally and doesn't offer an way to change the default options.

@FiniteReality
Copy link

@FiniteReality FiniteReality commented Oct 10, 2019

I really think this is a huge trade-off. It can be a huge pit-of-failure in libraries which interact with web APIs which expect a specific casing. Of course, they should be written in a way which is not vulnerable to this, but by making it the "default", it makes it a lot easier for bad code to be written.

Personally, I don't really think specifying JsonPropertyName for every property name is a negative - in fact, I feel that explicitly specifying property names is advantageous, even if it can seem like code duplication.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 10, 2019

Of course, they should be written in a way which is not vulnerable to this, but by making it the "default", it makes it a lot easier for bad code to be written.

I understand your point. However, I'm not sure if you misunderstood me or if I'm misunderstanding you 😄 but I'm not advocating for that behavior to be the default; instead, only to give users the option to have a default, whatever that might be.

Having said that, please note then that 1) you're focusing on case-sensitivity, when that was merely the reasoning behind my need for this. This issue is not about case sensitivity, it is about offering a way to change the defaults only once. No one is asking here for case-insensitive to be the default behavior anywhere and of anything; and 2) your point boils down to "giving developers a choice means they will write bad code and thus let's not give them a choice". And although I can see an argument like that being applied elsewhere, I don't really think it applies here.

I'm still going to reply to each one of your points, but I think this (whether or not case-sensitivity should be the default) is mostly something not worth arguing over, as it is not the point of this issue.


Please note that this has existed in JSON.NET for many many years and I don't think a lot of "bad" code has been written because of it. In fact, in JSON.NET, that behavior (case-insensitive) is the default behavior (but again, I am not saying it should be, I'm merely bringing that fact to light).

It can be a huge pit-of-failure in libraries which interact with web APIs which expect a specific casing

First of all, notice that I'm more interested in the deserialization behavior than the serialization one. I don't really care how the payload is going to be serialized.

As said above, 1) I am not advocating for this to be the default behavior; and 2) this has existed for years and I don't really see this ever being a problem. And, in fact, as I said before, I actually think it's the other way around: .NET developers are more familiar with cC -> CC because of the clash between the .NET naming guidelines and JS naming conventions. And that's not really a problem at all, since, as mentioned before, people simply don't go around there sending "foo: 1, "Foo": 2 JSON payloads.

Also, please note that this is the default behavior for MVC (and still continues to be, even after System.Text.Json: dotnet/aspnetcore#10723) and, again, people are not writing "bad" code because of it. And, most importantly, MVC was engineered in a way where you can, in fact, change the default only once.

I really think this is a huge trade-off.

So I don't think there is really any trade-off here (in the context of what you said -- note that I do understand that there may be some trade-offs in terms of performance and/or API design!).

By having the option to specify the defaults in System.Text.Json, not only it would give us more flexibility, since - and this is important to note - there are other behaviors besides case-sensitivity that can be controlled through the options, but it would also pave the way for a more seamless migration between the two libraries.

Personally, I don't really think specifying JsonPropertyName for every property name is a negative - in fact, I feel that explicitly specifying property names is advantageous, even if it can seem like code duplication.

Right. I'm a big advocate of the idea of being explicit. However:

  1. I do not like the idea of specifying behavior in that way. One thing is having a property with a name that is not a valid C# identifier and having to enable that scenario through the use of an attribute. Another thing entirely is peppering your entire code with JsonPropertyNameAttributes everywhere, specifying through the use of a string that the behavior you expect is "transformation" of camelCase to PascalCase. The behavior is encoded not in code, but in the developer's mind. [JsonPropertyName("foo")] string Foo { get; set; } doesn't really say what you're meaning. A future developer entering your project might not notice at first hand that that is a behavior expected by the entire project, instead of being just a few attributes here and there. On the other hand, something like Options.CaseInsensitive = true is explicit and conveys all the meaning needed. By only looking at a single line such as that, a future developer can understand how your project deals with JSON.
  2. It is brittle. You can't use nameof. You can't specify behavior, as said above (like something as [JsonProperty(JsonSerializationOptions.CaseInsensitive)] would). And logic is smeared all over your code, instead of of one place. Which leads me again to the obvious next point, also addressing one of yours;
  3. It is duplication. It not only looks like it. It is behavior what you're describing when writing that attribute and you're specifying it literally everywhere. Again, even though I do love the idea of being explicit, I just can't feel that the advantages there cover the disadvantages of having to use attributes everywhere, especially in a situation like this, with all these other mentioned disadvantages. We've seen this before many many times in the BCL and other MS frameworks (cough DataMemberAttribute cough). It is not by chance that they strayed away from this philosophy.

@steveharter
Copy link
Member

@steveharter steveharter commented Oct 10, 2019

The feature ask is valid IMO and was discussed early on when designing JsonSerializerOptions.

Currently the "default" option is on a private static variable. This means it can only contain default settings.

Also, FWIW, we also discussed add assembly-level attributes that can configure things like the naming policy. This would support some of the scenarios without having to specify where the default option lives.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Oct 17, 2019

((JsonSerializerOptions)typeof(JsonSerializerOptions)
                .GetField("s_defaultOptions", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic).GetValue(null))
            .PropertyNameCaseInsensitive = true;

😈😈😈

@Alexei-B
Copy link

@Alexei-B Alexei-B commented Nov 20, 2019

The solution @andre-ss6 came up with above using the internal default settings demonstrates that this is a real issue.
Can we get some movement on this? It's a pain to have to deal with libraries using System.Text.Json that don't let me configure these options.

@AndrewZenith

This comment has been minimized.

@francescocristallo

This comment has been minimized.

@ahsonkhan

This comment has been minimized.

@jdaaboul
Copy link

@jdaaboul jdaaboul commented Dec 4, 2019

It should be a given that .net let us set default settings for the serializer, having to repeat the options everywhere or use wrapper classes is too error prone and will cost more than slight performance regressions. Newtonsoft it is until this is fixed.

@AndrewZenith
Copy link

@AndrewZenith AndrewZenith commented Dec 4, 2019

@AndrewZenith, @francescocristallo - the request to allow setting "PascalCase" naming policy in the options is separate from the discussion here in this issue, which is discussing adding the capability to enable setting the json serializer options globally, once.

Can you please file a separate issue for this (in the dotnet/runtime repo - dotnet/corefx#42733), including details on your specific requirement and why the current behavior doesn't work.

The default behavior of System.Text.Json is that the .NET POCO properties are serialized using the exact property name. Similarly, deserialization requires an exact string match within the JSON payload (casing and format). In .NET/C#, since your object graph generally contains pascal cased properties, and the JSON being serialized is pascal-cased as well. Alternatively, you could apply the custom JsonPropertyName attribute to them.
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.propertynamingpolicy?view=netcore-3.0

We added camel case supported in 3.0 and are considering adding snake case support for 5.0: #782

I am going to mark this conversation as off-topic.

But setting PascalCase ONCE in the STATIC json serializer options is what I and everyone else moving from MVC5 to Core 2.2 then onto 3.0 will want to do. Because we've already written the code like that (PascalCase in C#, camelCase in Javascript), have an awfully large number of classes it affects and have no intention of going round and applying another attribute to everything and hoping we got them all. It's EXACTLY what the op is asking for, so very on topic.

I also just want to point out to those coming here that there is an option, which is NewtonSoft JSON can be added back in as middleware and be set to work exactly as it used to, so we can move to .Net Core 3.0 regardless of this substantial oversight by kicking out this implementation of a Json Serializer. Don't get me wrong, I'd like to use it, but without what the op has asked for - very unlikely.

@dquist
Copy link

@dquist dquist commented Dec 9, 2019

Ran into this issue when trying to create an asp.net web API with matching .net core console client.

The default behavior of the serializer is to use camelCase, but the default deserialization option is PascalCase. Had to google around as to why .net core couldn't deserialize an object I just serialized using the same model 🤔.

Passing in the same options value across the entire application seems like a huge violation of DRY and an easy way for bugs to creep in.

@ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Dec 9, 2019

Ran into this issue when trying to create an asp.net web API with matching .net core console client.

The default behavior of the serializer is to use camelCase, but the default deserialization option is PascalCase. Had to google around as to why .net core couldn't deserialize an object I just serialized using the same model 🤔.

The default behavior of System.Text.Json.JsonSerializer is to match the same name/casing as the .NET property name (the names in the .NET POCO) for both serialize and deserialize:

https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.propertynamingpolicy?view=netcore-3.0

Within aspnet however (which includes Mvc, SignalR, Web Api), the JsonSerializer is configured with camel casing by default, along with case insensitive matching: https://github.com/aspnet/AspNetCore/blob/a6bc6ce23dad5a9d02596cf2e91e3c4f965c61bc/src/Mvc/Mvc.Core/src/JsonOptions.cs#L28

https://github.com/aspnet/AspNetCore/blob/ec8304ae85d5a94cf3cd5efc5f89b986bc4eafd2/src/SignalR/common/Protocols.Json/src/Protocol/JsonHubProtocol.cs#L747-L763

That is likely why you are seeing the observing the discrepancy. Aspnet configures the defaults differently.

If your .NET object graph happens to contain PascalCase properties (which it most likely does so), then yes, you would need to configure your JsonSeriaizerOptions to be consistent across your asp.net web API and the .net core console client, because the defaults in both contexts are different.

You would have to override the options in either in your web api or in your console client to make sure they are consistent (you probably want your console clients json serializer options to be configured to match the web api serializer options). Would having a globally configurable serializer help in your scenario? If the two contexts are in separate assemblies, you would have to configure both anyway, no?

@jtreher
Copy link

@jtreher jtreher commented Dec 10, 2019

I just wanted to add a comment here that adding case-insensitivity appears to increase memory consumption and degrade performance on very large payloads. Since there does not appear to be a way to turn off case sensitivity with Newtonsoft deserialization, I am unable to compare to that with it disabled.

My memory as reported by BenchmarkDotNet was 2.5MB deserializing this massive stream and case insensitive enabled it went to 5.5MB. Performance was 20ms without and 26ms with.

For comparison, Newtonsoft is currently taking 32ms to deserialize the stream and consumes 3.5mb.

Default buffer size is set to 100KB.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Dec 10, 2019

@jtreher can you share the code for that benchmark?

@jtreher
Copy link

@jtreher jtreher commented Dec 11, 2019

@andre-ss6 My payload has a lot of data in it that would need scrubbed. The benchmark is pretty simple. You can either set the case insensitive in the benchmark method or just flip the flag and run the benchmarks again and you'll see the decline in performance insensitive vs. sensitive. My "response.json" is 1.5 MB for reference -- 100 very large and several nested levels deep objects.

    [MemoryDiagnoser]
    public class Test
    {
        private static readonly utf8json.JsonSerializerOptions _jsonOptions = new utf8json.JsonSerializerOptions
        {
            IgnoreNullValues = true,
            PropertyNameCaseInsensitive = true,
            DefaultBufferSize = 100_000,
        };

        private static string Contents;

        public Test()
        {
            using StreamReader sr = new StreamReader("C:/temp/response.json");
            Contents = sr.ReadToEnd();
        }

        [Benchmark]
        public async ValueTask<IReadOnlyCollection<ProductResponse>> SystemTextJsonDeserializeStream()
        {
            using StreamReader sr = new StreamReader("C:/temp/response.json");
            return await utf8json.JsonSerializer.DeserializeAsync<IReadOnlyCollection<ProductResponse>>(sr.BaseStream, _jsonOptions);
        }

        [Benchmark]
        public IReadOnlyCollection<ProductResponse> SystemTextJsonDeserializeObject()
        {
            return utf8json.JsonSerializer.Deserialize<IReadOnlyCollection<ProductResponse>>(Contents, _jsonOptions);
        }
}

@tdreid
Copy link

@tdreid tdreid commented Dec 17, 2019

Sincere question — how much design complexity would be introduced from this one particular proposed divergence from a static class containing purely static methods? I acknowledge there would be trade offs. I'd like to understand their magnitude.

On a separate note, a few different ways to approach setting the defaults in one place have been suggested on the related Stack Overflow question.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Dec 17, 2019

As I understand it, the tension is more on the design side than performance considerations. I think they're not sure yet if they want to expose a static property.

@tdreid
Copy link

@tdreid tdreid commented Dec 18, 2019

Thanks @andre-ss6. I had misread @ahsonkhan's comment at the head of the thread. I've updated my inquiry to reflect the more pertinent trade off.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Dec 18, 2019

Yea, it would be nice if they share that info with us. Simply saying there is complexity involved but then leaving us in the dark is not helpful.

I would hope we could help in the discussion. Simply requesting a feature and waiting in the dark feels like UserVoice.

@andre-ss6
Copy link
Author

@andre-ss6 andre-ss6 commented Dec 18, 2019

Alright, taking a stab at it.

Static defaults property (publicly expose today's field):

  • Will change the default behavior for every single loaded assembly unconditionally 👎;
    • However, library authors who need specific behaviors should know that they are not to trust in a public static property, and thus pass their own defaults every time to every method 💭.

Static generator property (JSON.NET):

  • Can change the defaults for every single loaded assembly 👍;
    • Although this would be technically supported, it still may suffer from the same issues as the first option above 👎.
  • Will incur an indirect method call for every [De]serialize call that don't specify defaults, thus affecting performance 👎.
    • We could cache the value as long as the property setter is never called, but complexity quickly arises when you start thinking about thread safety. Maybe JNK has already solved this in his library? 💭

Assembly attribute:

  • The worst option? Unclear and low discoverability (if that's a word); 👎
  • May or may not change behavior for all loaded assemblies depending ok implementation;
  • May or may not badly affect performance depending on implementation;
  • Needs design;

New C# 8.1 feature where you tell the compiler that you want a optional parameter to always have the same value:

  • Would probably get more dev team attention than this issue. 👍

@ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Dec 18, 2019

Yea, it would be nice if they share that info with us. Simply saying there is complexity involved but then leaving us in the dark is not helpful.

I would hope we could help in the discussion. Simply requesting a feature and waiting in the dark feels like UserVoice.

Apologies for that. I had been working through other JSON issues, since the backlog of feature requests is relatively large (and as you probably know, context switching is difficult). I will try to be more responsive here.

Can you update the OP with the exact API shape/change you'd like to propose that will address your concern and then we can discuss it. Once ready-for-review, we can take it to API review in the new year.
https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md
Here's a good example of an API proposal: https://github.com/dotnet/corefx/issues/271

Having the caller override the default JsonSerializerOptions assembly-wide (or I guess, it would be application wide - like you said for every single loaded assembly unconditionally), may be relatively easy to achieve on paper, but it would have ramifications across the entire stack (at the very least, it raises questions that need to be thought through). That's partially what I meant with introduction of complexity. Now, the previous invariants, that depend on the default options not changing, would have to be reconsidered.

I have a few questions that are worth looking into as part of the spec/requirements:

  1. What is the behavior when someone creates a "global" default but then wants to override that?
    • Override through locally defined options
    • Override through attributes/converters/etc.
  2. Currently, the JsonSerializerOptions are immutable once serialization has started. Would this change affect that?
  3. Would this affect converters in any way? I am assuming it wouldn't.

Can't we just create a new overload that doesn't have the JsonSerializerSettings optional parameter and that just redirects the call, passing the DefaultSettings as parameter?

  1. Overloads on what? The existing Serialize/Deserialize methods? What would these API additions look like and could they cause overload resolution to break in some cases?

There is clearly a vocal interest here. Would you (@andre-ss6 ) like to take a shot at an implementation as part of the design, especially since you have already put a lot of thought into this? While we wait for the API to get reviewed, I would be happy to take a look at a draft/WIP PR which helps showcases the feature in action along with tests to help design this. I would only do this after we have consensus on the this thread around the api proposal and it is marked as api-ready-for-review.

Moving this from future to 5.0.

@ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Dec 21, 2019

I just wanted to add a comment here that adding case-insensitivity appears to increase memory consumption and degrade performance on very large payloads.

@jtreher, were you running on 3.0 or 3.1? Can you try running them again on 5.0 and share your results, especially the difference in memory allocation? Also, is the sample model/payload representative of what you actually use and see in practice?

I understand you can't share the payload, but can you share your ProductResponse model (possibly sanitized if needed)? The shape/types would help narrow down why you are seeing such a difference. Does the delta only occur during deserialization (or does it happen during serialization as well)? A dummy/"auto-generated" payload that fits the schema would also help if you can create one, but as long as I have the model, I can do that myself.

My memory as reported by BenchmarkDotNet was 2.5MB deserializing this massive stream and case insensitive enabled it went to 5.5MB. Performance was 20ms without and 26ms with.

I am asking because I tried deserializing a 150 MB payload from NuGet into a SearchResult model (which isn't too deep), and though I do see a similar throughput difference as you when opting into ``PropertyNameCaseInsensitive`, I couldn't see the 2x+ difference in allocations like yours (2.5 MB to 5.5 MB). I saw a ~25% increase.
Something like the following:
https://github.com/NuGet/NuGet.Services.Metadata/blob/1a9a77014f4c477b222844c60ff549b93b6d5b4c/src/NuGet.Services.AzureSearch/SearchService/Models/V3SearchPackage.cs#L12

The perf difference is expected given how the caching of property names works currently. We have certain optimizations that work when we know exact comparisons are being done, which don't work for case insensitive comparisons (so we have more calls to ToArray on the property names). However, we can try to reduce some of that overhead to bridge the gap a bit.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@IanKemp

This comment has been minimized.

@krwq
Copy link
Member

@krwq krwq commented Oct 25, 2021

I'd consider two angles here:

  • caching JsonSerializerOptions is recommended scenario because of the perf which would make Func<JsonSerializerOptions> an anti-pattern
  • scenario is useful for apps which have specific set of serialization rules and want to make it easier to enforce these settings

given those two above I'd suggest:

static void SetDefaultOptions(JsonSerializerOptions);

which would clone the object, that should have benefit of both worlds.

Libraries should not use this API ever, it's a setting for end users

@IanKemp
Copy link

@IanKemp IanKemp commented Oct 25, 2021

Libraries should not use this API ever, it's a setting for end users

Good luck with that.

(Are y'all going to mark this one as "disruptive content" too because it disagrees with your hivemind?)

@krwq
Copy link
Member

@krwq krwq commented Oct 25, 2021

[Triage] One option to have the compromise here would be to create an assembly attribute or other solution which specifies default options per assembly. We'd need to see specific API proposal and prototype for such attribute (or else) and discuss implementation drawbacks and advantages.

ref: #31094 (comment)

@steveharter
Copy link
Member

@steveharter steveharter commented Oct 25, 2021

Another approach is to leverage the recent JsonSerializerContext source-gen feature that was added for V6. That would enable you to define a set of known types that have the same options. The actual Serialize() and Deserialize() methods could also be manually added to the context for ease-of-use as well as a prescribed API for the current application that avoids potential bugs where the "options" parameter was accidently omitted.

Example that works in V6 (could improve in V7)
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;

MyContext context = MyContext.Default; // Create a context variable to pass around

Person person = new Person { FirstName = "Steve" };

string json = context.Serialize(person);
Console.WriteLine(json); // Camel-cased output:{\"firstName\":\"Steve\"}

// Slightly better perf that avoids type lookup:
json = context.Serialize(person, context.Person);
Console.WriteLine(json);

[JsonSerializable(typeof(Person))]
[JsonSourceGenerationOptions(
    GenerationMode = JsonSourceGenerationMode.Metadata, // Don't need to generate Serialize methods
    PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase // Example of a feature example in the context
)]
internal partial class MyContext : JsonSerializerContext
{
    // Add some helper methods directly to MyContext to make the serializer easier to invoke.
    // We may only want to expose the best practices here for the given application, and we can
    // wrap the various calls here as well to provide additional validation or other logic.
    public string Serialize<T>(T obj, JsonTypeInfo<T> typeInfo) => JsonSerializer.Serialize(obj, typeInfo);
    public string Serialize<T>(T obj) => JsonSerializer.Serialize(obj, (JsonTypeInfo<T>)GetTypeInfo(typeof(T)));

    public T? Deserialize<T>(string json, JsonTypeInfo<T> typeInfo) => JsonSerializer.Deserialize(json, typeInfo);
    public T? Deserialize<T>(string json) => JsonSerializer.Deserialize(json, (JsonTypeInfo<T>)GetTypeInfo(typeof(T)));
}

public class Person
{
    public string? FirstName { get; set; }
}

@PieterjanDeClippel
Copy link

@PieterjanDeClippel PieterjanDeClippel commented Nov 13, 2021

I created a new Blazor Server application using .NET 6, and when I'm sending an HTTP request to retrieve data in my Client application, I get the following response:

[
  {
    "date": "2021-11-14T11:04:55.7007215+01:00",
    "temperatureC": 27,
    "summary": "Chilly",
    "temperatureF": 80
  },
  {
    "date": "2021-11-15T11:04:55.7009955+01:00",
    "temperatureC": 11,
    "summary": "Cool",
    "temperatureF": 51
  },
  {
    "date": "2021-11-16T11:04:55.7009985+01:00",
    "temperatureC": 19,
    "summary": "Scorching",
    "temperatureF": 66
  },
  {
    "date": "2021-11-17T11:04:55.7009988+01:00",
    "temperatureC": 29,
    "summary": "Bracing",
    "temperatureF": 84
  },
  {
    "date": "2021-11-18T11:04:55.700999+01:00",
    "temperatureC": 27,
    "summary": "Freezing",
    "temperatureF": 80
  }
]

However, when deserializing this response using the following code in my Blazor.Client app

var result = await System.Text.Json.JsonSerializer.DeserializeAsync<WeatherForecast[]>(content);

The fields are empty since System.Text.Json.JsonSerializer expects the properties to start with a capital:

[
  {
    "Date": "2021-11-18T11:04:55.700999+01:00",
    "TemperatureC": 27,
    "Summary": "Freezing",
    "TemperatureF": 80
  }
]

@Clockwork-Muse
Copy link
Contributor

@Clockwork-Muse Clockwork-Muse commented Nov 13, 2021

@PieterjanDeClippel - You need to use case-insensitive deserialization.

@Xyncgas
Copy link

@Xyncgas Xyncgas commented Dec 3, 2021

I would like this too, sometimes I am changing my old code instead of new code, to have a new behavior, then instead of going around making sure I did the right thing that I've passed in the correct Serializer option every where, I want to have a way to set it project wide, for all the System.text.json Serializations to use my enforced serializer option, instead of doing this myself which is possible, plus I think it is too late at this point when I am working on a project already written, I would like it to be possible.

@SuperMeip
Copy link

@SuperMeip SuperMeip commented Dec 6, 2021

I need this at the moment lol. It would be nice to be able to add default settings.

@iocio005
Copy link

@iocio005 iocio005 commented Dec 17, 2021

I cannot understand why this has not done yet. Adding the possibility of set a default global behaviour of a static class in the serializer have a lot of sense when you are using .net mvc.

@0xced
Copy link
Contributor

@0xced 0xced commented Dec 17, 2021

I'm really surprised that so many people are asking for a public static property in a JSON library. This is shared mutable state and the industry has pretty much settled that shared mutable state is evil and the source of so many bugs.

Last year, I submitted a pull request to a project that was altering Newtonsoft.Json JsonConvert.DefaultSettings. The pull request was not merged because the maintainer switched to System.Text.Json where this problem simply does not exist!

@iocio005
Copy link

@iocio005 iocio005 commented Dec 17, 2021

@ericsampson
Copy link

@ericsampson ericsampson commented Dec 17, 2021

@0xced FWIW, if you notice the original proposal, the options become immutable once the first serialization has occurred.

@ericsampson
Copy link

@ericsampson ericsampson commented Dec 17, 2021

@krwq and @steveharter, being able to put an annotation on a class could be useful to some people, but it would be really nice to be able to set the assembly level.
This would avoid the unfortunate issue in Newtonsoft where a poorly-behaved library (perhaps internally developed at a company by someone that doesn't know better) changes the default options out from under your application 😓. While still achieving the goals that people have requested in the thread. It's effectively a solved problem for MVC apps and that works well, but that leaves a lot of other app/service types out.

Are we at the point where someone needs to write up an API proposal to move things forward?

@krwq
Copy link
Member

@krwq krwq commented Dec 21, 2021

Next steps here are to design specific APIs (and if proof of concept is needed create a prototype). Any volunteers? We're currently still in a bug fixing/planning stage so we're not sure if we'll have time to pick this up in 7.0 without some help.

ref:
#31094 (comment)
#31094 (comment)

@Xyncgas
Copy link

@Xyncgas Xyncgas commented Dec 22, 2021

I'm really surprised that so many people are asking for a public static property in a JSON library. This is shared mutable state and the industry has pretty much settled that shared mutable state is evil and the source of so many bugs.

Last year, I submitted a pull request to a project that was altering Newtonsoft.Json JsonConvert.DefaultSettings. The pull request was not merged because the maintainer switched to System.Text.Json where this problem simply does not exist!

No one has agree anything, nothing has to be one way, don't commit to the path that has been there unless is necessary, by that it usually means solid logical constrains, if there isn't one then possibility shouldn't be limited.

Are you so smart to guaranteed it's a bad thing 5 years later, don't limit yourself until others proves you wrong since it's none lethal just like we as a society is making progress having multiple gender identities not instead because of we are biological different.

@rf-0
Copy link

@rf-0 rf-0 commented Mar 9, 2022

((JsonSerializerOptions)typeof(JsonSerializerOptions)
                .GetField("s_defaultOptions", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic).GetValue(null))
            .PropertyNameCaseInsensitive = true;

😈😈😈

😁I would be careful with this implementation though. In the case that a serialization already occurred, you're likely to get an exception.

@nguyenlamlll
Copy link

@nguyenlamlll nguyenlamlll commented Mar 24, 2022

@0xced Hey Mr-Know-It-All. Pardon me. :) Your use case is different. And read the author's original intention again, please. Implementation is open for discussion.
We don't want explicitly a public static property in a JSON library. We want to be able to set the default setting once in our projects.
My use case is different than yours. I'm not building a package to be consumed by others. I'm just building websites that make lots of requests to my APIs. Passing the default setting every single time? Not the brighest idea.

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Mar 24, 2022

((JsonSerializerOptions)typeof(JsonSerializerOptions)
                .GetField("s_defaultOptions", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic).GetValue(null))
            .PropertyNameCaseInsensitive = true;

😈😈😈

😁I would be careful with this implementation though. In the case that a serialization already occurred, you're likely to get an exception.

FWIW that particular snippet is going to break in .NET 7, since the default instance has been refactored to an auto property.

@udlose
Copy link

@udlose udlose commented Apr 6, 2022

@nguyenlamlll
If you are using a Microsoft.NET.Sdk.Web project, you can use AddJsonOptions in Startup/Program to customize your JsonSerializerOptions that .NET will use instead of having to create a static property - https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.mvcjsonmvcbuilderextensions.addjsonoptions

@jeroenheijmans
Copy link

@jeroenheijmans jeroenheijmans commented May 8, 2022

Want to add that this is especially a concern for me when writing WebApplicationFactory based integration tests (or related unit tests). I was looking for a way to make xUnit globally set the same "Web" defaults for deserializing data. A shame that it's not possible, and a real toss-up for me between the Reflection hack above or "having to remember" using my own convenience deserialization methods in all tests.

Being able to set a more sane default for PropertyNameCaseInsensitive globally should outweigh the desire to keep all static things not mutable.

But perhaps there's greater concerns that block making this issue from being resolved?


EDIT: Right now I contemplate doing something like this to support my xUnit API Integration tests:

private static readonly JsonSerializerOptions jsonSerializerOptions
    = new JsonSerializerOptions(JsonSerializerDefaults.Web);

static HttpClientExtensions()
{
    jsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
}

to use it along these lines:

public static async Task<T> GetAsync<T>(this HttpClient client, string url)
{
    return await client
        .GetAsync(url)
        .ReadAsDeserializedData<T>();
}

private static async Task<T> ReadAsDeserializedData<T>(this Task<HttpResponseMessage> responseTask)
{
    var response = await responseTask;
    response.EnsureSuccessStatusCode();
    var json = await response.Content.ReadAsStringAsync();
    var data = JsonSerializer.Deserialize<T>(json, jsonSerializerOptions);
    if (data == null) throw new Exception("Unexpectedly encountered null response body.");
    return data;
}

😢

@InsomniumBR
Copy link

@InsomniumBR InsomniumBR commented May 9, 2022

I am using a SignalROutput in an Azure Function, and that gets me to the same problem here, I cannot change the default behavior since the property is not exposed and the Output trigger is the one using the Serializing method, so I cannot send my settings either.

image

@krwq
Copy link
Member

@krwq krwq commented May 9, 2022

@InsomniumBR
Copy link

@InsomniumBR InsomniumBR commented May 9, 2022

@krwq in fact none worked. I thought my problem was in the System.Text.Json serialization. And that line of code worked in azure function constructor or inside the function method to make the JsonSerializer.Serialize works as I expected (using the converter). But didn't solve my problem anyway. I need to dig more here to understand with this SignalROutput is not using these settings, or if I am missing something:

image

@schoder-moreno
Copy link

@schoder-moreno schoder-moreno commented Jun 21, 2022

@steveharter @krwq I'm willing to help move this issue forward with an API design and a prototype as I have a need for this functionality in my own projects.

How were you envisioning the solution with assembly level attributes working?
One similar idea I had was to change JsonSerializerOptions.Default from

public static JsonSerializerOptions Default { get; } = CreateDefaultImmutableInstance();

to

        public static JsonSerializerOptions Default
        {
            set
            {
                string callingAssembly = Assembly.GetCallingAssembly().GetName().FullName;
                if (_defaultOptions.ContainsKey(callingAssembly))
                {
                    ThrowHelper.ThrowInvalidOperationException_SerializerOptionsImmutable(context: null);
                }

                value.InitializeCachingContext();
                _defaultOptions.Add(callingAssembly, value);
            }
        }

        internal static JsonSerializerOptions GetDefaultOptions(string callingAssembly)
        {
            return _defaultOptions.TryGetValue(callingAssembly, out JsonSerializerOptions? value) ? value : _defaultOption;
        }

        private static Dictionary<string, JsonSerializerOptions> _defaultOptions = new Dictionary<string, JsonSerializerOptions>();
        private static JsonSerializerOptions _defaultOption = CreateDefaultImmutableInstance();

Then the options could vary depending on which assembly is calling into System.Json.Text:

        public static TValue? Deserialize<TValue>([StringSyntax(StringSyntaxAttribute.Json)] string json, JsonSerializerOptions? options = null)
        {
            if (json is null)
            {
                ThrowHelper.ThrowArgumentNullException(nameof(json));
            }
            // Insert this in every public api method that takes a `JsonSerializerOptions?` input parameter
            options ??= JsonSerializerOptions.GetDefaultOptions(Assembly.GetCallingAssembly().GetName().FullName);

            JsonTypeInfo jsonTypeInfo = GetTypeInfo(options, typeof(TValue));
            return ReadFromSpan<TValue>(json.AsSpan(), jsonTypeInfo);
        }

Some quick benchmarking seemed to indicate that the performance hit from the third and any subsequent call to Assembly.GetCallingAssembly() was on par with adding another if statement. Not sure what your stance is on using reflection calls such as this throughout the apis to the extent that I am suggesting?

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jun 22, 2022

Then the options could vary depending on which assembly is calling into System.Json.Text

I think that might be worse than having a global setter. It absolutely has the potential of catching users by surprise.

@schoder-moreno
Copy link

@schoder-moreno schoder-moreno commented Jun 22, 2022

@eiriktsarpalis I totally get that and agree to some extent. I guess I am a little confused on what the sentiment is. Are we at a point where you are willing to accept a solution with a public static default options (mutable or immutable), or should the solution exhibit other characteristics/behavior, and if so which?

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jun 22, 2022

.NET 7 will expose an immutable default instance (via #61093). For reasons already stated in this thread, I don't believe we would ever consider making it settable.

One possible alternative might be supporting "JsonSerializer" instances: an object encapsulating JsonSerializerOptions and exposing instance method equivalents to the existing static API. That might be one way of avoiding the common pitfalls of forgetting to pass the right JsonSerializerOptions instance or creating a new options instance on each serialization op.

@schoder-moreno
Copy link

@schoder-moreno schoder-moreno commented Jun 24, 2022

Then I will reiterate @IanKemp's suggestion (#31094 (comment)):

public interface IJsonSerializer
{
    Task<T> DeserializeAsync<T>(Stream utf8Json, JsonSerializerOptions options, CancellationToken cancellationToken);

    // other methods elided for brevity
}

public class DefaultJsonSerializer : IJsonSerializer
{
    private JsonSerializerOptions _defaultOptions;

    public DefaultJsonSerializer(IOptions<JsonSerializerOptions> defaultOptions)
    {
        _defaultOptions = defaultOptions.Value;
    }

    public async Task<T> DeserializeAsync<T>(Stream utf8Json, JsonSerializerOptions options = default, CancellationToken cancellationToken = default)
        => await JsonSerializer.DeserializeAsync<T>(utf8Json, options ?? _defaultOptions, cancellationToken);
}

Is this closer to something you will consider @eiriktsarpalis?

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jun 25, 2022

Something like that, although probably without the interface and IOptions dependency. I also think that the instance methods should not accept an options parameter to emphasize encapsulation.

At risk of stating the obvious, such a class can easily be defined by users and would introduce duplication of serialization APIs. It's a fairly drastic intervention providing comparatively low benefit and as such we should not do it unless we are absolutely convinced it's the way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion area-System.Text.Json
Projects
None yet
Development

No branches or pull requests