Skip to content
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

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

Open
EelcoLos opened this issue Jan 15, 2024 · 6 comments

Comments

@EelcoLos
Copy link

EelcoLos commented Jan 15, 2024

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 (

[JsonPolymorphic(TypeDiscriminatorPropertyName = "k")]
) notes the test being with string derived types, while enums generally are with integers

Specifications

  • NSwag Version: 14.0.0-preview012
@EelcoLos EelcoLos changed the title [NSwag 14/NJsonSchema11] TypeDiscriminatorPropertyName with enum/int values breaks [NSwag 14/NJsonSchema11] JsonPolymorhic/JsonDerivedType TypeDiscriminatorPropertyName with enum/int values breaks Jan 15, 2024
@schnerring
Copy link

schnerring commented Feb 13, 2024

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
Copy link
Author

EelcoLos commented Feb 13, 2024

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 (

) 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
Copy link

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.

@schnerring
Copy link

schnerring commented Feb 13, 2024

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
Copy link
Author

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 #1675 too?

@schnerring
Copy link

No, it was out of scope for that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants