From 993225f2068a03e72233a0210acfeb77a524648f Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Thu, 19 Aug 2021 19:27:59 +0000 Subject: [PATCH 1/4] Treat parameters in oblivious nullability context as optional --- ...icrosoft.AspNetCore.Http.Extensions.csproj | 1 + .../src/RequestDelegateFactory.cs | 28 +++++++++++++----- .../test/RequestDelegateFactoryTests.cs | 29 +++++++++++++++++++ src/Shared/RoslynUtils/TypeHelper.cs | 28 ++++++++++++++++++ 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj b/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj index 7e4e66fe03aa..47a116e93031 100644 --- a/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj +++ b/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj @@ -16,6 +16,7 @@ + diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 94d04695dc99..16a3db54a423 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -5,6 +5,7 @@ using System.Linq; using System.Linq.Expressions; using System.Reflection; +using System.Runtime.CompilerServices; using System.Security.Claims; using System.Text; using Microsoft.AspNetCore.Http.Features; @@ -627,8 +628,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) @@ -637,8 +637,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"); @@ -671,7 +670,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; } @@ -862,8 +862,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; @@ -903,6 +902,21 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al return Expression.Convert(BodyValueExpr, parameter.ParameterType); } + private static bool IsOptionalParameter(ParameterInfo parameter) + { + // NullabilityInfo will treat all value types, regardless of + // nullability context as nullable. So the following code segment: + // #nullable disable + // app.MapGet("/{id}", (int id) => ...) + // will treat id as a non-nullable parameter even though + // the context is oblivious. To work around this, we check + // to see if the member is in a nullability context first. + var nullability = NullabilityContext.Create(parameter); + return parameter.HasDefaultValue + || !TypeHelper.IsInNullableContext(parameter.Member) + || nullability.ReadState != NullabilityState.NotNull; + } + private static MethodInfo GetMethodInfo(Expression 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 25b5ffe6750e..10c88054d5e5 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -2137,6 +2137,35 @@ public async Task CanSetParseableStringParamAsOptionalWithNullabilityDisability( Assert.Equal(expectedResponse, decodedResponseBody); } + [Theory] + [InlineData(true, "Age: 42")] + [InlineData(false, "Age: 0")] + public async Task TreatsUnknownNullabilityAsOptional(bool provideValue, string expectedResponse) + { + string optionalQueryParam(int age) => $"Age: {age}"; + + var httpContext = new DefaultHttpContext(); + var responseBodyStream = new MemoryStream(); + httpContext.Response.Body = responseBodyStream; + + if (provideValue) + { + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["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/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index b7e1f068d62e..a5f7f8891bfc 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -1,12 +1,16 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Linq; using System.Reflection; namespace System.Runtime.CompilerServices { internal static class TypeHelper { + private const string NullableContextAttributeFullName = "System.Runtime.CompilerServices.NullableContextAttribute"; + private const string NullableContextFlagsFieldName = "Flag"; + /// /// Checks to see if a given type is compiler generated. /// @@ -36,5 +40,29 @@ internal static bool IsCompilerGeneratedMethod(MethodInfo method) { return Attribute.IsDefined(method, typeof(CompilerGeneratedAttribute)) || IsCompilerGeneratedType(method.DeclaringType); } + + /// + /// Checks to see if a given member exists within an enabled nullability context. + /// + /// The member to evaluate. + /// if is within an enabled nullability context. + internal static bool IsInNullableContext(MemberInfo memberType) + { + for (var type = memberType; type != null; type = type.DeclaringType) + { + var nullableContextAttribute = type.GetCustomAttributes() + .FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableContextAttributeFullName, StringComparison.Ordinal)); + if (nullableContextAttribute != null) + { + if (nullableContextAttribute.GetType().GetField(NullableContextFlagsFieldName) is FieldInfo field && + field.GetValue(nullableContextAttribute) is byte @byte) + { + return @byte == 1; + } + } + } + + return false; + } } } \ No newline at end of file From ecfd6f503e44874a189ccaabbd5af7906b44fd0a Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Fri, 20 Aug 2021 01:19:42 +0000 Subject: [PATCH 2/4] Only apply fix for reference types --- ...icrosoft.AspNetCore.Http.Extensions.csproj | 1 - .../src/RequestDelegateFactory.cs | 15 ++++------ .../test/RequestDelegateFactoryTests.cs | 6 ++-- src/Shared/RoslynUtils/TypeHelper.cs | 28 ------------------- 4 files changed, 9 insertions(+), 41 deletions(-) diff --git a/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj b/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj index 47a116e93031..7e4e66fe03aa 100644 --- a/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj +++ b/src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj @@ -16,7 +16,6 @@ - diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 16a3db54a423..e0e1a7589a15 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -5,7 +5,6 @@ using System.Linq; using System.Linq.Expressions; using System.Reflection; -using System.Runtime.CompilerServices; using System.Security.Claims; using System.Text; using Microsoft.AspNetCore.Http.Features; @@ -904,16 +903,14 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al private static bool IsOptionalParameter(ParameterInfo parameter) { - // NullabilityInfo will treat all value types, regardless of - // nullability context as nullable. So the following code segment: - // #nullable disable - // app.MapGet("/{id}", (int id) => ...) - // will treat id as a non-nullable parameter even though - // the context is oblivious. To work around this, we check - // to see if the member is in a nullability context first. + // - 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 - || !TypeHelper.IsInNullableContext(parameter.Member) || nullability.ReadState != NullabilityState.NotNull; } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 10c88054d5e5..a6236e37c802 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -2139,10 +2139,10 @@ public async Task CanSetParseableStringParamAsOptionalWithNullabilityDisability( [Theory] [InlineData(true, "Age: 42")] - [InlineData(false, "Age: 0")] - public async Task TreatsUnknownNullabilityAsOptional(bool provideValue, string expectedResponse) + [InlineData(false, "Age: ")] + public async Task TreatsUnknownNullabilityAsOptionalForReferenceType(bool provideValue, string expectedResponse) { - string optionalQueryParam(int age) => $"Age: {age}"; + string optionalQueryParam(string age) => $"Age: {age}"; var httpContext = new DefaultHttpContext(); var responseBodyStream = new MemoryStream(); diff --git a/src/Shared/RoslynUtils/TypeHelper.cs b/src/Shared/RoslynUtils/TypeHelper.cs index a5f7f8891bfc..b7e1f068d62e 100644 --- a/src/Shared/RoslynUtils/TypeHelper.cs +++ b/src/Shared/RoslynUtils/TypeHelper.cs @@ -1,16 +1,12 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Linq; using System.Reflection; namespace System.Runtime.CompilerServices { internal static class TypeHelper { - private const string NullableContextAttributeFullName = "System.Runtime.CompilerServices.NullableContextAttribute"; - private const string NullableContextFlagsFieldName = "Flag"; - /// /// Checks to see if a given type is compiler generated. /// @@ -40,29 +36,5 @@ internal static bool IsCompilerGeneratedMethod(MethodInfo method) { return Attribute.IsDefined(method, typeof(CompilerGeneratedAttribute)) || IsCompilerGeneratedType(method.DeclaringType); } - - /// - /// Checks to see if a given member exists within an enabled nullability context. - /// - /// The member to evaluate. - /// if is within an enabled nullability context. - internal static bool IsInNullableContext(MemberInfo memberType) - { - for (var type = memberType; type != null; type = type.DeclaringType) - { - var nullableContextAttribute = type.GetCustomAttributes() - .FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableContextAttributeFullName, StringComparison.Ordinal)); - if (nullableContextAttribute != null) - { - if (nullableContextAttribute.GetType().GetField(NullableContextFlagsFieldName) is FieldInfo field && - field.GetValue(nullableContextAttribute) is byte @byte) - { - return @byte == 1; - } - } - } - - return false; - } } } \ No newline at end of file From 0e8360e58f8885ef1e0d2c91cf7f8654bbab962b Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Fri, 20 Aug 2021 03:28:23 +0000 Subject: [PATCH 3/4] Update optionality check in API descriptor --- .../EndpointMetadataApiDescriptionProvider.cs | 2 +- ...pointMetadataApiDescriptionProviderTest.cs | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 815d1b269259..f64d5b0a3b05 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -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 { diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 063199b39513..e987edf476b3 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -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() { From 039733cb2d72a80d1682bf7c8a41735e5522fc82 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Fri, 20 Aug 2021 15:42:36 +0000 Subject: [PATCH 4/4] Update check in BindAsync and Mvc.ApiExplorer test --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- .../test/EndpointMetadataApiDescriptionProviderTest.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index e0e1a7589a15..a735b9abee20 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -816,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)!; diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index e987edf476b3..707a80f8c8e8 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -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]