Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 11 commits
d284332
e38c7c5
1442800
dca1d71
8729383
2181840
e342f34
491648d
3b813ec
c6cd93d
5f6d394
01b1f27
9c6ca2c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?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.
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.Not in the prototype, but you're making a good point that this should be supported. We would probably need to make the
JsonKnownTypeAttribute
acceptobject
parameters but require that each type uses a unique identifier type, so the following configuration would be invalid:As far as supported identifier types go, am I right to assume that only
string
andint
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.
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 I'm aware, yes, only
string
andint
are used for these. However, I think it may be convenient to support enum types too, as they're easily converted toint
, and are (IMO) the natural type to use for a discriminator.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.