Swashbuckle.aspnetcore: Differences in generated OpenAPI schema between rc4 and rc5

[cremor]

I've found a few differences in my OpenAPI schema between rc4 and rc5:

1. All my GET parameters are now documented as "nullable: true" (previously the nullable attribute was not present for the parameters in the OpenAPI schema).
   a. Same as for properties discussed in #1346 before, required parameters are now both required and nullable. Since required properties are never nullable after #1346 required parameters shouldn't be either.
   b. Even value type parameters like int32 are marked as nullable, but those only if they are not required. I'm honestly not sure if this is a problem or fine. Does it make sense to pass a not required query string parameter as null? How would I even explicitly pass null to a query string parameter?
2. As already reported by @danzel in #1346 all response objects (arrays directly in the response "content" of methods in the schema, objects in the "components" part of the schema) are now marked as nullable. I assume this is technically true, but still a bit weird. Where would I need to put attributes to tell Swashbuckle that my response objects will never be null?
3. All my models in "components" now don't have the "additionalProperties: false" attribute any more. According to the specification the default value of that is true, so I don't think this is correct?
4. The ProblemDetails schema has a difference in it's "additionalProperties". Previously it had a "type: object" inline. Now it references a separate "Object" schema. Maybe not a problem by itself, but this causes an empty "Object" schema to be shown in the Swagger UI which is weird.

Versions:
Swashbuckle.AspNetCore 5.0.0-rc5
Swashbuckle.AspNetCore.Newtonsoft 5.0.0-rc5
ASP.NET Core 2.2 running on the .NET Framework runtime

[domaindrivendev]

Ugghhhh - this is no good. Most of them are unanticipated side-effects of setting Nullable for _all_ reference or nullable types, something that seemed like the right thing to do. Although, I'm confused as to why you would see value types with the Nullable flag set also, I'm not seeing this behavior.

Inline vs reference for object type really shouldn't make any difference - and it's "empty" either way. I switched to referenced schema's for this type for consistency so that _all_ object schema's are now referenced.

TBH, supporting this Nullable flag has been so much hassle and impossible to get right. As you're finding out, whether or not it makese sense to set is highly contextual (parameter vs response, parameter vs property etc.). However, a lot of the generated schema's are "referenced" and therefore could be re-used across parameters, responses, properties etc.

At this point, I feel like the best thing to do is leave it unset in all cases. @cremor as you use NSwag a lot, I'm interested to know what would be the implications of this for downstream client generation?

[cremor]

@cremor as you use NSwag a lot, I'm interested to know what would be the implications of this for downstream client generation?

You must be confusing me with someone else, I don't use NSwag.

[domaindrivendev]

@danzel I think I meant you. Can you advise as to what the implications would be to NSwag if Swashbuckle returned Nullable = false for all schema's?

[danzel]

Removing "nullable": false from the schema (or not setting it at all) is an improvement.
The return types of methods no longer have | null in them.

However, then properties that are objects are not nullable. example:

public class MyType
{
    [Required]
    public string RequiredString { get; set; }

    public string NonRequiredString { get; set; }

    [Required]
    public int? RequiredInteger { get; set; }

    public int? NonRequiredInteger { get; set; }

    [Required]
    public MyInnerType RequiredInner { get; set; }

    public MyInnerType NonRequiredInner { get; set; }
}

All of the [Required] ones should be nullable: false (or unspecified), and the others should be nullable: true

This works for the string and int? properties:

          "requiredString": {
            "type": "string"
          },
          "nonRequiredString": {
            "type": "string",
            "nullable": true
          },

But not the MyInnerType ones:

          "requiredInner": {
            "$ref": "#/components/schemas/MyInnerType"
          },
          "nonRequiredInner": {
            "$ref": "#/components/schemas/MyInnerType"
          }

Full swagger.json

If we generate nullable: true in here, and not in the schema block then NSwag generates the right code.
Desired swagger.json

Thanks!

[berhir]

We are using NSwag to generate the client code for our Angular applications and we also noticed this change in rc5.

The generated methods previously looked like this:

historyAll(): Observable<HistoryItemDto[]> { ... }

And this is how the method looks with rc5:

historyAll(): Observable<(HistoryItemDto | null)[] | null> { ... }

Our C# code looks like this:

public async Task<ActionResult<ICollection<HistoryItemDto>>> GetHistoryAsync(CancellationToken cancellationToken)
{ }

We never return null, we always return a List (but it might be empty). And a list item can never be null.

To get the output like it is now with rc5 I would expect that the C# code looks like this:

public async Task<ActionResult<ICollection<HistoryItemDto?>?>> GetHistoryAsync(CancellationToken cancellationToken)
{ }

Note that HistoryItemDto? and ICollection<HistoryItemDto?>? are both marked as nullable in this sample. But in our current code they are not marked as nullable.

[domaindrivendev]

Removing "nullable": false from the schema (or not setting it at all) is an improvement.
The return types of methods no longer have | null in them.
However, then properties that are objects are not nullable. example:

@danzel - it's currently impossible to satisfy both of these requirements. This is because custom DTO's are defined in the generated Swagger as _shared_ components, which are _referenced_ from different places in the Swagger document. As the nullable property lives on the canonical schema definition, it's value cannot vary based on the context where the type in question is being used.

For example if you had a Product type that is returned from one operation but also used for a property in an input DTO for a different operation, the same schema definition, and therefore nullable value, will be referenced in both places.

[domaindrivendev]

So, I'm not convinced there's a solution here that works for all use cases - at least not without a massive refactor and significant set of breaking changes to the generated Swagger. Therefore, I'm leaning toward not setting the flag at all. If your DTO's are structured in a way that avoids the shared definition problem I've described above (e.g. if you have use-case specific DTOs), then you could create a very simple ISchemaFilter to assign the nullable property accordignly

[domaindrivendev]

I think this is what I need to move forward with unless anyone has a better idea?

[danzel]

I agree that not generating nullable: true inside the components schemas is probably the best approach.

However, I hope that it is possible to set them as nullable when they are child properties.

This currently works correctly when a property is a string or int?, they will have nullable: true inside the properties collection of the type that contains them, unless [Required] is specified.
https://gist.github.com/danzel/ca305683ccf2611305b9d7fc5ae8eb1e#file-gistfile1-txt-L124-L139

However this doesn't behave the same when the property type is a class, [Required] in this case does nothing.
https://gist.github.com/danzel/ca305683ccf2611305b9d7fc5ae8eb1e#file-gistfile1-txt-L140-L145

I think we should generate nullable: true next to the $ref, which would restore the old behaviour: "All properties are nullable if the type is nullable in c# (a reference type / Nullable) except when [Required] is specified".
desired swagger: https://gist.github.com/danzel/b78c6e8daa1ef96470fac030d92e26ae#file-gistfile1-txt-L142-L145

(These are what is generated using the example class that I posted above.)

I can hopefully achieve this with a filter if this isn't desired default behaviour.

[domaindrivendev]

@danzel thanks for your feedback. Placing nullable: true next to $ref is not a feasible solution because any other properties in a $ref object MUST be ignored, as per the JSON Schema specification. That is, placing the nullable there is essentially meaningless to any client that _correctly_ interprets the Swagger specification.

[danzel]

Thanks, I didn't know that. That does seem to make this more difficult!

Maybe we should be using this method instead:
https://github.com/RicoSuter/NSwag/issues/2071#issuecomment-479187543
(See the stackoverflow link and the OpenAPI link in that thread)

[mattfrear]

The ProblemDetails schema has a difference in it's "additionalProperties". Previously it had a "type: object" inline. Now it references a separate "Object" schema. Maybe not a problem by itself, but this causes an empty "Object" schema to be shown in the Swagger UI which is weird.

My team uses NSwagStudio to generate typescript clients from the Swagger, and this "Object" schema causes weirdness. I have written a Swashbuckle filter as a hopefully temporary workaround:

    /// <summary>
    /// .NET Core 3.0 introduces a property called "extensions" on ProblemDetails.
    /// NSwagStudio has a problem importing it, so this filter removes it.
    /// </summary>
    public class ProblemDetailsCleanupFilter : ISchemaFilter
    {
        public void Apply(OpenApiSchema schema, SchemaFilterContext schemaFilterContext)
        {
            if (schemaFilterContext.Type == typeof(Microsoft.AspNetCore.Mvc.ProblemDetails))
            {
                if (schema.Properties.ContainsKey("extensions"))
                {
                    schema.Properties.Remove("extensions");
                }

                schema.AdditionalPropertiesAllowed = false;
                schema.AdditionalProperties = null;

                if (schemaFilterContext.SchemaRepository.Schemas.ContainsKey("Object"))
                {
                    schemaFilterContext.SchemaRepository.Schemas.Remove("Object");
                }
            }
        }
    }

[domaindrivendev]

@cremor @danzel @mattfrear - I just merged #1429 which _should hopefully_ address all of your reported issues. See the PR for a description of the changes.

For the most part, I just reverted to the behavior from rc4. However, you'll see one very notable difference to the way _complex type_ properties are handled. Rather than returning a "reference" schema that points to a shared definition (under #/components/schemas), Swashbuckle will now return an inline schema that "extends" the shared definition. This way, contextual metadata (e.g. property attributes, property description, and __nullable__) can be applied to the "extended" schema (as it's not shared). Previously, contextual metadata was simply omitted for any properties that had a complex type.

This appears to be the recommended approach (see https://github.com/OAI/OpenAPI-Specification/issues/1368) and is definitely valid from a spec perspective. I've also ensured that the Swagger UI handles this without any issue. However, I don't know about NSwag. @danzel could you possibly pull down the lates preview (i.e. from https://www.myget.org/feed/domaindrivendev/package/nuget/Swashbuckle.AspNetCore) and test out the NSwag client generation with this approach?

[danzel]

Yep looks perfect, thanks!