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

Support modifying already initialized properties and fields when deserializing JSON #78556

Closed
5 tasks done
Tracked by #71967
krwq opened this issue Nov 18, 2022 · 22 comments · Fixed by #83669
Closed
5 tasks done
Tracked by #71967

Support modifying already initialized properties and fields when deserializing JSON #78556

krwq opened this issue Nov 18, 2022 · 22 comments · Fixed by #83669
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:XL Work that requires one engineer more than 4 weeks partner-impact This issue impacts a partner who needs to be kept updated User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@krwq
Copy link
Member

krwq commented Nov 18, 2022

Support modifying already initialized properties and fields when deserializing JSON

Currently properties and fields which are read-only cannot be deserialized. This scenario could be supported though. Consider following example:

A obj = JsonSerializer.Deserialize<A>("""{"Foo": {"Value": 5}}""");

class A
{
    public B Foo { get; } = new B();
}

class B
{
    public int Value { get; set; }
}

Currently will be a no-op because we do not re-use value of property Foo.

Similarly another example:

A obj = JsonSerializer.Deserialize<A>("""{"Elements": [4,5,6]}""");

class A
{
    public List<int> Elements { get; } = new List<int>() { 1, 2, 3 };
}

will cause no-op while many users would expect Elements to end up with 1, 2, 3, 4, 5, 6.

There are currently no easy workaround for this - only writing your own custom converter combined with custom Set.

API Proposal

namespace System.Text.Json.Serialization;

public enum JsonObjectCreationHandling
{
    Replace = 0,
    Populate = 1,
}

// NOTE: System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Interface
//           For Property/Field - Populate is enforced
//           For Class/Struct/Interface - Populate is applied to properties where applicable
[System.AttributeUsageAttribute(System.AttributeTargets.Field | System.AttributeTargets.Property | System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Interface, AllowMultiple = false)]
public sealed partial class JsonObjectCreationHandlingAttribute : System.Text.Json.Serialization.JsonAttribute
{
    public JsonObjectCreationHandlingAttribute(System.Text.Json.Serialization.JsonObjectCreationHandling handling) { }
    public System.Text.Json.Serialization.JsonObjectCreationHandling Handling { get { throw null; } }
}

namespace System.Text.Json.Serialization.Metadata;
public abstract partial class JsonPropertyInfo
{
    public JsonObjectCreationHandling CreationHandling { get; set; } // Replace remains default behavior
}

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    // Also see global switch section below
    public JsonObjectCreationHandling DefaultCreationHandling { get; set; } /*= JsonObjectCreationHandling.Replace; */
}

// This part is currently cut from the proposal due to time constraints
// Respective issue will be open after the API review
/*
public static partial class JsonSerializer
{
    public static void PopulateObject(System.IO.Stream utf8Json, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
    public static void PopulateObject(System.ReadOnlySpan<byte> utf8Json, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
    public static void PopulateObject([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("Json")] System.ReadOnlySpan<char> json, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
    public static void PopulateObject([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("Json")] string json, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
    public static void PopulateObject(ref System.Text.Json.Utf8JsonReader reader, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
    
    public static System.Threading.Tasks.ValueTask PopulateObjectAsync(System.IO.Stream utf8Json, System.Type type, object target, System.Text.Json.JsonSerializerOptions? options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
    public static System.Threading.Tasks.ValueTask PopulateObjectAsync<TValue>(System.IO.Stream utf8Json, TValue target, System.Text.Json.JsonSerializerOptions? options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where TValue : class { throw null; }
    public static void PopulateObject<TValue>(System.IO.Stream utf8Json, TValue target, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }
    public static void PopulateObject<TValue>(System.ReadOnlySpan<byte> utf8Json, TValue target, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }
    public static void PopulateObject<TValue>([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("Json")] System.ReadOnlySpan<char> json, TValue target, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }
    public static void PopulateObject<TValue>([System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("Json")] string json, TValue target, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }
    
    public static void PopulateObject<TValue>(ref System.Text.Json.Utf8JsonReader reader, TValue target, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }
    
    // TODO: add also JsonTypeInfo<T> and JsonSerializerContext overloads
}*/

What does JSON.NET offer?

In Newtonsoft.Json currently most of these scenarios work by default. To control this behavior it has JsonSerializerSettings.ObjectCreationHandling property which allows to set one of three enum values:

  • ObjectCreationHandling.Reuse - tries reusing object when possible
  • ObjectCreationHandling.Auto - same as Reuse
  • ObjectCreationHandling.Replace - always replaces object with new one (similarly to our current System.Text.Json behavior)

Regardless of choice if property/field is null, it will always create new instance.

There are couple of things which are not exactly obvious with how JSON.NET handles reuse though:

  • it looks at the runtime type rather than property type - i.e. for public IEnumerable<int> SomeProperty { get; } = new List<int>() it will append even though type of property is theoretically read-only
  • if runtime type is known read-only collection (i.e. ReadOnlyCollection<T> which can be created from i.e. new List<int>() { 1, 2, 3}.AsReadOnly()) it will replace object and also try to match the same type for new object
  • if runtime type is custom read-only collection (i.e. custom implemented IList<T> with IsReadOnly = true) the deserialization ends up with silent no-op - nothing will get deserialized and no exception will happen
  • if deserialized field is a struct it will always replace it - on top of that it won't ever call parameterless constructor - if only parametrized exists it will be called. For properties the same behavior will occur but properties always return copy on getters and therefore replacing is justified.

Suggestion for System.Text.Json

  • System.Text.Json generally doesn't look at the runtime type and doing something else here would be inconsistent. We could potentially consider that in the future under separate switch.
  • Current behavior is to always replace and changing that would be presumably a massive breaking change, because of that default should stay as is.
  • Creating object when property/field is null seems like an overall good design and we'd also follow that (assuming property is writable).
  • Suggested behavior is to make it possible to enable this behavior only per property and if opt-in was specifically asked for we should always populate (minus null case) and never fall-back to replacing object
  • IList<T>.IsReadonly being true while property is populatable (and similar cases for other collections) will cause serializer (converter specifically) to throw
  • Global switch can be emulated with contract resolver modifier which can enable it for properties which can support it. Initally we will not provide such solution as public API but can provide it as a code sample.

Suggestion for System.Text.Json with comparison with Newtonsoft.JSON

  • not allowed - means that configuration is invalid and serializer will throw during contract validation - i.e. GetTypeInfo, Serialize, Deserialize
  • runtime error - means that configuration is valid at the contract level but specific setting at runtime (i.e. IsReadOnly is set) make it invalid and exception will be thrown during deserialization

Following table shows how property/field with Populate will behave for collections in System.Text.Json with comparison to Newtonsoft.JSON. Note that table does not mention Dictionary - like types but similar rules apply:

PropertyType RuntimeType Newtonsoft.JSON behavior Suggested System.Text.Json behavior
List<T> List<T> populate populate
IList<T> List<T> populate populate
IReadOnlyList<T> List<T> populate not allowed
IEnumerable<T> List<T> populate not allowed
IList<T> ReadOnlyCollection<T> replace with new ReadOnlyCollection<T> runtime error - user asked to populate but we're unable to fulfill
IList<T> custom implementation with IsReadOnly=true no-op - list is not replaced or modified runtime error - user asked to populate but we're unable to fulfill
IList<T> custom implementation with IsReadOnly=false populate populate
T[] T[] replace not allowed
ImmutableArray<T> ImmutableArray<T> replace not allowed for now, see section below

Following table shows how property/field with Populate will behave for value types and reference types with comparison to Newtonsoft.JSON:

Property/Field type description Newtonsoft.JSON behavior Suggested System.Text.Json behavior
Class with mutable properties populate populate
Class with parameterless constructor and read-only properties populate but no effect populate but no effect
Class with parametrized constructor matching all properties and read-only properties populate but no effect * populate but no effect *
Struct with parameterless constructor and mutable properties replace but start from default, ctor never called read struct, populate on copy, set struct - see Open questions
Struct with parametrized constructor and mutable properties replace, uses ctor read struct, populate on copy, set struct - see Open questions
Struct with parameterless constructor and read-only properties replace with default as above but will have no effect because there are no setters
Struct with parametrized constructor matching all properties and read-only properties replace, uses ctor as above but will have no effect because there are no setters
  • * if starting from null all properties are assigned but no effect on further populate calls

JsonSerializerOptions.DefaultCreationHandling potential issues

  • global option is not enforcement - we will populate only what's possible - this is different from property semantics:
    • converter must be allow populating, currently only internal converters can decide on that
    • for value types setter must be available
    • if type allows polymorphism it must not define type discriminator
  • adding support for polymorphic deserialization for populate or immutable types will be a breaking change (same applies for allowing attribute on types but attribute is much more scoped) - currently this doesn't make much sense but it's possible we will allow for enough customizability in the converters in the future that this will be highly requested
  • any property which can be populated but has runtime type which doesn't allow populating (i.e. IList implementation with IsReadonly = true) will cause serialization errors - we may potentially soften this and make it a no-op or replace in that case but it seems that would be the worst of the worlds where user can't tell if populate will happen or not

Interaction with required properties

Exactly the same rules apply as if Replace behavior is used. All properties marked with required need to be in the payload regardless of Populate or Replace.

Interaction with polymorphic types

If type discriminator is not specified polymorphism does not affect deserialization and therefore such combination is allowed and non-problematic.

When type discriminator is specified for now we recommend that this combination will not be allowed and we can re-iterate this in the future based on demand. There is too much ambiguity how it should be handled and depending on the use case different behavior might be expected - see code below.

Another consideration is that if we ever allow for type discrimnator to not occur first in the payload allowing this now could enforce design we haven't made yet - it's better to delay this decision.

Consider this code which shows ambiguity:

var obj = JsonSerializer.Deserialize<SomeObj>("""{"Foo":{"$type":"derived2", ...}}""");
// should this cause error, obj is Derived1 (treat `$type` as unmapped property and continue as Derived1) or replace with Derived2? Depending on use case different behavior might be expected.

class SomeObj
{
  [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
  public BaseType Foo { get; set; } = new Derived1();
}

[JsonDerivedType(typeof(Derived1), "derived1")]
[JsonDerivedType(typeof(Derived2), "derived2")]
class BaseType {}

class Derived1 : BaseType { /* ... */ }

class Derived2: BaseType { /* ... */ }

Immutable types (i.e. ImmutableArray) interaction

Current recommendation is that we do not allow this combination.

Current converter infrastructure does not allow to add this support and changing that would be significant amount of work.
On top of that Populate wouldn't actually re-use the object but rather create new object with appended elements and therefore semantics would be different than for actual populate.

Because of that, for now we will be producing error. We can re-iterate on this behavior in the future once converter infrastructure is ready to support that - that would require extra switch if we provide global setting in the current design.

IgnoreReadOnlyProperties interaction with read-only properties with Populate option

Not allowed. While this could be theoretically supported the setting name suggest that should not happen. As a workaround user can add a setter which throws.

Open questions

  • Should value types allow to Populate? Current recommendation is we allow this as long as there is setter available. We will modify a copy of a struct and then reassign it once we're done - it does deviate a bit from "Populate" idea which reuses object but we think this makes most sense from usability point of view
  • Should deserializing null value be allowed for populated properies? (i.e. {"SomePropertyWhichShouldBePopulated":null}) Recommendation is: yes it should be allowed as long as there is a setter. It's possible to easily disallow this behavior by replacing JsonPropertyInfo.Set if needed to reject null but if we disallow it the workaround will be much harder.

Reference handling interaction

Not allowed for now. It seems this kind of combination is unintended but we can re-iterate on that in the future if we find valid scenario. It would likely require separate options switch.

Callbacks interaction

Both OnDeserializing and OnDeserialized will be called regardless of Populate setting. OnDeserializing signifies that deserialization started and therefore it should be called.

TODOs

  • JsonTypeInfo, JsonSerializerContext PopulateObject overloads (this piece is cut out of the current proposal)
  • prototype
  • API review
  • Implementation + tests
  • Docs/blog-post

Related issues:

@krwq krwq added this to the 8.0.0 milestone Nov 18, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@krwq krwq self-assigned this Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

Support modifying already initialized properties and fields when deserializing JSON

Currently properties and fields which are read-only cannot be deserialized. This scenario could be supported though. Consider following example:

A obj = JsonSerializer.Deserialize<A>("""{"Foo": {"Value": 5}}""");

class A
{
    public B Foo { get; } = new B();
}

class B
{
  public int Value { get; set; }
}

Currently will be a no-op because we do not re-use value of property Foo.

Similarly another example:

A obj = JsonSerializer.Deserialize<A>("""{"Elements": [4,5,6]}""");

class A
{
	public List<int> Elements { get; } = new List<int>() { 1, 2, 3 };
}

will cause no-op while many users would expect Elements to end up with 1, 2, 3, 4, 5, 6.

There are currently no easy workaround for this - only writing your own custom converter.

What does JSON.NET offer?

In Newtonsoft.Json currently most of these scenarios work by default. To control this behavior it has JsonSerializerSettings.ObjectCreationHandling property which allows to set one of three enum values:

  • ObjectCreationHandling.Reuse - tries reusing object when possible
  • ObjectCreationHandling.Auto - same as Reuse
  • ObjectCreationHandling.Replace - always replaces object with new one (similarly to our current System.Text.Json behavior)

Regardless of choice if property/field is null, it will always create new instance.

There are couple of things which are not exactly obvious with how JSON.NET handles reuse though:

  • it looks at the runtime type rather than property type - i.e. for public IEnumerable<int> SomeProperty { get; } = new List<int>() it will append even though type of property is theoretically read-only
  • if runtime type is known read-only collection (i.e. ReadOnlyCollection<T> which can be created from i.e. new List<int>() { 1, 2, 3}.AsReadOnly()) it will replace object and also try to match the same type for new object
  • if runtime type is custom read-only collection (i.e. custom implemented IList<T> with IsReadOnly = true) the deserialization ends up with silent no-op - nothing will get deserialized and no exception will happen
  • if deserialized field is a struct it will always replace it - on top of that it won't ever call parameterless constructor - if only parametrized exists it will be called. For properties the same behavior will occur but properties always return copy on getters and therefore replacing is justified.

Suggestion for System.Text.Json

  • System.Text.Json generally doesn't look at the runtime type and doing something else here would seem inconsistent. We could potentially consider that in the future.
  • Current behavior is to always replace and changing that would be presumably a massive breaking change, because of that default should stay as is.
  • Creating object when property/field is null seems like an overall good design and we'd also follow that (assuming property is writable).
  • Creating global "enable populate object" switch might be problematic for collections like IList<T> with IsReadOnly = true where behavior might be non-intuitive and silent fallback to replace seems like too much heuristic.
  • Suggested behavior is to make it possible to enable this behavior only per property and if opt-in was specifically asked for we should always populate (minus null case) and never fall-back to replacing object
  • IList<T>.IsReadonly and similar cases for other collections may throw at runtime if Populate is set but collections reports read-onliness
  • Global switch can be emulated with contract resolver modifier which can enable it for properties which can support it. Initally we will not provide such solution as public API but can provide it as a code sample.

Suggestion for System.Text.Json with comparison with Newtonsoft.JSON

Following table shows how property/field with Populate will behave for collections in System.Text.Json with comparison to Newtonsoft.JSON. Note that table does not mention Dictionary - like types but similar rules apply:

PropertyType RuntimeType Newtonsoft.JSON behavior Suggested System.Text.Json behavior
List<T> List<T> populate populate
IList<T> List<T> populate populate
IReadOnlyList<T> List<T> populate not allowed
IEnumerable<T> List<T> populate not allowed
IList<T> ReadOnlyCollection<T> replace with new ReadOnlyCollection<T> runtime error - user asked to populate but we're unable to fulfill
IList<T> custom implementation with IsReadOnly=true no-op - list is not replaced or modified runtime error - user asked to populate but we're unable to fulfill
IList<T> custom implementation with IsReadOnly=false populate populate
T[] T[] replace not allowed
ImmutableArray<T> ImmutableArray<T> replace not allowed for now, see section below

Following table shows how property/field with Populate will behave for value types and reference types with comparison to Newtonsoft.JSON:

Property/Field type description Newtonsoft.JSON behavior Suggested System.Text.Json behavior
Class with mutable properties populate populate
Class with parameterless constructor and read-only properties populate but no effect populate but no effect
Class with parametrized constructor matching all properties and read-only properties populate but no effect * populate but no effect *
Struct with parameterless constructor and mutable properties replace but start from default, ctor never called read struct, populate on copy, set struct - see Open questions
Struct with parametrized constructor and mutable properties replace, uses ctor read struct, populate on copy, set struct - see Open questions
Struct with parameterless constructor and read-only properties replace with default as above but will have no effect because there are no setters
Struct with parametrized constructor matching all properties and read-only properties replace, uses ctor as above but will have no effect because there are no setters
  • * if starting from null all properties are assigned but no effect on further populate calls

Interaction with required properties

Exactly the same rules apply as if Replace behavior is used. All properties marked with required need to be in the payload regardless of Populate or Replace.

Interaction with polymorphic types

It's not clear how to handle type discriminator for those cases and if we allowed it, it could potentially allow to bypass strict rules we've introduced unless we only allow to assign members of closest allowed ancestor - there is too much ambiguity here.

Another consideration is that if we ever allow for type discrimnator to not occur first in the payload allowing this now could enforce design we haven't made yet and therefore it's better to delay this decision.
For now we recommend that this combination will not be allowed and we can re-iterate this in the future based on demand.

Immutable types (i.e. ImmutableArray) interaction

It's not immediatelly clear whether behavior of this feature will be replacing with collection with appended element or replacing with new collection.

Because of that, for now we will be producing error. We can re-iterate on this behavior in the future - it would likely need extra extensibility in JsonConverter or in the contract model to solve this for generic case.

Open questions

  • should value types allow to Populate? For fields that maybe makes more sense but for properties you'd normally get a copy if you called a getter - from metadata point of view there is no distinction between field or property though
    • one consistent solution could be to require setters for value types and do mutation by getting the copy, doing mutation, setting a mutated copy; If we go this path then can we still call it Populate?

API Proposal

namespace System.Text.Json.Serialization;

public enum JsonObjectCreationHandling
{
    Replace = 0,
    Reuse = 1,
}

[System.AttributeUsageAttribute(System.AttributeTargets.Field | System.AttributeTargets.Property, AllowMultiple = false)]
public sealed partial class JsonObjectCreationHandlingAttribute : System.Text.Json.Serialization.JsonAttribute
{
    public JsonObjectCreationHandlingAttribute(System.Text.Json.Serialization.JsonObjectCreationHandling handling) { }
    public System.Text.Json.Serialization.JsonObjectCreationHandling Handling { get { throw null; } }
}

namespace System.Text.Json.Serialization.Metadata;
public abstract partial class JsonPropertyInfo
{
    public JsonObjectCreationHandling CreationHandling { get; set; } // JsonObjectCreationHandling.Replace by default
}

Related issues:

Author: krwq
Assignees: -
Labels:

area-System.Text.Json

Milestone: 8.0.0

@krwq
Copy link
Member Author

krwq commented Nov 18, 2022

cc: @JamesNK

@krwq krwq added User Story A single user-facing feature. Can be grouped under an epic. Cost:XL Work that requires one engineer more than 4 weeks partner-impact This issue impacts a partner who needs to be kept updated labels Nov 18, 2022
@eiriktsarpalis
Copy link
Member

Sharing a workaround from #29538 (comment) providing a workaround for PopulateObject in .NET 7:

@RicoSuter that's a cool idea, however one issue I'm seeing is that you're creating a fresh JsonSerializerOptions instance on every serialization. This would result in metadata needing to be recomputed every time, killing serialization performance. I've adapted your example somewhat so that the same metadata can be used across serializations:

using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Text.Json;
using System.Text.Json.Serialization.Metadata;

var value = new MyPoco();
JsonSerializerExt.PopulateObject("""{"Value":42}""", typeof(MyPoco), value);
Console.Write(value.Value); // 42

public class MyPoco
{
    public int Value { get; set; }
}

public static class JsonSerializerExt
{
    // Dynamically attach a JsonSerializerOptions copy that is configured using PopulateTypeInfoResolver
    private readonly static ConditionalWeakTable<JsonSerializerOptions, JsonSerializerOptions> s_populateMap = new();

    public static void PopulateObject(string json, Type returnType, object destination, JsonSerializerOptions? options = null)
    {
        options = GetOptionsWithPopulateResolver(options);
        PopulateTypeInfoResolver.t_populateObject = destination;
        try
        {
            object? result = JsonSerializer.Deserialize(json, returnType, options);
            Debug.Assert(ReferenceEquals(result, destination));
        }
        finally
        {
            PopulateTypeInfoResolver.t_populateObject = null;
        }
    }

    private static JsonSerializerOptions GetOptionsWithPopulateResolver(JsonSerializerOptions? options)
    {
        options ??= JsonSerializerOptions.Default;

        if (!s_populateMap.TryGetValue(options, out JsonSerializerOptions? populateResolverOptions))
        {
            JsonSerializer.Serialize(value: 0, options); // Force a serialization to mark options as read-only
            Debug.Assert(options.TypeInfoResolver != null);

            populateResolverOptions = new JsonSerializerOptions(options)
            {
                TypeInfoResolver = new PopulateTypeInfoResolver(options.TypeInfoResolver)
            };

            s_populateMap.TryAdd(options, populateResolverOptions);
        }

        Debug.Assert(options.TypeInfoResolver is PopulateTypeInfoResolver);
        return populateResolverOptions;
    }

    private class PopulateTypeInfoResolver : IJsonTypeInfoResolver
    {
        private readonly IJsonTypeInfoResolver _jsonTypeInfoResolver;
        [ThreadStatic]
        internal static object? t_populateObject;

        public PopulateTypeInfoResolver(IJsonTypeInfoResolver jsonTypeInfoResolver)
        {
            _jsonTypeInfoResolver = jsonTypeInfoResolver;
        }

        public JsonTypeInfo? GetTypeInfo(Type type, JsonSerializerOptions options)
        {
            var typeInfo = _jsonTypeInfoResolver.GetTypeInfo(type, options);
            if (typeInfo != null && typeInfo.Kind != JsonTypeInfoKind.None)
            {
                Func<object>? defaultCreateObjectDelegate = typeInfo.CreateObject;
                typeInfo.CreateObject = () =>
                {
                    object? result = t_populateObject;
                    if (result != null)
                    {
                        // clean up to prevent reuse in recursive scenaria
                        t_populateObject = null;
                    }
                    else
                    {
                        // fall back to the default delegate
                        result = defaultCreateObjectDelegate?.Invoke();
                    }

                    return result!;
                };
            }

            return typeInfo;
        }
    }
}

@johncrim
Copy link

johncrim commented Dec 2, 2022

I opened System.Text.Json.JsonSerializer doesn't deserialize into existing sub-objects in 2019 and would still love to see a proper fix for this. The fact that there's a new issue for this might be promising? (4th one I've subscribed to.)

The only reason I didn't just up-vote this and skip commenting is:
I question whether there's a need for a new JsonSerializer.PopulateObject() method in the public API. Shouldn't object re-use be a Serialization option? Normally I would just want to set this behavior for all serialization instead of having to use a new API for Serialization. It's particularly important that nested objects be re-used, and I shouldn't have to use a new API for proper behavior (re-use should have been the default, it was a flaw in System.Text.Json design).

@BrunoBlanes
Copy link
Contributor

I know that making this the default behavior would be a breaking change and agree it shouldn't be done, however I don't understand the need for new public methods as mentioned by @johncrim.

At the start System.Text.Json didn't support JSON Reference, so when that eventually came, you lot came up with JsonSerializerOptions.ReferenceHandler which works great and elegantly, why can't it be done again? Is there a valid reason for those methods?

@krwq
Copy link
Member Author

krwq commented Dec 7, 2022

PopulateObject is specifically for root level object scenarios, i.e.:

Foo obj = new Foo() { BarObj = { /* ... */ } };
JsonSerializer.PopulateObject<Foo>(ref obj, "some payloadhere");

class Foo
{
  public Bar BarObj { get; set; }
}
class Bar
{
   // ...
}

Honestly I'm not sure if we would do PopulateObject methods on the first iteration anyway since that's a bit of work given all overloads (especially around testing) and I'm not sure we will have enough time.

@BrunoBlanes can you describe how you'd imagine handling scenario like I mentioned?

@BrunoBlanes
Copy link
Contributor

BrunoBlanes commented Dec 7, 2022

@krwq I don't think it would be confusing to just use overloads:

public static void Deserialize<TValue>(ref TValue target, string payload, System.Text.Json.JsonSerializerOptions? options = null) where TValue : class { throw null; }

As long as they are well documented I think it would be more intuitive, but maybe that's not a consent (or even possible to overload?).

@johncrim
Copy link

Thank you for the clarification @krwq - I suppose I should have been able to figure that out from the signatures.

I agree with @BrunoBlanes - I think reusing the method name Deserialize with a new overload pattern is clearer than adding a new concept PopulateObject. With the name PopulateObject a developer will wonder how it is different from deserialization, but if the intent is just to provide explicit/root support for deserializing into an existing object, a Deserialize overload seems like a better way to go.

@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 16, 2023
@bartonjs
Copy link
Member

  • We renamed JsonSerializerOptions.DefaultCreationHandling to PreferredObjectCreationHandling both to indicate that the setting may have contextual fallbacks, and to keep "object" in the name so it doesn't sound like it applies to serializing
  • We also added Object into JsonPropertyInfo.CreationHandling (now ObjectCreationHandling)
  • Just Handling seems fine on JsonObjectCreationHandlingAttribute, and consistent with the other Json*Handling attribute/options.
  • During the feature discussion we discussed "not allowed /for now/" on immutable arrays, and decided that they are not allowed for-ever, because of breaking change complexities.
namespace System.Text.Json.Serialization;

public enum JsonObjectCreationHandling
{
    Replace = 0,
    Populate = 1,
}

// NOTE: System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Interface
//           For Property/Field - Populate is enforced
//           For Class/Struct/Interface - Populate is applied to properties where applicable
[AttributeUsage(
  AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface,
  AllowMultiple = false)]
public sealed partial class JsonObjectCreationHandlingAttribute : System.Text.Json.Serialization.JsonAttribute
{
    public JsonObjectCreationHandlingAttribute(System.Text.Json.Serialization.JsonObjectCreationHandling handling) { }
    public System.Text.Json.Serialization.JsonObjectCreationHandling Handling { get { throw null; } }
}

namespace System.Text.Json.Serialization.Metadata;

public abstract partial class JsonPropertyInfo
{
    public JsonObjectCreationHandling ObjectCreationHandling { get; set; } // Replace remains default behavior
}

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; } /*= JsonObjectCreationHandling.Replace; */
}

@bartonjs bartonjs removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 28, 2023
@bartonjs bartonjs added the api-approved API was approved in API review, it can be implemented label Feb 28, 2023
@dersia
Copy link

dersia commented Feb 28, 2023

I would really love for this to allow a PopulateIfNull behavior by adding IfNull to the JsonObjectCreationHandling enum. I would like to use this for event sourcing where I populate from the event stream in reverse order and there I would like for it to only populate the property if it is null. This way I will end up with an up to date dto way quicker than going through the whole event stream and applying all events. This would than also mean that for collections it wouldn't append.

I know that this would work for all event sourcing projections, but this would solve ~90% of my projections and for the other 10% I would use the populate feature as suggested here or some custom logic.

This would make the "patching" stronger.

@eiriktsarpalis
Copy link
Member

@dersia "populate" in this context means reusing any instance already present on a deserialized property. Using that definition I'm not sure what populating null means... FWIW under the current proposal the deserializing will fall back to the default replace behavior if the property contains null.

