Support optionality via nullability and default values

captainsafia · captainsafia · commit c746a3a9bdb4 · 2021-07-19T17:38:50.000-07:00
diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs
@@ -22,6 +22,8 @@ namespace Microsoft.AspNetCore.Http
     /// </summary>
     public static partial class RequestDelegateFactory
     {
+        private static readonly NullabilityInfoContext nullabilityContext = new NullabilityInfoContext();
+
         private static readonly MethodInfo ExecuteTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTask), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo ExecuteTaskOfStringMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo ExecuteValueTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskOfT), BindingFlags.NonPublic | BindingFlags.Static)!;
@@ -31,12 +33,16 @@ public static partial class RequestDelegateFactory
         private static readonly MethodInfo ExecuteValueResultTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo ExecuteObjectReturnMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteObjectReturn), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo GetRequiredServiceMethod = typeof(ServiceProviderServiceExtensions).GetMethod(nameof(ServiceProviderServiceExtensions.GetRequiredService), BindingFlags.Public | BindingFlags.Static, new Type[] { typeof(IServiceProvider) })!;
+        private static readonly MethodInfo GetServiceMethod = typeof(ServiceProviderServiceExtensions).GetMethod(nameof(ServiceProviderServiceExtensions.GetService), BindingFlags.Public | BindingFlags.Static, new Type[] { typeof(IServiceProvider) })!;
         private static readonly MethodInfo ResultWriteResponseAsyncMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteResultWriteResponse), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo StringResultWriteResponseAsyncMethod = GetMethodInfo<Func<HttpResponse, string, Task>>((response, text) => HttpResponseWritingExtensions.WriteAsync(response, text, default));
         private static readonly MethodInfo JsonResultWriteResponseAsyncMethod = GetMethodInfo<Func<HttpResponse, object, Task>>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default));
         private static readonly MethodInfo LogParameterBindingFailureMethod = GetMethodInfo<Action<HttpContext, string, string, string>>((httpContext, parameterType, parameterName, sourceValue) =>
             Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue));
 
+        private static readonly MethodInfo LogRequiredParameterNotProvidedMethod = GetMethodInfo<Action<HttpContext, string, string>>((httpContext, parameterType, parameterName) =>
+            Log.RequiredParameterNotProvided(httpContext, parameterType, parameterName));
+
         private static readonly ParameterExpression TargetExpr = Expression.Parameter(typeof(object), "target");
         private static readonly ParameterExpression HttpContextExpr = Expression.Parameter(typeof(HttpContext), "httpContext");
         private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue");
@@ -217,11 +223,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
             }
             else if (parameterCustomAttributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } bodyAttribute)
             {
-                return BindParameterFromBody(parameter.ParameterType, bodyAttribute.AllowEmpty, factoryContext);
+                return BindParameterFromBody(parameter, bodyAttribute.AllowEmpty, factoryContext);
             }
             else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)))
             {
-                return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
+                return BindParameterFromService(parameter);
             }
             else if (parameter.ParameterType == typeof(HttpContext))
             {
@@ -256,16 +262,30 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
             }
             else
             {
+
+                var nullability = nullabilityContext.Create(parameter);
+                var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
                 if (factoryContext.ServiceProviderIsService is IServiceProviderIsService serviceProviderIsService)
                 {
-                    // If the parameter resolves as a service then get it from services
-                    if (serviceProviderIsService.IsService(parameter.ParameterType))
+                    // If the parameter is required
+                    if (!isOptional)
                     {
-                        return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
+                        // And we are able to resolve a service for it
+                        return serviceProviderIsService.IsService(parameter.ParameterType)
+                            ? Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr) // Then get it from the DI
+                            : BindParameterFromBody(parameter, allowEmpty: false, factoryContext); // Otherwise try to find it in the body
+                    }
+                    // If the parameter is optional
+                    else
+                    {
+                        // Then try to resolve it as an optional service and fallback to a body otherwise
+                        return Expression.Coalesce(
+                            Expression.Call(GetServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr),
+                            BindParameterFromBody(parameter, allowEmpty: false, factoryContext));
                     }
                 }
 
