-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
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. |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsSupport modifying already initialized properties and fields when deserializing JSONCurrently 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 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 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:
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:
Suggestion for System.Text.Json
Suggestion for System.Text.Json with comparison with Newtonsoft.JSONFollowing table shows how property/field with
Following table shows how property/field with
Interaction with required propertiesExactly the same rules apply as if Interaction with polymorphic typesIt'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. Immutable types (i.e. ImmutableArray) interactionIt'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
API Proposalnamespace 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:
|
cc: @JamesNK |
Sharing a workaround from #29538 (comment) providing a workaround for
|
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 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 |
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? |
@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?). |
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 |
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; */
} |
I would really love for this to allow a 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. |
@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. |
@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. 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, |
@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. |
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 |
@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. |
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. |
I'd like to request changes to approved shape of APIs:
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; */
} |
Seems too much for email, marking as for review again |
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; */
} |
@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) ( Perhaps good to document this limitation in the workaround (or even better update it with equivalent support to what Newtonsoft offers)? Thanks
|
Sounds like a bug, could you create a separete issue please? Thanks. |
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:
Currently will be a no-op because we do not re-use value of property
Foo
.Similarly another example:
will cause no-op while many users would expect
Elements
to end up with1, 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
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:
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:
public IEnumerable<int> SomeProperty { get; } = new List<int>()
it will append even though type of property is theoretically read-onlyReadOnlyCollection<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 objectIList<T>
withIsReadOnly = true
) the deserialization ends up with silent no-op - nothing will get deserialized and no exception will happenSuggestion for System.Text.Json
null
case) and never fall-back to replacing objectIList<T>.IsReadonly
being true while property is populatable (and similar cases for other collections) will cause serializer (converter specifically) to throwSuggestion 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, Deserializeruntime 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 deserializationFollowing 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 mentionDictionary
- like types but similar rules apply:List<T>
List<T>
IList<T>
List<T>
IReadOnlyList<T>
List<T>
IEnumerable<T>
List<T>
IList<T>
ReadOnlyCollection<T>
ReadOnlyCollection<T>
IList<T>
IList<T>
T[]
T[]
ImmutableArray<T>
ImmutableArray<T>
Following table shows how property/field with
Populate
will behave for value types and reference types with comparison to Newtonsoft.JSON:*
*
default
, ctor never calleddefault
*
if starting from null all properties are assigned but no effect on further populate callsJsonSerializerOptions.DefaultCreationHandling potential issues
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 notInteraction with required properties
Exactly the same rules apply as if
Replace
behavior is used. All properties marked withrequired
need to be in the payload regardless ofPopulate
orReplace
.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:
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
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{"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
Related issues:
The text was updated successfully, but these errors were encountered: