diff options
author | Bruno Oliveira <brunolins16@users.noreply.github.com> | 2022-11-05 05:01:25 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-11-05 05:01:25 +0300 |
commit | 46b5580e8a7ba2375820d95e3af80637be69f9de (patch) | |
tree | 6ce1a6e7a157e76ece597c5ceda5ae05ebf14994 | |
parent | 74568495602c809a1ef086d8d24d4665f5cf74a8 (diff) |
Set ApiParameterDescription.Type to `string` for Simple Types (#44747)
* Checking if IsPrimitive
* Fix comment
* Experimenting with additional types
* Adding DateTimeOffset
* clean up
* Adding unit test
* Using UnderlyingOrModelType
* Adding tests with nullable
* Adding primitive parsable types test
* Fix nullable warnings
* Adding checks to EndpointMetadataAPI
* Clean up
* clean up
* Adding using back
* Adding Timespan
* Adding more tests
6 files changed, 263 insertions, 6 deletions
diff --git a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs index e189fdac07..ffe76751eb 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs @@ -658,12 +658,23 @@ public class DefaultApiDescriptionProvider : IApiDescriptionProvider ModelMetadata = bindingContext.ModelMetadata, Name = GetName(containerName, bindingContext), Source = source, - Type = bindingContext.ModelMetadata.ModelType, + Type = GetModelType(bindingContext.ModelMetadata), ParameterDescriptor = Parameter, BindingInfo = bindingContext.BindingInfo }; } + private static Type GetModelType(ModelMetadata metadata) + { + // IsParseableType || IsConvertibleType + if (!metadata.IsComplexType) + { + return EndpointModelMetadata.GetDisplayType(metadata.ModelType); + } + + return metadata.ModelType; + } + private static string GetName(string containerName, ApiParameterDescriptionContext metadata) { var propertyName = !string.IsNullOrEmpty(metadata.BinderModelName) ? metadata.BinderModelName : metadata.PropertyName; diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 9319368440..76510c4d51 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -288,8 +288,8 @@ internal sealed class EndpointMetadataApiDescriptionProvider : IApiDescriptionPr else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType)) { // complex types will display as strings since they use custom parsing via TryParse on a string - var displayType = !parameter.ParameterType.IsPrimitive && Nullable.GetUnderlyingType(parameter.ParameterType)?.IsPrimitive != true - ? typeof(string) : parameter.ParameterType; + var displayType = EndpointModelMetadata.GetDisplayType(parameter.ParameterType); + // Path vs query cannot be determined by RequestDelegateFactory at startup currently because of the layering, but can be done here. if (parameter.Name is { } name && pattern.GetParameter(name) is { } routeParam) { diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointModelMetadata.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointModelMetadata.cs index 4ad1b72396..3fd46f1798 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointModelMetadata.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointModelMetadata.cs @@ -51,4 +51,20 @@ internal sealed class EndpointModelMetadata : ModelMetadata public override string? TemplateHint { get; } public override bool ValidateChildren { get; } public override IReadOnlyList<object> ValidatorMetadata { get; } = Array.Empty<object>(); + + public static Type GetDisplayType(Type type) + { + var underlyingType = Nullable.GetUnderlyingType(type) ?? type; + return underlyingType.IsPrimitive + // Those additional types have TypeConverter or TryParse and are not primitives + // but should not be considered string in the metadata + || underlyingType == typeof(DateTime) + || underlyingType == typeof(DateTimeOffset) + || underlyingType == typeof(DateOnly) + || underlyingType == typeof(TimeOnly) + || underlyingType == typeof(TimeSpan) + || underlyingType == typeof(decimal) + || underlyingType == typeof(Guid) + || underlyingType == typeof(Uri) ? type : typeof(string); + } } diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index ffb431bf6c..691aa28146 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -2,9 +2,13 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.ObjectModel; +using System.ComponentModel; using System.ComponentModel.DataAnnotations; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Reflection; using System.Text; +using System.Xml.Linq; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; @@ -1580,6 +1584,120 @@ public class DefaultApiDescriptionProviderTest } [Fact] + public void GetApiDescription_ParameterDescription_ParsablePrimitiveType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsTryParsablePrimitiveType)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "id"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(Guid), id.Type); + } + + [Fact] + public void GetApiDescription_ParameterDescription_NullableParsablePrimitiveType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsTryParsableNullablePrimitiveType)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "id"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(Guid?), id.Type); + } + + [Fact] + public void GetApiDescription_ParameterDescription_ParsableType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsTryParsableEmployee)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(string), id.Type); + } + + [Fact] + public void GetApiDescription_ParameterDescription_NullableParsableType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsNullableTryParsableEmployee)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(string), id.Type); + } + + [Fact] + public void GetApiDescription_ParameterDescription_ConvertibleType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsConvertibleEmployee)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(string), id.Type); + } + + [Fact] + public void GetApiDescription_ParameterDescription_NullableConvertibleType() + { + // Arrange + var action = CreateActionDescriptor(nameof(AcceptsNullableConvertibleEmployee)); + var parameterDescriptor = action.Parameters.Single(); + + // Act + var descriptions = GetApiDescriptions(action); + + // Assert + var description = Assert.Single(descriptions); + Assert.Equal(1, description.ParameterDescriptions.Count); + + var id = Assert.Single(description.ParameterDescriptions, p => p.Name == "employee"); + Assert.Same(BindingSource.Query, id.Source); + Assert.Equal(typeof(string), id.Type); + } + + [Fact] public void GetApiDescription_ParameterDescription_FromQueryManager() { // Arrange @@ -2374,6 +2492,34 @@ public class DefaultApiDescriptionProviderTest { } + private void AcceptsTryParsablePrimitiveType([FromQuery] Guid id) + { + } + + private void AcceptsTryParsableEmployee([FromQuery] TryParsableEmployee employee) + { + } + + private void AcceptsConvertibleEmployee([FromQuery] ConvertibleEmployee employee) + { + } + +#nullable enable + + private void AcceptsNullableTryParsableEmployee([FromQuery] TryParsableEmployee? employee) + { + } + + private void AcceptsTryParsableNullablePrimitiveType([FromQuery] Guid? id) + { + } + + private void AcceptsNullableConvertibleEmployee([FromQuery] ConvertibleEmployee? employee) + { + } + +#nullable restore + private void AcceptsOrderDTO(OrderDTO dto) { } @@ -2499,6 +2645,33 @@ public class DefaultApiDescriptionProviderTest public string Name { get; set; } } + [TypeConverter(typeof(EmployeeConverter))] + private class ConvertibleEmployee + { + public string Name { get; set; } + } + + private struct TryParsableEmployee : IParsable<TryParsableEmployee> + { + public string Name { get; set; } + + public static TryParsableEmployee Parse(string s, IFormatProvider provider) + { + if (TryParse(s, provider, out var result)) + { + return result; + } + + throw new FormatException($"{nameof(s)} is not in the correct format"); + } + + public static bool TryParse([NotNullWhen(true)] string s, IFormatProvider provider, [MaybeNullWhen(false)] out TryParsableEmployee result) + { + result = new() { Name = s }; + return true; + } + } + private class Manager { [FromQuery(Name = "managerid")] @@ -2727,4 +2900,22 @@ public class DefaultApiDescriptionProviderTest public bool IsOptional => false; } + + private class EmployeeConverter : TypeConverter + { + public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType) + { + return sourceType == typeof(string) || base.CanConvertFrom(context, sourceType); + } + + public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) + { + if (value is string input) + { + return new ConvertibleEmployee() { Name = input }; + } + + return base.ConvertFrom(context, culture, value); + } + } } diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 91df6a199c..3db8a4b611 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -299,6 +299,48 @@ public class EndpointMetadataApiDescriptionProviderTest } [Fact] + public void AddsFromRouteParameterAsPathWithSpecialPrimitiveType() + { + static void AssertPathParameter(ApiDescription apiDescription, Type expectedTYpe) + { + var param = Assert.Single(apiDescription.ParameterDescriptions); + Assert.Equal(expectedTYpe, param.Type); + Assert.Equal(expectedTYpe, param.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Path, param.Source); + } + + AssertPathParameter(GetApiDescription((DateTime foo) => { }, "/{foo}"), typeof(DateTime)); + AssertPathParameter(GetApiDescription((DateTimeOffset foo) => { }, "/{foo}"), typeof(DateTimeOffset)); + AssertPathParameter(GetApiDescription((DateOnly foo) => { }, "/{foo}"), typeof(DateOnly)); + AssertPathParameter(GetApiDescription((TimeOnly foo) => { }, "/{foo}"), typeof(TimeOnly)); + AssertPathParameter(GetApiDescription((TimeSpan foo) => { }, "/{foo}"), typeof(TimeSpan)); + AssertPathParameter(GetApiDescription((decimal foo) => { }, "/{foo}"), typeof(decimal)); + AssertPathParameter(GetApiDescription((Guid foo) => { }, "/{foo}"), typeof(Guid)); + AssertPathParameter(GetApiDescription((Uri foo) => { }, "/{foo}"), typeof(Uri)); + } + + [Fact] + public void AddsFromRouteParameterAsPathWithNullableSpecialPrimitiveType() + { + static void AssertPathParameter(ApiDescription apiDescription, Type expectedTYpe) + { + var param = Assert.Single(apiDescription.ParameterDescriptions); + Assert.Equal(expectedTYpe, param.Type); + Assert.Equal(expectedTYpe, param.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Path, param.Source); + } + + AssertPathParameter(GetApiDescription((DateTime? foo) => { }, "/{foo}"), typeof(DateTime?)); + AssertPathParameter(GetApiDescription((DateTimeOffset? foo) => { }, "/{foo}"), typeof(DateTimeOffset?)); + AssertPathParameter(GetApiDescription((DateOnly? foo) => { }, "/{foo}"), typeof(DateOnly?)); + AssertPathParameter(GetApiDescription((TimeOnly? foo) => { }, "/{foo}"), typeof(TimeOnly?)); + AssertPathParameter(GetApiDescription((TimeSpan? foo) => { }, "/{foo}"), typeof(TimeSpan?)); + AssertPathParameter(GetApiDescription((decimal? foo) => { }, "/{foo}"), typeof(decimal?)); + AssertPathParameter(GetApiDescription((Guid? foo) => { }, "/{foo}"), typeof(Guid?)); + AssertPathParameter(GetApiDescription((Uri? foo) => { }, "/{foo}"), typeof(Uri)); + } + + [Fact] public void AddsFromRouteParameterAsPathWithNullablePrimitiveType() { static void AssertPathParameter(ApiDescription apiDescription) diff --git a/src/OpenApi/src/OpenApiGenerator.cs b/src/OpenApi/src/OpenApiGenerator.cs index f6ceaf5ce2..51a31dc145 100644 --- a/src/OpenApi/src/OpenApiGenerator.cs +++ b/src/OpenApi/src/OpenApiGenerator.cs @@ -460,9 +460,6 @@ internal sealed class OpenApiGenerator } else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType)) { - // complex types will display as strings since they use custom parsing via TryParse on a string - var displayType = !parameter.ParameterType.IsPrimitive && Nullable.GetUnderlyingType(parameter.ParameterType)?.IsPrimitive != true - ? typeof(string) : parameter.ParameterType; // Path vs query cannot be determined by RequestDelegateFactory at startup currently because of the layering, but can be done here. if (parameter.Name is { } name && pattern.GetParameter(name) is not null) { |