-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Make ApiExplorer for minimal APIs trim-compatible #56827
Conversation
873ea08
to
11abf6e
Compare
src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj
Outdated
Show resolved
Hide resolved
src/Http/Http.Abstractions/src/Metadata/IParameterBindingMetadata.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Abstractions/src/Metadata/IParameterBindingMetadata.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/StaticRouteHandlerModel/StaticRouteHandlerModel.Emitter.cs
Outdated
Show resolved
Hide resolved
....Extensions/test/RequestDelegateGenerator/Baselines/VerifyAsParametersBaseline.generated.txt
Show resolved
Hide resolved
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.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.
The trimming infrastructure related changes LGTM
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
06f1097
to
a636d4a
Compare
After adding the NativeAoT test project in a636d4a, I realized there is another layer of trim-incompatible issues that we are running into. In addition to relying on the ParameterBindingMethodCache to resolve info related on parameters at the top-level aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs Line 194 in 6548eba
aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs Line 367 in 6548eba
aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs Line 406 in 6548eba
aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs Lines 438 to 439 in 6548eba
aspnetcore/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs Lines 23 to 32 in 6548eba
This dependency was introduced when we added support for a So there's another layer of changes that need to be made here to quarantine the |
d1a1365
to
6812d1d
Compare
...Microsoft.AspNetCore.OpenApi.NativeAotTests/Microsoft.AspNetCore.OpenApi.NativeAotTests.proj
Show resolved
Hide resolved
src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.WarningSuppressions.xml
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.WarningSuppressions.xml
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.WarningSuppressions.xml
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.
The trimming related changes LGTM. Just one minor comment that can be addressed in a follow-up if you don't want to reset CI.
<argument>ILLink</argument> | ||
<argument>IL2026</argument> | ||
<property name="Scope">member</property> | ||
<property name="Target">M:Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata.#ctor(Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ModelMetadataIdentity)</property> |
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.
Can we add a Justification
here? same as:
aspnetcore/src/SignalR/server/Core/src/Microsoft.AspNetCore.SignalR.Core.WarningSuppressions.xml
Line 9 in 9503913
<property name="Justification">This warning is left in the product so developers get an ILLink warning when trimming an app when Microsoft.AspNetCore.SignalR.Hub.IsCustomAwaitableSupported=true.</property> |
/backport to release/9.0-preview7 |
Started backporting to release/9.0-preview7: https://github.com/dotnet/aspnetcore/actions/runs/10050522403 |
Part of #56023.
This PR removes the trim warnings originating from Mvc.ApiExplorer when used on an application using minimal APIs + OpenAPI. The bulk of these trim warnings originate from the use of
ParameterBindingCache
to determine the status of parameters on a route handler andCoerecedAwaitable
to unwrap awaitable response types. The use of unbounded reflection from these types is replaced by the introductionIParameterBindingMetadata
and updates to RDF and RDG to produce this metadata as they analyze endpoints.This PR also sets
ProducesResponseType
metadata at the framework level for response types that are unwrapped from an awaitable to replace the use ofCoercedAwaitable
inEndpointMetadataApiDescriptionProvider
. There's also some ✨ surgical ✨ changes to the ApiDescriptionProvider implementation to work around issues are the ambiguity betweentypeof(void)
as a null-type versus an explicitly set type in the ApiResponseProvider.Specific changes include:
EndpointMetadataApiDescriptionProviderTests
to call intoRequestDelegateFactory
so we can pick up metadata inserted by the code generation layer from ApiExplorerParameterBindingMetadata
class in source generated code and addParameterBindingMetadata
to all parameters that are discovered during source genApiDescriptionProvider
implementation to use parameter-binding metadata as the source of truth for parameters on an endpoint instead ofMethodInfo.GetParameters
typeof(string)
forProducesResponseTypeMetadata
inferred by the platform for string-types to avoid falling into traps aroundtypeof(void)
vs.null
as unset response type