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

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

Open
Abrynos opened this issue Dec 11, 2023 · 2 comments
Open

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

Abrynos opened this issue Dec 11, 2023 · 2 comments

Comments

@Abrynos
Copy link

Abrynos commented Dec 11, 2023

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

Abrynos commented Dec 11, 2023

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

elgonzo commented Dec 11, 2023

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants