Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Added support for arrays of types with TryParse, StringValues and string[] for query strings and headers #39809

Merged
merged 17 commits into from
Jan 29, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 125 additions & 24 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
}

if (factoryContext.JsonRequestBodyParameter is not null &&
factoryContext.FirstFormRequestBodyParameter is not null)
{
Expand Down Expand Up @@ -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.
Expand All @@ -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()!))
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
{
// 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
davidfowl marked this conversation as resolved.
Show resolved Hide resolved

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)
Expand Down Expand Up @@ -409,7 +427,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(
// }

var localVariables = new ParameterExpression[factoryContext.ExtraLocals.Count + 1];
var checkParamAndCallMethod = new Expression[factoryContext.ParamCheckExpressions.Count + 1];
var blockExpressions = new Expression[factoryContext.ParamCheckExpressions.Count + 1];

for (var i = 0; i < factoryContext.ExtraLocals.Count; i++)
{
Expand All @@ -418,7 +436,7 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(

for (var i = 0; i < factoryContext.ParamCheckExpressions.Count; i++)
{
checkParamAndCallMethod[i] = factoryContext.ParamCheckExpressions[i];
blockExpressions[i] = factoryContext.ParamCheckExpressions[i];
}

localVariables[factoryContext.ExtraLocals.Count] = WasParamCheckFailureExpr;
Expand All @@ -433,9 +451,9 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(
set400StatusAndReturnCompletedTask,
AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType));

checkParamAndCallMethod[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure;
blockExpressions[factoryContext.ParamCheckExpressions.Count] = checkWasParamCheckFailure;

return Expression.Block(localVariables, checkParamAndCallMethod);
return Expression.Block(localVariables, blockExpressions);
}

private static Expression AddResponseWritingToMethodCall(Expression methodCall, Type returnType)
Expand Down Expand Up @@ -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}.");
}

Expand Down Expand Up @@ -940,8 +960,30 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
// param2_local = 42;
// }

// string[]? values = httpContext.Request.Query["param1"].ToArray();
halter73 marked this conversation as resolved.
Show resolved Hide resolved
// int[] param_local = values.Length > 0 ? new int[values.Length] : Array.Empty<int>();

// 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;
// }
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
// }
// }

// 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)),
Expand Down Expand Up @@ -970,33 +1012,77 @@ 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(
davidfowl marked this conversation as resolved.
Show resolved Hide resolved
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)
),
// index++
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);
Expand Down Expand Up @@ -1065,7 +1151,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)
{
Expand Down Expand Up @@ -1466,6 +1552,7 @@ private class FactoryContext
public Dictionary<string, string> TrackedParameters { get; } = new();
public bool HasMultipleBodyParameters { get; set; }
public bool HasInferredBody { get; set; }
public bool HasInferredQueryArray { get; set; }

public List<object> Metadata { get; } = new();

Expand Down Expand Up @@ -1706,6 +1793,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public static IEnumerable<object[]> TryParseStringParameterInfoData
[MemberData(nameof(TryParseStringParameterInfoData))]
public void HasTryParseStringMethod_ReturnsTrueWhenMethodExists(ParameterInfo parameterInfo)
{
Assert.True(new ParameterBindingMethodCache().HasTryParseMethod(parameterInfo));
Assert.True(new ParameterBindingMethodCache().HasTryParseMethod(parameterInfo.ParameterType));
}

[Fact]
Expand Down
80 changes: 80 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,86 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ
Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task RequestDelegateHandlesArraysFromQueryString(bool disableInferBodyFromParameters)
{
var httpContext = CreateHttpContext();
httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
{
["a"] = new(new[] { "1", "2", "3" })
});

var factoryResult = RequestDelegateFactory.Create((HttpContext context, int[] a) =>
{
context.Items["tryParsable"] = a;
}, 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<string, StringValues>
{
["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<string, StringValues>
{
["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()
{
Expand Down
Loading