Skip to content

Commit

Permalink
Fix ModelMetadata for TryParse-parameters in ApiExplorer (#58350)
Browse files Browse the repository at this point in the history
* Fix ModelMetadata for TryParse-parameters in ApiExplorer

* Add tests and update code comments

* Update src/OpenApi/src/Services/OpenApiDocumentService.cs
  • Loading branch information
captainsafia committed Oct 11, 2024
1 parent 00eecee commit cb2c856
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 14 deletions.
45 changes: 38 additions & 7 deletions src/OpenApi/src/Services/OpenApiDocumentService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -403,12 +403,6 @@ private async Task<OpenApiResponse> 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;
var openApiParameter = new OpenApiParameter
{
Name = parameter.Name,
Expand All @@ -420,7 +414,7 @@ private async Task<OpenApiResponse> 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)
};

Expand Down Expand Up @@ -671,4 +665,41 @@ private async Task<OpenApiRequestBody> GetJsonRequestBody(

return requestBody;
}

/// <remarks>
/// 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.
/// </remarks>
/// <remarks>
/// This method will also check if no target type was resolved from the <see cref="ApiParameterDescription"/>
/// 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.
/// </remarks>
private static Type GetTargetType(ApiDescription description, ApiParameterDescription parameter)
{
var bindingMetadata = description.ActionDescriptor.EndpointMetadata
.OfType<IParameterBindingMetadata>()
.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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -583,13 +584,82 @@ static void AssertOpenApiDocument(OpenApiDocument document)
}

[Route("/api/with-enum")]
private ItemStatus GetItemStatus([FromQuery] ItemStatus status) => status;
private Status GetItemStatus([FromQuery] Status status) => status;

[JsonConverter(typeof(JsonStringEnumConverter<ItemStatus>))]
internal enum ItemStatus
[Fact]
public async Task SupportsMvcActionWithAmbientRouteParameter()
{
// Arrange
var action = CreateActionDescriptor(nameof(AmbientRouteParameter));

// Assert
await VerifyOpenApiDocument(action, document =>
{
var operation = document.Paths["/api/with-ambient-route-param/{versionId}"].Operations[OperationType.Get];
var parameter = Assert.Single(operation.Parameters);
Assert.Equal("string", parameter.Schema.Type);
});
}

[Route("/api/with-ambient-route-param/{versionId}")]
private void AmbientRouteParameter() { }

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task SupportsRouteParameterWithCustomTryParse(bool useAction)
{
Pending = 0,
Approved = 1,
Rejected = 2,
// 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;
}
}
}

0 comments on commit cb2c856

Please sign in to comment.