Skip to content

[release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional #35526

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,7 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri

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)
Expand All @@ -637,8 +636,7 @@ private static Expression BindParameterFromService(ParameterInfo parameter)

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");

Expand Down Expand Up @@ -671,7 +669,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
}

// 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;
}
Expand Down Expand Up @@ -817,7 +816,7 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa
{
// 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)!;
Expand Down Expand Up @@ -862,8 +861,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
}
}

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;
Expand Down Expand Up @@ -903,6 +901,19 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
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;
Expand Down
29 changes: 29 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2137,6 +2137,35 @@ public async Task CanSetParseableStringParamAsOptionalWithNullabilityDisability(
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string

// 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
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public void AddsMultipleParameters()
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]
Expand Down Expand Up @@ -413,6 +413,27 @@ public void AddsMetadataFromRouteEndpoint()
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()
{
Expand Down