-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add Microsoft.AspNetCore.OpenApi package #41238
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
Conversation
Fix up schema type generation Post review polish Add new package to templates Split out SchemaGenerator Clean up usings
|
||
var hostEnvironment = serviceProvider.GetService<IHostEnvironment>(); | ||
var serviceProviderIsService = serviceProvider.GetService<IServiceProviderIsService>(); | ||
var generator = new OpenApiGenerator(hostEnvironment, serviceProviderIsService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that OpenApiGenerator
is no longer a singleton that's resolved from DI does it need to be refactored to make better use of its ParameterBindingMethodCache
instance, etc.? e.g. RequestDelegateFactory
creates a static instance of ParameterBindingMethodCache
. That said, given this is in a package it might not be feasible to share the instance without making the type public anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the addition of this package certainly ups the case for #33644 and introduces the additional challenge of this particular codepath needing to return ParameterLocation
instead of BindingSource
.
At least for all the complex types, the new IProvidesMetadata
stuff might help a bit but not sure what to do about primitives.
src/Http/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests.csproj
Outdated
Show resolved
Hide resolved
src/ProjectTemplates/Web.ProjectTemplates/content/WebApi-CSharp/Program.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like someone else to look at the PR too since I'm only passingly familiar with the area. But looks good overall 😃
var discoveredTypeAnnotation = responseMetadata.Type; | ||
var discoveredContentTypeAnnotation = new MediaTypeCollection(); | ||
|
||
if (discoveredTypeAnnotation == typeof(void)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Looks like we check null or void shortly below and set to responseType then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't needed. It exists as a legacy from the ApiExplorer days where == typeof(void)
was used to handle scenarios where the return type was not provided by the user via an attribute, in which case, we would only fall back to the defined responseType on a 2xx status code.
However, that was coupled with some weirdness in the way that content types were handled in
Anyhoo, I was going to clean this up in a follow-up commit had I gotten any feedback in addition to this on the PR but I'll do this as part of #41243 instead.
Close #40676
ServiceProvider
toEndpointBuilder
abstract classWithOpenApi()
extension method toRouteHandlerBuilder
WithOpenApi(Func<OpenApiOperation, OpenApiOperation> modifyOperation)
extension method toRouteHandlerBuilder
SchemaGenerator
to OpenApi package