-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add a design document for System.Text.Json polymorphic serialization #226
Add a design document for System.Text.Json polymorphic serialization #226
Conversation
Co-authored-by: Christopher Watford <83599748+watfordsuzy@users.noreply.github.com>
Co-authored-by: Christopher Watford <83599748+watfordsuzy@users.noreply.github.com>
Co-authored-by: Christopher Watford <83599748+watfordsuzy@users.noreply.github.com>
```csharp | ||
public class JsonSerializerOptions | ||
{ | ||
public Func<Type, bool> SupportedPolymorphicTypes { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property name seems to suggest it's a collection of types? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having similar thoughts. What would be some suggested alternatives? SupportedPolymorphicTypesPredicate
perhaps?
} | ||
}; | ||
``` | ||
or alternatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why provide two models? Could we get away with just this one (non-generic)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having just the non-generic one should be sufficient, however the generic one is type-safe so users might appreciate having that option.
At the core of the design is the introduction of `JsonKnownType` attribute that can | ||
be applied to type hierarchies like so: | ||
```csharp | ||
[JsonKnownType(typeof(Derived1), "derived1")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would users be able to specify the name of the discriminator field in the serialized model here? The examples use $type
, but not all web APIs which use discriminated union-like types use that naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore: are non-string values permitted? It's also often the case that web APIs use integers for these discriminator values, which would often be mapped to an enum in C# code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would users be able to specify the name of the discriminator field in the serialized model here? The examples use $type, but not all web APIs which use discriminated union-like types use that naming.
Yes, that is something we are considering although it has not been included in the prototype for now. There are a few infrastructural problems to be sorted out, for example our metadata parsing implementation currently requires that all metadata properties start with $
and are placed before any other properties in the object.
Furthermore: are non-string values permitted? It's also often the case that web APIs use integers for these discriminator values, which would often be mapped to an enum in C# code.
Not in the prototype, but you're making a good point that this should be supported. We would probably need to make the JsonKnownTypeAttribute
accept object
parameters but require that each type uses a unique identifier type, so the following configuration would be invalid:
[JsonKnownType(typeof(Bar), 1)]
[JsonKnownType(typeof(Baz), "baz")]
public class Foo {}
As far as supported identifier types go, am I right to assume that only string
and int
values should be accepted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example our metadata parsing implementation currently requires that all metadata properties start with
$
and are placed before any other properties in the object.
Yeah - in my own converters I've resorted to making a copy of the reader, finding and deserializing the discriminator, and then using the reader copy to deserialize as the actual type, updating the original reader with the copy after the fact. While this fixes the "must be the first field in the JSON object" issue, it's not that great because it parses the value multiple times.
As far as supported identifier types go, am I right to assume that only string and int values should be accepted?
As far as I'm aware, yes, only string
and int
are used for these. However, I think it may be convenient to support enum types too, as they're easily converted to int
, and are (IMO) the natural type to use for a discriminator.
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
I've split this design document into to separate API proposals in the relevant issues: dotnet/runtime#29937 and dotnet/runtime#30083. |
Draft Implementation PR