-                return BindParameterFromBody(parameter.ParameterType, allowEmpty: false, factoryContext);
+                return BindParameterFromBody(parameter, allowEmpty: false, factoryContext);
             }
         }
 
@@ -479,13 +499,9 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
 
             return async (target, httpContext) =>
             {
-                object? bodyValue;
+                object? bodyValue = defaultBodyValue;
 
-                if (factoryContext.AllowEmptyRequestBody && httpContext.Request.ContentLength == 0)
-                {
-                    bodyValue = defaultBodyValue;
-                }
-                else
+                if (httpContext.Request.ContentLength != 0 && httpContext.Request.HasJsonContentType())
                 {
                     try
                     {
@@ -516,21 +532,53 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri
             return Expression.Convert(indexExpression, typeof(string));
         }
 
+        private static Expression BindParameterFromService(ParameterInfo parameter)
+        {
+            var nullability = nullabilityContext.Create(parameter);
+            var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+
+            return isOptional
+                ? Expression.Call(GetServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr)
+                : Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
+        }
+
         private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext)
         {
+            var nullability = nullabilityContext.Create(parameter);
+            var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+
             if (parameter.ParameterType == typeof(string))
             {
-                if (!parameter.HasDefaultValue)
+                factoryContext.UsingTempSourceString = true;
+
+                if (!isOptional)
                 {
-                    return valueExpression;
+                    var checkRequiredStringParameterBlock = Expression.Block(
+                        Expression.Assign(TempSourceStringExpr, valueExpression),
+                        Expression.IfThen(Expression.Not(TempSourceStringNotNullExpr),
+                            Expression.Block(
+                                Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)),
+                                Expression.Call(LogRequiredParameterNotProvidedMethod, 
+                                    HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
+                            )
+                        )
+                    );
+
+                    factoryContext.TryParseParams.Add((TempSourceStringExpr, checkRequiredStringParameterBlock));
+                    return Expression.Block(TempSourceStringExpr);
+                }
+
+                // Allow nullable parameters that don't have a default value
+                if (nullability.ReadState == NullabilityState.Nullable && !parameter.HasDefaultValue)
+                {
+                    return Expression.Block(Expression.Assign(TempSourceStringExpr, valueExpression));
                 }
 
-                factoryContext.UsingTempSourceString = true;
                 return Expression.Block(
                     Expression.Assign(TempSourceStringExpr, valueExpression),
                     Expression.Condition(TempSourceStringNotNullExpr,
                         TempSourceStringExpr,
-                        Expression.Constant(parameter.DefaultValue)));
+                        Expression.Convert(Expression.Constant(parameter.DefaultValue), parameter.ParameterType)));
             }
 
             factoryContext.UsingTempSourceString = true;
@@ -598,6 +646,17 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
 
             var tryParseCall = tryParseMethodCall(parsedValue);
 
+            // If the parameter is required, fail to parse and log an error
+            var checkRequiredParaseableParameterBlock = Expression.Block(
+                Expression.IfThen(Expression.Not(TempSourceStringNotNullExpr),
+                    Expression.Block(
+                        Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)),
+                        Expression.Call(LogRequiredParameterNotProvidedMethod,
+                            HttpContextExpr, parameterTypeNameConstant, parameterNameConstant)
+                    )
+                )
+            );
+
             // If the parameter is nullable, we need to assign the "parsedValue" local to the nullable parameter on success.
             Expression tryParseExpression = isNotNullable ?
                 Expression.IfThen(Expression.Not(tryParseCall), failBlock) :
@@ -612,11 +671,18 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
                     tryParseExpression,
                     Expression.Assign(argument, Expression.Constant(parameter.DefaultValue)));
 
