JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.64k stars 3.24k forks source link

Non-Null default value leads to invalid cache value being assigned #2923

Open Abrynos opened 7 months ago

Abrynos commented 7 months ago

Destination types

// "Required.Always" is irrelevant; You can remove it
// Init-only setter is irrelevant; doesn't work with normal "set" either
// "private" modifier on the setter is irrelevant; doesn't work with a public one either
internal sealed class Container
{
    [JsonProperty(Required = Required.Always)]
    internal Contained FirstContained { get; private init; } = Contained.Empty;

    [JsonProperty(Required = Required.Always)]
    internal Contained SecondContained { get; private init; } = Contained.Empty;

    [JsonConstructor]
    private Container() { }
}

internal sealed class Contained
{
    internal static Contained Empty = new();

    [JsonProperty(Required = Required.Always)]
    internal string DifferentValue { get; private init; } = string.Empty;

    [JsonConstructor]
    private Contained() { }
}

Source JSON

{
   "FirstContained":{
      "DifferentValue":"test"
   },
   "SecondContained":{
      "DifferentValue":"asdf"
   }
}

Expected behavior

result.FirstContained.DifferentValue differs from result.SecondContained.DifferentValue

Actual behavior

DifferentValue always has the value which last occured in the input JSON

Steps to reproduce

Container result = JsonConvert.DeserializeObject<Container>(json);

Workaround

Do not assign a default-value to objects (remove the = Contained.Empty;).


Thank you for looking into this.

Abrynos commented 7 months ago

In case you're wondering why I'm assigning an empty default value in the first place:

  1. Enable nullable-feature
  2. Enable all warnings
  3. Enable "treat all warnings as errors"
  4. We definitely don't want anyone to instantiate the class manually; Therefore, make the only constructor private without arguments.
  5. Build fails due to CS8618: Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
elgonzo commented 7 months ago

Disclaimer: I am just a user of the library and not associated with the project nor its author/maintainer.

Not a bug.

It's the default for the ObjectCreationHandling setting in action. It's a bit unfortunate that this particular behavior has been chosen the default behavior, because it (unsurprisingly) catches quite a few users by surprise, but that choice had been made quite long ago, and with so many software projects relying on Newtonsoft.Json it is rather unlikely that a default setting will change... :(

In your case, both the FirstContained and DifferentValue properties are initialized with the same Contained.Empty instance. And when Newtonsoft.Json deserializes the json objects for FirstContained and DifferentValue, it uses the assigned Contained instance(s) to assign the value for the DifferentValue property to it.

In other words, you see the same value for result.FirstContained.DifferentValue and result.SecondContained.DifferentValue because result.FirstContained and result.SecondContained refer to the same Contained.Empty instance, and Newtonsoft.Json is reusing (populating) this Contained.Empty instance instead of creating and assigning new Contained instances to the FirstContained/SecondContained properties. This means, not only did you see a result you didn't expect, but the Contained.Empty instance itself got modified by the deserializer, too.

To instruct Newtonsoft.Json to not reuse object instances but replace existing object instances with new ones, either set the [JsonProperty] attribute's ObjectCreationHandling property to ObjectCreationHandling.Replace. Or, if this setting should affect the entire deserialization job, set the ObjectCreationHandling property in an JsonSerializerSettings instance and pass this JsonSerializerSettings instance to the (de)serializer.

P.S.: It is probably a good idea to spend a little time studying the default values used for the serialization settings used by the (de)serializer. Because there might be other default settings (and thus default behavior) of Newtonsoft.Json that you might or might not intuitively expect...