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
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eng/TrimmableProjects.props
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,6 @@
<TrimmableProject Include="Microsoft.Extensions.Diagnostics.HealthChecks.Abstractions" />
<TrimmableProject Include="Microsoft.Extensions.Diagnostics.HealthChecks" />
<TrimmableProject Include="Microsoft.Extensions.Features" />
<TrimmableProject Include="Microsoft.AspNetCore.OpenApi" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -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.

// and codegen. Emit a diagnostic when these are encountered to avoid producing buggy code.
else if (!(SymbolEqualityComparer.Default.Equals(Type, wellKnownTypes.Get(WellKnownType.Microsoft_Extensions_Primitives_StringValues))
|| Type.SpecialType == SpecialType.System_String
|| TryGetParsability(Type, wellKnownTypes, out var _)
|| (IsArray && TryGetParsability(ElementType, wellKnownTypes, out var _))))
{
var location = endpoint.Operation.Syntax.GetLocation();
endpoint.Diagnostics.Add(Diagnostic.Create(DiagnosticDescriptors.UnableToResolveParameterDescriptor, location, symbol.Name));
}
else
{
AssigningCode = !IsArray
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -737,4 +737,24 @@ public class ConcreteService : IGenericService<SomeInput, string?>
var diagnostics = updatedCompilation.GetDiagnostics();
Assert.Empty(diagnostics.Where(d => d.Severity >= DiagnosticSeverity.Warning));
}

[Fact]
public async Task RequestDelegateGenerator_SkipsComplexFormParameter()
{
var source = """
app.MapPost("/", ([FromForm] Todo todo) => { });
app.MapPost("/", ([FromForm] Todo todo, IFormFile formFile) => { });
app.MapPost("/", ([FromForm] Todo todo, [FromForm] int[] ids) => { });
""";
var (generatorRunResult, _) = await RunGeneratorAsync(source);

// Emits diagnostics but no generated sources
var result = Assert.IsType<GeneratorRunResult>(generatorRunResult);
Assert.Empty(result.GeneratedSources);
Assert.All(result.Diagnostics, diagnostic =>
{
Assert.Equal(DiagnosticDescriptors.UnableToResolveParameterDescriptor.Id, diagnostic.Id);
Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity);
});
}
}
2 changes: 2 additions & 0 deletions src/OpenApi/sample/Controllers/TestController.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.ComponentModel.DataAnnotations;
using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Mvc;

[ApiController]
Expand Down Expand Up @@ -26,6 +27,7 @@ public class RouteParamsContainer

[FromRoute]
[MinLength(5)]
[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "Suppress trim-related warnings for MVC.")]
captainsafia marked this conversation as resolved.
Show resolved Hide resolved
public string? Name { get; set; }
}

Expand Down
9 changes: 9 additions & 0 deletions src/OpenApi/sample/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

var builder = WebApplication.CreateBuilder(args);

#pragma warning disable IL2026 // MVC isn't trim-friendly yet
builder.Services.AddControllers();
#pragma warning restore IL2026
builder.Services.AddAuthentication().AddJwtBearer();

builder.Services.AddOpenApi("v1", options =>
Expand Down Expand Up @@ -44,8 +46,15 @@
forms.MapPost("/form-file", (IFormFile resume) => Results.Ok(resume.FileName));
forms.MapPost("/form-files", (IFormFileCollection files) => Results.Ok(files.Count));
forms.MapPost("/form-file-multiple", (IFormFile resume, IFormFileCollection files) => Results.Ok(files.Count + resume.FileName));
// Disable warnings because RDG does not support complex form binding yet.
#pragma warning disable IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code
#pragma warning disable IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
#pragma warning disable RDG003 // Unable to resolve parameter
forms.MapPost("/form-todo", ([FromForm] Todo todo) => Results.Ok(todo));
forms.MapPost("/forms-pocos-and-files", ([FromForm] Todo todo, IFormFile file) => Results.Ok(new { Todo = todo, File = file.FileName }));
#pragma warning restore RDG003 // Unable to resolve parameter
#pragma warning restore IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
#pragma warning restore IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code

var v1 = app.MapGroup("v1")
.WithGroupName("v1");
Expand Down
9 changes: 9 additions & 0 deletions src/OpenApi/sample/Sample.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<IsTrimmable>true</IsTrimmable>
captainsafia marked this conversation as resolved.
Show resolved Hide resolved
captainsafia marked this conversation as resolved.
Show resolved Hide resolved
<!-- Required to generated trimmable Map-invocations. -->
<EnableRequestDelegateGenerator>true</EnableRequestDelegateGenerator>
<IsAotCompatible>true</IsAotCompatible>
</PropertyGroup>

<ItemGroup>
Expand All @@ -22,4 +26,9 @@
<Compile Include="../test/SharedTypes.cs" />
</ItemGroup>

<!-- Required to generated trimmable Map-invocations. -->
<ItemGroup>
<ProjectReference Include="$(RepoRoot)/src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.RequestDelegateGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
</ItemGroup>

</Project>
11 changes: 3 additions & 8 deletions src/OpenApi/src/Extensions/JsonObjectSchemaExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System.ComponentModel.DataAnnotations;
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Text.Json.Nodes;
using JsonSchemaMapper;
using Microsoft.AspNetCore.Mvc.ApiExplorer;
Expand Down Expand Up @@ -248,14 +247,10 @@ internal static void ApplyParameterInfo(this JsonObject schema, ApiParameterDesc
// based on our model binding heuristics. In that case, to access the validation attributes that the
// model binder will respect we will need to get the property from the container type and map the
// attributes on it to the schema.
if (parameterDescription.ModelMetadata.PropertyName is { } propertyName)
if (parameterDescription.ModelMetadata is { PropertyName: { }, ContainerType: { }, HasValidators: true, ValidatorMetadata: { } validations })
{
var property = parameterDescription.ModelMetadata.ContainerType?.GetProperty(propertyName, BindingFlags.Public | BindingFlags.Instance);
if (property is not null)
{
var attributes = property.GetCustomAttributes(true).OfType<ValidationAttribute>();
schema.ApplyValidationAttributes(attributes);
}
var attributes = validations.OfType<ValidationAttribute>();
schema.ApplyValidationAttributes(attributes);
}
// Route constraints are only defined on parameters that are sourced from the path. Since
// they are encoded in the route template, and not in the type information based to the underlying
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Http;
Expand All @@ -17,12 +18,16 @@ namespace Microsoft.AspNetCore.Builder;
/// </summary>
public static class OpenApiEndpointConventionBuilderExtensions
{
private const string TrimWarningMessage = "Calls Microsoft.AspNetCore.OpenApi.OpenApiGenerator.GetOpenApiOperation(MethodInfo, EndpointMetadataCollection, RoutePattern) which uses dynamic analysis. Use IServiceCollection.AddOpenApi() to generate OpenAPI metadata at startup for all endpoints,";

/// <summary>
/// Adds an OpenAPI annotation to <see cref="Endpoint.Metadata" /> associated
/// with the current endpoint.
/// </summary>
/// <param name="builder">The <see cref="IEndpointConventionBuilder"/>.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresDynamicCode(TrimWarningMessage)]
[RequiresUnreferencedCode(TrimWarningMessage)]
public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder
{
builder.Finally(builder => AddAndConfigureOperationForEndpoint(builder));
Expand All @@ -36,13 +41,17 @@ public static class OpenApiEndpointConventionBuilderExtensions
/// <param name="builder">The <see cref="IEndpointConventionBuilder"/>.</param>
/// <param name="configureOperation">An <see cref="Func{T, TResult}"/> that returns a new OpenAPI annotation given a generated operation.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
[RequiresDynamicCode(TrimWarningMessage)]
[RequiresUnreferencedCode(TrimWarningMessage)]
public static TBuilder WithOpenApi<TBuilder>(this TBuilder builder, Func<OpenApiOperation, OpenApiOperation> configureOperation)
where TBuilder : IEndpointConventionBuilder
{
builder.Finally(endpointBuilder => AddAndConfigureOperationForEndpoint(endpointBuilder, configureOperation));
return builder;
}

[RequiresDynamicCode(TrimWarningMessage)]
[RequiresUnreferencedCode(TrimWarningMessage)]
private static void AddAndConfigureOperationForEndpoint(EndpointBuilder endpointBuilder, Func<OpenApiOperation, OpenApiOperation>? configure = null)
{
foreach (var item in endpointBuilder.Metadata)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Json;
using Microsoft.AspNetCore.OpenApi;
using Microsoft.Extensions.ApiDescriptions;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Options;

namespace Microsoft.Extensions.DependencyInjection;

Expand Down Expand Up @@ -67,6 +70,8 @@ private static IServiceCollection AddOpenApiCore(this IServiceCollection service
services.AddSingleton<IDocumentProvider, OpenApiDocumentProvider>();
// Required to resolve document names for build-time generation
services.AddSingleton(new NamedService<OpenApiDocumentService>(documentName));
// Required to support JSON serializations
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<JsonOptions>, OpenApiSchemaJsonOptions>());
return services;
}
}
9 changes: 9 additions & 0 deletions src/OpenApi/src/Microsoft.AspNetCore.OpenApi.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -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>
<IsAotCompatible>true</IsAotCompatible>
<!-- Required to generated native AoT friendly `MapOpenApi` endpoint. -->
<EnableRequestDelegateGenerator>true</EnableRequestDelegateGenerator>
captainsafia marked this conversation as resolved.
Show resolved Hide resolved
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.AspNetCore.Http.Generated</InterceptorsPreviewNamespaces>
captainsafia marked this conversation as resolved.
Show resolved Hide resolved
</PropertyGroup>

<ItemGroup>
Expand Down Expand Up @@ -36,4 +40,9 @@
<Compile Include="$(RepoRoot)/src/SignalR/common/Shared/MemoryBufferWriter.cs" LinkBase="Shared" />
</ItemGroup>

<!-- Required to generated native AoT friendly `MapOpenApi` endpoint. -->
<ItemGroup>
<ProjectReference Include="$(RepoRoot)/src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.RequestDelegateGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
</ItemGroup>

</Project>
23 changes: 11 additions & 12 deletions src/OpenApi/src/Schemas/OpenApiJsonSchema.Helpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
using Microsoft.AspNetCore.OpenApi;
using Microsoft.OpenApi.Any;
using Microsoft.OpenApi.Models;

Expand All @@ -14,9 +15,8 @@ internal sealed partial class OpenApiJsonSchema
/// </summary>
/// <typeparam name="T">The type of the elements that will populate the list.</typeparam>
/// <param name="reader">The <see cref="Utf8JsonReader"/> to consume the list from.</param>
/// <param name="options">The <see cref="JsonSerializerOptions"/> instance.</param>
/// <returns>A list parsed from the JSON array.</returns>
public static List<T>? ReadList<T>(ref Utf8JsonReader reader, JsonSerializerOptions options)
public static List<T>? ReadList<T>(ref Utf8JsonReader reader)
{
if (reader.TokenType == JsonTokenType.Null)
{
Expand All @@ -29,7 +29,7 @@ internal sealed partial class OpenApiJsonSchema
reader.Read();
while (reader.TokenType != JsonTokenType.EndArray)
{
values.Add(JsonSerializer.Deserialize<T>(ref reader, options)!);
values.Add((T)JsonSerializer.Deserialize(ref reader, typeof(T), OpenApiJsonSchemaContext.Default)!);
reader.Read();
}

Expand All @@ -49,10 +49,9 @@ internal sealed partial class OpenApiJsonSchema
/// </summary>
/// <typeparam name="T">The type associated with the values in the dictionary.</typeparam>
/// <param name="reader">The <see cref="Utf8JsonReader"/> to consume the dictionary from.</param>
/// <param name="options">The <see cref="JsonSerializerOptions"/> instance.</param>
/// <returns>A dictionary parsed from the JSON object.</returns>
/// <exception cref="JsonException">Thrown if JSON object is not valid.</exception>
public static Dictionary<string, T>? ReadDictionary<T>(ref Utf8JsonReader reader, JsonSerializerOptions options)
public static Dictionary<string, T>? ReadDictionary<T>(ref Utf8JsonReader reader)
{
if (reader.TokenType == JsonTokenType.Null)
{
Expand All @@ -75,7 +74,7 @@ internal sealed partial class OpenApiJsonSchema

var key = reader.GetString()!;
reader.Read();
values[key] = JsonSerializer.Deserialize<T>(ref reader, options)!;
values[key] = (T)JsonSerializer.Deserialize(ref reader, typeof(T), OpenApiJsonSchemaContext.Default)!;
reader.Read();
}

Expand Down Expand Up @@ -170,7 +169,7 @@ public static void ReadProperty(ref Utf8JsonReader reader, string propertyName,
reader.Read();
if (reader.TokenType == JsonTokenType.StartArray)
{
var types = ReadList<string>(ref reader, options);
var types = ReadList<string>(ref reader);
foreach (var type in types ?? [])
{
// JSON Schema represents nullable types using an array consisting of
Expand All @@ -195,7 +194,7 @@ public static void ReadProperty(ref Utf8JsonReader reader, string propertyName,
break;
case OpenApiSchemaKeywords.EnumKeyword:
reader.Read();
var enumValues = ReadList<string>(ref reader, options);
var enumValues = ReadList<string>(ref reader);
if (enumValues is not null)
{
schema.Enum = enumValues.Select(v => new OpenApiString(v)).ToList<IOpenApiAny>();
Expand Down Expand Up @@ -224,7 +223,7 @@ public static void ReadProperty(ref Utf8JsonReader reader, string propertyName,
break;
case OpenApiSchemaKeywords.RequiredKeyword:
reader.Read();
schema.Required = ReadList<string>(ref reader, options)?.ToHashSet();
schema.Required = ReadList<string>(ref reader)?.ToHashSet();
break;
case OpenApiSchemaKeywords.MinLengthKeyword:
reader.Read();
Expand Down Expand Up @@ -263,13 +262,13 @@ public static void ReadProperty(ref Utf8JsonReader reader, string propertyName,
break;
case OpenApiSchemaKeywords.PropertiesKeyword:
reader.Read();
var props = ReadDictionary<OpenApiJsonSchema>(ref reader, options);
var props = ReadDictionary<OpenApiJsonSchema>(ref reader);
schema.Properties = props?.ToDictionary(p => p.Key, p => p.Value.Schema);
break;
case OpenApiSchemaKeywords.AnyOfKeyword:
reader.Read();
schema.Type = "object";
var schemas = ReadList<OpenApiJsonSchema>(ref reader, options);
var schemas = ReadList<OpenApiJsonSchema>(ref reader);
schema.AnyOf = schemas?.Select(s => s.Schema).ToList();
break;
case OpenApiSchemaKeywords.DiscriminatorKeyword:
Expand All @@ -282,7 +281,7 @@ public static void ReadProperty(ref Utf8JsonReader reader, string propertyName,
break;
case OpenApiSchemaKeywords.DiscriminatorMappingKeyword:
reader.Read();
var mappings = ReadDictionary<string>(ref reader, options);
var mappings = ReadDictionary<string>(ref reader);
schema.Discriminator.Mapping = mappings;
break;
}
Expand Down
10 changes: 10 additions & 0 deletions src/OpenApi/src/Schemas/OpenApiJsonSchemaContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text.Json.Serialization;

namespace Microsoft.AspNetCore.OpenApi;

[JsonSerializable(typeof(OpenApiJsonSchema))]
[JsonSerializable(typeof(string))]
internal sealed partial class OpenApiJsonSchemaContext : JsonSerializerContext { }
16 changes: 16 additions & 0 deletions src/OpenApi/src/Schemas/OpenApiSchemaJsonOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Json;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.OpenApi;

internal sealed class OpenApiSchemaJsonOptions : IConfigureOptions<JsonOptions>
{
public void Configure(JsonOptions options)
{
// Put our resolver in front of the reflection-based one. See ProblemDetailsJsonOptionsSetup for more info.
options.SerializerOptions.TypeInfoResolverChain.Insert(0, OpenApiJsonSchemaContext.Default);
}
}
3 changes: 2 additions & 1 deletion src/OpenApi/src/Services/OpenApiComponentService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ internal OpenApiSchema GetOrCreateSchema(Type type, ApiParameterDescription? par
{
schemaAsJsonObject.ApplyParameterInfo(parameterDescription);
}
return JsonSerializer.Deserialize<OpenApiJsonSchema>(schemaAsJsonObject)?.Schema ?? new OpenApiSchema();
var deserializedSchema = JsonSerializer.Deserialize(schemaAsJsonObject, OpenApiJsonSchemaContext.Default.OpenApiJsonSchema);
return deserializedSchema != null ? deserializedSchema.Schema : new OpenApiSchema();
}

private JsonObject CreateSchema((Type Type, ParameterInfo? ParameterInfo) key)
Expand Down
3 changes: 3 additions & 0 deletions src/OpenApi/src/Services/OpenApiGenerator.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Reflection;
Expand All @@ -25,6 +26,8 @@ namespace Microsoft.AspNetCore.OpenApi;
/// <summary>
/// Defines a set of methods for generating OpenAPI definitions for endpoints.
/// </summary>
[RequiresUnreferencedCode("OpenApiGenerator performs reflection to generate OpenAPI descriptors. This cannot be statically analyzed.")]
[RequiresDynamicCode("OpenApiGenerator performs reflection to generate OpenAPI descriptors. This cannot be statically analyzed.")]
internal sealed class OpenApiGenerator
{
private readonly IHostEnvironment? _environment;
Expand Down
Loading
Loading