Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use built-in OpenAPI support in templates #55343

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

captainsafia
Copy link
Member

  • Update templates to use new APIs in Microsoft.AspNetCore.OpenApi for document generation
  • Remove WithOpenApi calls on endpoints

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 24, 2024
@KennethHoff
Copy link

How come WithOpenApi() is being removed? No longer necessary?

@captainsafia
Copy link
Member Author

How come WithOpenApi() is being removed? No longer necessary?

WithOpenApi was our strategy for adding built-in OpenAPI support, but only at the level of a single endpoint. When you called WithOpenApi on an endpoint, it would generate the OpenApiOperation using logic in our Microsoft.AspNetCore.OpenApi package, set that operation in metadata, then it would be up to 3rd party packages to merge that operation into their generated documents.

Moving forward, we don't need to follow this strategy. Instead, WithOpenApi will be an operation transformer on top of the document that is generated. You no longer need to call WithOpenApi to opt-in to the built-in generation.

@Varorbc
Copy link
Contributor

Varorbc commented Apr 25, 2024

This should also be removed

<SwashbuckleAspNetCoreVersion>6.4.0</SwashbuckleAspNetCoreVersion>

@Varorbc
Copy link
Contributor

Varorbc commented Apr 25, 2024

I have a few questions

  1. Do you support the AOT project?
  2. The default in the launch setting is to open the swagger. Will there be the swagger ui in the future?

@captainsafia
Copy link
Member Author

Do you support the AOT project?

There will be no changes to the AoT project template at the moment. However, we expect the new implementation to be AoT friendly for minimal APIs.

The default in the launch setting is to open the swagger. Will there be the swagger ui in the future?

We don't anticipate bundling Swagger UI in ASP.NET Core as part of this work. I'll update the launch settings to reflect this. We're investigating ways to provide a dev-time UI for viewing the generated OpenAPI documents that doesn't require bundling Swagger UI in the core framework.

@Varorbc
Copy link
Contributor

Varorbc commented Apr 29, 2024

We don't anticipate bundling Swagger UI in ASP.NET Core as part of this work. I'll update the launch settings to reflect this. We're investigating ways to provide a dev-time UI for viewing the generated OpenAPI documents that doesn't require bundling Swagger UI in the core framework.

Sometimes we need the ui not just for development, but even in production environments

@captainsafia
Copy link
Member Author

Sometimes we need the ui not just for development, but even in production environments

You'll still be able to include Swagger UI independently for your production scenarios. Swagger UI's web assets can be configured to point at the OpenAPI document that is generated by ASP.NET Core. You can view an example of this in the sample app used for testing.

Note: you'll want to be cautious about configuring Swagger UI correctly so that you're not unintentionally leaking OAuth client secrets, limiting XSS risks, and placing the UI endpoint behind authentication if necessary.

@Varorbc
Copy link
Contributor

Varorbc commented Apr 30, 2024

Maybe I'd prefer MapSwaggerUi to be part of the framework, rather than everyone implementing it

@captainsafia
Copy link
Member Author

Maybe I'd prefer MapSwaggerUi to be part of the framework, rather than everyone implementing it

The MapSwaggerUi implementation that you see here is strictly demoware. Bringing an actual implementation into the framework would require considerable more work.

Also, it's possible to use the Swagger UI bundled in Swashbuckle and NSwag and point them to the OpenAPI document that is generated by the framework so the re-implementation cost is not as extreme since it already exists in other packages.

@Varorbc
Copy link
Contributor

Varorbc commented Apr 30, 2024

Also, it's possible to use the Swagger UI bundled in Swashbuckle and NSwag and point them to the OpenAPI document that is generated by the framework so the re-implementation cost is not as extreme since it already exists in other packages.

If I continue to use Swashbuckle or NSwag, they both provide document generation themselves, Why should I use the framework-generated OpenAPI documentation? I think the framework should provide a complete experience, rather than relying on external packages

@captainsafia
Copy link
Member Author

If I continue to use Swashbuckle or NSwag, they both provide document generation themselves, Why should I use the framework-generated OpenAPI documentation?

Each implementation doesn't generate the same document. The goal with the built-in support is to provide a high-quality document that takes advantage of the fact that doc generation lives closer to the framework.

Anyhow, thanks for posting your comment on the issue. Let's continue the conversation there. 😄

@martincostello
Copy link
Member

The Swashbuckle.AspNetCore.SwaggerUI package doesn't depend on any of the Swashbuckle generation code, and essentially just distributes swagger-ui itself. Repackaging it again in a third package wouldn't add any value other than being "from Microsoft" for those eschew non-Microsoft packages for whatever reasons they may have.

It's also minimal effort to provide your own one-page of HTML that consumes swagger-ui directly from something such as cdnjs if you don't want a NuGet package to provide it for you.

NSwag/Swashbuckle can easily co-exist with the new in-box OpenAPI support and the ASP.NET Core team can spend their finite development time on areas they can differentiate or expand on (schemas, native AoT support).

@Varorbc
Copy link
Contributor

Varorbc commented Apr 30, 2024

But the project template went from the original direct launch swagger ui to the current open.json, A disruptive change to the user experience can make the user feel overwhelmed, and it would be better to explain a return to the original experience.

@captainsafia
Copy link
Member Author

@Varorbc I hear you on the concern around the experience break here. It's definitely something that was considered as part of this decision. However, there are ways to solve that problem that don't require the framework to bundle Swagger UI (and build an API layer around it).

But the project template went from the original direct launch swagger ui to the current open.json,

FWIW, this change modifies the templates so that it launches the /weatherforecast URL. This matches the behavior that you would see if you didn't enable OpenAPI in the templates.

@Varorbc
Copy link
Contributor

Varorbc commented May 1, 2024

However, there are ways to solve that problem that don't require the framework to bundle Swagger UI (and build an API layer around it).

It would be better if you had documentation to explain this, or in template

@captainsafia
Copy link
Member Author

It would be better if you had documentation to explain this, or in template

Yes, the revamped docs for OpenAPI will include information on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants