From 579b3fd4c32e949c95e99f238c3cf7dadafdb55b Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Thu, 10 Oct 2024 18:29:49 -0700 Subject: [PATCH] Fix ModelMetadata for TryParse-parameters in ApiExplorer (#58350) * Fix ModelMetadata for TryParse-parameters in ApiExplorer * Add tests and update code comments * Update src/OpenApi/src/Services/OpenApiDocumentService.cs --- .../src/Services/OpenApiDocumentService.cs | 49 ++++++++++--- .../OpenApiSchemaService.ParameterSchemas.cs | 72 ++++++++++++++++--- 2 files changed, 100 insertions(+), 21 deletions(-) diff --git a/src/OpenApi/src/Services/OpenApiDocumentService.cs b/src/OpenApi/src/Services/OpenApiDocumentService.cs index bc0094faa108..a9a7ecf6a1a6 100644 --- a/src/OpenApi/src/Services/OpenApiDocumentService.cs +++ b/src/OpenApi/src/Services/OpenApiDocumentService.cs @@ -403,16 +403,6 @@ private async Task GetResponseAsync( continue; } - // MVC's ModelMetadata layer will set ApiParameterDescription.Type to string when the parameter - // is a parsable or convertible type. In this case, we want to use the actual model type - // to generate the schema instead of the string type. - var targetType = parameter.Type == typeof(string) && parameter.ModelMetadata.ModelType != parameter.Type - ? parameter.ModelMetadata.ModelType - : parameter.Type; - // If the type is null, then we're dealing with an inert - // route parameter that does not define a specific parameter type in the - // route handler or in the response. In this case, we default to a string schema. - targetType ??= typeof(string); var openApiParameter = new OpenApiParameter { Name = parameter.Name, @@ -424,7 +414,7 @@ private async Task GetResponseAsync( _ => throw new InvalidOperationException($"Unsupported parameter source: {parameter.Source.Id}") }, Required = IsRequired(parameter), - Schema = await _componentService.GetOrCreateSchemaAsync(targetType, scopedServiceProvider, schemaTransformers, parameter, cancellationToken: cancellationToken), + Schema = await _componentService.GetOrCreateSchemaAsync(GetTargetType(description, parameter), scopedServiceProvider, schemaTransformers, parameter, cancellationToken: cancellationToken), Description = GetParameterDescriptionFromAttribute(parameter) }; @@ -675,4 +665,41 @@ private async Task GetJsonRequestBody( return requestBody; } + + /// + /// This method is used to determine the target type for a given parameter. The target type + /// is the actual type that should be used to generate the schema for the parameter. This is + /// necessary because MVC's ModelMetadata layer will set ApiParameterDescription.Type to string + /// when the parameter is a parsable or convertible type. In this case, we want to use the actual + /// model type to generate the schema instead of the string type. + /// + /// + /// This method will also check if no target type was resolved from the + /// and default to a string schema. This will happen if we are dealing with an inert route parameter + /// that does not define a specific parameter type in the route handler or in the response. + /// + private static Type GetTargetType(ApiDescription description, ApiParameterDescription parameter) + { + var bindingMetadata = description.ActionDescriptor.EndpointMetadata + .OfType() + .SingleOrDefault(metadata => metadata.Name == parameter.Name); + var parameterType = parameter.Type is not null + ? Nullable.GetUnderlyingType(parameter.Type) ?? parameter.Type + : parameter.Type; + + // parameter.Type = typeof(string) + // parameter.ModelMetadata.Type = typeof(TEnum) + var requiresModelMetadataFallbackForEnum = parameterType == typeof(string) + && parameter.ModelMetadata.ModelType != parameter.Type + && parameter.ModelMetadata.ModelType.IsEnum; + // Enums are exempt because we want to set the OpenApiSchema.Enum field when feasible. + // parameter.Type = typeof(TEnum), typeof(TypeWithTryParse) + // parameter.ModelMetadata.Type = typeof(string) + var hasTryParse = bindingMetadata?.HasTryParse == true && parameterType is not null && !parameterType.IsEnum; + var targetType = requiresModelMetadataFallbackForEnum || hasTryParse + ? parameter.ModelMetadata.ModelType + : parameter.Type; + targetType ??= typeof(string); + return targetType; + } } diff --git a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ParameterSchemas.cs b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ParameterSchemas.cs index 671fc166e87a..96e3b3ccbe15 100644 --- a/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ParameterSchemas.cs +++ b/src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ParameterSchemas.cs @@ -547,7 +547,8 @@ public async Task SupportsParameterWithEnumType(bool useAction) if (!useAction) { var builder = CreateBuilder(); - builder.MapGet("/api/with-enum", (ItemStatus status) => status); + builder.MapGet("/api/with-enum", (Status status) => status); + await VerifyOpenApiDocument(builder, AssertOpenApiDocument); } else { @@ -583,15 +584,7 @@ static void AssertOpenApiDocument(OpenApiDocument document) } [Route("/api/with-enum")] - private ItemStatus GetItemStatus([FromQuery] ItemStatus status) => status; - - [JsonConverter(typeof(JsonStringEnumConverter))] - internal enum ItemStatus - { - Pending = 0, - Approved = 1, - Rejected = 2, - } + private Status GetItemStatus([FromQuery] Status status) => status; [Fact] public async Task SupportsMvcActionWithAmbientRouteParameter() @@ -610,4 +603,63 @@ await VerifyOpenApiDocument(action, document => [Route("/api/with-ambient-route-param/{versionId}")] private void AmbientRouteParameter() { } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task SupportsRouteParameterWithCustomTryParse(bool useAction) + { + // Arrange + var builder = CreateBuilder(); + + // Act + if (!useAction) + { + builder.MapGet("/api/{student}", (Student student) => student); + await VerifyOpenApiDocument(builder, AssertOpenApiDocument); + } + else + { + var action = CreateActionDescriptor(nameof(GetStudent)); + await VerifyOpenApiDocument(action, AssertOpenApiDocument); + } + + // Assert + static void AssertOpenApiDocument(OpenApiDocument document) + { + // Parameter is a plain-old string when it comes from the route or query + var operation = document.Paths["/api/{student}"].Operations[OperationType.Get]; + var parameter = Assert.Single(operation.Parameters); + Assert.Equal("string", parameter.Schema.Type); + + // Type is fully serialized in the response + var response = Assert.Single(operation.Responses).Value; + Assert.True(response.Content.TryGetValue("application/json", out var mediaType)); + var schema = mediaType.Schema.GetEffective(document); + Assert.Equal("object", schema.Type); + Assert.Collection(schema.Properties, property => + { + Assert.Equal("name", property.Key); + Assert.Equal("string", property.Value.Type); + }); + } + } + + [Route("/api/{student}")] + private Student GetStudent(Student student) => student; + + public record Student(string Name) + { + public static bool TryParse(string value, out Student result) + { + if (value is null) + { + result = null; + return false; + } + + result = new Student(value); + return true; + } + } }