-            var fullTryParseBlock = Expression.Block(
-                // tempSourceString = httpContext.RequestValue["id"];
-                Expression.Assign(TempSourceStringExpr, valueExpression),
-                // if (tempSourceString != null) { ... }
-                ifNotNullTryParse);
+            var fullTryParseBlock = !isOptional
+                ? Expression.Block(
+                    // tempSourceString = httpContext.RequestValue["id"];
+                    Expression.Assign(TempSourceStringExpr, valueExpression),
+                    checkRequiredParaseableParameterBlock,
+                    // if (tempSourceString != null) { ... }
+                    ifNotNullTryParse) 
+                : Expression.Block(
+                    // tempSourceString = httpContext.RequestValue["id"];
+                    Expression.Assign(TempSourceStringExpr, valueExpression),
+                    // if (tempSourceString != null) { ... }
+                    ifNotNullTryParse);
 
             factoryContext.TryParseParams.Add((argument, fullTryParseBlock));
 
@@ -633,17 +699,46 @@ private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo
             return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext);
         }
 
-        private static Expression BindParameterFromBody(Type parameterType, bool allowEmpty, FactoryContext factoryContext)
+        private static Expression BindParameterFromBody(ParameterInfo parameter, bool allowEmpty, FactoryContext factoryContext)
         {
             if (factoryContext.JsonRequestBodyType is not null)
             {
                 throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");
             }
 
-            factoryContext.JsonRequestBodyType = parameterType;
-            factoryContext.AllowEmptyRequestBody = allowEmpty;
+            var nullability = nullabilityContext.Create(parameter);
+            var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable;
+
+            factoryContext.JsonRequestBodyType = parameter.ParameterType;
+            factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;
+
+            var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");
 
-            return Expression.Convert(BodyValueExpr, parameterType);
+            if (!isOptional && !allowEmpty)
+            {
+                var checkRequiredBodyBlock = Expression.Block(
+                    Expression.Assign(argument, Expression.Convert(BodyValueExpr, parameter.ParameterType)),
+                    Expression.IfThen(Expression.Equal(argument, Expression.Constant(null)),
+                        Expression.Block(
+                            Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)),
+                            Expression.Call(LogRequiredParameterNotProvidedMethod, 
+                                    HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
+                        )
+                    )
+                );
+                factoryContext.TryParseParams.Add((argument, checkRequiredBodyBlock));
+                return argument;
+            }
+
+            if (parameter.HasDefaultValue)
+            {
+                // Convert(bodyValue ?? SomeDefault, Todo)
+                return Expression.Convert(
+                    Expression.Coalesce(BodyValueExpr, Expression.Constant(parameter.DefaultValue)),
+                    parameter.ParameterType);
+            }
+
+            return Expression.Convert(BodyValueExpr, parameter.ParameterType); 
         }
 
         private static MethodInfo GetMethodInfo<T>(Expression<T> expr)
@@ -847,11 +942,19 @@ public static void RequestBodyInvalidDataException(HttpContext httpContext, Inva
             public static void ParameterBindingFailed(HttpContext httpContext, string parameterTypeName, string parameterName, string sourceValue)
                 => ParameterBindingFailed(GetLogger(httpContext), parameterTypeName, parameterName, sourceValue);
 
+            public static void RequiredParameterNotProvided(HttpContext httpContext, string parameterTypeName, string parameterName)
+                => RequiredParameterNotProvided(GetLogger(httpContext), parameterTypeName, parameterName);
+
             [LoggerMessage(3, LogLevel.Debug,
                 @"Failed to bind parameter ""{ParameterType} {ParameterName}"" from ""{SourceValue}"".",
                 EventName = "ParamaterBindingFailed")]
             private static partial void ParameterBindingFailed(ILogger logger, string parameterType, string parameterName, string sourceValue);
 
+            [LoggerMessage(4, LogLevel.Debug,
+                @"Required parameter ""{ParameterType} {ParameterName}"" was not provided.",
+                EventName = "RequiredParameterNotProvided")]
+            private static partial void RequiredParameterNotProvided(ILogger logger, string parameterType, string parameterName);
+
             private static ILogger GetLogger(HttpContext httpContext)
             {
                 var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
diff --git a/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj b/src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj
@@ -2,6 +2,8 @@
 
   <PropertyGroup>
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
+    <!-- This is needed to support codepaths that use NullabilityInfoContext. -->
+    <Features>$(Features.Replace('nullablePublicOnly', '')</Features>
   </PropertyGroup>
 
   <ItemGroup>
diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
