-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
SwaggerDiscriminator Attribute: discriminator field moved from paths to components #2052
Comments
@domaindrivendev It's rather hard to pinpoint when exactly this change was introduced, but I think it might be #1983 . I looked through the whole history and don't find any discussion of this change. It'd be really helpful if I could find some reasoning behind this change and how exactly clients should deal with the new schema. |
Is there any update regarding this issue? My client generator started producing invalid code after the update... |
I don't think this change violates the specification, but this at least breaks NSwag client generation. Without the discriminator property defined on the schema object, NSwag will simply generate the using the first schema object or reference object in the oneOf array. The specification states there is a discriminator field on the schema object and isn't specific as to when it is required or optional. It would make sense to generate the discriminator field if the user is returning a polymorphic type, as the previous versions had done before. https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/ The following code is present in v5.6.3, but is then removed by v6.0.0. Swashbuckle.AspNetCore/src/Swashbuckle.AspNetCore.SwaggerGen/SchemaGenerator/SchemaGenerator.cs Lines 221 to 227 in 90a6382
Swashbuckle.AspNetCore/src/Swashbuckle.AspNetCore.SwaggerGen/SchemaGenerator/SchemaGenerator.cs Lines 174 to 179 in 364006b
This is the offending commit bfa6429. Pulling down the master branch, I was naively able to re-add the removed lines, but I don't know why it was removed to begin with. @domaindrivendev would you be able to chime in as it appears you removed the original code? |
This issue is breaking the openapi typescript fetch client generator and others. Any news on this?. |
I'm also interested in a solution to this - NSwag doesn't know how to apply the JsonInheritanceAttribute with the correct mappings anymore. |
As other have alluded to, I believe this change breaks the spec. As per the swagger documentation: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/ the discriminator name and mapping belong on the polymorphic type as it was previously. Putting the discriminator name on the subtypes is somewhat meaningless, as it is the polymorphic type that determines which field it wants to use on the underlying subtypes. Only the discriminator property belongs on the underlying subtypes. I can imagine a scenario where you have some overlap between two different polymorphic hierarchies with some common subtypes in both. Each hierarchy may use a different discriminator name operating over the same types and therefore, the discriminator name must be on the polymorphic type and not the subtype. This change in behavior should be reverted. |
I have submitted a Pull Request #2683 that fixes this bug. |
Is there a known workaround for this bug? |
A workaround that I used for this, using services.AddSwaggerGen(opts =>
{
opts.UseOneOfForPolymorphism();
opts.SelectDiscriminatorNameUsing(_ => "$type");
}); It adds a property to polymorphic types: "blahtype": {
"properties": {
"$type": {
"type": "string"
},
}
} |
So did this ever got fixed? i am using a custom OperationFilter for now... |
I expanded on this and am using the .NET Serialization attributes to fill out the correct information. This gives me the descriminator and its mapping in the output schema, all customizable with // using System.Text.Json.Serialization;
opts.SelectDiscriminatorNameUsing(
_ =>
{
if (Attribute.GetCustomAttribute(_, typeof(JsonPolymorphicAttribute))
is JsonPolymorphicAttribute polymorphic)
return polymorphic.TypeDiscriminatorPropertyName;
return null;
}
);
// Use the JsonDerivedTypeAttribute on the base class to determine
// the discriminator value for the derived class.
opts.SelectDiscriminatorValueUsing(
_ =>
{
var baseType = _;
string? output = null;
while (baseType != null && output == null)
{
output =
Attribute
.GetCustomAttributes(baseType, typeof(JsonDerivedTypeAttribute), inherit: true)
.Cast<JsonDerivedTypeAttribute>()
.Where(a => a.DerivedType == _.UnderlyingSystemType)
.Select(a => a.TypeDiscriminator?.ToString())
.Where(d => d != null)
.Distinct()
.SingleOrDefault();
baseType = baseType.BaseType;
}
return output;
}
); |
After updating from 5.6.3 to 6.x (tested both 6.1.0 and 6.0.7) when using the SwaggerDiscriminator attribute the "discriminator" field moved from
paths/<path>/responses/<..>/content/schema
into/components/schemas/BaseType
.I.e. in the old code we had:
now this has become
and the discriminator and mapping can only be found under components of the base type. I assume this is an intentional change?
I can see the point of not repeating the discriminator mappings repeatedly, but it seems rather cumbersome to find the discriminator and mapping with the new structure (basically find the common allOf() parent of all the types referenced in oneOf I think? Not sure this would work for more complicated inheritance hierarchies though).
The sample shown at swagger.io has the discriminator under the response part as it was done in the old version though.
Reproduction
Using the basic ASP.NET Core 3.1 template with the following controller:
and those changes added to the Startup.cs:
results in different swagger.jsons depending on whether 5.6.3 or 6.1.0 is used.
Swagger.json 5.6.3
Swagger.json 6.1.0
The text was updated successfully, but these errors were encountered: