[WIP] Improve middleware api #1

RicoSuter · 2018-10-21T15:37:14Z

Add IDocumentProvider as done in this PR: Add IDocumentProvider service and its implementation RicoSuter/NSwag#1658
Renamed DocumentRegistry to SwaggerDocumentRegistry
Rename DocumentProvider to SwaggerDocumentProvider
Add ISwaggerGenerator to NSwag.SwaggerGeneration (OpenApiDocumentSettings no longer used, less code)
- AspNetCoreToSwaggerGenerator implements ISwaggerGenerator and thus can be added to the SwaggerDocumentRegistry
Split UseSwaggerUiWithApiExplorer() into two methods UseSwagger() and UseSwaggerUi()
- We have to break the API anyway so this is to make it more SRP
Make AddDocument extension methods and use ISwaggerDocumentBuilder (avoid direct access to SwaggerDocumentRegistry)
Prepare everything so that the WebApiToSwaggerGenerator dependency can eventually be removed

Question:

Do the the documents be in DI or can't we just add them to the registry which is in DI?

Simple config:

    public void ConfigureServices(IServiceCollection services)
    {
        ...

        // Add NSwag OpenAPI/Swagger DI services and configure documents
        services.AddSwagger(options =>
            options.AddDocument(settings => settings.SchemaType = SchemaType.OpenApi3));
    }

    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        ...
        // Add middlewares to service the OpenAPI/Swagger document and the web UI
        app.UseSwagger();
        app.UseSwaggerUi3();
    }

Advanced config:

    public void ConfigureServices(IServiceCollection services)
    {
        …

        // Add OpenAPI and Swagger DI services and configure documents

        // Adds the NSwag services
        services.AddSwagger(options => options
            // Register a Swagger 2.0 document generator
            .AddDocument("swagger", settings =>
            {
                // Add custom document processors, etc.
                settings.DocumentProcessors.Add(new SecurityDefinitionAppender("TEST_HEADER", new SwaggerSecurityScheme
                {
                    Type = SwaggerSecuritySchemeType.ApiKey,
                    Name = "TEST_HEADER",
                    In = SwaggerSecurityApiKeyLocation.Header,
                    Description = "TEST_HEADER"
                }));
            })
            // Register an OpenAPI 3.0 document generator
            .AddDocument("openapi", settings => settings.SchemaType = SchemaType.OpenApi3));
    }

    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        ...

        // Add OpenAPI and Swagger middlewares to serve documents and web UIs

        // Add Swagger 2.0 document serving middleware
        app.UseSwagger("swagger", options => options.Path = "/swagger/v1/swagger.json");

        // Add web UIs to interact with the document
        app.UseSwaggerUi3(options =>
        {
            // Define OpenAPI/Swagger document route (defined with UseSwaggerWithApiExplorer)
            options.SwaggerRoute = "/swagger/v1/swagger.json";

            // Define web UI route
            options.SwaggerUiRoute = "/swagger_ui";
        });
        app.UseSwaggerReDoc(options =>
        {
            options.SwaggerRoute = "/swagger/v1/swagger.json";
            options.SwaggerUiRoute = "/swagger_redoc";
        });

        // Add OpenAPI 3.0 document serving middleware
        app.UseSwagger("openapi", options => options.Path = "/openapi/v1/openapi.json");

        // Add web UIs to interact with the document
        app.UseSwaggerUi3(options =>
        {
            options.SwaggerRoute = "/openapi/v1/openapi.json";
            options.SwaggerUiRoute = "/openapi_ui";
        });
        app.UseSwaggerReDoc(options =>
        {
            options.SwaggerRoute = "/openapi/v1/openapi.json";
            options.SwaggerUiRoute = "/openapi_redoc";
        });

        // Add Swagger UI with multiple documents
        app.UseSwaggerUi3(options =>
        {
            // Add multiple OpenAPI/Swagger documents to the Swagger UI 3 web frontend
            options.SwaggerRoutes.Add(new SwaggerUi3Route("Swagger", "/swagger/v1/swagger.json"));
            options.SwaggerRoutes.Add(new SwaggerUi3Route("Openapi", "/openapi/v1/openapi.json"));
            options.SwaggerRoutes.Add(new SwaggerUi3Route("Petstore", "http://petstore.swagger.io/v2/swagger.json"));

            // Define web UI route
            options.SwaggerUiRoute = "/swagger_all";
        });
    }

@rynowak

- #1626 - does not "provide an api to generate multiple / all documents"; could be layered on this work - otherwise, meets requirements listed in #1626 - assumes the following answers to open questions in #1626 (generally means everything is opt-in) - without an `AddSwagger()` call, `IDocumentProvider` service will not exist - registered document name / identifier is new `SwaggerSettings<>.DocumentName` property - document is not registered unless `DocumentName` is non-`null` i.e. `IDocumentProvider` won't know about it - `SwaggerSettings<>.MiddlewareBasePath` and `SwaggerRoute` are independent of `DocumentName` - middleware does not depend on the `IDocumentProvider` or registrar services; duplicating some code - many thanks to @rynowak for writing this proposal

…umentProvider` - remove some code from files added in 5b7e0a3b34 - revert changes made in 5b7e0a3b34 to existing files

- `internal` again - commit 4057107.

dougbu · 2018-10-22T23:18:46Z

src/NSwag.AspNetCore/Documents/AspNetCoreToSwaggerDocument.cs

+namespace NSwag.AspNetCore.Documents
+{
+    /// <summary>The document based on the <see cref="AspNetCoreToSwaggerGeneratorSettings"/> settings.</summary>
+    public class AspNetCoreToSwaggerDocument : AspNetCoreToSwaggerGeneratorSettings, ISwaggerDocument


I'm fine changing names around and moved a few classes myself. But GitHub doesn't seem to be showing anything as a rename, making it difficult to compare and contrast. Let me know what names you want and comparisons will be easier.

FYI my equivalent of this class includes DocumentName and PostProcess methods and leaves generation to the document provider.

The final goal is to have a ISwaggerGenerator interface which is directly implemented by the generator class so this here can be completely avoided. Coming soon.

Should have mentioned: It's probably better for AspNetCoreToSwaggerDocument and AspNetCoreToSwaggerGeneratorSettings to have a "has a" rather than "is a" relationship. The depth of inheritance here is rather deep.

To avoid more and more code, I removed AspNetCoreToSwaggerDocument and use ISwaggerGenerator which is directly implemented by AspNetCoreToSwaggerGenerator and then registered in the SwaggerDocumentRegistry… this way we the logic for correctly setting up the settings is where it belongs and we dont need all those new classes...

dougbu · 2018-10-22T23:19:16Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

+        /// <summary>Adds services required for OpenAPI 3.0 generation (enforces OpenAPI 3.0 for all registered documents).</summary>
+        /// <param name="serviceCollection">The <see cref="IServiceCollection"/>.</param>
+        /// <param name="configure">Configure the document registry.</param>
+        public static IServiceCollection AddOpenApi(this IServiceCollection serviceCollection, Action<SwaggerDocumentRegistry> configure = null)


FYI Rather than expose the registry to users, I used builder methods.

dougbu · 2018-10-22T23:23:41Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

+
+                foreach (var settings in registry.Documents.Select(t => t.Value).OfType<AspNetCoreToSwaggerDocument>())
+                {
+                    settings.SchemaType = SchemaType.OpenApi3;


I like the idea but worry about overriding choices users make in their configure delegate. Would prefer the property was of type Nullable<SchemaType> and NJsonSchema defaulted to Swagger2 when it's null. Then, could make this change conditional on SchemaType == null.

Another alternative when using my builder approach (AddDocument(...) on the IOpenApiBuilder (or ISwaggerBuilder sticking with "Swagger" more as you suggested) would be to use a AspNetCoreToSwaggerDocument subclass that sets SchemaType in its constructor. The AddOpenApi(...) method would then take an Action<AspNetCoreToOpenApiDocument> parameter.

…s(...)` - add NSwag.Sample.OpenApi project - rework bd9c7fc; revert part of that commit e.g. undo `SwaggerExtensions` changes nit: - correct doc comment for `AspNetCoreToSwaggerGeneratorSettings` WIP: - some doc comments are currently empty - names are tentative

dougbu · 2018-10-23T16:13:21Z

src/NSwag.AspNetCore/SwaggerDocumentRegistry.cs

+        /// <param name="documentName">The document name.</param>
+        /// <param name="configure">The configure action.</param>
+        /// <returns>The registry.</returns>
+        public SwaggerDocumentRegistry AddDocument<TDocument>(string documentName, Action<TDocument> configure = null)


What is the use case for additional IDocument implementations?

dougbu · 2018-10-23T16:14:42Z

src/NSwag.AspNetCore/SwaggerDocumentRegistry.cs

+        {
+            var settings = new TDocument();
+            configure?.Invoke(settings);
+            _documents[documentName] = settings;


This is brand new code. I thought you wanted to require unique document names?

dougbu · 2018-10-23T16:22:32Z

src/NSwag.AspNetCore/Documents/AspNetCoreToSwaggerDocument.cs

+namespace NSwag.AspNetCore.Documents
+{
+    /// <summary>The document based on the <see cref="AspNetCoreToSwaggerGeneratorSettings"/> settings.</summary>
+    public class AspNetCoreToSwaggerDocument : AspNetCoreToSwaggerGeneratorSettings, ISwaggerDocument


Should have mentioned: It's probably better for AspNetCoreToSwaggerDocument and AspNetCoreToSwaggerGeneratorSettings to have a "has a" rather than "is a" relationship. The depth of inheritance here is rather deep.

dougbu · 2018-10-23T16:24:26Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

+
+                foreach (var settings in registry.Documents.Select(t => t.Value).OfType<AspNetCoreToSwaggerDocument>())
+                {
+                    settings.SchemaType = SchemaType.OpenApi3;


Another alternative when using my builder approach (AddDocument(...) on the IOpenApiBuilder (or ISwaggerBuilder sticking with "Swagger" more as you suggested) would be to use a AspNetCoreToSwaggerDocument subclass that sets SchemaType in its constructor. The AddOpenApi(...) method would then take an Action<AspNetCoreToOpenApiDocument> parameter.

dougbu · 2018-10-23T16:27:33Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

+        /// <remarks>This is the same as UseSwagger(), configure the document in AddSwagger/AddOpenApi().</remarks>
+        /// <param name="app">The app.</param>
+        /// <param name="configure">Configure additional settings.</param>
+        public static IApplicationBuilder UseOpenApi(this IApplicationBuilder app, Action<SwaggerMiddlewareSettings> configure = null)


We have an opportunity here to reduce the depth of the middleware pipeline when users configure multiple documents. No longer necessary to have a middleware per document.

Further, the user now needs to be concerned about making both SwaggerRoute and documentName unique. Routing avoids that.

dougbu · 2018-10-23T16:28:58Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

+        /// <remarks>This is the same as UseOpenApi(), configure the document in AddSwagger/AddOpenApi().</remarks>
+        /// <param name="app">The app.</param>
+        /// <param name="configure">Configure additional settings.</param>
+        public static IApplicationBuilder UseSwagger(this IApplicationBuilder app, Action<SwaggerMiddlewareSettings> configure = null)


Suggest picking one method name for the Use...(...) methods. The UseSwagger(...) and UseOpenApi(...) methods do exactly the same thing.

Let's keep using only UseSwagger/AddSwagger and maybe ill rename the whole project from Swagger to OpenApi at some point.. otherwise it gets really confusing as some methods do the exact same thing with different names... for me Swagger is v2 and OpenAPI is v3 but this is not really true - so let's stick to only swagger (as done in this PR).

dougbu · 2018-10-23T16:32:08Z

src/NSwag.AspNetCore/SwaggerExtensions.cs

-            });
-
-            return app;
+            throw new NotSupportedException("Use " + nameof(AddSwagger) + "(), " + nameof(UseSwaggerWithApiExplorer) +


[Obsolete] is intended for the case where a method or class continues to work as it did before. That is, it allows users to upgrade their dependencies and temporarily ignore the warnings. This breaks users and is equivalent to removing the existing method.

dougbu

I'm going to open a PR in RSuter/NSwag with this branch to make it easier to compare and contrast. I'll also remove the new sample from the other PR to reduce the clutter.

dougbu · 2018-10-23T17:00:57Z

src/NSwag.Sample.NETCore11/Startup.cs


-            app.UseSwaggerUi(typeof(Startup).GetTypeInfo().Assembly, s =>
+            app.UseSwaggerUi3(options =>


Ui3 handles multiple documents and it seems this ignores that feature while increasing the chances UseSwagger(...) and UseSwaggerUi3(...) are misaligned.

RicoSuter · 2018-10-23T17:10:36Z

@dougbu sorry for all this mess. I've updated the description of this PR with more information.

I'd really like to merge this PR into your PR and then maybe work from there if needed - but i think all scenarios are covered in the most clean and extensible way.

dougbu · 2018-10-23T17:19:28Z

Could you please open the PR I suggested in your repo to make it easier to compare and contrast? It's very difficult to see what remains of the original proposals here.

Make AddDocument extension method and use ISwaggerDocumentBuilder

That is a significant improvement.

covered in the most clean and extensible way

My main concerns here are the ease with which users can misconfigure their middleware and the depth of the middleware pipeline when users have multiple documents. A smaller concern is that middleware are conventionally configured using IOptions<T> and added just once i.e. with one gesture regardless of the number of supported pages / actions / exposed APIs / et cetera.

RicoSuter · 2018-10-23T17:50:22Z

Can you post a sample of the advanced config how it would look like with your solution?

One ugly thing which remains here (for now) is the configuration of the UIs, i.e. they use a config of SwaggerUiSettings but they should use SwaggerUiSettings. But because the former is needed in the legacy library and WebApiToSwaggerGeneratorSettings/GeneratorSettings must just be ignored I think it's fine for now... will remove the Web API based generator eventually and all this too :-)

RicoSuter · 2018-10-23T17:51:19Z

Created a new PR: RicoSuter#1678

dougbu · 2018-10-23T18:03:21Z

Can you post a sample of the advanced config how it would look like with your solution?

From RicoSuter#1658 discussions
For example, ConfigureServices(...) might contain

services.AddSwagger();
services.Configure<SwaggerMiddlewareOptions>(o => o.SwaggerRoute = "/openApi/{documentName}/swagger.json");
services.Configure<SwaggerUi3Options>(o => o.SwaggerUiRoute = "/myUi");

dougbu · 2018-10-26T16:30:52Z

Turning off the duplicate noise coming from this PR. RicoSuter#1678 is the place.

rynowak and others added 12 commits October 8, 2018 16:16

Revert changes to support WebApiToSwaggerGeneratorSettings in `IDoc…

8bd56ec

…umentProvider` - remove some code from files added in 5b7e0a3b34 - revert changes made in 5b7e0a3b34 to existing files

Make IDocumentPovider public

4057107

Set SwaggerSettings<T>.DocumentName to "v1" by default

19c4c94

nit: Correct doc comment in SwaggerExtensions

4000ad8

Remove RegisteredDocument.ControllerTypes

60bd944

nit: Clean up some usings

a770674

nit: Ignore VS Code files

005a198

Revert "Make IDocumentPovider public"

b743686

- `internal` again - commit 4057107.

Move method

2da52e8

Improve middleware api

fe4dab4

Refactoring

94056c9

RicoSuter changed the title ~~Improve middleware api~~ [WIP] Improve middleware api Oct 21, 2018

RicoSuter added 4 commits October 21, 2018 21:56

Improve comments

22f4e92

Improve api

5a01899

Improve sample

73a496b

Minor fix

8c51390

RicoSuter mentioned this pull request Oct 22, 2018

Add IDocumentProvider service and its implementation RicoSuter/NSwag#1658

Closed

Move method

69c1039

dougbu reviewed Oct 22, 2018

View reviewed changes

dougbu force-pushed the dougbu/document-generator branch from 2da52e8 to a13c550 Compare October 23, 2018 01:12

RicoSuter added 3 commits October 23, 2018 08:36

Merge

f388a2e

Merge

ced77f5

Add ISwaggerGenerator

c0dee3c

dougbu reviewed Oct 23, 2018

View reviewed changes

RicoSuter added 3 commits October 23, 2018 18:43

Update startup

833875a

Merge

60a9369

Remove unused files

4b263d3

dougbu reviewed Oct 23, 2018

View reviewed changes

Make AddDocument extension method and use ISwaggerDocumentBuilder

17220ce

Update sln

4b9520f

dougbu mentioned this pull request Oct 23, 2018

Add IDocumentProvider service and its implementation RicoSuter/NSwag#1678

Merged

dougbu force-pushed the dougbu/document-generator branch 2 times, most recently from bbf9e37 to 2f71924 Compare October 24, 2018 21:52

RicoSuter added 4 commits October 26, 2018 14:43

Refactoring

a5b6b27

Move method

ea7fc86

Add documentName support

43dfcab

Remove unused usings

7f84a3a

dougbu closed this Oct 26, 2018

RicoSuter deleted the features/improve-middleware-api branch December 19, 2018 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Improve middleware api #1

[WIP] Improve middleware api #1

RicoSuter commented Oct 21, 2018 •

edited

Loading

dougbu Oct 22, 2018

RicoSuter Oct 23, 2018

dougbu Oct 23, 2018

RicoSuter Oct 23, 2018

dougbu Oct 22, 2018

dougbu Oct 22, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

dougbu Oct 23, 2018

RicoSuter Oct 23, 2018

dougbu Oct 23, 2018

dougbu left a comment

dougbu Oct 23, 2018

RicoSuter commented Oct 23, 2018

dougbu commented Oct 23, 2018

RicoSuter commented Oct 23, 2018

RicoSuter commented Oct 23, 2018

dougbu commented Oct 23, 2018

dougbu commented Oct 26, 2018


		app.UseSwaggerUi(typeof(Startup).GetTypeInfo().Assembly, s =>
		app.UseSwaggerUi3(options =>

[WIP] Improve middleware api #1

[WIP] Improve middleware api #1

Conversation

RicoSuter commented Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RicoSuter commented Oct 23, 2018

dougbu commented Oct 23, 2018

RicoSuter commented Oct 23, 2018

RicoSuter commented Oct 23, 2018

dougbu commented Oct 23, 2018

dougbu commented Oct 26, 2018

RicoSuter commented Oct 21, 2018 •

edited

Loading