System.Text.Json: JsonExtensionData + IgnoreNullValues bug

[CodeBlanch]

Porting a service from .NET Framework to .NET Core we noticed a lot of NULLs started showing up in Kibana for certain fields (but not all fields) pushed from our structured json logs. I tracked this down to a bug in the handling of JsonExtensionData with IgnoreNullValues=true.

Consider this class:

public class TestClass
{
   public string Prop1 { get; set; } = "value";
   public string Prop2 { get; set; }
}

With IgnoreNullValues=true that will serialize as:

{"Prop1":"value"}

Now consider this class:

public class TestClass
{
   [JsonExtensionData]
   public IDictionary<string, object> Fields { get; set; } = new Dictionary<string, object>
   {
      ["Prop1"] = "value",
      ["Prop2"] = null
   }
}

Should be identical with IgnoreNullValues=true, but ends up as:

{"Prop1":"value","Prop2":null}

[ahsonkhan]

The IgnoreNullValues flag doesn't apply to collections (for example, list, dictionary, etc.) and only applies to when property values themselves is null. This includes dictionaries that are annotated with [JsonExtensionData]. This is by-design and matches Newtonsoft.Json behavior

Changing this would be a breaking change.

For example:

public static void Main()
{
    var options = new JsonSerializerOptions { IgnoreNullValues = true };
    var obj = new TestClass();

    // {"List":["a",null,"b"]}
    Console.WriteLine(JsonSerializer.Serialize(obj, options));

    // {"List":["a",null,"b"]}
    Console.WriteLine(JsonConvert.SerializeObject(obj, new JsonSerializerSettings { NullValueHandling = NullValueHandling.Ignore }));
}

public class TestClass
{
    public List<string> List { get; set; } = new List<string> { "a", null, "b" };
}

See: #29495 (comment)

This is a duplicate of #682

[CodeBlanch]

@ahsonkhan Hang on I wouldn't say this is the same issue as #682.

@steveharter's comments on #682 are spot-on. We don't want IgnoreNullValues=true to mess with the content of enumerables, I'm in no way suggesting that. But [JsonExtensionData] is a special case, right? It is a Dictionary, sure, but as a means to an end; a convenience to attach properties at the root. IgnoreNullValues does apply to root-level properties. In this way, it is inconsistent. Some null properties will be ignored, but properties stuffed into JsonExtensionData that are null will be included. Inconsistency leads to fear, leads to anger, anger to hate, and then we arrive at the dark side :)

What I'm arguing here is that options impacting properties at the root should be applied to JsonExtensionData, which at the end of the day is managing properties at the root.

Anyone relying on this behavior would break yes, but it could also fix people being impacted by this behavior. It's such a crazy rare edge case, I think it's worth considering fixing the inconsistency.

Consider a class like this...

public class TestClassServer
{
   public string Prop2 { get; set; }

   public TestClassServer() { Prop2 = "DefaultValue"; }
}

Now my client does an implementation like this:

public class TestClassClient
{
   [JsonExtensionData]
   public IDictionary<string, object> Fields { get; set; } = new Dictionary<string, object>
   {
      ["Prop1"] = "value",
      ["Prop2"] = null
   }
}

string JSONToSendToServer= JsonSerializer.Serialize(
   new TestClassClient(),
   new JsonSerializerOptions { IgnoreNullValues = true });

That JSON sent to the server will clear the "DefaultValue" which seems unintended given the usage of IgnoreNullValues=true. Could lead to data loss.

The real cost of this for us is that we're now pumping in a ton of NULL we didn't intend to into our log storage which is more expensive as far as $$ for disk space goes.

Anyway hopefully that better explained the nuance and subtlety of the issue. Thanks for reading friends!

PS: I did check on Json.NET and you are correct, it has the same behavior. @JamesNK

[ahsonkhan]

@CodeBlanch, thanks for clarifying and sharing your scenario details.

Anyone relying on this behavior would break yes, but it could also fix people being impacted by this behavior. It's such a crazy rare edge case, I think it's worth considering fixing the inconsistency.

We discussed this previously not only for enumerables but for all collections including dictionaries and JsonExtensionData as well (see the specific issue/API review that discussed this feature: #29495 (comment)). Having the behavior you are suggesting would make dictionaries marked as extension be inconsistent with how any of the other dictionary properties serialized/deserialized, which I think would be surprising to people.

Similarly, like you observed, given that Newtonsoft.Json has the same behavior on how extension dictionaries are serialized, changing it would introduce inconsistency.

The real cost of this for us is that we're now pumping in a ton of NULL we didn't intend to into our log storage which is more expensive as far as $$ for disk space goes.

We could consider a way for the caller to opt-in to this behavior, maybe with a different option (that's part of the discussion to resolve #682).

But [JsonExtensionData] is a special case, right?

I don't know if we should special case extension dictionaries. To me, the serializer should treat them as any other dictionary. Maybe we extend the attribute to allow for such custom ignore behavior. Again, I think it has to be opt-in.

Some null properties will be ignored, but properties stuffed into JsonExtensionData that are null will be included. Inconsistency leads to fear, leads to anger, anger to hate, and then we arrive at the dark side :)

Lol :)

[CodeBlanch]

@ahsonkhan Sounds good, I threw up an idea for opt-in on #30687. Thanks for the link to the JsonExtensionData discussion, very insightful. The most special of all the dictionaries!