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]: Extend JsonIgnoreCondition #66490

Closed
WeihanLi opened this issue Mar 11, 2022 · 32 comments · Fixed by #104562
Closed

[API Proposal]: Extend JsonIgnoreCondition #66490

WeihanLi opened this issue Mar 11, 2022 · 32 comments · Fixed by #104562
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@WeihanLi
Copy link
Contributor

WeihanLi commented Mar 11, 2022

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

@WeihanLi WeihanLi added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 11, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Mar 11, 2022
@ghost
Copy link

ghost commented Mar 11, 2022

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

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

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

API Proposal

namespace System.Text.Json.Serialization;

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

API Usage

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<TestModel>(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: -

@WeihanLi WeihanLi changed the title [API Proposal]: Extension JsonIgnore [API Proposal]: Extend JsonIgnoreCondition Mar 11, 2022
@eiriktsarpalis
Copy link
Member

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 eiriktsarpalis added this to the Future milestone Mar 11, 2022
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Mar 11, 2022
@eiriktsarpalis
Copy link
Member

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

@Symbai
Copy link

Symbai commented Mar 12, 2022

#52584

@ScarletKuro
Copy link

ScarletKuro commented Jun 1, 2022

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 #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
Copy link

Stabzs commented Jun 16, 2022

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
Copy link

ScarletKuro commented Jun 16, 2022

@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
Copy link
Member

krwq commented Jun 28, 2022

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

@eiriktsarpalis
Copy link
Member

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

@waynebrantley
Copy link

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
Copy link
Member

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
Copy link

ScarletKuro commented Nov 21, 2022

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
Copy link
Member

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
Copy link

ScarletKuro commented Nov 21, 2022

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
Copy link
Member

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
Copy link

ScarletKuro commented Nov 21, 2022

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(#30795 (comment)).

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
Copy link
Member

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
Copy link

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 bartonjs added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2023
@Stabzs
Copy link

Stabzs commented Aug 3, 2023

@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
Copy link
Contributor Author

WeihanLi commented Aug 4, 2023

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
Copy link
Member

eiriktsarpalis commented Aug 4, 2023

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
Copy link

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
Copy link
Member

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

@eiriktsarpalis
Copy link
Member

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
Copy link

It appears that a new issue was created(#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
Copy link
Contributor Author

WeihanLi commented Aug 4, 2023

@ScarletKuro add a new proposal here #90011

@eiriktsarpalis
Copy link
Member

Let's use #83706 to track WhenReadingNull.

@onurkanbakirci
Copy link

onurkanbakirci commented Sep 20, 2023

@eiriktsarpalis I've already implemented in #69574

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Jul 3, 2024

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
Copy link
Member

@WeihanLi feel free to open a PR.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2024
@eiriktsarpalis eiriktsarpalis modified the milestones: 9.0.0, 10.0.0 Jul 23, 2024
@eiriktsarpalis
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants