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

Enable trimming for OpenAPI package #55465

Merged
merged 6 commits into from
May 3, 2024
Merged

Conversation

captainsafia
Copy link
Member

Part of #54598.

  • Use RDG to source generate the MapOpenApi endpoint
  • Use STJ source generation for (de)serialization
  • Add RUC/RDC attributes where required
  • Fix bug where RDG would emit code for complex form bound parameters

@captainsafia captainsafia added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks labels May 1, 2024
@captainsafia captainsafia requested a review from amcasey May 1, 2024 19:57
@captainsafia captainsafia requested review from wtgodbe and a team as code owners May 1, 2024 20:19
@@ -107,6 +107,16 @@ private void ProcessEndpointParameterSource(Endpoint endpoint, ISymbol symbol, I
{
AssigningCode = "httpContext.Request.Form";
}
// Complex form binding is only supported in RDF because it uses shared source with Blazor that requires dynamic analysis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like it's blaming blazor for dynamic codegen? Does that mean we can make it work by splitting from blazor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimal APIs shares the same implementation that Blazor uses for complex form binding (see here) which uses reflection. We could implement a source gen layer for form binding but the plan has been to do that in conjunction with Blazor so we're not solving the same problem twice in different ways.

@captainsafia captainsafia requested a review from eerhardt May 2, 2024 18:25
src/OpenApi/sample/Sample.csproj Outdated Show resolved Hide resolved
src/OpenApi/sample/Sample.csproj Outdated Show resolved Hide resolved
src/OpenApi/src/Schemas/OpenApiSchemaJsonOptions.cs Outdated Show resolved Hide resolved
using Microsoft.Extensions.DependencyInjection;
using Microsoft.OpenApi.Models;

namespace Microsoft.AspNetCore.OpenApi;

internal sealed class TypeBasedOpenApiDocumentTransformer(Type transformerType) : IOpenApiDocumentTransformer
internal sealed class TypeBasedOpenApiDocumentTransformer : IOpenApiDocumentTransformer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need the extra field. You can simply add [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] to the parameter, and it will work.

internal sealed class TypeBasedOpenApiDocumentTransformer([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type transformerType) : IOpenApiDocumentTransformer
{
    private readonly ObjectFactory _transformerFactory = ActivatorUtilities.CreateFactory(transformerType, []);

    public async Task TransformAsync(OpenApiDocument document, OpenApiDocumentTransformerContext context, CancellationToken cancellationToken)
    {
        var transformer = _transformerFactory.Invoke(context.ApplicationServices, []) as IOpenApiDocumentTransformer;
        Debug.Assert(transformer != null, $"The type {transformerType} does not implement {nameof(IOpenApiDocumentTransformer)}.");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but I believe there's an issue at the intersection of primary constructors + linker + DAM attributes:

Trim analysis warning IL2077: Microsoft.AspNetCore.OpenApi.TypeBasedOpenApiDocumentTransformer.TypeBasedOpenApiDocumentTransformer(Type): 'instanceType' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors' in call to 'Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateFactory(Type, Type[])'. The field 'Microsoft.AspNetCore.OpenApi.TypeBasedOpenApiDocumentTransformer.<_transformerType>P' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a great issue to open. 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -7,6 +7,10 @@
<!-- Needed to support compiling Utf8BufferTextWriter implementation shared with SignalR -->
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<Description>Provides APIs for annotating route handler endpoints in ASP.NET Core with OpenAPI annotations.</Description>
<IsTrimmable>true</IsTrimmable>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<IsTrimmable>true</IsTrimmable>
<IsAotCompatible>true</IsAotCompatible>

We should use this property which will enable all the analzyers. (honestly I thought that's what TrimmableProjects.props was supposed to be doing...?)

Copy link
Member Author

@captainsafia captainsafia May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, TrimmableProjects allows us to explicitly define the list of projects that the LinkabilityChecker will run against. This MSBuild check keeps the TrimmableProjects.props and IsTrimmable flags in sync.

As for IsAotCompatible, I could've sworn there was some MSBuild that was bringing in a PackageReference and borking the build althoug that doesn't seem to to be the case.

Edit: it's PublishAoT that brings in the PackageReference not PublishAoT.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments. It would be good to log that ILLink Roslyn Analyzer issue. I'll bet more people will run into it with primary constructors becoming popular.

@captainsafia
Copy link
Member Author

Just a few minor comments. It would be good to log that ILLink Roslyn Analyzer issue. I'll bet more people will run into it with primary constructors becoming popular.

Yep, I'm working on getting a minimal repro with this. Just running the linker on a console project with the buggy scenario doesn't reproduce so I suspect that something about the way our LinkabilityChecker is configured is causing the problem here.

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@captainsafia captainsafia enabled auto-merge (squash) May 3, 2024 19:17
@captainsafia captainsafia merged commit d28d0c5 into main May 3, 2024
26 checks passed
@captainsafia captainsafia deleted the safia/openapi-nativeaot branch May 3, 2024 19:33
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview5 milestone May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants