use IsDevelopment #1081
use IsDevelopment #1081
Conversation
|
@Rick-Anderson what is the rationale behind this PR? I can't see the internal notes so I'm not entirely sure what the context is. I see that the change just conditionally adds whether the Swagger UI is added. I understand why it would conditional in a template. This example is explicitly meant to show the SwaggerUI. I'm trying to determine when you wouldn't want that enabled. If this were to be conditional, then all of the examples that can show the SwaggerUI should be brought into alignment; at least, the ASP.NET Core examples. I honestly don't remember how or if that conditional setup was possible back in Web API. |
@captainsafia asked me to make this change.
That's why this is a draft PR, I'll make them all consistent. @commonsensesoftware thanks for the comments and hopefully you'll review my changes when this is out of draft. |
Yep, to add some color to this, we're trying to standardize around the best practice of only enabling Swagger UI for development scenarios across samples/docs in the repo. We've done this with the eShop repo and have a few PRs to make our docs pages consistent here. WRT to why only enabling it in development is a best practice, there's some information and secret disclosure concerns with Swagger UI in production scenarios so we want to guide users towards the most conservative/secure option by default. There's also the challenge that the middleware-based approach for Swagger UI is a little bit hard to enable authentication on to reduce information disclosure risks. For documentation and samples where Swagger UI is primarily used to aid with ad-hoc dev-time testing and exploration of APIs, it helps to be explicit with the isDevelopment check. |
|
I don't really have an objection; it just came out of the blue. 😉 I know showing the Swagger UI outside of development is not typically a best practice. The examples in question were explicitly meant to show the Swagger UI (all the time) and I had removed all other cruft to dial into what really matters. I do, however, prefer and try to make examples represent the real world and what you ought to do. It's very unlikely anyone would ever run these examples in a |
|
Speaking of the eShop repo @captainsafia, I'm still waiting on that PR review to integrate API Versioning as you had asked. 😁 I believe I've fixed or answered all your feedback. There was some other feedback about making the rendered information more ergonomic if it's present. There currently isn't any such information for the eShop repo to see what it would look like, but I've already ported it back here in the latest examples. If you pull and run any of the OpenAPI examples, you can see what it'll look like. |
Yes, I'm so sorry it's taken me so long to get back to you! 😓 I've been heads down working on landing OpenAPI doc generation support for preview4 and have had to be more focused with my time. I hope to take another look by middle of next week. I specifically want to make sure that the new OpenAPI work + the ASP versioning changes will connect well once we update eShop to target the built-in OpenAPI implementation |
|
@captainsafia Happy to carve out some time and have a discussion about it some time. I'm also happy to chime in or comment on anything that needs review or is ready to vet (if it's before preview). Feel free to email me. I've already come to terms with the fact it will likely be a full rewrite and new library/package on my part. It would be great to align for the Nov release. |
| if ( app.Environment.IsDevelopment() ) | ||
| { | ||
| // navigate to ~/$odata to determine whether any endpoints did not match an odata route template | ||
| // Access ~/$odata to identify OData endpoints that failed to match a route template. | ||
| app.UseODataRouteDebug(); | ||
| } | ||
|
|
||
| app.UseSwagger(); | ||
| if ( app.Environment.IsDevelopment() ) | ||
| { |
There was a problem hiding this comment.
@commonsensesoftware should I move app.UseSwagger(); up so I only need one
if ( app.Environment.IsDevelopment() )
block
There was a problem hiding this comment.
Less blocks would be good so I don't have a problem with that. Is the established practice to disable OpenAPI docs and the Swagger UI outside of Debug? It's more for my edification.
There was a problem hiding this comment.
Where'd we land on this? Leave it on the outside or move it inside so it's only for development? I'm not sure the practice here. Everything else looks good. If you're happy with this, then I'll merge it. 😉
There was a problem hiding this comment.
Is the established practice to disable OpenAPI docs and the Swagger UI outside of Debug? It's more for my edification.
General best practice is to disable UIs that might be susceptible to a web security issues (leaked creds, XSS, etc.) outside of development. The generated JSON is fine to enable in production and, in fact, might be necessary for certain integration scenarios (e.g. client generators, OpenAI plugins, API management front-ends).
|
@commonsensesoftware please review. |
| if ( app.Environment.IsDevelopment() ) | ||
| { | ||
| // navigate to ~/$odata to determine whether any endpoints did not match an odata route template | ||
| // Access ~/$odata to identify OData endpoints that failed to match a route template. | ||
| app.UseODataRouteDebug(); | ||
| } | ||
|
|
||
| app.UseSwagger(); | ||
| if ( app.Environment.IsDevelopment() ) | ||
| { |
There was a problem hiding this comment.
Less blocks would be good so I don't have a problem with that. Is the established practice to disable OpenAPI docs and the Swagger UI outside of Debug? It's more for my edification.
|
@mikekistler please review |
|
@commonsensesoftware can you merge this? |
See https://github.com/dotnet/aspnetcore-internal/issues/4522
Summary of the changes: see Files changed
Description
see Files changed
contributes to https://github.com/dotnet/aspnetcore-internal/issues/4522