Set IsRequired on ApiDescriptions for endpoints (#35233)

davidfowl · web-flow · commit 123bd0611ac6 · 2021-08-11T16:01:00.000-07:00
* Set isRequired on ApiDescriptions for endpoints
- Use the same logic we have in RequestDelegateFactory.Create to
determine if a method parameter is required or not. We then set the
IsRequired property on the ApiParameterDesciption.
diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
@@ -28,6 +28,7 @@ internal class EndpointMetadataApiDescriptionProvider : IApiDescriptionProvider
         private readonly IHostEnvironment _environment;
         private readonly IServiceProviderIsService? _serviceProviderIsService;
         private readonly TryParseMethodCache TryParseMethodCache = new();
+        private readonly NullabilityInfoContext NullabilityContext = new();
 
         // Executes before MVC's DefaultApiDescriptionProvider and GrpcHttpApiDescriptionProvider for no particular reason.
         public int Order => -1100;
@@ -132,45 +133,50 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string
 
         private ApiParameterDescription? CreateApiParameterDescription(ParameterInfo parameter, RoutePattern pattern)
         {
-            var (source, name) = GetBindingSourceAndName(parameter, pattern);
+            var (source, name, allowEmpty) = GetBindingSourceAndName(parameter, pattern);
 
             // Services are ignored because they are not request parameters.
             if (source == BindingSource.Services)
             {
                 return null;
             }
 
+            // Determine the "requiredness" based on nullability, default value or if allowEmpty is set
+            var nullability = NullabilityContext.Create(parameter);
+            var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable || allowEmpty;
+
             return new ApiParameterDescription
             {
                 Name = name,
                 ModelMetadata = CreateModelMetadata(parameter.ParameterType),
                 Source = source,
                 DefaultValue = parameter.DefaultValue,
                 Type = parameter.ParameterType,
+                IsRequired = !isOptional
             };
         }
 
         // TODO: Share more of this logic with RequestDelegateFactory.CreateArgument(...) using RequestDelegateFactoryUtilities
         // which is shared source.
-        private (BindingSource, string) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern)
+        private (BindingSource, string, bool) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern)
         {
             var attributes = parameter.GetCustomAttributes();
 
             if (attributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute)
             {
-                return (BindingSource.Path, routeAttribute.Name ?? parameter.Name ?? string.Empty);
+                return (BindingSource.Path, routeAttribute.Name ?? parameter.Name ?? string.Empty, false);
             }
             else if (attributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
             {
-                return (BindingSource.Query, queryAttribute.Name ?? parameter.Name ?? string.Empty);
+                return (BindingSource.Query, queryAttribute.Name ?? parameter.Name ?? string.Empty, false);
             }
             else if (attributes.OfType<IFromHeaderMetadata>().FirstOrDefault() is { } headerAttribute)
             {
-                return (BindingSource.Header, headerAttribute.Name ?? parameter.Name ?? string.Empty);
+                return (BindingSource.Header, headerAttribute.Name ?? parameter.Name ?? string.Empty, false);
             }
-            else if (parameter.CustomAttributes.Any(a => typeof(IFromBodyMetadata).IsAssignableFrom(a.AttributeType)))
+            else if (attributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } fromBodyAttribute)
             {
-                return (BindingSource.Body, parameter.Name ?? string.Empty);
+                return (BindingSource.Body, parameter.Name ?? string.Empty, fromBodyAttribute.AllowEmpty);
             }
             else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)) ||
                      parameter.ParameterType == typeof(HttpContext) ||
@@ -180,23 +186,23 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string
                      parameter.ParameterType == typeof(CancellationToken) ||
                      _serviceProviderIsService?.IsService(parameter.ParameterType) == true)
             {
-                return (BindingSource.Services, parameter.Name ?? string.Empty);
+                return (BindingSource.Services, parameter.Name ?? string.Empty, false);
             }
             else if (parameter.ParameterType == typeof(string) || TryParseMethodCache.HasTryParseMethod(parameter))
             {
                 // 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)
                 {
-                    return (BindingSource.Path, name);
+                    return (BindingSource.Path, name, false);
                 }
                 else
                 {
-                    return (BindingSource.Query, parameter.Name ?? string.Empty);
+                    return (BindingSource.Query, parameter.Name ?? string.Empty, false);
                 }
             }
             else
             {
-                return (BindingSource.Body, parameter.Name ?? string.Empty);
+                return (BindingSource.Body, parameter.Name ?? string.Empty, false);
             }
         }
 
diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs
@@ -76,28 +76,31 @@ static void AssertJsonRequestFormat(ApiDescription apiDescription)
         [Fact]
         public void AddsRequestFormatFromMetadata()
         {
-            static void AssertustomRequestFormat(ApiDescription apiDescription)
+            static void AssertCustomRequestFormat(ApiDescription apiDescription)
             {
                 var requestFormat = Assert.Single(apiDescription.SupportedRequestFormats);
                 Assert.Equal("application/custom", requestFormat.MediaType);
                 Assert.Null(requestFormat.Formatter);
             }
 
-            AssertustomRequestFormat(GetApiDescription(
+            AssertCustomRequestFormat(GetApiDescription(
                 [Consumes("application/custom")]
-                (InferredJsonClass fromBody) => { }));
+            (InferredJsonClass fromBody) =>
+                { }));
 
-            AssertustomRequestFormat(GetApiDescription(
+            AssertCustomRequestFormat(GetApiDescription(
                 [Consumes("application/custom")]
-                ([FromBody] int fromBody) => { }));
+            ([FromBody] int fromBody) =>
+                { }));
         }
 
         [Fact]
         public void AddsMultipleRequestFormatsFromMetadata()
         {
             var apiDescription = GetApiDescription(
                 [Consumes("application/custom0", "application/custom1")]
-                (InferredJsonClass fromBody) => { });
+            (InferredJsonClass fromBody) =>
+                { });
 
             Assert.Equal(2, apiDescription.SupportedRequestFormats.Count);
 
@@ -167,8 +170,8 @@ public void AddsResponseFormatFromMetadata()
         {
             var apiDescription = GetApiDescription(
                 [ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created)]
-                [Produces("application/custom")]
-                () => new InferredJsonClass());
+            [Produces("application/custom")]
+            () => new InferredJsonClass());
 
             var responseType = Assert.Single(apiDescription.SupportedResponseTypes);
 
@@ -185,8 +188,8 @@ public void AddsMultipleResponseFormatsFromMetadataWithPoco()
         {
             var apiDescription = GetApiDescription(
                 [ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created)]
-                [ProducesResponseType(StatusCodes.Status400BadRequest)]
-                () => new InferredJsonClass());
+            [ProducesResponseType(StatusCodes.Status400BadRequest)]
+            () => new InferredJsonClass());
 
             Assert.Equal(2, apiDescription.SupportedResponseTypes.Count);
 
@@ -214,8 +217,8 @@ public void AddsMultipleResponseFormatsFromMetadataWithIResult()
         {
             var apiDescription = GetApiDescription(
                 [ProducesResponseType(typeof(InferredJsonClass), StatusCodes.Status201Created)]
-                [ProducesResponseType(StatusCodes.Status400BadRequest)]
-                () => Results.Ok(new InferredJsonClass()));
+            [ProducesResponseType(StatusCodes.Status400BadRequest)]
+            () => Results.Ok(new InferredJsonClass()));
 
             Assert.Equal(2, apiDescription.SupportedResponseTypes.Count);
 
@@ -324,18 +327,68 @@ public void AddsMultipleParameters()
             Assert.Equal(typeof(int), fooParam.Type);
             Assert.Equal(typeof(int), fooParam.ModelMetadata.ModelType);
             Assert.Equal(BindingSource.Path, fooParam.Source);
+            Assert.True(fooParam.IsRequired);
 
             var barParam = apiDescription.ParameterDescriptions[1];
             Assert.Equal(typeof(int), barParam.Type);
             Assert.Equal(typeof(int), barParam.ModelMetadata.ModelType);
             Assert.Equal(BindingSource.Query, barParam.Source);
+            Assert.True(barParam.IsRequired);
 
             var fromBodyParam = apiDescription.ParameterDescriptions[2];
             Assert.Equal(typeof(InferredJsonClass), fromBodyParam.Type);
             Assert.Equal(typeof(InferredJsonClass), fromBodyParam.ModelMetadata.ModelType);
             Assert.Equal(BindingSource.Body, fromBodyParam.Source);
+            Assert.True(fromBodyParam.IsRequired);
         }
 
+        [Fact]
+        public void TestParameterIsRequired()
+        {
+            var apiDescription = GetApiDescription(([FromRoute] int foo, int? bar) => { });
+            Assert.Equal(2, apiDescription.ParameterDescriptions.Count);
+
+            var fooParam = apiDescription.ParameterDescriptions[0];
+            Assert.Equal(typeof(int), fooParam.Type);
+            Assert.Equal(typeof(int), fooParam.ModelMetadata.ModelType);
+            Assert.Equal(BindingSource.Path, fooParam.Source);
+            Assert.True(fooParam.IsRequired);
+
+            var barParam = apiDescription.ParameterDescriptions[1];
+            Assert.Equal(typeof(int?), barParam.Type);
+            Assert.Equal(typeof(int?), barParam.ModelMetadata.ModelType);
+            Assert.Equal(BindingSource.Query, barParam.Source);
+            Assert.False(barParam.IsRequired);
+        }
+
+#nullable enable
+
+        [Fact]
+        public void TestIsRequiredFromBody()
+        {
+            var apiDescription0 = GetApiDescription(([FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] InferredJsonClass fromBody) => { });
+            var apiDescription1 = GetApiDescription((InferredJsonClass? fromBody) => { });
+            Assert.Equal(1, apiDescription0.ParameterDescriptions.Count);
+            Assert.Equal(1, apiDescription1.ParameterDescriptions.Count);
+
+            var fromBodyParam0 = apiDescription0.ParameterDescriptions[0];
+            Assert.Equal(typeof(InferredJsonClass), fromBodyParam0.Type);
+            Assert.Equal(typeof(InferredJsonClass), fromBodyParam0.ModelMetadata.ModelType);
+            Assert.Equal(BindingSource.Body, fromBodyParam0.Source);
+            Assert.False(fromBodyParam0.IsRequired);
+
+            var fromBodyParam1 = apiDescription1.ParameterDescriptions[0];
+            Assert.Equal(typeof(InferredJsonClass), fromBodyParam1.Type);
+            Assert.Equal(typeof(InferredJsonClass), fromBodyParam1.ModelMetadata.ModelType);
+            Assert.Equal(BindingSource.Body, fromBodyParam1.Source);
+            Assert.False(fromBodyParam1.IsRequired);
+        }
+
+        // This is necessary for TestIsRequiredFromBody to pass until https://github.com/dotnet/roslyn/issues/55254 is resolved.
+        private object RandomMethod() => throw new NotImplementedException();
+
+#nullable disable
+
         [Fact]
         public void AddsDisplayNameFromRouteEndpoint()
         {
diff --git a/src/Mvc/Mvc.ApiExplorer/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test.csproj b/src/Mvc/Mvc.ApiExplorer/test/Microsoft.AspNetCore.Mvc.ApiExplorer.Test.csproj
@@ -3,6 +3,7 @@
   <PropertyGroup>
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
     <LangVersion>Preview</LangVersion>
+    <Features>$(Features.Replace('nullablePublicOnly', '')</Features>
   </PropertyGroup>
 
   <ItemGroup>

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ internal class EndpointMetadataApiDescriptionProvider : IApiDescriptionProvider`
`28`	`28`	`private readonly IHostEnvironment _environment;`
`29`	`29`	`private readonly IServiceProviderIsService? _serviceProviderIsService;`
`30`	`30`	`private readonly TryParseMethodCache TryParseMethodCache = new();`
	`31`	`+ private readonly NullabilityInfoContext NullabilityContext = new();`
`31`	`32`
`32`	`33`	`// Executes before MVC's DefaultApiDescriptionProvider and GrpcHttpApiDescriptionProvider for no particular reason.`
`33`	`34`	`public int Order => -1100;`
`@@ -132,45 +133,50 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string`
`132`	`133`
`133`	`134`	`private ApiParameterDescription? CreateApiParameterDescription(ParameterInfo parameter, RoutePattern pattern)`
`134`	`135`	`{`
`135`		`- var (source, name) = GetBindingSourceAndName(parameter, pattern);`
	`136`	`+ var (source, name, allowEmpty) = GetBindingSourceAndName(parameter, pattern);`
`136`	`137`
`137`	`138`	`// Services are ignored because they are not request parameters.`
`138`	`139`	`if (source == BindingSource.Services)`
`139`	`140`	`{`
`140`	`141`	`return null;`
`141`	`142`	`}`
`142`	`143`
	`144`	`+ // Determine the "requiredness" based on nullability, default value or if allowEmpty is set`
	`145`	`+ var nullability = NullabilityContext.Create(parameter);`
	`146`	`+ var isOptional = parameter.HasDefaultValue \|\| nullability.ReadState == NullabilityState.Nullable \|\| allowEmpty;`
	`147`	`+`
`143`	`148`	`return new ApiParameterDescription`
`144`	`149`	`{`
`145`	`150`	`Name = name,`
`146`	`151`	`ModelMetadata = CreateModelMetadata(parameter.ParameterType),`
`147`	`152`	`Source = source,`
`148`	`153`	`DefaultValue = parameter.DefaultValue,`
`149`	`154`	`Type = parameter.ParameterType,`
	`155`	`+ IsRequired = !isOptional`
`150`	`156`	`};`
`151`	`157`	`}`
`152`	`158`
`153`	`159`	`// TODO: Share more of this logic with RequestDelegateFactory.CreateArgument(...) using RequestDelegateFactoryUtilities`
`154`	`160`	`// which is shared source.`
`155`		`- private (BindingSource, string) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern)`
	`161`	`+ private (BindingSource, string, bool) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern)`
`156`	`162`	`{`
`157`	`163`	`var attributes = parameter.GetCustomAttributes();`
`158`	`164`
`159`	`165`	`if (attributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute)`
`160`	`166`	`{`
`161`		`- return (BindingSource.Path, routeAttribute.Name ?? parameter.Name ?? string.Empty);`
	`167`	`+ return (BindingSource.Path, routeAttribute.Name ?? parameter.Name ?? string.Empty, false);`
`162`	`168`	`}`
`163`	`169`	`else if (attributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)`
`164`	`170`	`{`
`165`		`- return (BindingSource.Query, queryAttribute.Name ?? parameter.Name ?? string.Empty);`
	`171`	`+ return (BindingSource.Query, queryAttribute.Name ?? parameter.Name ?? string.Empty, false);`
`166`	`172`	`}`
`167`	`173`	`else if (attributes.OfType<IFromHeaderMetadata>().FirstOrDefault() is { } headerAttribute)`
`168`	`174`	`{`
`169`		`- return (BindingSource.Header, headerAttribute.Name ?? parameter.Name ?? string.Empty);`
	`175`	`+ return (BindingSource.Header, headerAttribute.Name ?? parameter.Name ?? string.Empty, false);`
`170`	`176`	`}`
`171`		`- else if (parameter.CustomAttributes.Any(a => typeof(IFromBodyMetadata).IsAssignableFrom(a.AttributeType)))`
	`177`	`+ else if (attributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } fromBodyAttribute)`
`172`	`178`	`{`
`173`		`- return (BindingSource.Body, parameter.Name ?? string.Empty);`
	`179`	`+ return (BindingSource.Body, parameter.Name ?? string.Empty, fromBodyAttribute.AllowEmpty);`
`174`	`180`	`}`
`175`	`181`	`else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)) \|\|`
`176`	`182`	`parameter.ParameterType == typeof(HttpContext) \|\|`
`@@ -180,23 +186,23 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string`
`180`	`186`	`parameter.ParameterType == typeof(CancellationToken) \|\|`
`181`	`187`	`_serviceProviderIsService?.IsService(parameter.ParameterType) == true)`
`182`	`188`	`{`
`183`		`- return (BindingSource.Services, parameter.Name ?? string.Empty);`
	`189`	`+ return (BindingSource.Services, parameter.Name ?? string.Empty, false);`
`184`	`190`	`}`
`185`	`191`	`else if (parameter.ParameterType == typeof(string) \|\| TryParseMethodCache.HasTryParseMethod(parameter))`
`186`	`192`	`{`
`187`	`193`	`// Path vs query cannot be determined by RequestDelegateFactory at startup currently because of the layering, but can be done here.`
`188`	`194`	`if (parameter.Name is { } name && pattern.GetParameter(name) is not null)`
`189`	`195`	`{`
`190`		`- return (BindingSource.Path, name);`
	`196`	`+ return (BindingSource.Path, name, false);`
`191`	`197`	`}`
`192`	`198`	`else`
`193`	`199`	`{`
`194`		`- return (BindingSource.Query, parameter.Name ?? string.Empty);`
	`200`	`+ return (BindingSource.Query, parameter.Name ?? string.Empty, false);`
`195`	`201`	`}`
`196`	`202`	`}`
`197`	`203`	`else`
`198`	`204`	`{`
`199`		`- return (BindingSource.Body, parameter.Name ?? string.Empty);`
	`205`	`+ return (BindingSource.Body, parameter.Name ?? string.Empty, false);`
`200`	`206`	`}`
`201`	`207`	`}`
`202`	`208`