dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

[API Proposal]: Extend `JsonIgnoreCondition` #66490

Closed WeihanLi closed 3 weeks ago

WeihanLi commented 2 years ago

Background and motivation

Sometimes we may want to ignore some properties only when serialize, included when deserialize, currently, we had JsonIgnoreCondition

public enum JsonIgnoreCondition
{
  Never,
  Always,
  WhenWritingDefault,
  WhenWritingNull,
}

Maybe we could add new conditions like WhenWriting/WhenReading(or WhenSerializing/WhenDeserializing ...)

API Proposal

edited

namespace System.Text.Json.Serialization;

public enum JsonIgnoreCondition
{
  Never = 0,
  Always = 1,
  WhenWritingDefault = 2,
  WhenWritingNull = 3,
+ WhenWriting = 4,
+ WhenReading = 5,
}

API Usage

public class TestModel
{  
  [JsonIgnore(Condition = JsonIgnoreCondition.WhenReading)]
  public int Age { get; set; }
  [JsonIgnore(Condition = JsonIgnoreCondition.WhenWriting)]
  public string Name { get; set; }
}

var json = JsonSerializer.Serialize(new TestModel{ Age = 10, Name="Mike" });
Assert.Equal("{\"Age\":10}", json);

json = "{\"Age\":10, \"Name\":\"Mike\"}";
var model = JsonSerializer.Deserialize<TestModel>(json);
Assert.Equal("Mike", model.Name);
Assert.Equal(0, model.Age);

Alternative Designs

No response

Risks

No response

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-text-json See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation Sometimes we may want to ignore some properties only when serialize, included when deserialize, currently, we had `JsonIgnoreCondition` ``` c# public enum JsonIgnoreCondition { Never, Always, WhenWritingDefault, WhenWritingNull, } ``` Maybe we could add new conditions like `WhenWriting`/`WhenReading`(or `WhenSerializing`/`WhenDeserializing` ...) ### API Proposal ```C# namespace System.Text.Json.Serialization; public enum JsonIgnoreCondition { Never, Always, WhenWritingDefault, WhenWritingNull, WhenWriting, WhenReading } ``` ### API Usage ```C# public class TestModel { public int Age { get; set; } [JsonIgnore(Condition = JsonIgnoreCondition.WhenWriting)] public string Name { get; set; } } var json = JsonSerializer.Serialize(new TestModel{ Age = 10, Name="Mike" }); Assert.Equal("{\"Age\":10}", json); json = "{\"Age\":10, \"Name\":\"Mike\"}"; var model = JsonSerializer.Deserialize(json); Assert.Equal("Mike", model.Name); ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: WeihanLi
Assignees: -
Labels: `api-suggestion`, `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 2 years ago

Related to #55781. In general we want to be careful about adding more states in the JsonIgnoreCondition enum, and we generally want to encourage a contract model customization approach via #63686. However your proposals seem fairly straightforward and simple to implement, so I'd support their addition.

eiriktsarpalis commented 2 years ago

Tagging @layomia for a second opinion. If this seems reasonable, I think I can mark it ready for review.

Symbai commented 2 years ago

52584

ScarletKuro commented 2 years ago

There should be WhenReadingNull too, I'm actually surprised that it's absent. Because the obsolete IgnoreNullValus was working on reading too. For example:

public class TestClass
{
      public string FirstName { get; set; } = string.Empty;
      public string[] List { get; set; } = Array.Empty<string>();
      public TestClass(){}
}

And deserialization

var testClassJson = "{\"FirstName\":null,\"List\":null}";
TestClass testclass1 = JsonSerializer.Deserialize<TestClass>(testClassJson, new JsonSerializerOptions()
{
      IgnoreNullValues = true
});

This will produce:

FirstName: ""
List: string[0]

Meanwhile you can't express such behavior with DefaultIgnoreCondition or the [JsonIgnore(Condition = ...)] yet the Microsoft Documentation says you not to use IgnoreNullValues when all the behavior of this property wasn't ported. NB! small note that if you won't explicitly set the FirstName:null in json the the FirstName will have empty string after deserialization, but this is not enough. I'd say this is a major problem when you want to counteract null values when some 3rd party sends you null but you don't want it to have, you want an empty array, empty string, etc etc instead.

FYI: the Newtonsoft.Json is able to ignore null values on reading with [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]

Also, the issue https://github.com/dotnet/runtime/issues/62086 shows that I'm not alone thinking the same way that there should be attribute for null deserialization and how misleading the documentation / obsolete attribute on IgnoreNullValues is.

Stabzs commented 2 years ago

This is a tremendous problem and I JUST ran into this during an upgrade.

The fact that this wasn't thought through as a potential migration path and called out in the documentation is incredibly frustrating, especially since as @ScarletKuro pointed out, this behavior previously worked in IgnoreNullValues and an upgrade has no immediately obvious backwards-compatible option.

It would be so greatly appreciated if there was a little more foresight around deprecating functionality coupled with an appreciation for how that functionality is being used, and a little less opinion on what you think is the appropriate usage, without regards to how it worked previously.

ScarletKuro commented 2 years ago

@layomia said to just suppress the warring and use IgnoreNullValues because they do not plan to remove it. But my problem with that is that IgnoreNullValues is an global parameter within JsonSerializerOptions and it works in both direction - serialization and deserialization. In my case, I want it to be only for deserialization. Also, I want to have control over what property can ignore null and what must not ignore on deserialization. Like having an attribute is nowhere close as having just IgnoreNullValues. I know that the aim is not to copy paste Newtonsoft.Json features, but this one is a simple and useful feature(for some even a stop factor). I was surprised when team said they didn't find much reason to add it in JsonIgnoreCondition, but as I said earlier the ability to counteract nulls, for example, that you don't want the list to be null but empty array is pretty big already, imo.

Update: Also, what I found out, is that IgnoreNullValues is not supported for source generator(i.e. JsonSourceGenerationOptions) meanwhile there is support for the JsonIgnoreCondition that's another reason why there should be WhenReadingNull and etc options.

krwq commented 2 years ago

@ScarletKuro with contract customization you can simply remove Get/Set from property to prevent serialization or deserialization

eiriktsarpalis commented 2 years ago

@krwq would it be possible to write a small snippet here illustrating how this can be done?

waynebrantley commented 2 years ago

As an extension, null should be treated the same way

public class TestClass
{
      public bool Human { get; set; } 
      public int Age { get; set; }
}

what would be nice is if 'null' values were treated as 'no value' and let the defaults applied

var testClassJson = "{\"Human\":null,\"Age\":null}";
TestClass testclass1 = JsonSerializer.Deserialize<TestClass>(testClassJson);

Today the above will throw 'Cannot get the value of a token type 'Null' as a number.' What I would expect is those 'null' values to be ignored. If it is a nullable type, they will be null (the default) and if non-nullable they will be the proper default values (false and 0 in this case).

I can write a converter for every type to do this easily today - but think this should be an option. (I would need this boiler plate code for boolean, int, short, int64, decimal, double, etc...)

        public class NullIntToDefaultConverter : JsonConverter<int>
        {
            public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options) =>
                writer.WriteNumberValue(value);

            public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) =>
                reader.TokenType switch
                {
                    JsonTokenType.Null => default,
                    JsonTokenType.Number => reader.TryGetInt32(out int l) ? Convert.ToInt32(l) : 0,
                    _ => throw new JsonException(),
                };
        }
eiriktsarpalis commented 2 years ago

Here's an example of how the feature can be implemented using .NET 7 contract customization:

[Flags]
public enum MyIgnoreCondition
{
    WhenWriting = 1,
    WhenReading = 2,
}

[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false, Inherited = false)]
public sealed class MyIgnoreConditionAttribute : Attribute
{
    public MyIgnoreCondition IgnoreCondition { get; }
    public MyIgnoreConditionAttribute(MyIgnoreCondition ignoreCondition) => IgnoreCondition = ignoreCondition;
}

public class MyIgnoreConditionResolver : DefaultJsonTypeInfoResolver
{
    public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        JsonTypeInfo jti = base.GetTypeInfo(type, options);

        foreach (JsonPropertyInfo jsonPropertyInfo in jti.Properties)
        {
            if (jsonPropertyInfo.AttributeProvider?.GetCustomAttributes(typeof(MyIgnoreConditionAttribute), inherit: false) is [MyIgnoreConditionAttribute attr, ..])
            {
                MyIgnoreCondition ignoreCondition = attr.IgnoreCondition;
                if ((ignoreCondition & MyIgnoreCondition.WhenWriting) != 0)
                {
                    jsonPropertyInfo.Get = null;
                }

                if ((ignoreCondition & MyIgnoreCondition.WhenReading) != 0)
                {
                    jsonPropertyInfo.Set = null;
                }
            }
        }

        return jti;
    }
}

which can be used as follows:

var options = new JsonSerializerOptions { TypeInfoResolver = new MyIgnoreConditionResolver() };
MyPoco value = new MyPoco { IgnoreOnWrite = 1, IgnoreOnRead = 2 }; 
Console.WriteLine(JsonSerializer.Serialize(value, options)); // {"IgnoreOnRead":2}

string json = """{"IgnoreOnWrite":1,"IgnoreOnRead":2}""";
value = JsonSerializer.Deserialize<MyPoco>(json, options);
Console.WriteLine(value.IgnoreOnRead); // 0

public class MyPoco
{
    [MyIgnoreCondition(MyIgnoreCondition.WhenWriting)]
    public int IgnoreOnWrite { get; set; }

    [MyIgnoreCondition(MyIgnoreCondition.WhenReading)]
    public int IgnoreOnRead { get; set; }
}
ScarletKuro commented 2 years ago

How do you combine it with the source generator when you have your own JsonSerializerContext? I tried like this

jsonSerializerOptions.TypeInfoResolver = JsonTypeInfoResolver.Combine(new MyIgnoreConditionResolver(), MySerializerContext.Default);

But looks like MySerializerContext is not involved, when I put a breaking points on the generated code they do not hit, when I remove the the MyIgnoreConditionResolver then MySerializerContext is working. Or I need to define the MySerializerContext somewhere in the MyIgnoreConditionResolver?

eiriktsarpalis commented 2 years ago

The MyIgnoreConditionResolver as implemented above provides metadata for all types, as such MySerializerContext.Default is never consulted. You would need to modify the example somewhat:

var ignoreConditionResolver = new MyIgnoreConditionResolver(MySerializerContext.Default);

public class MyIgnoreConditionResolver : IJsonTypeInfoResolver
{
    private readonly IJsonTypeInfoResolver _source;
    public class MyIgnoreConditionResolver(IJsonTypeInfoResolver source) => _source = source;

    public override JsonTypeInfo? GetTypeInfo(Type type, JsonSerializerOptions options)
    {
        JsonTypeInfo? jti = _source.GetTypeInfo(type, options);

        if (jti is null) 
             return null;

        foreach (JsonPropertyInfo jsonPropertyInfo in jti.Properties)
        {
            if (jsonPropertyInfo.AttributeProvider?.GetCustomAttributes(typeof(MyIgnoreConditionAttribute), inherit: false) is [MyIgnoreConditionAttribute attr, ..])
            {
                MyIgnoreCondition ignoreCondition = attr.IgnoreCondition;
                if ((ignoreCondition & MyIgnoreCondition.WhenWriting) != 0)
                {
                    jsonPropertyInfo.Get = null;
                }

                if ((ignoreCondition & MyIgnoreCondition.WhenReading) != 0)
                {
                    jsonPropertyInfo.Set = null;
                }
            }
        }

        return jti;
    }
}
ScarletKuro commented 2 years ago

Thanks, the source generator starts to work. However, there is another problem now. With

JsonTypeInfo? jti = _source.GetTypeInfo(type, options);

the jsonPropertyInfo.AttributeProvider is always null on any property so the condition never works. I made a repository with xUnit test to show the problem https://github.com/ScarletKuro/IgnoreConditionResolverTest and that the test fails. When there is no SerializerContext the IgnoreConditionResolve works correctly and when the SerializerContext is present then it doesn't work.

eiriktsarpalis commented 2 years ago

the jsonPropertyInfo.AttributeProvider is always null on any property so the condition never works.

Ah yes. Unfortunately AttributeProvider cannot be populated by the source generator since it would require using reflection, breaking any linker-safety guarantees. Unfortunately, this cannot be supported -- I would recommend using the default contract resolver in that case.

ScarletKuro commented 2 years ago

If the custom attributes do not work together with the source generator and contract customization, then this proposal is valid. Community needs a solution to ignore on WhenWriting / WhenReading that could be respected by the source generator. IgnoreCondition is already included in the JsonPropertyInfoValues. This means that this proposal can be visible to the source generator. Sorry, I didn't make it clear in my first comment, that this also should work with the source generator. I know that the team doesn't want to boilerplate the lib. For example, with pre-made JsonConverters like for the DateTime and tries to provide customization, but I feel like this one is a pretty common functionality for the json and if I remember correctly team was debating whenever this should be included out of box from the start or not(https://github.com/dotnet/runtime/issues/30795#issuecomment-529574768).

upd: or maybe in the future it would be possible to make the source generator to write somewhere the list of custom attributes and could be added to the "contract customization improvements" plan.

Also, what I found out, is that IgnoreNullValues (JsonSourceGenerationOptions) is not supported for source generator.

I think I was wrong, because apparently it is respected by the source generator. Only concerned is that this can be removed in future .NET since its marked as obsolete.

eiriktsarpalis commented 2 years ago

Community needs a solution to ignore on WhenWriting / WhenReading that could be respected by the source generator.

Generally speaking, it should be possible to fine tune ignore settings via the JsonPropertyInfo.ShouldSerialize/Get/Set properties, even when consuming source gen metadata. What is not possible/supported in the case of source gen is automatic access the underlying MemberInfo/ICustomAttributeProvider metadata, since that's not how the source generator works.

If you're willing to go the extra mile, it should be possible to recover the MemberInfo using regular reflection on the JsonTypeInfo.Type property, however that would largely invalidate any benefits of using a source generator in the first place.

JohanPetersson commented 1 year ago

As an extension, null should be treated the same way

public class TestClass
{
      public bool Human { get; set; } 
      public int Age { get; set; }
}

what would be nice is if 'null' values were treated as 'no value' and let the defaults applied

var testClassJson = "{\"Human\":null,\"Age\":null}";
TestClass testclass1 = JsonSerializer.Deserialize<TestClass>(testClassJson);

Today the above will throw 'Cannot get the value of a token type 'Null' as a number.' What I would expect is those 'null' values to be ignored. If it is a nullable type, they will be null (the default) and if non-nullable they will be the proper default values (false and 0 in this case).

I can write a converter for every type to do this easily today - but think this should be an option. (I would need this boiler plate code for boolean, int, short, int64, decimal, double, etc...)

        public class NullIntToDefaultConverter : JsonConverter<int>
        {
            public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options) =>
                writer.WriteNumberValue(value);

            public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) =>
                reader.TokenType switch
                {
                    JsonTokenType.Null => default,
                    JsonTokenType.Number => reader.TryGetInt32(out int l) ? Convert.ToInt32(l) : 0,
                    _ => throw new JsonException(),
                };
        }

Should we create a separate issue for this? In JavaScript, NULL is same as 0 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#number_coercion. JSON is JavaScript. I would expect, at least behind an option, that value types should treat NULL as 'default' when deserializing.

bartonjs commented 1 year ago

Video

None of us seem to be convinced that there's a real scenario here, but the shape as proposed is correct for what it's proposing.

namespace System.Text.Json.Serialization;

public enum JsonIgnoreCondition
{
  Never = 0,
  Always = 1,
  WhenWritingDefault = 2,
  WhenWritingNull = 3,
+ WhenWriting = 4,
+ WhenReading = 5,
}
Stabzs commented 1 year ago

@bartonjs thanks for posting the design review. Very helpful.

I think there is some misunderstanding about common use cases in the thread above. The ask was not to ignore on read always, it was to ignore on read when null, which this design proposal absolutely does not handle.

The need is to replace behavior guaranteed by [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] and IgnoreNullValues. You cannot do this with Always since you'll get The value cannot be 'JsonIgnoreCondition.Always'. And you can't do this with WhenWritingNull, because it doesn't handle the read case.

To make things worse, you also didn't provide a solution in the design review to use combinatorial conditions.

What we really need is WhenReadingNull to complement WhenWritingNull, and WhenNull.

WeihanLi commented 1 year ago

In our case, we have some properties which expected not to be serialized, we're using the ShouldSerialize for Newtonsoft.Json(https://www.newtonsoft.com/json/help/html/conditionalproperties.htm), the WhenWriting would be helpful for the migration

Added the WhenReadingNull to the proposal. @Stabzs @bartonjs @eiriktsarpalis

eiriktsarpalis commented 1 year ago

Added the WhenReadingNull to the proposal.

The proposal has already been approved with the two new additions, it would be impractical to re-review it just because the scope got extended after the fact.

I'm also not sure what the purpose of WhenReadingNull is? Are you looking to prevent the default value of a property from being overwritten with a null value? That doesn't seem like a particularly common scenario. It can easily be achieved via contract customization so it's unlikely we would consider it at this point.

I've edited your OP to remove the WhenReadingNull component to avoid potential confusion when the approved API gets implemented.

ScarletKuro commented 1 year ago

Are you looking to prevent the default value of a property from being overwritten with a null value?

Yes, this is the main use case of it. It's useful when you deal with 3rd party json, which may send sets as null values for list / array when you want it to be empty collection instead of null (as example). If we search in the repository, there is a community interest in having this feature

It can easily be achieved via contract customization so it's unlikely we would consider it at this point.

This, unfortunately, doesn't currently work with one of the main aspects - source generation, if you want to make it attribute based like in the Newtonsoft.Json [JsonProperty(NullValueHandling = NullValueHandling.Ignore)].

eiriktsarpalis commented 1 year ago

Please open a separate issue. API review is complete and we can't change the scope after the fact.

eiriktsarpalis commented 1 year ago

This, unfortunately, doesn't currently work with one of the main aspects - source generation

This is not entirely accurate. You can in fact run contract customization against source gen, and even query for custom attributes: it's the type of reflection that works even in Native AOT applications.

ScarletKuro commented 1 year ago

It appears that a new issue was created(https://github.com/dotnet/runtime/issues/90007) and the problem is similar to the one we discussed here. Based on my understanding, implementing the WhenReadingNull feature would address the problem described in the issue.

@WeihanLi do you have interest in creating a new API proposal, or I should do it?

WeihanLi commented 1 year ago

@ScarletKuro add a new proposal here https://github.com/dotnet/runtime/issues/90011

eiriktsarpalis commented 1 year ago

Let's use https://github.com/dotnet/runtime/issues/83706 to track WhenReadingNull.

onurkanbakirci commented 1 year ago

@eiriktsarpalis I've already implemented in #69574

WeihanLi commented 4 months ago

Hello @onurkanbakirci are you still working on this? I'd like to try to take this if you're not going to implement this

eiriktsarpalis commented 4 months ago

@WeihanLi feel free to open a PR.

eiriktsarpalis commented 4 months ago

Moving to 10.0.0 since feature development for .NET 9 is now completed.