Browse Source

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
Bruno Oliveira 3 years ago
parent
commit
46b5580e8a

+ 12 - 1
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;

+ 2 - 2
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)
             {

+ 16 - 0
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);
+    }
 }

+ 191 - 0
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;
@@ -1579,6 +1583,120 @@ public class DefaultApiDescriptionProviderTest
         Assert.Equal(typeof(string), id.Type);
     }
 
+    [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()
     {
@@ -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);
+        }
+    }
 }

+ 42 - 0
src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs

@@ -298,6 +298,48 @@ public class EndpointMetadataApiDescriptionProviderTest
         AssertPathParameter(GetApiDescription((int foo) => { }, "/{foo}"));
     }
 
+    [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()
     {

+ 0 - 3
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)
             {