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

polymorphism issue in 5.6 with NSwagStudio code gen #1853

Closed
pmiddleton opened this issue Oct 3, 2020 · 8 comments
Closed

polymorphism issue in 5.6 with NSwagStudio code gen #1853

pmiddleton opened this issue Oct 3, 2020 · 8 comments

Comments

@pmiddleton
Copy link

We are currently using NSwagStudio to generate dto based on our Swashbuckle generated Swagger definitions.

We have one polymorphic class hierarchy in our api and prior to 5.6.x everything worked fine. NSwagSudio generated code using a Newtonsoft JsonConverter to deserialize into the correct derived class based on the discriminator field.

We just tried moving to 5.6.x and that code generation broke. The issue seems to be that the discriminator field information is no longer generating in the components schemas section of the swagger json. Instead the discriminator information is now being generated in the paths responses section.

Is there a way with 5.6 to continue to have the discriminator generate in the components section so that NSwag can continue to generate the proper serialization code for us?

@domaindrivendev
Copy link
Owner

I have tested the latest version with NSwag and it’s client generation appears to work correctly for polymorphic types.

For me to troubleshoot further, could you please create a minimal project to demonstrate the issue and post to GitHub. Then I can pull it down and switch between Swashbuckle versions to understand your issue better

@pmiddleton
Copy link
Author

I put a sample at https://github.com/pmiddleton/SwashbuckleTest

It is set to build vs 5.5.1 and show what we expect the output to be. Load it up and use NSwagStudio to view the output. Notice that the shape class looks like this.

[Newtonsoft.Json.JsonConverter(typeof(JsonInheritanceConverter), "discriminator")]
[JsonInheritanceAttribute("Rectangle", typeof(Rectangle))]
[JsonInheritanceAttribute("Circle", typeof(Circle))]
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.2.1.0 (Newtonsoft.Json v11.0.0.0)")]
public partial class Shape 
{
    [Newtonsoft.Json.JsonProperty("sides", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
    public int Sides { get; set; }
}

You can then switch the nuget references to 5.6.3 and uncomment the attributes on the shape class and uncomment the new 5.6.x setup calls in AddSwaggerGen in the setup class. I had to do a build clean / rebuild all for the nuget to update correctly.

Then run again and see that the output from NSwagStudio looks like this.

[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.2.1.0 (Newtonsoft.Json v11.0.0.0)")]
public partial class Shape 
{
    [Newtonsoft.Json.JsonProperty("shapeType", Required = Newtonsoft.Json.Required.Always)]
    [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)]
    public string ShapeType { get; set; }
  
    [Newtonsoft.Json.JsonProperty("sides", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
    public int Sides { get; set; }
}

The JsonConverter information is missing.

I am just missing a step in the configuration to get the output the same?

Thanks,

Paul

@JonathanLydall
Copy link
Contributor

I have also encountered this problem with 5.6. @pmiddleton's example is a very concise solution which demonstrates the difference in the generated swagger.json files.

The full files are below, but it seems that the discriminator objects in the JSON have been moved from components > schemas > <Object>s to paths > <URL> / get / responses / schema.

Full 5.5.1 file:

{
    "openapi": "3.0.1",
    "info": {
      "title": "Shape",
      "version": "v1"
    },
    "paths": {
      "/api/Shape/GetShape": {
        "get": {
          "tags": [
            "Shape"
          ],
          "responses": {
            "200": {
              "description": "Success",
              "content": {
                "text/plain": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ]
                  }
                },
                "application/json": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ]
                  }
                },
                "text/json": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ]
                  }
                }
              }
            }
          }
        }
      }
    },
    "components": {
      "schemas": {
        "Shape": {
          "required": [
            "discriminator"
          ],
          "type": "object",
          "properties": {
            "discriminator": {
              "type": "string"
            },
            "sides": {
              "type": "integer",
              "format": "int32"
            }
          },
          "additionalProperties": false,
          "discriminator": {
            "propertyName": "discriminator"
          }
        },
        "Rectangle": {
          "allOf": [
            {
              "$ref": "#/components/schemas/Shape"
            },
            {
              "type": "object",
              "properties": {
                "area": {
                  "type": "integer",
                  "format": "int32"
                }
              },
              "additionalProperties": false
            }
          ]
        },
        "Circle": {
          "allOf": [
            {
              "$ref": "#/components/schemas/Shape"
            },
            {
              "type": "object",
              "properties": {
                "radius": {
                  "type": "integer",
                  "format": "int32"
                }
              },
              "additionalProperties": false
            }
          ]
        }
      }
    }
  }

Full 5.6.3 file:

{
    "openapi": "3.0.1",
    "info": {
      "title": "Shape",
      "version": "v1"
    },
    "paths": {
      "/api/Shape/GetShape": {
        "get": {
          "tags": [
            "Shape"
          ],
          "responses": {
            "200": {
              "description": "Success",
              "content": {
                "text/plain": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ],
                    "discriminator": {
                      "propertyName": "discriminator"
                    }
                  }
                },
                "application/json": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ],
                    "discriminator": {
                      "propertyName": "discriminator"
                    }
                  }
                },
                "text/json": {
                  "schema": {
                    "oneOf": [
                      {
                        "$ref": "#/components/schemas/Rectangle"
                      },
                      {
                        "$ref": "#/components/schemas/Circle"
                      }
                    ],
                    "discriminator": {
                      "propertyName": "discriminator"
                    }
                  }
                }
              }
            }
          }
        }
      }
    },
    "components": {
      "schemas": {
        "Circle": {
          "type": "object",
          "allOf": [
            {
              "$ref": "#/components/schemas/Shape"
            }
          ],
          "properties": {
            "radius": {
              "type": "integer",
              "format": "int32"
            }
          },
          "additionalProperties": false
        },
        "Shape": {
          "required": [
            "discriminator"
          ],
          "type": "object",
          "properties": {
            "discriminator": {
              "type": "string"
            },
            "sides": {
              "type": "integer",
              "format": "int32"
            }
          },
          "additionalProperties": false
        },
        "Rectangle": {
          "type": "object",
          "allOf": [
            {
              "$ref": "#/components/schemas/Shape"
            }
          ],
          "properties": {
            "area": {
              "type": "integer",
              "format": "int32"
            }
          },
          "additionalProperties": false
        }
      }
    }
  }

@domaindrivendev
Copy link
Owner

domaindrivendev commented Oct 7, 2020

So, it's my understanding that the use of discriminator in the prior version was actually incorrect, according to the Swagger / OpenAPI specification. Specifically, if you look at the Discriminator example from the Swagger docs, you can see that the discriminator property should be used inline with the oneOf keyword rather than being defined on the schema(s) that are referenced by the oneOf construct:

components:
  responses:
    sampleObjectResponse:
      content:
        application/json:
          schema:
            oneOf:
              - $ref: '#/components/schemas/simpleObject'
              - $ref: '#/components/schemas/complexObject'
            discriminator:
              propertyName: objectType

In fact, the docs specifically state that ...

The discriminator is used with anyOf or oneOf keywords only.

So, while this doesn't really help you, I wanted to first help you understand the rationale for the change. As NSwag needs the discriminator to be defined elsewhere, I think it may be using the spec incorrectly. Of course NSwag is an end-to-end toolchain, so it doesn't always matter if it's misinterpreting the Swagger / OpenAPI spec so long as it's consistent in doing so across it's tool chain.

With that said, I can kind of understand why it needs to do things the way it does. So, rather than baking what I believe to be invalid spec generation into Swashbuckle, I'd prefer to keep the Swashbuckle behavior as is, but provide you with a simple Schema filter workaround that will tweak the generated schema to meet the needs of NSwag. Does that sound reasonable?

@domaindrivendev
Copy link
Owner

OK - here's the workaround as promised. With this approach, you can annotate your source classes exactly as described in the NSwag docs for inheritance, and Swashbuckle will automatically pick up the subtypes specified with KnownType, and generate schema definitions that will in turn be interpreted correctly by the NSwag client generator tooling:

// Startup.cs
services.AddSwaggerGen(c =>
{
    // Tell Swashbuckle to emit inheritance hierarchies using "allOf"  
    c.UseAllOfForInheritance();

    // Tell Swashbuckle to identify subtypes using the KnownTypeAttribute (as per NSwag)
    c.SelectSubTypesUsing(baseType => baseType.GetCustomAttributes<KnownTypeAttribute>().Select(attr => attr.Type));

    // Wire up a schema filter to apply the Discriminator info on the base schema (as per NSwag)
    c.SchemaFilter<NSwagDiscriminatorSchemaFilter>();
});

// NSwagDiscriminatorSchemaFilter.cs
public class NSwagDiscriminatorSchemaFilter : ISchemaFilter
{
    public void Apply(OpenApiSchema schema, SchemaFilterContext context)
    {
        var jsonConverterAttribute = context.Type.GetCustomAttribute<JsonConverterAttribute>(false);

        if (jsonConverterAttribute?.ConverterType == typeof(JsonInheritanceConverter))
        {
            var discriminatorName = jsonConverterAttribute.ConverterParameters[0].ToString();

            // Add the discriminator property
            schema.Properties.Add(discriminatorName, new OpenApiSchema { Type = "string" });

            // Flag it as required
            schema.Required.Add(discriminatorName);

            // Add "discriminator" metadata
            schema.Discriminator = new OpenApiDiscriminator
            {
                PropertyName = jsonConverterAttribute.ConverterParameters[0].ToString()
            };
        }
    }
}

@pmiddleton
Copy link
Author

Thank you for this. I will give it a try tomorrow and let you know.

Will this work around work long term for future swashbuckle versions? I will open an issue with NSwag and see if they can conform to the specification.

@JonathanLydall
Copy link
Contributor

Thank you very much @domaindrivendev for your explanation and provided workaround. I had to tweak your Schema filter example to work for my particular setup, but managed to get it working easily enough.

@pmiddleton, I have already created an issue in the NSwag repository and quoted @domaindrivendev's comments there:
RicoSuter/NSwag#3099 (comment)

@pmiddleton
Copy link
Author

Thanks @domaindrivendev - I just got this working and the fix works. Thank you for the help.

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

3 participants