Support for Ignore Default Values during JSON serialization in System.Text.Json #42288
Conversation
Provided support for Ignoring Default Value during JSON Serialization
|
|
|
Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:
|
There was a problem hiding this comment.
Thanks for the PR! API review is needed for this feature to be implemented in dotnet/runtime.
| @@ -452,6 +452,7 @@ public sealed partial class JsonSerializerOptions | |||
| public int DefaultBufferSize { get { throw null; } set { } } | |||
| public System.Text.Json.JsonNamingPolicy DictionaryKeyPolicy { get { throw null; } set { } } | |||
| public System.Text.Encodings.Web.JavaScriptEncoder Encoder { get { throw null; } set { } } | |||
| public bool IgnoreDefaultValues { get { throw null; } set { } } | |||
There was a problem hiding this comment.
This requires API review: please see the process here: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md.
The overall "ignore default values" feature has to consider
- if there should be options for the deserialization side (e.g. to populate object with default value when the corresponding JSON is missing). As a starting point, we'd consider the options Newtonsoft.Json provides.
- if this it should apply only to properties or also to collection elements and root objects.
- how the default values for value types should be serialized, and so on.
| @@ -1,9 +1,10 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // Check default value property. | ||
| if (options.IgnoreDefaultValues) | ||
| { | ||
| DefaultValueAttribute defaultValueAttribute = JsonPropertyInfo.GetAttribute<DefaultValueAttribute>(jsonPropertyInfo.PropertyInfo); |
There was a problem hiding this comment.
We should cache the presence of this attribute for each property once during start-up (during the creation of JsonPropertyInfos) rather than every time we see a value.
| @@ -96,7 +97,20 @@ private static bool WriteEndObject(ref WriteStack state) | |||
|
|
|||
| if (jsonPropertyInfo.ClassType == ClassType.Value) | |||
| { | |||
| jsonPropertyInfo.Write(ref state, writer); | |||
| bool includeProperty = true; | |||
There was a problem hiding this comment.
We need to consider properties of other class types as well.
| @@ -0,0 +1,80 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
Nit: this should be named to something like DefaultValueHandlingTests.cs.
|
|
||
| namespace System.Text.Json.Serialization.Tests | ||
| { | ||
| public static partial class DefaultTests |
There was a problem hiding this comment.
Based on API review, we might also need more tests for default value handling, e.g.
- on deserialization
- for collection properties on classes/structs
- for collection elements
- for nested collections
|
Thank you for the interest in contributing. This PR is attempting to add public API surface area, and that's something we have very strict processes around, outlined at https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md. We don't accept PRs for new surface area until an issue proposing the new APIs has been discussed, reviewed, and marked api-approved. As such, I'm going to close this PR. Please feel free to open such an issue, following the mentioned process. Once that’s approved we can accept a PR that implements the new API. Thanks. |
No description provided.