Add IDocumentProvider service and its implementation

[dougbu]

• Add an IDocumentProvider interface and service implementing it #1626
• does not "provide an api to generate multiple / all documents"; could be layered on this work
  • otherwise, meets requirements listed in Add an IDocumentProvider interface and service implementing it #1626
• assumes the following answers to open questions in Add an IDocumentProvider interface and service implementing it #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

[RicoSuter]

think we should support only the AspNetCoreToSwaggerGenerator and ignore the Web API (legacy) generator. I think this property would not be needed this way...

[dougbu]

🆗

[dougbu]

🆙📅d to address your comments @RSuter. I look forward to any alternative suggestions or further thoughts you have.

[RicoSuter]

So this interface is the only public API which is relevant for you, is this correct?

Shouldn't it be renamed to IOpenApiDocumentProvider or also use a document type enum - because there might be other document types in the future and we maybe should be more specific about the document type it generates?

[rynowak]

This doesn't need to be public

[rynowak]

This is a contract that we're implementing via late binding - it's not going to have types like enums - because there's no library. It's a way for us to consume a document in a generic way.

If you want a different abstraction for inside NSwag that sounds good, but it's not what we will consume.

[RicoSuter]

Ok, if we can make it internal and it doesnt give name collisions problems with other libraries (e.g. Swashbuckle in the same app) I think it's fine.

[dougbu]

🆙📅

[RicoSuter]

@dougbu is this now ready to be merged?

[dougbu]

is this now ready to be merged?

It won't harm anything to merge this (though squashing the 10 commits might help 😃.

But, based on our offline thread about the Startup.Configure(...) method, we might go with a different design -- especially w.r.t. the document registry. Suggest holding off until you choose an option.

[RicoSuter]

@dougbu I made a PR against the branch in this PR, dougbu#1

I think this should cover scenario 3 (change middleware api). Is this ok and will work with your other code?

Open task: Update wiki here: https://github.com/RSuter/NSwag/wiki/Middlewares

[dougbu]

@RSuter well, I was just testing the changes I'd made. I'll push to my remote branch so that you can compare and contrast…

[dougbu]

🆙📅

@RSuter other than WIP items mentioned in the top top commit, the main thing I haven't done but is recommended is switch to the IOptions<T> pattern in the middleware and the Use*(...) methods I've added. The middleware will have an IOptions<T> parameter and the IApplicationBuilder extension methods will have two overloads each: IApplicationBuilder UseBlah(this IApplicationBuilder app) and IApplicationBuilder UseBlah(this IApplicationBuilder app, , BlahOptions options). This allows middleware configuration wherever the user chooses.

FYI @rynowak

[RicoSuter]

I'm now trying to merge all my work but it's now a big mess. But there are main problems:

• With your approach, we cannot add a default document because we cannot detect when the user didnt provide one (i.e. AddOpenApi() is not able to detect that AddDocument is not called
• AddOpenApi and AddDocument can add a document ATM which doesnt make sense from an API perspective, see

            services
                .AddOpenApi(settings => settings.DocumentName = "v1") // Why the settings here?
                .AddDocument(settings =>
                {
                    settings.DocumentName = "v2";
                    settings.GeneratorSettings.SchemaType = SchemaType.OpenApi3;
                });

• If we break all users, I want to split the swagger.json and swagger UI middlewares into separate Use* functions to reduce the number of methods and not do two things with one method...
• I think documentName should not be part of the options/settings as it must be clear that it's only used to connect AddSwagger with UseSwagger (which IMO is not really nice, but there is not a better solution with how ASP.NET Core is designed)
• Internally we should keep the name Swagger - otherwise the code base gets really confusing

Will update my PR against your PR soon.

[dougbu]

With your approach, we cannot add a default document because we cannot detect when the user didn't provide one (i.e. AddOpenApi() is not able to detect that AddDocument is not called

AddOpenApi() adds the first document. AddDocument(...) is only for additional documents.

// Why the settings here?

Because users may want to configure the first, possibly only, document.

separate Use* functions to reduce the number of methods and not do two things with one method.

I tried that approach but found it falls down because the UI middleware needs to know where the generator middleware is hosted. But, this will be much simpler to do once I move to an IOptions<T> approach.

I think documentName should not be part of the options/settings as it must be clear that it's only used to connect AddSwagger with UseSwagger (which IMO is not really nice, but there is not a better solution with how ASP.NET Core is designed)

🆗 though I'm not quite sure I understand the concern.

All middleware in my approach handles all the documents. Well, except the Redoc UI one. Names are primarily important for routing and the IDocumentProvider calls.

keep the name Swagger

Sure.

[dougbu]

Now that I look again at the existing samples, this one isn't needed. And, I started with the wrong template here, ignoring the lack of actual APIs exposed and bulking the project up with irrelevant files.

[dougbu]

The namespace here is very important. Users should be able to do mainline scenarios as soon as they add the NSwag.AspNetCore package to their project. No need to add a using NSwag.* statement in their Startup class.

[RicoSuter]

Without a call to AddSwagger() the interface will not be registered and thus the whole thing wont work, right?

[dougbu]

That's a different concern. Everything fails nice and loudly when AddSwagger() is not called but the Use*(...) methods are.

[RicoSuter]

AddOpenApi() adds the first document. AddDocument(...) is only for additional documents.

I think this API is really confusing because AddOpenApi() and AddDocument() are doing the same but have a different name... That's why I'd like to have this nested configuration, i.e. AddOpenApi() adds the required services and AddDocument (as in AddOpenApi(options => options.AddDocument()) actually adds a document - if none is specified a default one is regeistered by AddOpenApi.

Like this one:

🆗 though I'm not quite sure I understand the concern.

I think it should be like this in a common scenario (simplest case):

AddSwagger() // registers services
UseSwagger() // register swagger.json middleware
UseSwaggerUi3() // register swagger.json consuming UI

This way each call has its own defined responsibility...

But, this will be much simpler to do once I move to an IOptions approach.

How should this solve this structural problem? Can you give an example?

[RicoSuter]

And let's not use OpenApi in the method and stick with Swagger for now... otherwise it gets really confusing because OpenApi does not enforce OpenAPI v3.0 and v2.0 is still supported so we have two methods which should do the same but internally dont (one registers the services the other doesnt).

[dougbu]

AddOpenApi() and AddDocument() are doing the same but have a different name

Not quite. AddOpenApi(...) does the whole shebang and AddDocument(...) just extends the available documents. Note AddDocument(...) isn't an extension method on the IServiceCollection.

I think it should be like this in a common scenario (simplest case)

That will work fine using my approach as well. The AddSwagger() call registers the default document, UseSwagger() uses the default IOptions<SwaggerMiddlewareOptions> and UseSwaggerUi3() uses the default IOptions<SwaggerUi3Options> and also the SwaggerRoute set in IOptions<SwaggerMiddlewareOptions>.

Users will be able to mess themselves up by calling UseSwaggerUi3() without UseSwagger() or by passing SwaggerMiddlewareOptions into UseSwagger() that doesn't have the same route as the default. But, both cases are avoidable e.g. by having the middleware flip a bit in SwaggerMiddlewareOptions to indicate they're actually in use.

How should this solve this structural problem?

Users would be able to stick with the usual ASP.NET Core configuration patterns. (MVC and EF get a bit more complicated because they aren't themselves exposed as middleware.)

For example, ConfigureServices(...) might contain

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

then the Use*() methods in Configure(...) stitch everything together.

[RicoSuter]

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

This is really bad as you can have only one SwaggerMiddlewareOptions or SwaggerUi3Options option...

What if I need two Swagger UIs?
E.g one with the category a, one with category b and one with both (dropdown). See sample in my updated PR: dougbu#1

[RicoSuter]

… also SwaggerMiddlewareOptions has a PostProcess (Action(request, document)) which cannot be specified per document this way...

[RicoSuter]

Users will be able to mess themselves up by calling UseSwaggerUi3() without UseSwagger() or by passing SwaggerMiddlewareOptions into UseSwagger() that doesn't have the same route as the default. But, both cases are avoidable e.g. by having the middleware flip a bit in SwaggerMiddlewareOptions to indicate they're actually in use.

Yes, but Swashbuckle does it the same way - and I'd rather trade this problem for more flexibility. If you don't change the routes it just works - otherwise you have to be aware that you can misconfigure the thing...

For me these (AddSwagger, UseSwagger and UseSwaggerUi) are 3 different services... you can use UseSwaggerUi without the other two (e.g. with an external spec)

[RicoSuter]

Ok.

[dougbu]

🆙📅 only remaining WIP bits are a couple of names and a few empty doc comments.

From my perspective, this PR has the following advantages:

• uses IOptions<T> for UseSwagger(...) and UseSwaggerUi4(...) scenarios
• changes fewer files
• reduces the middleware depth when multiple documents are registered (by default)
• supports post-processing in IDocumentProvider scenarios
• places extensions and the ISwaggerBuilder interface in appropriate namespaces
• does not nest lambdas when using AddSwagger(...) or AddOpenApi(...)
  • note MVC uses nested lambdas only for changing MvcOptions, which would be relevant if SwaggerDocumentProvider took options
• leaves SwaggerDocumentRegistry with a single purpose
• leaves existing code unchanged except for a few nits

[dougbu]

@RSuter please note the "Reviewers" comments in a few files

[dougbu]

@RSuter I should have mentioned: If you choose this PR over #1678 and want me to, I'll squash the commits, perform any renaming, and fill in the blank doc comments. LMK

[RicoSuter]

I updated my PR to support {documentName} and decided to merge this in...

• Aligned stuff with your PR (namespaces, documentName support etc.)
• Moved stuff to the Microsoft.* namespaces
• Avoid duplicating stuff, avoid *Options classes (which makes stuff even more confusing)
• Avoid copying settings (hard to maintain as property copies might be forgotten)
• IOptions support prepared but will be added in v12 (when settings classes are cleaned up => only possible when the WebAPI generator feature is removed)
• Multiple documents are added in nested lambda => advanced scenario and makes more sense for me as it is all in the scope of AddSwagger()

Some cleanup can only be done when removing features (WebApi generator middleware etc.), see #984

I hope this is ok for you? And I'm really sorry for all this back and forth... but I really need to move on and work on other issues - we already lost too much time here...

[dougbu]

I hope this is ok for you?

@RSuter yes, this is fine. Much better than fine: You've now got an IDocumentProvider implementation and have moved NSwag.AspNetCore to be more familiar to ASP.NET Core developers with plans to do more in v12!

I was somewhat sorry to see the middleware pipeline still contains multiple SwaggerMiddleware instances and that users don't immediately get IOptions<T> goodness. But, these are your decisions about your code. And, I agree this took more of both of our time than we'd hoped.

One more real concern, echoing my earlier "supports post-processing in IDocumentProvider scenarios" point: This lack in NSwag.AspNetCore may mean the Microsoft document tool will get a different document than the SwaggerMiddleware would provide. That is, users may do more than adjust to the request host and schemes in their SwaggerMiddlewareSettings.PostProcess actions. Can / should SwaggerGeneratorSettings.DocumentProcessors and OperationProcessors be used for that purpose?

[RicoSuter]

This lack in NSwag.AspNetCore may mean the Microsoft document tool will get a different document than the SwaggerMiddleware would provide. That is, users may do more than adjust to the request host and schemes in their SwaggerMiddlewareSettings.PostProcess actions.

You are right, this also bothers me. The following code is actually not executed when running via command line:
https://github.com/RSuter/NSwag/blob/master/src/NSwag.AspNetCore/Middlewares/SwaggerMiddleware.cs#L87-L91

The idea is that the PostProcess here should only be used to change BasePath, Schemes and Host based on the request, but of course it cannot be enforced. How would you proceed?

• Remove these lines completely (breaking change for some, host and schemes may be wrong),
• remove just PostProcess (but then you cannot change host, schemes, etc.) or
• keep as is and add documentation that PostProcess should only be used to change host, schemes etc?

Can / should SwaggerGeneratorSettings.DocumentProcessors and OperationProcessors be used for that purpose?

Yes, exactly - you should use a custom document processor for that - but you don't have access to a request there (because a spec is independent of an actual HTTP request).

[dougbu]

keep as is and add documentation that PostProcess should only be used to change host, schemes etc?

If it were me, this is the route I'd go because adjusting these values to match the request is so useful. Suggest also mentioning the processing order of OperationProcessors, DocumentProcessors and PostProcess.

Separately, including all schemes, hosts and PathBase values may be done using the IServerAddressesFeature in IWebHost.ServerFeatures.

you should use a custom document processor for that

Reasonable.

In retrospect, the main thing my trial addition of a PostProcess action to SwaggerDocumentSettings improved was ease of use for simple cases. Implementing IDocumentProcessor is slightly more involved but covers more cases because the DocumentProcessorContext provides more context.

[RicoSuter]

In retrospect, the main thing my trial addition of a PostProcess action to SwaggerDocumentSettings improved was ease of use for simple cases. Implementing IDocumentProcessor is slightly more involved but covers more cases because the DocumentProcessorContext provides more context.

Yes, but I don't like having two concepts (document processors and postprocess) for the same scenario... we could just provide a generic GenericDocumentProcessor(Action<Context> action) so that it's very easy to add a lambda/inline based document processor - this way you dont need a new class...

If it were me, this is the route I'd go because adjusting these values to match the request is so useful. Suggest also mentioning the processing order of OperationProcessors, DocumentProcessors and PostProcess.

Ok, then we'll do that. But in the case of using the command line you need to define these properties via config anyway so why not force it also for the hosted scenario?

Separately, including all schemes, hosts and PathBase values may be done using the IServerAddressesFeature in IWebHost.ServerFeatures.

Good point.

[dougbu]

so why not force it also for the hosted scenario?

I'm missing something. Force what?

If you mean forcing use of config, aren't IDocumentProcessors and PostProcess already added through config?

[RicoSuter]

I meant force the user to load the host, schemes etc from the config (eg appsettings.json) so that it works in the host scenario and cli scenario.. but this might upset users :-)

[dougbu]

I agree setting the document properties statically would be a regression.

After talking more internally, I'm sorry to say the IServerAddressesFeature isn't going to be helpful until after the production server is started, possibly after the first request. Even then, for the middleware case, you'll be able to detect that HTTP is enabled in addition to HTTPs but that won't be 100% reliable. (E.g. for an HTTP request -- the service may be deployed behind a TLS gateway and not itself support HTTPS. Or the service may support both schemes but be behind a reverse proxy that supports just one.) And, IServerAddressesFeature won't improve IDocumentProvider scenarios.