From f3f154f24480ecb14d44c8794cd66614c07e87fc Mon Sep 17 00:00:00 2001
From: Safia Abdalla <safia@microsoft.com>
Date: Fri, 20 Aug 2021 13:41:48 -0700
Subject: [PATCH] Treat reference type parameters in oblivious nullability
context as optional (#35563)
* Treat parameters in oblivious nullability context as optional
* Only apply fix for reference types
* Update optionality check in API descriptor
* Update check in BindAsync and Mvc.ApiExplorer test
---
.../src/RequestDelegateFactory.cs | 27 ++++++++++++-----
.../test/RequestDelegateFactoryTests.cs | 29 +++++++++++++++++++
.../EndpointMetadataApiDescriptionProvider.cs | 2 +-
...pointMetadataApiDescriptionProviderTest.cs | 23 ++++++++++++++-
4 files changed, 71 insertions(+), 10 deletions(-)
diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs
index 94d04695dc9..a735b9abee2 100644
--- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs
+++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs
@@ -627,8 +627,7 @@ namespace Microsoft.AspNetCore.Http
private static Expression BindParameterFromService(ParameterInfo parameter)
{
- var nullability = NullabilityContext.Create(parameter);
- var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+ var isOptional = IsOptionalParameter(parameter);
return isOptional
? Expression.Call(GetServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr)
@@ -637,8 +636,7 @@ namespace Microsoft.AspNetCore.Http
private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext)
{
- var nullability = NullabilityContext.Create(parameter);
- var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+ var isOptional = IsOptionalParameter(parameter);
var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");
@@ -671,7 +669,8 @@ namespace Microsoft.AspNetCore.Http
}
// Allow nullable parameters that don't have a default value
- if (nullability.ReadState == NullabilityState.Nullable && !parameter.HasDefaultValue)
+ var nullability = NullabilityContext.Create(parameter);
+ if (nullability.ReadState != NullabilityState.NotNull && !parameter.HasDefaultValue)
{
return valueExpression;
}
@@ -817,7 +816,7 @@ namespace Microsoft.AspNetCore.Http
{
// We reference the boundValues array by parameter index here
var nullability = NullabilityContext.Create(parameter);
- var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+ var isOptional = IsOptionalParameter(parameter);
// Get the BindAsync method
var body = TryParseMethodCache.FindBindAsyncMethod(parameter.ParameterType)!;
@@ -862,8 +861,7 @@ namespace Microsoft.AspNetCore.Http
}
}
- var nullability = NullabilityContext.Create(parameter);
- var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+ var isOptional = IsOptionalParameter(parameter);
factoryContext.JsonRequestBodyType = parameter.ParameterType;
factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;
@@ -903,6 +901,19 @@ namespace Microsoft.AspNetCore.Http
return Expression.Convert(BodyValueExpr, parameter.ParameterType);
}
+ private static bool IsOptionalParameter(ParameterInfo parameter)
+ {
+ // - Parameters representing value or reference types with a default value
+ // under any nullability context are treated as optional.
+ // - Value type parameters without a default value in an oblivious
+ // nullability context are required.
+ // - Reference type parameters without a default value in an oblivious
+ // nullability context are optional.
+ var nullability = NullabilityContext.Create(parameter);
+ return parameter.HasDefaultValue
+ || nullability.ReadState != NullabilityState.NotNull;
+ }
+
private static MethodInfo GetMethodInfo<T>(Expression<T> expr)
{
var mc = (MethodCallExpression)expr.Body;
diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
index 00832e35454..5eb0dec95d3 100644
--- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
+++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
@@ -2137,6 +2137,35 @@ namespace Microsoft.AspNetCore.Routing.Internal
Assert.Equal(expectedResponse, decodedResponseBody);
}
+ [Theory]
+ [InlineData(true, "Age: 42")]
+ [InlineData(false, "Age: ")]
+ public async Task TreatsUnknownNullabilityAsOptionalForReferenceType(bool provideValue, string expectedResponse)
+ {
+ string optionalQueryParam(string age) => $"Age: {age}";
+
+ var httpContext = new DefaultHttpContext();
+ var responseBodyStream = new MemoryStream();
+ httpContext.Response.Body = responseBodyStream;
+
+ if (provideValue)
+ {
+ httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
+ {
+ ["age"] = "42"
+ });
+ }
+
+ var requestDelegate = RequestDelegateFactory.Create(optionalQueryParam);
+
+ await requestDelegate(httpContext);
+
+ Assert.Equal(200, httpContext.Response.StatusCode);
+ Assert.False(httpContext.RequestAborted.IsCancellationRequested);
+ var decodedResponseBody = Encoding.UTF8.GetString(responseBodyStream.ToArray());
+ Assert.Equal(expectedResponse, decodedResponseBody);
+ }
+
#nullable enable
private class Todo : ITodo
diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
index 815d1b26925..f64d5b0a3b0 100644
--- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
+++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
@@ -143,7 +143,7 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer
// 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;
+ var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull || allowEmpty;
return new ApiParameterDescription
{
diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs
index c6d59ce1502..439fc686e53 100644
--- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs
+++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs
@@ -348,7 +348,7 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer
Assert.Equal(typeof(InferredJsonClass), fromBodyParam.Type);
Assert.Equal(typeof(InferredJsonClass), fromBodyParam.ModelMetadata.ModelType);
Assert.Equal(BindingSource.Body, fromBodyParam.Source);
- Assert.True(fromBodyParam.IsRequired);
+ Assert.False(fromBodyParam.IsRequired); // Reference type in oblivious nullability context
}
[Fact]
@@ -421,6 +421,27 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer
Assert.True(apiExplorerSettings.IgnoreApi);
}
+ [Fact]
+ public void TestParameterIsRequiredForObliviousNullabilityContext()
+ {
+ // In an oblivious nullability context, reference type parameters without
+ // annotations are optional. Value type parameters are always required.
+ var apiDescription = GetApiDescription((string foo, int bar) => { });
+ Assert.Equal(2, apiDescription.ParameterDescriptions.Count);
+
+ var fooParam = apiDescription.ParameterDescriptions[0];
+ Assert.Equal(typeof(string), fooParam.Type);
+ Assert.Equal(typeof(string), fooParam.ModelMetadata.ModelType);
+ Assert.Equal(BindingSource.Query, fooParam.Source);
+ Assert.False(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);
+ }
+
[Fact]
public void RespectsProducesProblemExtensionMethod()
{
--
GitLab