From cbb7b5c60a45a6c3508b0fe55635062d63db5dc9 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 00:36:49 -0800 Subject: [PATCH 01/17] Added support for arrays of types with TryParse - Added inferred query support for methods where the body inference is disabled. - Added a test --- .../src/RequestDelegateFactory.cs | 178 ++++++++++++++---- .../test/ParameterBindingMethodCacheTests.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 20 ++ src/Shared/ParameterBindingMethodCache.cs | 4 +- 4 files changed, 162 insertions(+), 42 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 62f87635face..15a93ea26e4c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -171,7 +171,7 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions var arguments = CreateArguments(methodInfo.GetParameters(), factoryContext); - var responseWritingMethodCall = factoryContext.ParamCheckExpressions.Count > 0 ? + var responseWritingMethodCall = factoryContext.Expressions.Count > 0 ? CreateParamCheckingResponseWritingMethodCall(methodInfo, targetExpression, arguments, factoryContext) : CreateResponseWritingMethodCall(methodInfo, targetExpression, arguments); @@ -202,6 +202,13 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); throw new InvalidOperationException(errorMessage); } + + if (factoryContext.HasInferredBody && factoryContext.HasInferredQueryArray) + { + var errorMessage = BuildErrorMessageForQueryAndJsonBodyParameters(factoryContext); + throw new InvalidOperationException(errorMessage); + } + if (factoryContext.JsonRequestBodyParameter is not null && factoryContext.FirstFormRequestBodyParameter is not null) { @@ -317,7 +324,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { return BindParameterFromBindAsync(parameter, factoryContext); } - else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter)) + else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType)) { // 1. We bind from route values only, if route parameters are non-null and the parameter name is in that set. // 2. We bind from query only, if route parameters are non-null and the parameter name is NOT in that set. @@ -342,6 +349,17 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.RouteOrQueryStringParameter); return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } + else if (parameter.ParameterType.IsArray && factoryContext.DisableInferredFromBody && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) + { + // We only infer parameter types if you have an array of TryParsables and: + // - DisableInferredFromBody is true + // - TBD: You have explicitly declared a [FromBody] so it's no longer ambiguous + + factoryContext.HasInferredQueryArray = true; + + factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.QueryStringParameter); + return BindParameterFromProperty(parameter, QueryExpr, parameter.Name, factoryContext, "query string"); + } else { if (factoryContext.ServiceProviderIsService is IServiceProviderIsService serviceProviderIsService) @@ -408,20 +426,20 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( // }; // } - var localVariables = new ParameterExpression[factoryContext.ExtraLocals.Count + 1]; - var checkParamAndCallMethod = new Expression[factoryContext.ParamCheckExpressions.Count + 1]; + var localVariables = new ParameterExpression[factoryContext.Locals.Count + 1]; + var blockExpressions = new Expression[factoryContext.Expressions.Count + 1]; - for (var i = 0; i < factoryContext.ExtraLocals.Count; i++) + for (var i = 0; i < factoryContext.Locals.Count; i++) { - localVariables[i] = factoryContext.ExtraLocals[i]; + localVariables[i] = factoryContext.Locals[i]; } - for (var i = 0; i < factoryContext.ParamCheckExpressions.Count; i++) + for (var i = 0; i < factoryContext.Expressions.Count; i++) { - checkParamAndCallMethod[i] = factoryContext.ParamCheckExpressions[i]; + blockExpressions[i] = factoryContext.Expressions[i]; } - localVariables[factoryContext.ExtraLocals.Count] = WasParamCheckFailureExpr; + localVariables[factoryContext.Locals.Count] = WasParamCheckFailureExpr; var set400StatusAndReturnCompletedTask = Expression.Block( Expression.Assign(StatusCodeExpr, Expression.Constant(400)), @@ -433,9 +451,9 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( set400StatusAndReturnCompletedTask, AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType)); - checkParamAndCallMethod[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure; + blockExpressions[factoryContext.Expressions.Count] = checkWasParamCheckFailure; - return Expression.Block(localVariables, checkParamAndCallMethod); + return Expression.Block(localVariables, blockExpressions); } private static Expression AddResponseWritingToMethodCall(Expression methodCall, Type returnType) @@ -891,15 +909,17 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres factoryContext.UsingTempSourceString = true; - var underlyingNullableType = Nullable.GetUnderlyingType(parameter.ParameterType); + var targetParseType = parameter.ParameterType.IsArray ? parameter.ParameterType.GetElementType()! : parameter.ParameterType; + + var underlyingNullableType = Nullable.GetUnderlyingType(targetParseType); var isNotNullable = underlyingNullableType is null; - var nonNullableParameterType = underlyingNullableType ?? parameter.ParameterType; + var nonNullableParameterType = underlyingNullableType ?? targetParseType; var tryParseMethodCall = ParameterBindingMethodCache.FindTryParseMethod(nonNullableParameterType); if (tryParseMethodCall is null) { - var typeName = TypeNameHelper.GetTypeDisplayName(parameter.ParameterType, fullName: false); + var typeName = TypeNameHelper.GetTypeDisplayName(targetParseType, fullName: false); throw new InvalidOperationException($"No public static bool {typeName}.TryParse(string, out {typeName}) method found for {parameter.Name}."); } @@ -940,8 +960,30 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // param2_local = 42; // } + // string[]? values = httpContext.Request.Query["param1"].ToArray(); + // int[] param_local = values.Length > 0 ? new int[values.Length] : Array.Empty(); + + // if (values != null) + // { + // int index = 0; + // while (index < values.Length) + // { + // tempSourceString = values[i]; + // if (int.TryParse(tempSourceString, out var parsedValue)) + // { + // param_local[i] = parsedValue; + // } + // else + // { + // wasParamCheckFailure = true; + // Log.ParameterBindingFailed(httpContext, "Int32[]", "param1", tempSourceString); + // break; + // } + // } + // } + // If the parameter is nullable, create a "parsedValue" local to TryParse into since we cannot use the parameter directly. - var parsedValue = isNotNullable ? argument : Expression.Variable(nonNullableParameterType, "parsedValue"); + var parsedValue = Expression.Variable(nonNullableParameterType, "parsedValue"); var failBlock = Expression.Block( Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)), @@ -970,36 +1012,79 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres ) ); + var index = Expression.Variable(typeof(int), "index"); + // 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) : - Expression.Block(new[] { parsedValue }, + var tryParseExpression = Expression.Block(new[] { parsedValue }, Expression.IfThenElse(tryParseCall, - Expression.Assign(argument, Expression.Convert(parsedValue, parameter.ParameterType)), + Expression.Assign(parameter.ParameterType.IsArray ? Expression.ArrayAccess(argument, index) : argument, Expression.Convert(parsedValue, targetParseType)), failBlock)); - var ifNotNullTryParse = !parameter.HasDefaultValue ? - Expression.IfThen(TempSourceStringNotNullExpr, tryParseExpression) : - Expression.IfThenElse(TempSourceStringNotNullExpr, - tryParseExpression, - Expression.Assign(argument, Expression.Constant(parameter.DefaultValue))); + var ifNotNullTryParse = !parameter.HasDefaultValue + ? Expression.IfThen(TempSourceStringNotNullExpr, tryParseExpression) + : Expression.IfThenElse(TempSourceStringNotNullExpr, tryParseExpression, + Expression.Assign(argument, + Expression.Constant(parameter.DefaultValue))); + + var loopExit = Expression.Label(); + // We can reuse this like we reuse temp source string + ParameterExpression? stringArrayExpr = parameter.ParameterType.IsArray ? Expression.Variable(typeof(string[]), "tempStringArray") : null; - var fullParamCheckBlock = !isOptional - ? Expression.Block( + var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch + { + (true, _) => + + Expression.Block( + new[] { index, stringArrayExpr! }, + // values = httpContext.Request.Query["id"]; + Expression.Assign(stringArrayExpr!, valueExpression), + Expression.IfThen( + Expression.NotEqual(stringArrayExpr!, Expression.Constant(null)), + Expression.Block( + // param_local = new int[values.Length]; + Expression.Assign(argument, Expression.NewArrayBounds(parameter.ParameterType.GetElementType()!, Expression.ArrayLength(stringArrayExpr!))), + // index = 0 + Expression.Assign(index, Expression.Constant(0)), + // while (index < values.Length) + Expression.Loop( + Expression.Block( + Expression.IfThenElse( + Expression.LessThan(index, Expression.ArrayLength(stringArrayExpr!)), + // tempSourceString = values[index]; + Expression.Block( + Expression.Assign(TempSourceStringExpr, Expression.ArrayIndex(stringArrayExpr!, index)), + tryParseExpression + ), + // else break + Expression.Break(loopExit) + ), + Expression.PostIncrementAssign(index) + ) + , loopExit) + ) + ) + ), + (false, false) => + + Expression.Block( // tempSourceString = httpContext.RequestValue["id"]; Expression.Assign(TempSourceStringExpr, valueExpression), // if (tempSourceString == null) { ... } only produced when parameter is required checkRequiredParaseableParameterBlock, // if (tempSourceString != null) { ... } - ifNotNullTryParse) - : Expression.Block( + ifNotNullTryParse), + + (false, true) => + + Expression.Block( // tempSourceString = httpContext.RequestValue["id"]; Expression.Assign(TempSourceStringExpr, valueExpression), // if (tempSourceString != null) { ... } - ifNotNullTryParse); + ifNotNullTryParse) + }; - factoryContext.ExtraLocals.Add(argument); - factoryContext.ParamCheckExpressions.Add(fullParamCheckBlock); + factoryContext.Locals.Add(argument); + factoryContext.Expressions.Add(fullParamCheckBlock); return argument; } @@ -1041,8 +1126,8 @@ private static Expression BindParameterFromExpression( ) ); - factoryContext.ExtraLocals.Add(argument); - factoryContext.ParamCheckExpressions.Add(checkRequiredStringParameterBlock); + factoryContext.Locals.Add(argument); + factoryContext.Expressions.Add(checkRequiredStringParameterBlock); return argument; } @@ -1065,7 +1150,7 @@ private static Expression BindParameterFromExpression( } private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source) => - BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext, source); + BindParameterFromValue(parameter, GetValueFromProperty(property, key, parameter.ParameterType.IsArray ? typeof(string[]) : null), factoryContext, source); private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { @@ -1111,7 +1196,7 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa ) ); - factoryContext.ParamCheckExpressions.Add(checkRequiredBodyBlock); + factoryContext.Expressions.Add(checkRequiredBodyBlock); } // (ParameterType)boundValues[i] @@ -1197,7 +1282,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al // wasParamCheckFailure = true; // Log.ImplicitBodyNotProvided(httpContext, "todo", ThrowOnBadRequest); // } - factoryContext.ParamCheckExpressions.Add(Expression.Block( + factoryContext.Expressions.Add(Expression.Block( Expression.IfThen( Expression.Equal(BodyValueExpr, Expression.Constant(null)), Expression.Block( @@ -1235,7 +1320,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al ) ) ); - factoryContext.ParamCheckExpressions.Add(checkRequiredBodyBlock); + factoryContext.Expressions.Add(checkRequiredBodyBlock); } } else if (parameter.HasDefaultValue) @@ -1459,13 +1544,14 @@ private class FactoryContext public bool AllowEmptyRequestBody { get; set; } public bool UsingTempSourceString { get; set; } - public List ExtraLocals { get; } = new(); - public List ParamCheckExpressions { get; } = new(); + public List Locals { get; } = new(); + public List Expressions { get; } = new(); public List>> ParameterBinders { get; } = new(); public Dictionary TrackedParameters { get; } = new(); public bool HasMultipleBodyParameters { get; set; } public bool HasInferredBody { get; set; } + public bool HasInferredQueryArray { get; set; } public List Metadata { get; } = new(); @@ -1706,6 +1792,20 @@ private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryConte return errorMessage.ToString(); } + private static string BuildErrorMessageForQueryAndJsonBodyParameters(FactoryContext factoryContext) + { + var errorMessage = new StringBuilder(); + errorMessage.AppendLine("An action cannot use both inferred query and JSON body parameters. This is ambiguous"); + errorMessage.AppendLine("Below is the list of parameters that we found: "); + errorMessage.AppendLine(); + errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); + errorMessage.AppendLine("---------------------------------------------------------------------------------"); + + FormatTrackedParameters(factoryContext, errorMessage); + + return errorMessage.ToString(); + } + private static void FormatTrackedParameters(FactoryContext factoryContext, StringBuilder errorMessage) { foreach (var kv in factoryContext.TrackedParameters) diff --git a/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs b/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs index dad9ad33752f..0bbd48d1f9f3 100644 --- a/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs +++ b/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs @@ -144,7 +144,7 @@ public static IEnumerable TryParseStringParameterInfoData [MemberData(nameof(TryParseStringParameterInfoData))] public void HasTryParseStringMethod_ReturnsTrueWhenMethodExists(ParameterInfo parameterInfo) { - Assert.True(new ParameterBindingMethodCache().HasTryParseMethod(parameterInfo)); + Assert.True(new ParameterBindingMethodCache().HasTryParseMethod(parameterInfo.ParameterType)); } [Fact] diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index ef22b01c11a0..939803d0c5f4 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -718,6 +718,26 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } + [Fact] + public async Task RequestDelegateHandlesArraysFromQueryString() + { + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["a"] = new(new[] { "1", "2", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[] a) => + { + context.Items["tryParsable"] = a; + }); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(new[] { 1, 2, 3 }, (int[])httpContext.Items["tryParsable"]!); + } + [Fact] public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString() { diff --git a/src/Shared/ParameterBindingMethodCache.cs b/src/Shared/ParameterBindingMethodCache.cs index 228ec6fad549..810dd7923618 100644 --- a/src/Shared/ParameterBindingMethodCache.cs +++ b/src/Shared/ParameterBindingMethodCache.cs @@ -43,9 +43,9 @@ public ParameterBindingMethodCache(bool preferNonGenericEnumParseOverload) _enumTryParseMethod = GetEnumTryParseMethod(preferNonGenericEnumParseOverload); } - public bool HasTryParseMethod(ParameterInfo parameter) + public bool HasTryParseMethod(Type type) { - var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + var nonNullableParameterType = Nullable.GetUnderlyingType(type) ?? type; return FindTryParseMethod(nonNullableParameterType) is not null; } From b8df8ae35ef60aae6dccfc6ed25db2b56a0ced00 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 00:42:01 -0800 Subject: [PATCH 02/17] Need to disable inference in the test --- src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 939803d0c5f4..8cd3fe1b0cd7 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -730,7 +730,8 @@ public async Task RequestDelegateHandlesArraysFromQueryString() var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[] a) => { context.Items["tryParsable"] = a; - }); + }, new() { DisableInferBodyFromParameters = true }); + var requestDelegate = factoryResult.RequestDelegate; await requestDelegate(httpContext); From 46a1d696c9a90a8a35907a4e8b0d41e553dc65a4 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 07:45:27 -0800 Subject: [PATCH 03/17] Handle the api description for query string as parameters --- .../EndpointMetadataApiDescriptionProvider.cs | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 9b22fd7c6e6f..97b30966fd2d 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -68,6 +68,17 @@ public void OnProvidersExecuted(ApiDescriptionProviderContext context) private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string httpMethod, MethodInfo methodInfo) { + static bool ShouldDisableInferredBody(string method) + { + // GET, DELETE, HEAD, CONNECT, TRACE, and OPTIONS normally do not contain bodies + return method.Equals(HttpMethods.Get, StringComparison.Ordinal) || + method.Equals(HttpMethods.Delete, StringComparison.Ordinal) || + method.Equals(HttpMethods.Head, StringComparison.Ordinal) || + method.Equals(HttpMethods.Options, StringComparison.Ordinal) || + method.Equals(HttpMethods.Trace, StringComparison.Ordinal) || + method.Equals(HttpMethods.Connect, StringComparison.Ordinal); + } + // Swashbuckle uses the "controller" name to group endpoints together. // For now, put all methods defined the same declaring type together. string controllerName; @@ -99,10 +110,11 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string }; var hasBodyOrFormFileParameter = false; + var disableInferredBody = ShouldDisableInferredBody(httpMethod); foreach (var parameter in methodInfo.GetParameters()) { - var parameterDescription = CreateApiParameterDescription(parameter, routeEndpoint.RoutePattern); + var parameterDescription = CreateApiParameterDescription(parameter, routeEndpoint.RoutePattern, disableInferredBody); if (parameterDescription is null) { @@ -155,9 +167,9 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string return apiDescription; } - private ApiParameterDescription? CreateApiParameterDescription(ParameterInfo parameter, RoutePattern pattern) + private ApiParameterDescription? CreateApiParameterDescription(ParameterInfo parameter, RoutePattern pattern, bool disableInferredBody) { - var (source, name, allowEmpty, paramType) = GetBindingSourceAndName(parameter, pattern); + var (source, name, allowEmpty, paramType) = GetBindingSourceAndName(parameter, pattern, disableInferredBody); // Services are ignored because they are not request parameters. if (source == BindingSource.Services) @@ -230,7 +242,7 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param // TODO: Share more of this logic with RequestDelegateFactory.CreateArgument(...) using RequestDelegateFactoryUtilities // which is shared source. - private (BindingSource, string, bool, Type) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern) + private (BindingSource, string, bool, Type) GetBindingSourceAndName(ParameterInfo parameter, RoutePattern pattern, bool disableInferredBody) { var attributes = parameter.GetCustomAttributes(); @@ -265,7 +277,7 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.Services, parameter.Name ?? string.Empty, false, parameter.ParameterType); } - else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter)) + else if (parameter.ParameterType == typeof(string) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType)) { // complex types will display as strings since they use custom parsing via TryParse on a string var displayType = !parameter.ParameterType.IsPrimitive && Nullable.GetUnderlyingType(parameter.ParameterType)?.IsPrimitive != true @@ -284,6 +296,10 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.FormFile, parameter.Name ?? string.Empty, false, parameter.ParameterType); } + else if (disableInferredBody && parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) + { + return (BindingSource.Query, parameter.Name ?? string.Empty, false, parameter.ParameterType); + } else { return (BindingSource.Body, parameter.Name ?? string.Empty, false, parameter.ParameterType); From 7ca6bd6176053ef32bc623ef04825d2f77ee65c0 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 12:31:13 -0800 Subject: [PATCH 04/17] Preserve name of check expressions --- .../src/RequestDelegateFactory.cs | 37 +++++----- .../test/RequestDelegateFactoryTests.cs | 67 +++++++++++++++++-- 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 15a93ea26e4c..584a2afcfed4 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -171,7 +171,7 @@ private static FactoryContext CreateFactoryContext(RequestDelegateFactoryOptions var arguments = CreateArguments(methodInfo.GetParameters(), factoryContext); - var responseWritingMethodCall = factoryContext.Expressions.Count > 0 ? + var responseWritingMethodCall = factoryContext.ParamCheckExpressions.Count > 0 ? CreateParamCheckingResponseWritingMethodCall(methodInfo, targetExpression, arguments, factoryContext) : CreateResponseWritingMethodCall(methodInfo, targetExpression, arguments); @@ -426,20 +426,20 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( // }; // } - var localVariables = new ParameterExpression[factoryContext.Locals.Count + 1]; - var blockExpressions = new Expression[factoryContext.Expressions.Count + 1]; + var localVariables = new ParameterExpression[factoryContext.ExtraLocals.Count + 1]; + var blockExpressions = new Expression[factoryContext.ParamCheckExpressions.Count + 1]; - for (var i = 0; i < factoryContext.Locals.Count; i++) + for (var i = 0; i < factoryContext.ExtraLocals.Count; i++) { - localVariables[i] = factoryContext.Locals[i]; + localVariables[i] = factoryContext.ExtraLocals[i]; } - for (var i = 0; i < factoryContext.Expressions.Count; i++) + for (var i = 0; i < factoryContext.ParamCheckExpressions.Count; i++) { - blockExpressions[i] = factoryContext.Expressions[i]; + blockExpressions[i] = factoryContext.ParamCheckExpressions[i]; } - localVariables[factoryContext.Locals.Count] = WasParamCheckFailureExpr; + localVariables[factoryContext.ExtraLocals.Count] = WasParamCheckFailureExpr; var set400StatusAndReturnCompletedTask = Expression.Block( Expression.Assign(StatusCodeExpr, Expression.Constant(400)), @@ -451,7 +451,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( set400StatusAndReturnCompletedTask, AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType)); - blockExpressions[factoryContext.Expressions.Count] = checkWasParamCheckFailure; + blockExpressions[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure; return Expression.Block(localVariables, blockExpressions); } @@ -1058,6 +1058,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // else break Expression.Break(loopExit) ), + // index++ Expression.PostIncrementAssign(index) ) , loopExit) @@ -1083,8 +1084,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres ifNotNullTryParse) }; - factoryContext.Locals.Add(argument); - factoryContext.Expressions.Add(fullParamCheckBlock); + factoryContext.ExtraLocals.Add(argument); + factoryContext.ParamCheckExpressions.Add(fullParamCheckBlock); return argument; } @@ -1126,8 +1127,8 @@ private static Expression BindParameterFromExpression( ) ); - factoryContext.Locals.Add(argument); - factoryContext.Expressions.Add(checkRequiredStringParameterBlock); + factoryContext.ExtraLocals.Add(argument); + factoryContext.ParamCheckExpressions.Add(checkRequiredStringParameterBlock); return argument; } @@ -1196,7 +1197,7 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa ) ); - factoryContext.Expressions.Add(checkRequiredBodyBlock); + factoryContext.ParamCheckExpressions.Add(checkRequiredBodyBlock); } // (ParameterType)boundValues[i] @@ -1282,7 +1283,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al // wasParamCheckFailure = true; // Log.ImplicitBodyNotProvided(httpContext, "todo", ThrowOnBadRequest); // } - factoryContext.Expressions.Add(Expression.Block( + factoryContext.ParamCheckExpressions.Add(Expression.Block( Expression.IfThen( Expression.Equal(BodyValueExpr, Expression.Constant(null)), Expression.Block( @@ -1320,7 +1321,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al ) ) ); - factoryContext.Expressions.Add(checkRequiredBodyBlock); + factoryContext.ParamCheckExpressions.Add(checkRequiredBodyBlock); } } else if (parameter.HasDefaultValue) @@ -1544,8 +1545,8 @@ private class FactoryContext public bool AllowEmptyRequestBody { get; set; } public bool UsingTempSourceString { get; set; } - public List Locals { get; } = new(); - public List Expressions { get; } = new(); + public List ExtraLocals { get; } = new(); + public List ParamCheckExpressions { get; } = new(); public List>> ParameterBinders { get; } = new(); public Dictionary TrackedParameters { get; } = new(); diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8cd3fe1b0cd7..ac4f87be8835 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -718,8 +718,10 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } - [Fact] - public async Task RequestDelegateHandlesArraysFromQueryString() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RequestDelegateHandlesArraysFromQueryString(bool disableInferBodyFromParameters) { var httpContext = CreateHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary @@ -730,13 +732,70 @@ public async Task RequestDelegateHandlesArraysFromQueryString() var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[] a) => { context.Items["tryParsable"] = a; - }, new() { DisableInferBodyFromParameters = true }); + }, new() { DisableInferBodyFromParameters = disableInferBodyFromParameters }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + if (disableInferBodyFromParameters) + { + Assert.Equal(new[] { 1, 2, 3 }, (int[])httpContext.Items["tryParsable"]!); + } + else + { + Assert.Null(httpContext.Items["tryParsable"]); + } + } + + [Fact] + public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalArray() + { + var httpContext = CreateHttpContext(); + var wasCalled = false; + + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["a"] = new(new[] { "1", "2", "", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[]? a) => + { + wasCalled = true; + context.Items["tryParsable"] = a; + }, new() { DisableInferBodyFromParameters = true, }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Null(httpContext.Items["tryParsable"]); + Assert.True(wasCalled); + } + + [Fact] + public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalValues() + { + var httpContext = CreateHttpContext(); + var wasCalled = false; + + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["a"] = new(new[] { "1", "2", "", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, int?[] a) => + { + wasCalled = true; + context.Items["tryParsable"] = a; + }, new() { DisableInferBodyFromParameters = true, }); var requestDelegate = factoryResult.RequestDelegate; await requestDelegate(httpContext); - Assert.Equal(new[] { 1, 2, 3 }, (int[])httpContext.Items["tryParsable"]!); + Assert.Equal(new int?[] { 1, 2, null, 3 }, (int?[])httpContext.Items["tryParsable"]!); + Assert.True(wasCalled); } [Fact] From 7343752f42f91ffc5a6812b9343a66f8c51936ba Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 20:23:26 -0800 Subject: [PATCH 05/17] Make string[] work --- .../src/RequestDelegateFactory.cs | 18 +++++++++----- .../test/RequestDelegateFactoryTests.cs | 24 ++++++++++++++++++- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 584a2afcfed4..6c5dcd62974b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -349,11 +349,10 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.RouteOrQueryStringParameter); return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } - else if (parameter.ParameterType.IsArray && factoryContext.DisableInferredFromBody && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) + else if ((parameter.ParameterType.IsArray && factoryContext.DisableInferredFromBody) && + (parameter.ParameterType == typeof(string[]) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!))) { - // We only infer parameter types if you have an array of TryParsables and: - // - DisableInferredFromBody is true - // - TBD: You have explicitly declared a [FromBody] so it's no longer ambiguous + // We only infer parameter types if you have an array of TryParsables/string[]/StringValues, and DisableInferredFromBody is true factoryContext.HasInferredQueryArray = true; @@ -902,7 +901,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parameterNameConstant = Expression.Constant(parameter.Name); var sourceConstant = Expression.Constant(source); - if (parameter.ParameterType == typeof(string)) + if (parameter.ParameterType == typeof(string) || parameter.ParameterType == typeof(string[])) { return BindParameterFromExpression(parameter, valueExpression, factoryContext, source); } @@ -979,6 +978,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // Log.ParameterBindingFailed(httpContext, "Int32[]", "param1", tempSourceString); // break; // } + // + // index++ // } // } @@ -1151,7 +1152,12 @@ private static Expression BindParameterFromExpression( } private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source) => - BindParameterFromValue(parameter, GetValueFromProperty(property, key, parameter.ParameterType.IsArray ? typeof(string[]) : null), factoryContext, source); + BindParameterFromValue(parameter, GetValueFromProperty(property, key, GetExpressionType(parameter.ParameterType)), factoryContext, source); + + private static Type? GetExpressionType(Type type) => + type.IsArray ? typeof(string[]) : + type == typeof(StringValues) ? typeof(StringValues) : + null; private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index ac4f87be8835..346518f27a3f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -748,6 +748,28 @@ public async Task RequestDelegateHandlesArraysFromQueryString(bool disableInferB } } + [Fact] + public async Task RequestDelegateHandlesStringArraysFromQueryString() + { + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["a"] = new(new[] { "1", "2", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, string[] a) => + { + context.Items["tryParsable"] = a; + }, + new() { DisableInferBodyFromParameters = true }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(new[] { "1", "2", "3" }, (string[])httpContext.Items["tryParsable"]!); + } + [Fact] public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalArray() { @@ -781,7 +803,7 @@ public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOpt httpContext.Request.Query = new QueryCollection(new Dictionary { - ["a"] = new(new[] { "1", "2", "", "3" }) + ["a"] = new(new[] { "1", "2", null, "3" }) }); var factoryResult = RequestDelegateFactory.Create((HttpContext context, int?[] a) => From e75b85a56937a477ba23dd94bd079963b51bedac Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 20:33:19 -0800 Subject: [PATCH 06/17] Make StringValues work --- .../src/RequestDelegateFactory.cs | 8 ++++--- .../test/RequestDelegateFactoryTests.cs | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6c5dcd62974b..f5ca2cc0828c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -349,8 +349,10 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.RouteOrQueryStringParameter); return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } - else if ((parameter.ParameterType.IsArray && factoryContext.DisableInferredFromBody) && - (parameter.ParameterType == typeof(string[]) || ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!))) + else if (factoryContext.DisableInferredFromBody && + (parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) || + parameter.ParameterType == typeof(string[]) || + parameter.ParameterType == typeof(StringValues)) { // We only infer parameter types if you have an array of TryParsables/string[]/StringValues, and DisableInferredFromBody is true @@ -901,7 +903,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parameterNameConstant = Expression.Constant(parameter.Name); var sourceConstant = Expression.Constant(source); - if (parameter.ParameterType == typeof(string) || parameter.ParameterType == typeof(string[])) + if (parameter.ParameterType == typeof(string) || parameter.ParameterType == typeof(string[]) || parameter.ParameterType == typeof(StringValues)) { return BindParameterFromExpression(parameter, valueExpression, factoryContext, source); } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 346518f27a3f..b3e266a959aa 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -770,6 +770,28 @@ public async Task RequestDelegateHandlesStringArraysFromQueryString() Assert.Equal(new[] { "1", "2", "3" }, (string[])httpContext.Items["tryParsable"]!); } + [Fact] + public async Task RequestDelegateHandlesStringValuesFromQueryString() + { + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["a"] = new(new[] { "1", "2", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, StringValues a) => + { + context.Items["tryParsable"] = a.ToArray(); + }, + new() { DisableInferBodyFromParameters = true }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(new[] { "1", "2", "3" }, (string[])httpContext.Items["tryParsable"]!); + } + [Fact] public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalArray() { From 54fd84ead18fe95e2f133c834e11471e560aebcf Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 20:35:19 -0800 Subject: [PATCH 07/17] Update the metadata --- .../src/EndpointMetadataApiDescriptionProvider.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 97b30966fd2d..2603abef7f7a 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -16,6 +16,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Internal; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.ApiExplorer; @@ -296,7 +297,10 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.FormFile, parameter.Name ?? string.Empty, false, parameter.ParameterType); } - else if (disableInferredBody && parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) + else if (disableInferredBody && + (parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) || + parameter.ParameterType == typeof(string[]) || + parameter.ParameterType == typeof(StringValues)) { return (BindingSource.Query, parameter.Name ?? string.Empty, false, parameter.ParameterType); } From 6436a9b1d21bf6183dace8b91a6793637d7107f2 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 20:37:15 -0800 Subject: [PATCH 08/17] Restore naming --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index f5ca2cc0828c..e0f0a729b219 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -428,7 +428,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( // } var localVariables = new ParameterExpression[factoryContext.ExtraLocals.Count + 1]; - var blockExpressions = new Expression[factoryContext.ParamCheckExpressions.Count + 1]; + var checkParamAndCallMethod = new Expression[factoryContext.ParamCheckExpressions.Count + 1]; for (var i = 0; i < factoryContext.ExtraLocals.Count; i++) { @@ -437,7 +437,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( for (var i = 0; i < factoryContext.ParamCheckExpressions.Count; i++) { - blockExpressions[i] = factoryContext.ParamCheckExpressions[i]; + checkParamAndCallMethod[i] = factoryContext.ParamCheckExpressions[i]; } localVariables[factoryContext.ExtraLocals.Count] = WasParamCheckFailureExpr; @@ -452,9 +452,9 @@ private static Expression CreateParamCheckingResponseWritingMethodCall( set400StatusAndReturnCompletedTask, AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType)); - blockExpressions[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure; + checkParamAndCallMethod[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure; - return Expression.Block(localVariables, blockExpressions); + return Expression.Block(localVariables, checkParamAndCallMethod); } private static Expression AddResponseWritingToMethodCall(Expression methodCall, Type returnType) From 235e15b65781bd66613469577e9cb399dda17c8d Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 22:25:41 -0800 Subject: [PATCH 09/17] Added alot more tests! --- .../src/RequestDelegateFactory.cs | 12 +- .../test/RequestDelegateFactoryTests.cs | 172 +++++++++--------- 2 files changed, 90 insertions(+), 94 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index e0f0a729b219..d0dd35acbd07 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -197,15 +197,15 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory args[i] = CreateArgument(parameters[i], factoryContext); } - if (factoryContext.HasInferredBody && factoryContext.DisableInferredFromBody) + if (factoryContext.HasInferredBody && factoryContext.HasInferredQueryArray) { - var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); + var errorMessage = BuildErrorMessageForQueryAndJsonBodyParameters(factoryContext); throw new InvalidOperationException(errorMessage); } - if (factoryContext.HasInferredBody && factoryContext.HasInferredQueryArray) + if (factoryContext.HasInferredBody && factoryContext.DisableInferredFromBody) { - var errorMessage = BuildErrorMessageForQueryAndJsonBodyParameters(factoryContext); + var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); throw new InvalidOperationException(errorMessage); } @@ -349,10 +349,10 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.RouteOrQueryStringParameter); return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } - else if (factoryContext.DisableInferredFromBody && + else if (factoryContext.DisableInferredFromBody && ( (parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) || parameter.ParameterType == typeof(string[]) || - parameter.ParameterType == typeof(StringValues)) + parameter.ParameterType == typeof(StringValues))) { // We only infer parameter types if you have an array of TryParsables/string[]/StringValues, and DisableInferredFromBody is true diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index b3e266a959aa..6900d068dd05 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -451,6 +451,55 @@ void TestAction([FromRoute] int foo) Assert.Equal(400, httpContext.Response.StatusCode); } + public static object?[][] TryParsableArrayParameters + { + get + { + static void Store(HttpContext httpContext, T tryParsable) + { + httpContext.Items["tryParsable"] = tryParsable; + } + + var now = DateTime.Now; + + return new[] + { + // string is not technically "TryParsable", but it's the special case. + new object[] { (Action)Store, new[] { "plain string" }, new[] { "plain string" } }, + new object[] { (Action)Store, new[] { "1", "2", "3" }, new StringValues(new[] { "1", "2", "3" }) }, + new object[] { (Action)Store, new[] { "-1", "2", "3" }, new[] { -1,2,3 } }, + new object[] { (Action)Store, new[] { "1","42","32"}, new[] { 1U, 42U, 32U } }, + new object[] { (Action)Store, new[] { "true", "false" }, new[] { true, false } }, + new object[] { (Action)Store, new[] { "-42" }, new[] { (short)-42 } }, + new object[] { (Action)Store, new[] { "42" }, new[] { (ushort)42 } }, + new object[] { (Action)Store, new[] { "-42" }, new[] { -42L } }, + new object[] { (Action)Store, new[] { "42" }, new[] { 42UL } }, + new object[] { (Action)Store, new[] { "-42" },new[] { new IntPtr(-42) } }, + new object[] { (Action)Store, new[] { "A" }, new[] { 'A' } }, + new object[] { (Action)Store, new[] { "0.5" },new[] { 0.5 } }, + new object[] { (Action)Store, new[] { "0.5" },new[] { 0.5f } }, + new object[] { (Action)Store, new[] { "0.5" }, new[] { (Half)0.5f } }, + new object[] { (Action)Store, new[] { "0.5" },new[] { 0.5m } }, + new object[] { (Action)Store, new[] { now.ToString("o") },new[] { now.ToUniversalTime() } }, + new object[] { (Action)Store, new[] { "1970-01-01T00:00:00.0000000+00:00" },new[] { DateTimeOffset.UnixEpoch } }, + new object[] { (Action)Store, new[] { "00:00:42" },new[] { TimeSpan.FromSeconds(42) } }, + new object[] { (Action)Store, new[] { "00000000-0000-0000-0000-000000000000" },new[] { Guid.Empty } }, + new object[] { (Action)Store, new[] { "6.0.0.42" }, new[] { new Version("6.0.0.42") } }, + new object[] { (Action)Store, new[] { "-42" },new[]{ new BigInteger(-42) } }, + new object[] { (Action)Store, new[] { "127.0.0.1" }, new[] { IPAddress.Loopback } }, + new object[] { (Action)Store, new[] { "127.0.0.1:80" },new[] { new IPEndPoint(IPAddress.Loopback, 80) } }, + new object[] { (Action)Store, new[] { "Unix" },new[] { AddressFamily.Unix } }, + new object[] { (Action)Store, new[] { "Nop" }, new[] { ILOpCode.Nop } }, + new object[] { (Action)Store, new[] { "PublicKey,Retargetable" },new[] { AssemblyFlags.PublicKey | AssemblyFlags.Retargetable } }, + new object[] { (Action)Store, new[] { "42" }, new int?[] { 42 } }, + new object[] { (Action)Store, new[] { "ValueB" },new[] { MyEnum.ValueB } }, + new object[] { (Action)Store, new[] { "https://example.org" },new[] { new MyTryParseRecord(new Uri("https://example.org")) } }, + new object?[] { (Action)Store, null, null }, + new object?[] { (Action)Store, new string[] {}, Array.Empty() }, + }; + } + } + public static object?[][] TryParsableParameters { get @@ -702,12 +751,12 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromR [Theory] [MemberData(nameof(TryParsableParameters))] - public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQueryString(Delegate action, string? routeValue, object? expectedParameterValue) + public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQueryString(Delegate action, string? queryValue, object? expectedParameterValue) { var httpContext = CreateHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary { - ["tryParsable"] = routeValue + ["tryParsable"] = queryValue }); var factoryResult = RequestDelegateFactory.Create(action); @@ -719,59 +768,26 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ } [Theory] - [InlineData(true)] - [InlineData(false)] - public async Task RequestDelegateHandlesArraysFromQueryString(bool disableInferBodyFromParameters) + [MemberData(nameof(TryParsableArrayParameters))] + public async Task RequestDelegateHandlesArraysFromQueryString(Delegate action, string[]? queryValues, object? expectedParameterValue) { var httpContext = CreateHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary { - ["a"] = new(new[] { "1", "2", "3" }) + ["tryParsable"] = queryValues }); - var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[] a) => - { - context.Items["tryParsable"] = a; - }, new() { DisableInferBodyFromParameters = disableInferBodyFromParameters }); + var factoryResult = RequestDelegateFactory.Create(action, new() { DisableInferBodyFromParameters = true }); var requestDelegate = factoryResult.RequestDelegate; await requestDelegate(httpContext); - if (disableInferBodyFromParameters) - { - Assert.Equal(new[] { 1, 2, 3 }, (int[])httpContext.Items["tryParsable"]!); - } - else - { - Assert.Null(httpContext.Items["tryParsable"]); - } - } - - [Fact] - public async Task RequestDelegateHandlesStringArraysFromQueryString() - { - var httpContext = CreateHttpContext(); - httpContext.Request.Query = new QueryCollection(new Dictionary - { - ["a"] = new(new[] { "1", "2", "3" }) - }); - - var factoryResult = RequestDelegateFactory.Create((HttpContext context, string[] a) => - { - context.Items["tryParsable"] = a; - }, - new() { DisableInferBodyFromParameters = true }); - - var requestDelegate = factoryResult.RequestDelegate; - - await requestDelegate(httpContext); - - Assert.Equal(new[] { "1", "2", "3" }, (string[])httpContext.Items["tryParsable"]!); + Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } [Fact] - public async Task RequestDelegateHandlesStringValuesFromQueryString() + public async Task RequestDelegateHandlesArraysFromExplicitQueryStringSource() { var httpContext = CreateHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary @@ -779,68 +795,48 @@ public async Task RequestDelegateHandlesStringValuesFromQueryString() ["a"] = new(new[] { "1", "2", "3" }) }); - var factoryResult = RequestDelegateFactory.Create((HttpContext context, StringValues a) => - { - context.Items["tryParsable"] = a.ToArray(); - }, - new() { DisableInferBodyFromParameters = true }); - - var requestDelegate = factoryResult.RequestDelegate; - - await requestDelegate(httpContext); - - Assert.Equal(new[] { "1", "2", "3" }, (string[])httpContext.Items["tryParsable"]!); - } - - [Fact] - public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalArray() - { - var httpContext = CreateHttpContext(); - var wasCalled = false; + httpContext.Request.Headers["Custom"] = new(new[] { "4", "5", "6" }); - httpContext.Request.Query = new QueryCollection(new Dictionary + var factoryResult = RequestDelegateFactory.Create((HttpContext context, + [FromHeader(Name = "Custom")] int[] headerValues, + [FromQuery(Name = "a")] int[] queryValues) => { - ["a"] = new(new[] { "1", "2", "", "3" }) + context.Items["headers"] = headerValues; + context.Items["query"] = queryValues; }); - var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[]? a) => - { - wasCalled = true; - context.Items["tryParsable"] = a; - }, new() { DisableInferBodyFromParameters = true, }); - var requestDelegate = factoryResult.RequestDelegate; await requestDelegate(httpContext); - Assert.Null(httpContext.Items["tryParsable"]); - Assert.True(wasCalled); + Assert.Equal(new[] { 1, 2, 3 }, (int[])httpContext.Items["query"]!); + Assert.Equal(new[] { 4, 5, 6 }, (int[])httpContext.Items["headers"]!); } - [Fact] - public async Task RequestDelegateHandlesArraysFromQueryStringParesFailureWithOptionalValues() - { - var httpContext = CreateHttpContext(); - var wasCalled = false; + //[Fact] + //public async Task RequestDelegateHandlesArraysFromQueryStringParseFailureWithOptionalValues() + //{ + // var httpContext = CreateHttpContext(); + // var wasCalled = false; - httpContext.Request.Query = new QueryCollection(new Dictionary - { - ["a"] = new(new[] { "1", "2", null, "3" }) - }); + // httpContext.Request.Query = new QueryCollection(new Dictionary + // { + // ["a"] = new(new[] { "1", "2", "", "3" }) + // }); - var factoryResult = RequestDelegateFactory.Create((HttpContext context, int?[] a) => - { - wasCalled = true; - context.Items["tryParsable"] = a; - }, new() { DisableInferBodyFromParameters = true, }); + // var factoryResult = RequestDelegateFactory.Create((HttpContext context, int?[] a) => + // { + // wasCalled = true; + // context.Items["tryParsable"] = a; + // }, new() { DisableInferBodyFromParameters = true, }); - var requestDelegate = factoryResult.RequestDelegate; + // var requestDelegate = factoryResult.RequestDelegate; - await requestDelegate(httpContext); + // await requestDelegate(httpContext); - Assert.Equal(new int?[] { 1, 2, null, 3 }, (int?[])httpContext.Items["tryParsable"]!); - Assert.True(wasCalled); - } + // Assert.Equal(new int?[] { 1, 2, null, 3 }, (int?[])httpContext.Items["tryParsable"]!); + // Assert.True(wasCalled); + //} [Fact] public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString() From 52cf850884879aea4fef092b7855cbcec467337a Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 23:34:45 -0800 Subject: [PATCH 10/17] Added last test cases --- .../src/RequestDelegateFactory.cs | 23 ----- .../test/RequestDelegateFactoryTests.cs | 83 +++++++++++++------ 2 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index d0dd35acbd07..cbd09b0b867b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -197,12 +197,6 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory args[i] = CreateArgument(parameters[i], factoryContext); } - if (factoryContext.HasInferredBody && factoryContext.HasInferredQueryArray) - { - var errorMessage = BuildErrorMessageForQueryAndJsonBodyParameters(factoryContext); - throw new InvalidOperationException(errorMessage); - } - if (factoryContext.HasInferredBody && factoryContext.DisableInferredFromBody) { var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); @@ -356,8 +350,6 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { // We only infer parameter types if you have an array of TryParsables/string[]/StringValues, and DisableInferredFromBody is true - factoryContext.HasInferredQueryArray = true; - factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.QueryStringParameter); return BindParameterFromProperty(parameter, QueryExpr, parameter.Name, factoryContext, "query string"); } @@ -1560,7 +1552,6 @@ private class FactoryContext public Dictionary TrackedParameters { get; } = new(); public bool HasMultipleBodyParameters { get; set; } public bool HasInferredBody { get; set; } - public bool HasInferredQueryArray { get; set; } public List Metadata { get; } = new(); @@ -1801,20 +1792,6 @@ private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryConte return errorMessage.ToString(); } - private static string BuildErrorMessageForQueryAndJsonBodyParameters(FactoryContext factoryContext) - { - var errorMessage = new StringBuilder(); - errorMessage.AppendLine("An action cannot use both inferred query and JSON body parameters. This is ambiguous"); - errorMessage.AppendLine("Below is the list of parameters that we found: "); - errorMessage.AppendLine(); - errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); - errorMessage.AppendLine("---------------------------------------------------------------------------------"); - - FormatTrackedParameters(factoryContext, errorMessage); - - return errorMessage.ToString(); - } - private static void FormatTrackedParameters(FactoryContext factoryContext, StringBuilder errorMessage) { foreach (var kv in factoryContext.TrackedParameters) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 6900d068dd05..f07b7e7fea08 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -496,6 +496,7 @@ static void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, new[] { "https://example.org" },new[] { new MyTryParseRecord(new Uri("https://example.org")) } }, new object?[] { (Action)Store, null, null }, new object?[] { (Action)Store, new string[] {}, Array.Empty() }, + new object?[] { (Action)Store, new string[] { "1", "2", "", "4" }, new int?[] { 1,2, null, 4 } }, }; } } @@ -786,6 +787,28 @@ public async Task RequestDelegateHandlesArraysFromQueryString(Delegate action, s Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } + [Theory] + [MemberData(nameof(TryParsableArrayParameters))] + public async Task RequestDelegateHandlesDoesNotHandleArraysFromQueryStringWhenBodyIsInferred(Delegate action, string[]? queryValues, object? expectedParameterValue) + { + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["tryParsable"] = queryValues + }); + + var factoryResult = RequestDelegateFactory.Create(action); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Null(httpContext.Items["tryParsable"]); + + // Ignore this parameter but we want to reuse the dataset + GC.KeepAlive(expectedParameterValue); + } + [Fact] public async Task RequestDelegateHandlesArraysFromExplicitQueryStringSource() { @@ -813,31 +836,6 @@ public async Task RequestDelegateHandlesArraysFromExplicitQueryStringSource() Assert.Equal(new[] { 4, 5, 6 }, (int[])httpContext.Items["headers"]!); } - //[Fact] - //public async Task RequestDelegateHandlesArraysFromQueryStringParseFailureWithOptionalValues() - //{ - // var httpContext = CreateHttpContext(); - // var wasCalled = false; - - // httpContext.Request.Query = new QueryCollection(new Dictionary - // { - // ["a"] = new(new[] { "1", "2", "", "3" }) - // }); - - // var factoryResult = RequestDelegateFactory.Create((HttpContext context, int?[] a) => - // { - // wasCalled = true; - // context.Items["tryParsable"] = a; - // }, new() { DisableInferBodyFromParameters = true, }); - - // var requestDelegate = factoryResult.RequestDelegate; - - // await requestDelegate(httpContext); - - // Assert.Equal(new int?[] { 1, 2, null, 3 }, (int?[])httpContext.Items["tryParsable"]!); - // Assert.True(wasCalled); - //} - [Fact] public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString() { @@ -1100,6 +1098,41 @@ void TestAction([FromRoute] int tryParsable, [FromRoute] int tryParsable2) Assert.Equal(400, badHttpRequestException.StatusCode); } + [Fact] + public async Task RequestDelegateThrowsForTryParsableFailuresIfThrowOnBadRequestWithArrays() + { + var invoked = false; + + void TestAction([FromQuery] int[] values) + { + invoked = true; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary() + { + ["values"] = new(new[] { "1", "NAN", "3" }) + }); + + var factoryResult = RequestDelegateFactory.Create(TestAction, new() { ThrowOnBadRequest = true }); + var requestDelegate = factoryResult.RequestDelegate; + + var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + + Assert.False(invoked); + + // The httpContext should be untouched. + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + // We don't log bad requests when we throw. + Assert.Empty(TestSink.Writes); + + Assert.Equal(@"Failed to bind parameter ""int[] values"" from ""NAN"".", badHttpRequestException.Message); + Assert.Equal(400, badHttpRequestException.StatusCode); + } + [Fact] public async Task RequestDelegateLogsBindAsyncFailuresAndSets400Response() { From 871b97a2409cd4c1bea5d4eca1fe51e18566116f Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 27 Jan 2022 23:55:46 -0800 Subject: [PATCH 11/17] Fixed empty values with nullable arrays --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 9 ++++++++- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index cbd09b0b867b..4a2bcdcd1b6d 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -39,6 +39,7 @@ public static partial class RequestDelegateFactory private static readonly MethodInfo ResultWriteResponseAsyncMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteResultWriteResponse), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo StringResultWriteResponseAsyncMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteWriteStringResponseAsync), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo JsonResultWriteResponseAsyncMethod = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); + private static readonly MethodInfo StringIsNullOrEmptyMethod = typeof(string).GetMethod(nameof(string.IsNullOrEmpty), BindingFlags.Static | BindingFlags.Public)!; private static readonly MethodInfo LogParameterBindingFailedMethod = GetMethodInfo>((httpContext, parameterType, parameterName, sourceValue, shouldThrow) => Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue, shouldThrow)); @@ -71,6 +72,8 @@ public static partial class RequestDelegateFactory private static readonly ParameterExpression TempSourceStringExpr = ParameterBindingMethodCache.TempSourceStringExpr; private static readonly BinaryExpression TempSourceStringNotNullExpr = Expression.NotEqual(TempSourceStringExpr, Expression.Constant(null)); private static readonly BinaryExpression TempSourceStringNullExpr = Expression.Equal(TempSourceStringExpr, Expression.Constant(null)); + private static readonly UnaryExpression TempSourceStringIsNotNullOrEmptyExpr = Expression.Not(Expression.Call(StringIsNullOrEmptyMethod, TempSourceStringExpr)); + private static readonly string[] DefaultAcceptsContentType = new[] { "application/json" }; private static readonly string[] FormFileContentType = new[] { "multipart/form-data" }; @@ -1027,6 +1030,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch { + // (isArray: false, optional: true | false) (true, _) => Expression.Block( @@ -1048,7 +1052,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // tempSourceString = values[index]; Expression.Block( Expression.Assign(TempSourceStringExpr, Expression.ArrayIndex(stringArrayExpr!, index)), - tryParseExpression + isNotNullable ? tryParseExpression : Expression.IfThen(TempSourceStringIsNotNullOrEmptyExpr, tryParseExpression) ), // else break Expression.Break(loopExit) @@ -1060,6 +1064,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres ) ) ), + + // (isArray: false, optional: false) (false, false) => Expression.Block( @@ -1070,6 +1076,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // if (tempSourceString != null) { ... } ifNotNullTryParse), + // (isArray: false, optional: true) (false, true) => Expression.Block( diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index f07b7e7fea08..8f217180539a 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -496,6 +496,7 @@ static void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, new[] { "https://example.org" },new[] { new MyTryParseRecord(new Uri("https://example.org")) } }, new object?[] { (Action)Store, null, null }, new object?[] { (Action)Store, new string[] {}, Array.Empty() }, + new object?[] { (Action)Store, new string?[] { "1", "2", null, "4" }, new int?[] { 1,2, null, 4 } }, new object?[] { (Action)Store, new string[] { "1", "2", "", "4" }, new int?[] { 1,2, null, 4 } }, }; } From 8f5b17a04a0c9eb2ed59f5de88e87e453c15b976 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 00:10:24 -0800 Subject: [PATCH 12/17] Added support the nullable ref types in array elements --- .../Http.Extensions/src/RequestDelegateFactory.cs | 13 +++++++++---- .../test/RequestDelegateFactoryTests.cs | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 4a2bcdcd1b6d..3ee9eb25258e 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1025,8 +1025,13 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres Expression.Constant(parameter.DefaultValue))); var loopExit = Expression.Label(); - // We can reuse this like we reuse temp source string - ParameterExpression? stringArrayExpr = parameter.ParameterType.IsArray ? Expression.Variable(typeof(string[]), "tempStringArray") : null; + + // TODO: We can reuse this like we reuse temp source string + var stringArrayExpr = parameter.ParameterType.IsArray ? Expression.Variable(typeof(string[]), "tempStringArray") : null; + var elementTypeNullabilityInfo = parameter.ParameterType.IsArray ? factoryContext.NullabilityContext.Create(parameter)?.ElementType : null; + + // Determine optionality of the element type of the array + var elementTypeOptional = !isNotNullable || (elementTypeNullabilityInfo?.ReadState != NullabilityState.NotNull); var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch { @@ -1052,7 +1057,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // tempSourceString = values[index]; Expression.Block( Expression.Assign(TempSourceStringExpr, Expression.ArrayIndex(stringArrayExpr!, index)), - isNotNullable ? tryParseExpression : Expression.IfThen(TempSourceStringIsNotNullOrEmptyExpr, tryParseExpression) + elementTypeOptional ? Expression.IfThen(TempSourceStringIsNotNullOrEmptyExpr, tryParseExpression) + : tryParseExpression ), // else break Expression.Break(loopExit) @@ -1170,7 +1176,6 @@ private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo private static Expression BindParameterFromBindAsync(ParameterInfo parameter, FactoryContext factoryContext) { // We reference the boundValues array by parameter index here - var nullability = factoryContext.NullabilityContext.Create(parameter); var isOptional = IsOptionalParameter(parameter, factoryContext); // Get the BindAsync method for the type. diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8f217180539a..649ea622a65f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -498,6 +498,7 @@ static void Store(HttpContext httpContext, T tryParsable) new object?[] { (Action)Store, new string[] {}, Array.Empty() }, new object?[] { (Action)Store, new string?[] { "1", "2", null, "4" }, new int?[] { 1,2, null, 4 } }, new object?[] { (Action)Store, new string[] { "1", "2", "", "4" }, new int?[] { 1,2, null, 4 } }, + new object[] { (Action)Store, new[] { "" }, new MyTryParseRecord?[] { null } }, }; } } From fa9f2edfe7b5934556f67d727913416d3394809e Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 00:15:36 -0800 Subject: [PATCH 13/17] Fixed expression in endpoint metadata provider --- .../src/EndpointMetadataApiDescriptionProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 2603abef7f7a..5c5531a6048f 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -297,10 +297,10 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.FormFile, parameter.Name ?? string.Empty, false, parameter.ParameterType); } - else if (disableInferredBody && + else if (disableInferredBody && ( (parameter.ParameterType.IsArray && ParameterBindingMethodCache.HasTryParseMethod(parameter.ParameterType.GetElementType()!)) || parameter.ParameterType == typeof(string[]) || - parameter.ParameterType == typeof(StringValues)) + parameter.ParameterType == typeof(StringValues))) { return (BindingSource.Query, parameter.Name ?? string.Empty, false, parameter.ParameterType); } From d55c0d6a804efb449a3a998ab44260d1506b2429 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 08:12:24 -0800 Subject: [PATCH 14/17] Added tests for query string inference --- ...EndpointMetadataApiDescriptionProviderTest.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index aef0b7ba2e4b..2658b5b9dcfd 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -16,6 +16,7 @@ using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Options; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.ApiExplorer; @@ -329,16 +330,21 @@ static void AssertPathParameter(ApiDescription apiDescription) [Fact] public void AddsFromQueryParameterAsQuery() { - static void AssertQueryParameter(ApiDescription apiDescription) + static void AssertQueryParameter(ApiDescription apiDescription) { var param = Assert.Single(apiDescription.ParameterDescriptions); - Assert.Equal(typeof(int), param.Type); - Assert.Equal(typeof(int), param.ModelMetadata.ModelType); + Assert.Equal(typeof(T), param.Type); + Assert.Equal(typeof(T), param.ModelMetadata.ModelType); Assert.Equal(BindingSource.Query, param.Source); } - AssertQueryParameter(GetApiDescription((int foo) => { }, "/")); - AssertQueryParameter(GetApiDescription(([FromQuery] int foo) => { })); + AssertQueryParameter(GetApiDescription((int foo) => { }, "/")); + AssertQueryParameter(GetApiDescription(([FromQuery] int foo) => { })); + AssertQueryParameter(GetApiDescription(([FromQuery] TryParseStringRecordStruct foo) => { })); + AssertQueryParameter(GetApiDescription((int[] foo) => { }, "/")); + AssertQueryParameter(GetApiDescription((string[] foo) => { }, "/")); + AssertQueryParameter(GetApiDescription((StringValues foo) => { }, "/")); + AssertQueryParameter(GetApiDescription((TryParseStringRecordStruct[] foo) => { }, "/")); } [Fact] From c5e7bdc7812bd94ae662e1cc1ab710e3c5aaf2e8 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 13:15:38 -0800 Subject: [PATCH 15/17] Fixed issue required arrays of ref types --- .../src/RequestDelegateFactory.cs | 45 +++++++++---- .../test/RequestDelegateFactoryTests.cs | 65 ++++++++++++++++++- 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 3ee9eb25258e..c3600713584c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1026,25 +1026,15 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var loopExit = Expression.Label(); - // TODO: We can reuse this like we reuse temp source string + // REVIEW: We can reuse this like we reuse temp source string var stringArrayExpr = parameter.ParameterType.IsArray ? Expression.Variable(typeof(string[]), "tempStringArray") : null; var elementTypeNullabilityInfo = parameter.ParameterType.IsArray ? factoryContext.NullabilityContext.Create(parameter)?.ElementType : null; // Determine optionality of the element type of the array var elementTypeOptional = !isNotNullable || (elementTypeNullabilityInfo?.ReadState != NullabilityState.NotNull); - var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch - { - // (isArray: false, optional: true | false) - (true, _) => - - Expression.Block( - new[] { index, stringArrayExpr! }, - // values = httpContext.Request.Query["id"]; - Expression.Assign(stringArrayExpr!, valueExpression), - Expression.IfThen( - Expression.NotEqual(stringArrayExpr!, Expression.Constant(null)), - Expression.Block( + // The loop that populates the resulting array values + var arrayLoop = parameter.ParameterType.IsArray ? Expression.Block( // param_local = new int[values.Length]; Expression.Assign(argument, Expression.NewArrayBounds(parameter.ParameterType.GetElementType()!, Expression.ArrayLength(stringArrayExpr!))), // index = 0 @@ -1067,7 +1057,34 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres Expression.PostIncrementAssign(index) ) , loopExit) - ) + ) : null; + + var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch + { + // (isArray: false, optional: true) + (true, true) => + + Expression.Block( + new[] { index, stringArrayExpr! }, + // values = httpContext.Request.Query["id"]; + Expression.Assign(stringArrayExpr!, valueExpression), + Expression.IfThen( + Expression.NotEqual(stringArrayExpr!, Expression.Constant(null)), + arrayLoop! + ) + ), + + // (isArray: false, optional: false) + (true, false) => + + Expression.Block( + new[] { index, stringArrayExpr! }, + // values = httpContext.Request.Query["id"]; + Expression.Assign(stringArrayExpr!, valueExpression), + Expression.IfThenElse( + Expression.NotEqual(stringArrayExpr!, Expression.Constant(null)), + arrayLoop!, + failBlock ) ), diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 649ea622a65f..a580201b936f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -494,7 +494,6 @@ static void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, new[] { "42" }, new int?[] { 42 } }, new object[] { (Action)Store, new[] { "ValueB" },new[] { MyEnum.ValueB } }, new object[] { (Action)Store, new[] { "https://example.org" },new[] { new MyTryParseRecord(new Uri("https://example.org")) } }, - new object?[] { (Action)Store, null, null }, new object?[] { (Action)Store, new string[] {}, Array.Empty() }, new object?[] { (Action)Store, new string?[] { "1", "2", null, "4" }, new int?[] { 1,2, null, 4 } }, new object?[] { (Action)Store, new string[] { "1", "2", "", "4" }, new int?[] { 1,2, null, 4 } }, @@ -767,6 +766,7 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ await requestDelegate(httpContext); + Assert.NotEmpty(httpContext.Items); Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } @@ -786,6 +786,7 @@ public async Task RequestDelegateHandlesArraysFromQueryString(Delegate action, s await requestDelegate(httpContext); + Assert.NotEmpty(httpContext.Items); Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } @@ -805,12 +806,37 @@ public async Task RequestDelegateHandlesDoesNotHandleArraysFromQueryStringWhenBo await requestDelegate(httpContext); + // Assert.NotEmpty(httpContext.Items); Assert.Null(httpContext.Items["tryParsable"]); // Ignore this parameter but we want to reuse the dataset GC.KeepAlive(expectedParameterValue); } + [Fact] + public async Task RequestDelegateHandlesOptionalArraysFromNullQueryString() + { + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["tryParsable"] = (string?)null + }); + + static void StoreNullableIntArray(HttpContext httpContext, int?[]? tryParsable) + { + httpContext.Items["tryParsable"] = tryParsable; + } + + var factoryResult = RequestDelegateFactory.Create(StoreNullableIntArray, new() { DisableInferBodyFromParameters = true }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.NotEmpty(httpContext.Items); + Assert.Null(httpContext.Items["tryParsable"]); + } + [Fact] public async Task RequestDelegateHandlesArraysFromExplicitQueryStringSource() { @@ -1116,7 +1142,7 @@ void TestAction([FromQuery] int[] values) ["values"] = new(new[] { "1", "NAN", "3" }) }); - var factoryResult = RequestDelegateFactory.Create(TestAction, new() { ThrowOnBadRequest = true }); + var factoryResult = RequestDelegateFactory.Create(TestAction, new() { ThrowOnBadRequest = true, DisableInferBodyFromParameters = true }); var requestDelegate = factoryResult.RequestDelegate; var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); @@ -1135,6 +1161,41 @@ void TestAction([FromQuery] int[] values) Assert.Equal(400, badHttpRequestException.StatusCode); } + [Fact] + public async Task RequestDelegateThrowsForTryParsableFailuresIfThrowOnBadRequestWithNonOptionalArrays() + { + var invoked = false; + + static void StoreNullableIntArray(HttpContext httpContext, int?[] values) + { + httpContext.Items["values"] = values; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary() + { + ["values"] = (string?)null + }); + + var factoryResult = RequestDelegateFactory.Create(StoreNullableIntArray, new() { ThrowOnBadRequest = true, DisableInferBodyFromParameters = true }); + var requestDelegate = factoryResult.RequestDelegate; + + var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + + Assert.False(invoked); + + // The httpContext should be untouched. + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + // We don't log bad requests when we throw. + Assert.Empty(TestSink.Writes); + + Assert.Equal(@"Failed to bind parameter ""Nullable[] values"" from """".", badHttpRequestException.Message); + Assert.Equal(400, badHttpRequestException.StatusCode); + } + [Fact] public async Task RequestDelegateLogsBindAsyncFailuresAndSets400Response() { From 4e45d3c951e31d56fc9b068354611b24553f9fad Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 13:39:20 -0800 Subject: [PATCH 16/17] Apply suggestions from code review --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index c3600713584c..4622220c2087 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1061,7 +1061,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var fullParamCheckBlock = (parameter.ParameterType.IsArray, isOptional) switch { - // (isArray: false, optional: true) + // (isArray: true, optional: true) (true, true) => Expression.Block( @@ -1074,7 +1074,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres ) ), - // (isArray: false, optional: false) + // (isArray: true, optional: false) (true, false) => Expression.Block( From f0d9503f52d9decd439a3f1d333b9b1662af9a07 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 28 Jan 2022 17:03:29 -0800 Subject: [PATCH 17/17] PR feedback --- .../test/RequestDelegateFactoryTests.cs | 4 +-- .../EndpointMetadataApiDescriptionProvider.cs | 31 ++++++++++--------- ...pointMetadataApiDescriptionProviderTest.cs | 22 +++++++++++-- 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index a580201b936f..87589f9ce955 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1166,9 +1166,9 @@ public async Task RequestDelegateThrowsForTryParsableFailuresIfThrowOnBadRequest { var invoked = false; - static void StoreNullableIntArray(HttpContext httpContext, int?[] values) + void StoreNullableIntArray(HttpContext httpContext, int?[] values) { - httpContext.Items["values"] = values; + invoked = true; } var httpContext = CreateHttpContext(); diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 5c5531a6048f..57037d5ef14e 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -45,6 +45,18 @@ public EndpointMetadataApiDescriptionProvider( public void OnProvidersExecuting(ApiDescriptionProviderContext context) { + // Keep in sync with EndpointRouteBuilderExtensions.cs + static bool ShouldDisableInferredBody(string method) + { + // GET, DELETE, HEAD, CONNECT, TRACE, and OPTIONS normally do not contain bodies + return method.Equals(HttpMethods.Get, StringComparison.Ordinal) || + method.Equals(HttpMethods.Delete, StringComparison.Ordinal) || + method.Equals(HttpMethods.Head, StringComparison.Ordinal) || + method.Equals(HttpMethods.Options, StringComparison.Ordinal) || + method.Equals(HttpMethods.Trace, StringComparison.Ordinal) || + method.Equals(HttpMethods.Connect, StringComparison.Ordinal); + } + foreach (var endpoint in _endpointDataSource.Endpoints) { if (endpoint is RouteEndpoint routeEndpoint && @@ -52,12 +64,15 @@ public void OnProvidersExecuting(ApiDescriptionProviderContext context) routeEndpoint.Metadata.GetMetadata() is { } httpMethodMetadata && routeEndpoint.Metadata.GetMetadata() is null or { ExcludeFromDescription: false }) { + // We need to detect if any of the methods allow inferred body + var disableInferredBody = httpMethodMetadata.HttpMethods.Any(ShouldDisableInferredBody); + // REVIEW: Should we add an ApiDescription for endpoints without IHttpMethodMetadata? Swagger doesn't handle // a null HttpMethod even though it's nullable on ApiDescription, so we'd need to define "default" HTTP methods. // In practice, the Delegate will be called for any HTTP method if there is no IHttpMethodMetadata. foreach (var httpMethod in httpMethodMetadata.HttpMethods) { - context.Results.Add(CreateApiDescription(routeEndpoint, httpMethod, methodInfo)); + context.Results.Add(CreateApiDescription(routeEndpoint, httpMethod, methodInfo, disableInferredBody)); } } } @@ -67,19 +82,8 @@ public void OnProvidersExecuted(ApiDescriptionProviderContext context) { } - private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string httpMethod, MethodInfo methodInfo) + private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string httpMethod, MethodInfo methodInfo, bool disableInferredBody) { - static bool ShouldDisableInferredBody(string method) - { - // GET, DELETE, HEAD, CONNECT, TRACE, and OPTIONS normally do not contain bodies - return method.Equals(HttpMethods.Get, StringComparison.Ordinal) || - method.Equals(HttpMethods.Delete, StringComparison.Ordinal) || - method.Equals(HttpMethods.Head, StringComparison.Ordinal) || - method.Equals(HttpMethods.Options, StringComparison.Ordinal) || - method.Equals(HttpMethods.Trace, StringComparison.Ordinal) || - method.Equals(HttpMethods.Connect, StringComparison.Ordinal); - } - // Swashbuckle uses the "controller" name to group endpoints together. // For now, put all methods defined the same declaring type together. string controllerName; @@ -111,7 +115,6 @@ static bool ShouldDisableInferredBody(string method) }; var hasBodyOrFormFileParameter = false; - var disableInferredBody = ShouldDisableInferredBody(httpMethod); foreach (var parameter in methodInfo.GetParameters()) { diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 2658b5b9dcfd..56c00a475b82 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -347,6 +347,24 @@ static void AssertQueryParameter(ApiDescription apiDescription) AssertQueryParameter(GetApiDescription((TryParseStringRecordStruct[] foo) => { }, "/")); } + [Theory] + [InlineData("Put")] + [InlineData("Post")] + public void BodyIsInferredForArraysInsteadOfQuerySomeHttpMethods(string httpMethod) + { + static void AssertBody(ApiDescription apiDescription) + { + var param = Assert.Single(apiDescription.ParameterDescriptions); + Assert.Equal(typeof(T), param.Type); + Assert.Equal(typeof(T), param.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Body, param.Source); + } + + AssertBody(GetApiDescription((int[] foo) => { }, "/", httpMethods: new[] { httpMethod })); + AssertBody(GetApiDescription((string[] foo) => { }, "/", httpMethods: new[] { httpMethod })); + AssertBody(GetApiDescription((TryParseStringRecordStruct[] foo) => { }, "/", httpMethods: new[] { httpMethod })); + } + [Fact] public void AddsFromHeaderParameterAsHeader() { @@ -1169,8 +1187,8 @@ private static IList GetApiDescriptions( private static TestEndpointRouteBuilder CreateBuilder() => new TestEndpointRouteBuilder(new ApplicationBuilder(new TestServiceProvider())); - private static ApiDescription GetApiDescription(Delegate action, string pattern = null, string displayName = null) => - Assert.Single(GetApiDescriptions(action, pattern, displayName: displayName)); + private static ApiDescription GetApiDescription(Delegate action, string pattern = null, string displayName = null, IEnumerable httpMethods = null) => + Assert.Single(GetApiDescriptions(action, pattern, displayName: displayName, httpMethods: httpMethods)); private static void TestAction() {