Closed andre-ss6 closed 2 years ago
I'd consider two angles here:
Func<JsonSerializerOptions>
an anti-patterngiven 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
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?)
[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: https://github.com/dotnet/runtime/issues/31094#issuecomment-540675858
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.
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
}
]
@PieterjanDeClippel - You need to use case-insensitive deserialization.
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.
I need this at the moment lol. It would be nice to be able to add default settings.
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.
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!
That could explain why this proposal has not done yet. Thanks for your comment Cédric.
El 17 dic 2021, a las 15:16, Cédric Luthi @.***> escribió:
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!
— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.
@0xced FWIW, if you notice the original proposal, the options become immutable once the first serialization has occurred.
@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?
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: https://github.com/dotnet/runtime/issues/31094#issuecomment-951164825 https://github.com/dotnet/runtime/issues/31094#issuecomment-951071151
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.
((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.
@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.
((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.
@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
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;
}
😢
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.
@InsomniumBR wouldn't following https://docs.microsoft.com/en-us/aspnet/core/signalr/configuration?view=aspnetcore-6.0&tabs=dotnet work for you?
@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:
@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?
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.
@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?
.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.
Then I will reiterate @IanKemp's suggestion (https://github.com/dotnet/runtime/issues/31094#issuecomment-674990782):
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?
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.
I think as a stop gap, registering in DI a IJsonSerializer
in the with the JsonOptions.JsonSerializerOptions
set to whatever has been configured in AspNetCore would be a good starting point so that we can use it inside the app, or retrieve it from services in testing. Would also add an easier access / discoverability for it in WebApplicationFactory
as more often then not you will be serializing/deserializing Json and most likely want to use the same options.
Most of my use cases have been either integration testing an AspNetCore app, or within the app itself where I want to do some logging of some sort, or persisting Json into external storage systems.
Here's another reason why we might want to consider introducing a JsonSerializer
instance API: all current serialization methods accepting JsonSerializerOptions
have been marked RequiresUnreferencedCode
due to them using reflection-based serialization by default. This is behavior that predates the introduction of source generators in .NET 6 and contract customization in .NET 7.
The virality of RequiresUnreferencedCode
can be a nuisance when using otherwise perfectly linker-safe code in trimmed applications. Using instance serialization methods means that we can push RUC annotations to specific constructors/factories and have linker warnings only surface in the app's composition root.
@krwq @eerhardt @jeffhandley thoughts?
we might want to consider introducing a JsonSerializer instance API
I chatted a bit with @vitek-karas about this yesterday. Being able to put all the "JSON source gen information" (i.e. JsonTypeInfos / JsonSerializerContext, or an instance of JsonSerializer) into DI makes a lot of sense to me. This could be a nice way for ASP.NET to use the JSON source generated code when they serialize objects.
One scenario that might need thinking about is if you want to serialize the same Type in multiple ways - lets say PascalCase in one case, and camelCase in another (or any other option that the source gen respects - like WriteIndented).
Either way - I think having a "DependencyInjection-friendly" way of serializing objects makes sense and would be valuable. This could be to make JsonSerializer
be instantiable and have normal instance methods, or some other design like where the DependencyInjection info (JsonTypeInfos / JsonSerializerContext) gets passed to the static JsonSerializer
methods. I do like the idea of instance methods on JsonSerializer, and then having certain ways of creating the instance of the JsonSerializer be RequiresUnreferenced/DynamicCode
. That seems clean.
One more issue is that you'd still want the default experience to work in ASP.NET, when JSON source generation isn't used at all. That means a "fallback" or "default" experience (i.e. Reflection) needs to still work for "normal" apps. However, this "fallback" or "default" code will raise trimming warnings. To get rid of those, we could introduce a feature switch that removes the Reflection experience and ONLY works with trim-safe serialization code. Then your app wouldn't use Reflection to do serialization at all.
cc @davidfowl - FYI since we've discussed this "how can ASP.NET use JSON source generation code" topic a lot.
I think another important capability which ASP.NET would probably make use of is the ability to "merge" several such instances. For example let's say my app uses a library FabulousObjects which contains source generated serialization code for its objects because it calls into ASP.NET on my app's behalf. But my app also contains other source generated code for app's objects, which again are serialized by ASP.NET. For this to work both the app and the FabulousObjects would need to register the source generated code with the DI - but the fact that there are 2 sources should be transparent to ASP.NET itself, thus the need to "merge" the two instances into one which can serialize objects from both the app the library.
Such design would probably work better if we DI the contexts which can be merged, but maybe there's a way to do this with serializer instances themselves.
Having an "instance" serializer makes a lot of sense to me. The interesting pivot point to me is the concept of having a "serializer" vs a "serialization context".
What I mean by that is that we could consider three separate levels of serialization state.
At the highest level we have a static Serialize
method that takes a type and some options, and preferably consumes no global state at all. This is what we already have. Each invocation is entirely self-contained. There is no state preserved between invocations.
The next level is a kind of serialization context, which bakes in some set of options. This version would allow you to preserve options between invocations, but wouldn't preserve any information about the serialization process itself.
The last level is the actual serialization process. This is an inherently stateful operation where the output and input are held in intermediate state as we walk the type graph (or text in the case of deserialization). This level would fix the input and the output and would be essentially one-shot, not reusable across multiple serializations.
Right now in serde-dn I have support for the first and last abstraction levels, but not the middle one, i.e. this is the implementation of JsonSerializer.Serialize, which is missing a set of options:
public static string Serialize<T>(T s) where T : ISerialize
{
using var bufferWriter = new PooledByteBufferWriter(16 * 1024);
using var writer = new Utf8JsonWriter(bufferWriter);
var serializer = new JsonSerializer(writer);
s.Serialize(serializer);
writer.Flush();
return Encoding.UTF8.GetString(bufferWriter.WrittenMemory.Span);
}
One way to fix this would be to introduce a JsonSerializerContext
type which takes and stores options.
The tricky bit in my mind here is the source generation stuff. To me that doesn't necessarily fit in any of the abstraction levels, as IMHO the source generation context should be carried with the type, not the options.
Serde solves the association problem by directly providing the source generation into the user type by implementing an appropriate interface. I wrote up some details in the docs on how that works, and how it can generate wrappers for types which aren't under the user's control: https://commentout.com/serde-dn/generator.html
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; }
😢
Cleanest solution for now.
Still not great to have to pass it in every serialization. Id rather have the risk of not configuring it when needed than to add the options everytime....
Per previous discussions in this thread, we don't plan on making JsonSerializerOptions.Default
mutable in future releases. Exposing global mutable state can have unintended consequences: within the context of one application, multiple components can depend on the default options object, including internal implementations of third-party components. As such, assumptions about the application author owning all serialization calls within the process are transient at best.
At the same time, the conversation has spun into a discussion about potentially exposing the serialization APIs as instance methods: I've opened a separate issue (https://github.com/dotnet/runtime/issues/74492) to track this, feel free to contribute to the discussion there.
@eiriktsarpalis Nobody would have wanted to change the default unless you guys chose some completely irrational value as default. With the current default, it is almost impossible to use the JsonSerializer without changing the current default values. Without any second thought following two should have been defaulted in the library:
PropertyNameCaseInsensitive = true,
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
If your default is needed to change every time, then your default is entirely wrong and useless.
You can ask the whole world (except dotNET Team), and almost everybody will say that JSON deserialization must be case insensitive and serialization should be in camelCase by default.
What's wrong with changing the above two defaults in the library as these would not break any existing thing?
New API:
Newtonsoft \ JSON.NET does it this way:
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:Currently you have to keep passing the options around and remembering to pass it to each call to
[De]Serialize