brantburnett
commented
1 year ago At this point I don't believe we can safely implement the built-in approach to polymorphism. It would require too many compromises compared to the current implementation and compared to how JSON schemas work in OpenAPI specifications.
First Attribute
This is probably the biggest limitation. The built-in polymorphic deserializer requires that the discriminator attribute be the first attribute in the object. This has clear advantages in terms of performance, so I understand the limitation. In particular, our approach using a JsonConverter requires that the entire object be read into a buffer from the asynchronous stream before it can try to deserialize it. This is the case even if the attribute is the first in the stream. This is because the converter is synchronous and can't dynamically request more data from the stream as needed.
However, there is no guarantee provided by OpenAPI and schemas that the discriminator will be the first attribute in the object. In order to maintain the highest degree of compatibility, we can't work with this limitation.
Discriminator on the POCO
To properly use built-in polymorphism, the discriminator property shouldn't be included on the POCO. It is added dynamically during serialization, and including it on the POCO causes conflicts. However, our SDKs aren't designed to be JSON only (though they generally are today). But, in theory, we could be mixing XML and JSON serialization on the same POCOs, and the XML flavor could require the attribute.
Additionally, the child schema could be referenced by an operation directly without polymorphism, as a direct reference, but still expect the discriminator to be present. Built-in polymorphism won't include the discriminator in that scenario.
The first part of this may be addressed with a JsonIgnore attribute. However, this would then also break the second part of this for multi-use schemas.
Shared POCOs
OpenAPI discriminators using explicit mappings may reference the same child schema for more than one discriminator value. When this occurs, the SDK only has a single POCO and uses our separate discriminator property to write the correct value. With built-in polymorphism this throws an exception because it can't know which discriminator value to serialize based on the POCO type.
This could be addressed by dynamically generated separate schemas, but this would then create compatibility issues with XML serializers, etc. It could also cause SemVer breaks in the generated SDKs as mappings are added in new API versions.
We shouldn't need our custom approach to polymorphism in System.Text.Json anymore, now that it's built-in. The built-in version is doubtless better tested and more performant.