Skip to content

Include source in RequestDelegateFactory missing required parameter log #35580

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Include source in RequestDelegateFactory missing required parameter log #35580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 29 additions & 22 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ public static partial class RequestDelegateFactory

private static readonly MethodInfo LogParameterBindingFailedMethod = GetMethodInfo<Action<HttpContext, string, string, string>>((httpContext, parameterType, parameterName, sourceValue) =>
Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue));
private static readonly MethodInfo LogRequiredParameterNotProvidedMethod = GetMethodInfo<Action<HttpContext, string, string>>((httpContext, parameterType, parameterName) =>
Log.RequiredParameterNotProvided(httpContext, parameterType, parameterName));
private static readonly MethodInfo LogRequiredParameterNotProvidedMethod = GetMethodInfo<Action<HttpContext, string, string, string>>((httpContext, parameterType, parameterName, source) =>
Log.RequiredParameterNotProvided(httpContext, parameterType, parameterName, source));

private static readonly ParameterExpression TargetExpr = Expression.Parameter(typeof(object), "target");
private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue");
Expand Down Expand Up @@ -219,17 +219,17 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
throw new InvalidOperationException($"{parameter.Name} is not a route paramter.");
}

return BindParameterFromProperty(parameter, RouteValuesExpr, routeAttribute.Name ?? parameter.Name, factoryContext);
return BindParameterFromProperty(parameter, RouteValuesExpr, routeAttribute.Name ?? parameter.Name, factoryContext, "route");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: can we move these string literals to constants or something?

}
else if (parameterCustomAttributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.QueryAttribue);
return BindParameterFromProperty(parameter, QueryExpr, queryAttribute.Name ?? parameter.Name, factoryContext);
return BindParameterFromProperty(parameter, QueryExpr, queryAttribute.Name ?? parameter.Name, factoryContext, "query string");
}
else if (parameterCustomAttributes.OfType<IFromHeaderMetadata>().FirstOrDefault() is { } headerAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.HeaderAttribue);
return BindParameterFromProperty(parameter, HeadersExpr, headerAttribute.Name ?? parameter.Name, factoryContext);
return BindParameterFromProperty(parameter, HeadersExpr, headerAttribute.Name ?? parameter.Name, factoryContext, "header");
}
else if (parameterCustomAttributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } bodyAttribute)
{
Expand Down Expand Up @@ -278,12 +278,12 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
// We're in the fallback case and we have a parameter and route parameter match so don't fallback
// to query string in this case
factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.RouteParameter);
return BindParameterFromProperty(parameter, RouteValuesExpr, parameter.Name, factoryContext);
return BindParameterFromProperty(parameter, RouteValuesExpr, parameter.Name, factoryContext, "route");
}
else
{
factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.QueryStringParameter);
return BindParameterFromProperty(parameter, QueryExpr, parameter.Name, factoryContext);
return BindParameterFromProperty(parameter, QueryExpr, parameter.Name, factoryContext, "query string");
}
}

Expand Down Expand Up @@ -634,12 +634,16 @@ private static Expression BindParameterFromService(ParameterInfo parameter)
: Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
}

private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext)
private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext, string source)
{
var isOptional = IsOptionalParameter(parameter);

var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");

var parameterTypeNameConstant = Expression.Constant(parameter.ParameterType.Name);
var parameterNameConstant = Expression.Constant(parameter.Name);
var sourceConstant = Expression.Constant(source);

if (parameter.ParameterType == typeof(string))
{
if (!isOptional)
Expand All @@ -658,7 +662,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogRequiredParameterNotProvidedMethod,
HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, sourceConstant)
)
)
);
Expand Down Expand Up @@ -740,9 +744,6 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
// 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 parameterTypeNameConstant = Expression.Constant(parameter.ParameterType.Name);
var parameterNameConstant = Expression.Constant(parameter.Name);

var failBlock = Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogParameterBindingFailedMethod,
Expand All @@ -763,7 +764,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogRequiredParameterNotProvidedMethod,
HttpContextExpr, parameterTypeNameConstant, parameterNameConstant)
HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, sourceConstant)
)
)
);
Expand Down Expand Up @@ -802,14 +803,14 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
return argument;
}

private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext) =>
BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext);
private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source) =>
BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext, source);

private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext)
{
var routeValue = GetValueFromProperty(RouteValuesExpr, key);
var queryValue = GetValueFromProperty(QueryExpr, key);
return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext);
return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext, "route or query string");
}

private static Expression BindParameterFromBindAsync(ParameterInfo parameter, FactoryContext factoryContext)
Expand All @@ -836,7 +837,10 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogRequiredParameterNotProvidedMethod,
HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
HttpContextExpr,
Expression.Constant(parameter.ParameterType.Name),
Expression.Constant(parameter.Name),
Expression.Constant($"{parameter.ParameterType.Name}.BindAsync(HttpContext)"))
)
)
);
Expand Down Expand Up @@ -883,7 +887,10 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogRequiredParameterNotProvidedMethod,
HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
HttpContextExpr,
Expression.Constant(parameter.ParameterType.Name),
Expression.Constant(parameter.Name),
Expression.Constant("body"))
)
)
);
Expand Down Expand Up @@ -1153,13 +1160,13 @@ public static void ParameterBindingFailed(HttpContext httpContext, string parame
EventName = "ParamaterBindingFailed")]
private static partial void ParameterBindingFailed(ILogger logger, string parameterType, string parameterName, string sourceValue);

public static void RequiredParameterNotProvided(HttpContext httpContext, string parameterTypeName, string parameterName)
=> RequiredParameterNotProvided(GetLogger(httpContext), parameterTypeName, parameterName);
public static void RequiredParameterNotProvided(HttpContext httpContext, string parameterTypeName, string parameterName, string source)
=> RequiredParameterNotProvided(GetLogger(httpContext), parameterTypeName, parameterName, source);

[LoggerMessage(4, LogLevel.Debug,
@"Required parameter ""{ParameterType} {ParameterName}"" was not provided.",
@"Required parameter ""{ParameterType} {ParameterName}"" was not provided from {Source}.",
EventName = "RequiredParameterNotProvided")]
private static partial void RequiredParameterNotProvided(ILogger logger, string parameterType, string parameterName);
private static partial void RequiredParameterNotProvided(ILogger logger, string parameterType, string parameterName, string source);

public static void ParameterBindingFromHttpContextFailed(HttpContext httpContext, string parameterTypeName, string parameterName)
=> ParameterBindingFromHttpContextFailed(GetLogger(httpContext), parameterTypeName, parameterName);
Expand Down
13 changes: 6 additions & 7 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,11 +819,11 @@ public async Task RequestDelegateLogsBindAsyncFailuresAndSets400Response()

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[0].EventId);
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg1"" was not provided.", logs[0].Message);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg1"" was not provided from MyBindAsyncRecord.BindAsync(HttpContext).", logs[0].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[1].EventId);
Assert.Equal(LogLevel.Debug, logs[1].LogLevel);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg2"" was not provided.", logs[1].Message);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg2"" was not provided from MyBindAsyncRecord.BindAsync(HttpContext).", logs[1].Message);
}

[Fact]
Expand Down Expand Up @@ -1859,7 +1859,7 @@ public async Task RequestDelegateHandlesQueryParamOptionality(Delegate @delegate
Assert.Equal(LogLevel.Debug, log.LogLevel);
Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), log.EventId);
var expectedType = paramName == "age" ? "Int32 age" : "String name";
Assert.Equal($@"Required parameter ""{expectedType}"" was not provided.", log.Message);
Assert.Equal($@"Required parameter ""{expectedType}"" was not provided from route or query string.", log.Message);
}
else
{
Expand Down Expand Up @@ -1935,7 +1935,7 @@ public async Task RequestDelegateHandlesRouteParamOptionality(Delegate @delegate
Assert.Equal(LogLevel.Debug, log.LogLevel);
Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), log.EventId);
var expectedType = paramName == "age" ? "Int32 age" : "String name";
Assert.Equal($@"Required parameter ""{expectedType}"" was not provided.", log.Message);
Assert.Equal($@"Required parameter ""{expectedType}"" was not provided from query string.", log.Message);
}
else
{
Expand Down Expand Up @@ -2006,7 +2006,7 @@ public async Task RequestDelegateHandlesBodyParamOptionality(Delegate @delegate,
var log = Assert.Single(logs);
Assert.Equal(LogLevel.Debug, log.LogLevel);
Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), log.EventId);
Assert.Equal(@"Required parameter ""Todo todo"" was not provided.", log.Message);
Assert.Equal(@"Required parameter ""Todo todo"" was not provided from body.", log.Message);
}
else
{
Expand Down Expand Up @@ -2126,7 +2126,6 @@ public async Task AllowEmptyOverridesOptionality(Delegate @delegate, bool allows
var factoryResult = RequestDelegateFactory.Create(@delegate);
var requestDelegate = factoryResult.RequestDelegate;


await requestDelegate(httpContext);

var logs = TestSink.Writes.ToArray();
Expand All @@ -2137,7 +2136,7 @@ public async Task AllowEmptyOverridesOptionality(Delegate @delegate, bool allows
var log = Assert.Single(logs);
Assert.Equal(LogLevel.Debug, log.LogLevel);
Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), log.EventId);
Assert.Equal(@"Required parameter ""Todo todo"" was not provided.", log.Message);
Assert.Equal(@"Required parameter ""Todo todo"" was not provided from body.", log.Message);
}
else
{
Expand Down