@dersia
Copy link

dersia commented Mar 1, 2023

@eiriktsarpalis yeah this is exactly what I understand what populate does. Let me try to clarify what I am doing. Imagine the following dto/poco

public partial record User
{
    [JsonPropertyName("userId")] 
    public string? UserId { get; set; } = Guid.NewGuid().ToString();
    [JsonPropertyName("firstname")] 
    public string? Firstname { get; set; } = null;
    [JsonPropertyName("lastname")] 
    public string? Lastname { get; set; } = null;
    [JsonPropertyName("address")] 
    public string? Adrese { get; set; } = null;
    [JsonPropertyName("housenumber")] 
    public string? Housenumber { get; set; } = null;
    [JsonPropertyName("postcode")] 
    public string? Postcode { get; set; } = null;
    [JsonPropertyName("city")] 
    public string? City { get; set; } = null;
    [JsonPropertyName("studentId")] 
    public string? StudentId { get; set; } = null;
    [JsonPropertyName("height")] 
    public double? Height{ get; set; } = null;
}

Now there are different Event-DTOs

public partial record UserAddedEvent
{
    [JsonPropertyName("userId")] 
    public required string UserId { get; set; }
    [JsonPropertyName("firstname")] 
    public string? Firstname { get; set; } = null;
    [JsonPropertyName("lastname")] 
    public string? Lastname { get; set; } = null;
}

public partial record UserChangedEvent
{
    [JsonPropertyName("userId")] 
    public required string UserId { get; set; }
    [JsonPropertyName("firstname")] 
    public string? Firstname { get; set; } = null;
    [JsonPropertyName("lastname")] 
    public string? Lastname { get; set; } = null;
}

public partial record AddressSetForUserEvent
{
    [JsonPropertyName("userId")] 
    public required string UserId { get; set; }
    [JsonPropertyName("address")] 
    public string? Adrese { get; set; } = null;
    [JsonPropertyName("housenumber")] 
    public string? Housenumber { get; set; } = null;
    [JsonPropertyName("postcode")] 
    public string? Postcode { get; set; } = null;
    [JsonPropertyName("city")] 
    public string? City { get; set; } = null;
}

public partial record StudentIdSetForUserEvent
{
    [JsonPropertyName("userId")] 
    public required string UserId { get; set; }
    [JsonPropertyName("studentId")] 
    public string? StudentId { get; set; } = null;
}

public partial record HreightSetForUserEvent
{
    [JsonPropertyName("userId")] 
    public required string UserId { get; set; }
    [JsonPropertyName("height")] 
    public double? Height { get; set; } = null;
}

Now when the Event UserAdded comes I create a new instance of User from the UserAddedEvent and save it to the statestore.
Now a few hours later the AdressSetForUserEvent comes in and I load the instance from the statestore and populate it with the new event. Same for the other two. Now so far so good and this would all work fine with the populate spec'd in this issue.

The problem that I'd like to solve is, that I don't know in what order the events come in and I'd like for the populate to only populate what is left null on the instance of User. So my EventStream could look like that:

UserAdded, AdressSet, HeightSet, AddressSet, HeightSet, UserChanged, StudentNumberSet, UserChanged, AdressSet

Now in a usual EventSourcing manner I would walk the whole stream and apply/populate the User instance. Since my EventStream can be very long and it might take a long time to have a up-to-date state, I do it in reverse order and check at what point my model is fully populated, so I can stop walking the EventStream when rehydrating. So I would be done with AddressSet, UserChanged, StudentNumberSet, UserChanged, HeightSet. In this case the second UserChanged would not replace the properties, because they are not null anymore.

I hope this makes more sense and clarifies my case.

Cheers,
Sia

@krwq
Copy link
Member Author

krwq commented Mar 1, 2023

@dersia you probably want ShouldDeserialize callback similar to ShouldSerialize we currently support rather than another bool/enum flag. I'd suggest to create an issue about it and describe your scenario and proposed APIs there.

@krwq
Copy link
Member Author

krwq commented Mar 1, 2023

Actually when I think about it you can just replace JsonPropertyInfo.Set in your contract model and either do nothing or throw when current value is not null

@eiriktsarpalis
Copy link
Member

@dersia Bare minimum I think that would require some for of top-level populate method support, which was cut from this iteration of the feature.

Apropos, when folding state in event sourcing I would suggest making your apply function a part of your domain layer rather than delegating it to any deserializer.

@dersia
Copy link

dersia commented Mar 3, 2023

Yeah, so far I was doing it that way, but I was thinking about awaitable enumeration with cosmos client and only read the stream as far as needed and stop as soon as possible and hand of some of the logic to the serializer. My thinking is, that the domain should only be aware of when the complete state is reached and should not have any other logic, so it would be agnostic to the domain how it is hydrated and from where...

Now I'll continue putting that logic somewhere in the middle, so that the domain is still unaware, probably will create a custom serializer. Thanks for the replys, though 😊

PS: I am looking forward to the top-level populate methods that weren't part of this proposal, I think those can be super handy.

@krwq
Copy link
Member Author

krwq commented Mar 16, 2023

I'd like to request changes to approved shape of APIs:

  • change JsonPropertyInfo.JsonObjectCreationHandling to nullable - null means there is no attribute; setting to null means: 'cancel out effect of attribute' - now the value will be more consistent with remaining properties. It also simplifies implementation: previously we had to resolve and use default converter to find initial value which might have not been compatible with final value - this is not currently major issue but we have 2 issues open which could potentially make it bigger problem in the future
  • add JsonTypeInfo.PreferredPropertyObjectCreationHandling which reflects attribute on type - null means there is no attribute; setting to null cancel effects of attribute i.e. all properties will by default try do whatever options default do - previously this value changed. Note it's now easier to document behavior and how attribute applies to properties of this type not to the type itself

final shape

namespace System.Text.Json.Serialization;

public enum JsonObjectCreationHandling
{
    Replace = 0,
    Populate = 1,
}

// NOTE: System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Interface
//           For Property/Field - Populate is enforced
//           For Class/Struct/Interface - Populate is applied to properties where applicable
[AttributeUsage(
  AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface,
  AllowMultiple = false)]
public sealed partial class JsonObjectCreationHandlingAttribute : System.Text.Json.Serialization.JsonAttribute
{
    public JsonObjectCreationHandlingAttribute(System.Text.Json.Serialization.JsonObjectCreationHandling handling) { }
    public System.Text.Json.Serialization.JsonObjectCreationHandling Handling { get { throw null; } }
}

namespace System.Text.Json.Serialization.Metadata;

public abstract partial class JsonPropertyInfo
{
    public JsonObjectCreationHandling? ObjectCreationHandling { get; set; }
}

public abstract partial class JsonTypeInfo
{
    public JsonObjectCreationHandling? PreferredPropertyObjectCreationHandling { get; set; }
}


namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; } /*= JsonObjectCreationHandling.Replace; */
}

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-approved API was approved in API review, it can be implemented labels Mar 16, 2023
@terrajobst
Copy link
Member

Seems too much for email, marking as for review again

@terrajobst
Copy link
Member

terrajobst commented Mar 16, 2023

Video

  • Didn't realize it's a tiny change; @eiriktsarpalis represented it. Looks good as proposed.
namespace System.Text.Json.Serialization;

public enum JsonObjectCreationHandling
{
    Replace = 0,
    Populate = 1,
}

// NOTE: System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Interface
//           For Property/Field - Populate is enforced
//           For Class/Struct/Interface - Populate is applied to properties where applicable
[AttributeUsage(
  AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface,
  AllowMultiple = false)]
public sealed partial class JsonObjectCreationHandlingAttribute : System.Text.Json.Serialization.JsonAttribute
{
    public JsonObjectCreationHandlingAttribute(System.Text.Json.Serialization.JsonObjectCreationHandling handling) { }
    public System.Text.Json.Serialization.JsonObjectCreationHandling Handling { get { throw null; } }
}

namespace System.Text.Json.Serialization.Metadata;

public abstract partial class JsonPropertyInfo
{
    public JsonObjectCreationHandling? ObjectCreationHandling { get; set; }
}

public abstract partial class JsonTypeInfo
{
    public JsonObjectCreationHandling? PreferredPropertyObjectCreationHandling { get; set; }
}


namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; } /*= JsonObjectCreationHandling.Replace; */
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 16, 2023
@VincentH-Net
Copy link

VincentH-Net commented Mar 20, 2023

@eiriktsarpalis unfortunately, the workaround you provided throws a NRE if the target type contains e.g. an object of this type:

public record BasketItem(int ProductId = 0, string ProductName = "", decimal UnitPrice = 0, int Quantity = 0)

(defaultCreateObjectDelegate is null for this type)
However NewtonSoft.Json's PopulateObject does support this.

Perhaps good to document this limitation in the workaround (or even better update it with equivalent support to what Newtonsoft offers)? Thanks

Sharing a workaround from #29538 (comment) providing a workaround for PopulateObject in .NET 7:

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 20, 2023
@eiriktsarpalis
Copy link
Member

unfortunately, the workaround you provided throws a NRE if the target type contains e.g. an object of this type:
(defaultCreateObjectDelegate is null for this type)

Sounds like a bug, could you create a separete issue please? Thanks.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 25, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators May 25, 2023
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 Cost:XL Work that requires one engineer more than 4 weeks partner-impact This issue impacts a partner who needs to be kept updated User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants