Aspnetcore: Improvements to Swagger generation

[rynowak]

This is a laundry list of gaps we have between the current swagger experience and the ideal experience. Basically what are all of the things you have to configure or provide manually.

Things we need to provide

P0

P1

• [ ] Security - securityDefinintions element - information about auth schemes and URLs. We should surface this when we have it.
• [ ] in operation - security/scopes - see above
• [ ] in operation - consumes and produces are not realistic - how does this affect clients? We can move this down if it's not used in client.
• [ ] in parameter - support for describing the serialization format for array types. (See https://swagger.io/docs/specification/2-0/describing-parameters/#array)

P2

Things a swagger provider should provide

P0

• [ ] in parameter - look at collections and enums as parameters/return types - we want to make sure that idiomatic .NET code works well with swagger + generated clients

P1

• [ ] Host/Base-Path/Schemes - We don't know what value this would provide or how to get it right. This would look like the framework exposing settings. This could be important for the generated client.
• [ ] In operation - summary, operationid, description
• [ ] in parameter - description
• [ ] in operation response - description. This is marked as a required field in the OpenAPI specs - https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#response-object

P2

• [ ] Description/Title/Version - everything in info element
• [ ] Tags - tags element. Tags are used to provide grouping and descriptions of groups of related operations.
• [ ] in operation - tags - see above
• [ ] Docs- externalDocs element - this is an informational link to developer documentation

[mkArtakMSFT]

@danroth27, @rynowak what's is the status of this? Should this be pushed out to some future release (like 3.0.0)?

[tugberkugurlu]

Is this related to OpenAPI 3 support?

[danroth27]

@glennc Could you take a look at these gaps and see which ones we want to address for 2.2?

[rynowak]

Action items:

@glennc - discuss with your boy @blowdart about the security related stuff
@dougbu - investigate the effects of collections, enums, produces/consumes, as well as scheme/host/port on generated clients

[dougbu]

Temporarily marked this as Working as I investigate a bit

[dougbu]

Interim report

I first did a baseline check comparing NSwag and Swashbuckle document generation for ASP.NET MVC Api projects and for both 2.0 and 2.1. The largest differences (as expected) were between 2.0 or 2.1 versus 2.1 plus [ApiController]. That difference was just in defaults exposed in the application model and API explorer when actions lack [Consumes], [Produces] or explicit [FromXYZ] on parameters.

That said, NSwag does a better job filling in info, host, schemes, consumes and produces at the top of an Open API / Swagger document. Swagger leaves that information out. More importantly, NSwag correctly marks non-simple parameters as required when [ApiController] is in play.

It may also be important that NSwag's host, basePath and schemes information comes from the HttpContext.Request of the document request by default. This means we should encourage developers to place the NSwag middleware and MVC in the same app.Map(...) branch, avoid Map entirely, or use the basePath overrides NSwag supports. As is, NSwag (and Swagger because it never generates basePath) misleads readers into thinking the whole path is /api/Values and not /basePath/api/Values.

Two iffy differences:

1. NSwag recognises services returning e.g. IEnumerable<string> can produce text/json or text/plain instead of just application/json. But, without additional work on the developer's part or a narrow Accept header, Swagger is likely correct that the response will be application/json. Should we encourage one approach over the other?
2. Describing enums in a Swagger / Open API document is inherently verbose but NSwag is more verbose e.g. where Swagger generates

{
    "name": "comparisonType",
    "in": "query",
    "required": false,
    "type": "integer",
    "format": "int32",
    "enum": [
        0,
        1,
        2,
        3,
        4,
        5
    ]
}

NSwag would include

"x-schema": {
    "$ref": "#/definitions/StringComparison"
},

in the above and also emit

"definitions": {
  "StringComparison": {
    "type": "integer",
    "description": "",
    "x-enumNames": [
      "CurrentCulture",
      "CurrentCultureIgnoreCase",
      "InvariantCulture",
      "InvariantCultureIgnoreCase",
      "Ordinal",
      "OrdinalIgnoreCase"
    ],
    "enum": [
      0,
      1,
      2,
      3,
      4,
      5
    ]
  }
}

In this case, code generators may use the additional information to emit more readable code. I'll see as I go through the C# classes.

Style

Separately NSwag and Swagger both have options to change enum handling. They don't use names by default but can e.g.

{
    "name": "comparisonType",
    "in": "query",
    "required": false,
    "type": "string",
    "enum": [
        "CurrentCulture",
        "CurrentCultureIgnoreCase",
        "InvariantCulture",
        "InvariantCultureIgnoreCase",
        "Ordinal",
        "OrdinalIgnoreCase"
    ]
}

Nits

FYI NSwag's app.UseSwagger(...) overloads should be avoided even with GeneratorSettings.IsAspNetCore = true;. Anything with a name that includes ApiExplorer e.g app.UseSwaggerWithApiExplorer(...) is required for NSwag to fill in consumes based on [Consumes].

NSwag includes x-nullable properties that Swashbuckle does not. Swashbuckle fills in non-empty "description": "Success" properties (where NSwag says "description": "") and includes uniqueItems properties that NSwag does not. I don't expect these differences to affect client code generation at all.

[dougbu]

Quick follow-up: NSwag cannot generate C# or TypeScript from a default Swashbuckle-generated API description i.e. one where the user has done no Startup customization beyond adding Swashbuckle. Without a top-level title and version in the info value, NSwag just blows up. I can work around this by setting Info.Title but Swashbuckle generating "info": {} by default violates the Swagger 2.0 spec and NSwag is behaving as it should.

[dougbu]

Done the investigation with NSwag C# generation based on NSwag and Swashbuckle documents. Any need to check what AutoRest generates?

Issues

• Generated code assumes JSON responses and it's messy to handle other content types (related to RSuter/NSwag#420, RSuter/NSwag#701 and RSuter/NSwag#731)
• Default type for collection responses is ObservableCollection<string> which seems more suited to UI scenarios than the general case (this is planned for v12.0, see RSuter/NSwag#984)
• Minor: NSwag command has incorrect help for the /ResponseArrayType option, saying the default is ICollection when it's ObservableCollection

Details

basePath, host and schemes

Including top-level basePath, host or schemes properties in the API description works fine when the service is actually hosted at the given URI. Of course, this information is incorrect when the URI changes and may be left out (as Swashbuckle does). Fortunately, it's just one extra step for the caller in those cases: The client code includes a BaseUrl property used when configuring an HttpClient. No lock-ins w.r.t. these properties.

[Consumes] and [Produces]

I missed it earlier but NSwag-generated documents include produces values only at the top level (reflecting the configured IOutputFormatters). NSwag ignores [Produces] on actions. If the response isn't JSON, using the generated client results in

Unhandled Exception: ClientConsole.SwaggerException: Could not deserialize the response body. ---> Newtonsoft.Json.JsonReaderException: Unexpected character encountered while parsing value: <. Path '', line 0, position 0.

If the produces property exists for an action e.g. when using a Swashbuckle-generated document, NSwag only updates the Accept header. The above SwaggerException is still thrown.

NSwag basically chooses application/json if it's available in the consumes property and leaves it at that. When the only choice for a parameter is e.g. application/xml, NSwag kind-of gives up: It updates the Content-type header, then requires the user to pass an XML string in.

Workaround for non-JSON responses

It's possible to handle other response types e.g.
``` c#
partial void ProcessResponse(System.Net.Http.HttpClient client, System.Net.Http.HttpResponseMessage response)
{
var text = response.Content.ReadAsStringAsync().Result;
if (text.Length > 0 && text[0] == '<')
{
var document = new System.Xml.XmlDocument();
document.LoadXml(text);

    text = Newtonsoft.Json.JsonConvert.SerializeXmlNode(document, Newtonsoft.Json.Formatting.None);
    var headers = response.Content.Headers;
    var enumerator = headers.ContentEncoding?.GetEnumerator();
    enumerator?.MoveNext();
    var encoding = enumerator?.Current ?? "UTF-8";

    response.Content = new System.Net.Http.StringContent(
        text,
        System.Text.Encoding.GetEncoding(encoding),
        headers.ContentType.MediaType);
}

}

(The above doesn't work for XML responses of collection types.)

#### Collections
I don't see anything ugly in the generated code when the actions include parameters of collection types. Generated methods use `IEnumerable<T>` by default.

NSwag's generated methods return `ObservableCollection<string>` instead of say `List<string>` when an action returns a collection or enumeration. Fortunately, this is easily overridden using (say) `/ResponseArrayType:List`.

#### `enum`s
The generated code isn't too bad when the API includes `enum` parameters or responses. It does however look worse without the schema information or the "enum as string" option enabled e.g.
``` c#
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "9.10.67.0 (Newtonsoft.Json v9.0.0.0)")]
public enum ComparisonType
{
    _0 = 0,
    _1 = 1,
    _2 = 2,
    _3 = 3,
    _4 = 4,
    _5 = 5,
}

[dougbu]

Yes, an ObservableCollection<T> implements ICollection<T> but help readers are likely to expect a more common collection type.

[mkArtakMSFT]

@glennc , I'm moving this to 3.0.

[mkArtakMSFT]

Talked to @glennc and we're going to review this with https://github.com/aspnet/AspNetCore/issues/7333 work.