RicoSuter / NJsonSchema

JSON Schema reader, generator and validator for .NET
http://NJsonSchema.org
MIT License
1.38k stars 532 forks source link

[NSwag 14/NJsonSchema11] JsonPolymorhic/JsonDerivedType TypeDiscriminatorPropertyName with enum/int values breaks #1666

Open EelcoLos opened 8 months ago

EelcoLos commented 8 months ago

Expected Behavior

A derived class with a custom TypeDiscriminator property name, which is based on an enum (therefore the cast will be of type int) will be cast properly in nswag

public enum EDocumentItemType
{
    Invalid = 0,
    Document = 1,
    Type2 = 2,
    Type3 = 3,
    Type4 = 4,
    Type5 = 5,
    Type7 = 7
}
[JsonPolymorphic(TypeDiscriminatorPropertyName = "documentItemType")]
[JsonDerivedType(typeof(Document), typeDiscriminator: 1)]
[JsonDerivedType(typeof(Type2), typeDiscriminator: 2)]
[JsonDerivedType(typeof(Type3), typeDiscriminator: 3)]
[JsonDerivedType(typeof(Type4), typeDiscriminator: 4)]
[JsonDerivedType(typeof(Type5), typeDiscriminator: 5)]
[JsonDerivedType(typeof(Type7), typeDiscriminator: 7)]
public class DocumentItem : IDeletedOn, ICreatedOn, IModifiedOn
{
    public Guid Id { get; set; }
    public DateTime? DeletedOn { get; set; }
    public DateTime CreatedOn { get; set; }
    public DateTime ModifiedOn { get; set; }
}
public class Document : DocumentItem
{
    public EDocumentItemType DocumentItemType { get; } = EDocumentItemType.Document;
}

in NSwag 13.2.0, this is generated properly:

"Document": {
        "allOf": [
          {
            "$ref": "#/components/schemas/DocumentItem"
          },
          {
            "type": "object",
            "additionalProperties": false,
            "properties": {
              "documentItemType": {
                "$ref": "#/components/schemas/EDocumentItemType"
              },
"EDocumentItemType": {
        "type": "integer",
        "description": "",
        "x-enumNames": [
          "Invalid",
          "Document",
          "Type2",
          "Type3",
          "Type4",
          "Type5",
          "Type7"
        ],
        "enum": [
          0,
          1,
          2,
          3,
          4,
          5,
          7
        ]
      },

Actual Behavior

when this is trying to be generated with nswag 14, this is resulting in the following error:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: Cannot implicitly convert type 'int' to 'string' at CallSite.Target(Closure, CallSite, Object) at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0) at NJsonSchema.Generation.JsonSchemaGenerator.SystemTextJsonInheritanceWrapper.GetDiscriminatorValue(Type type) at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)

Steps to Reproduce the Problem

  1. Create a class with a jsonpolymorphic attribute with typedescriminator based on an enum
  2. have jsonderivedtypes based on that enum integer
  3. try to run an api or the nswag cli based on this
  4. the error will show

the test here (https://github.com/RicoSuter/NJsonSchema/blob/3585d60e949e43284601e0bea16c33de4c6c21f5/src/NJsonSchema.Tests/Generation/SystemTextJson/SystemTextJsonInheritanceTests.cs#L80) notes the test being with string derived types, while enums generally are with integers

Specifications

schnerring commented 7 months ago

Is the following the property that you want to assign the discriminator value to?

public EDocumentItemType DocumentItemType { get; }

System.Text.Json, by design, cannot map discriminator values to runtime properties.

There is an open proposal, however.

The System.Text.Json docs explicitly state:

Avoid using a JsonPolymorphicAttribute.TypeDiscriminatorPropertyName that conflicts with a property in your type hierarchy.

Effectively this issue is blocked until dotnet/runtime#91274 is implemented. Until then enum-valued type discriminators with System.Text.Json are not possible.

EelcoLos commented 7 months ago

Is the following the property that you want to assign the discriminator value to?

public EDocumentItemType DocumentItemType { get; }

System.Text.Json, by design, cannot map discriminator values to runtime properties.

There is an open proposal, however.

The System.Text.Json docs explicitly state:

Avoid using a JsonPolymorphicAttribute.TypeDiscriminatorPropertyName that conflicts with a property in your type hierarchy.

Effectively this issue is blocked until dotnet/runtime#91274 is implemented. Until then enum-valued type discriminators with System.Text.Json are not possible.

Thanks @schnerring for replying 🙏

In NSwag13, and in .Net 7 this is supported. I do agree on your reading that the TypeDiscriminatorPropertyName should not have the property in type hierarchy, though why make this possible then, with no Roslyn warning whatsoever. We moved to this opportunity in .Net7, seeing it was available and working for us. We're using this polymorphism in a storage solution where that solution is now expecting a number to be stored, refering the enum. Hence the DerivedType needs to be a number for the storage solution to work. Here (https://github.com/RicoSuter/NJsonSchema/blob/3585d60e949e43284601e0bea16c33de4c6c21f5/src/NJsonSchema.Tests/Generation/SystemTextJson/SystemTextJsonInheritanceTests.cs#L79) it is only tested that the JsonDerivedType is a string. The integer option is not tested

The other solution is trying to revert back to our custom deserializer middleware.

schnerring commented 7 months ago

In NSwag13, and in .Net 7 this is supported. I do agree on your reading that the TypeDiscriminatorPropertyName should not have the property in type hierarchy, though why make this possible then, with no Roslyn warning whatsoever.

That would probably be a good issue. ;)

it is only tested that the JsonDerivedType is a string. The integer option is not tested

I'll have to rebase my STJ PR https://github.com/RicoSuter/NJsonSchema/pull/1595, and will have a look if it's possible to support integers, too.

schnerring commented 7 months ago

and will have a look if it's possible to support integers, too.

I'm not 100% sure, but I think we could support integer type discriminators using OpenAPI specification extensions, which is how it's currently done for enums when using Newtonsoft.Json.

By default, the discriminator mapping field is of type Map[string, string].

EelcoLos commented 7 months ago

In NSwag13, and in .Net 7 this is supported. I do agree on your reading that the TypeDiscriminatorPropertyName should not have the property in type hierarchy, though why make this possible then, with no Roslyn warning whatsoever.

That would probably be a good issue. ;)

it is only tested that the JsonDerivedType is a string. The integer option is not tested

I'll have to rebase my STJ PR #1595, and will have a look if it's possible to support integers, too.

since you've created a new PR, did you check it in https://github.com/RicoSuter/NJsonSchema/pull/1675 too?

schnerring commented 6 months ago

No, it was out of scope for that PR.

EelcoLos commented 1 month ago

just tested this with 14.1.0/11.0.2 and this problem nswag version and this problem persists.