Added new overloads to RequestDelegateFactory #34375
Conversation
- Throw exceptions if route parameters are specified via attributes but not declared in the list of route parameters. - Don't fall back to the query string if the the route parameter is specified. - Added tests
davidfowl
requested review from
BrennanConroy,
javiercn and
Tratcher
as code owners
ghost
added
the
| @@ -202,6 +200,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext | |||
|
|
|||
| if (parameterCustomAttributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute) | |||
| { | |||
| if (factoryContext.RouteParameters is { } routeParams && !routeParams.Contains(parameter.Name)) | |||
| { | |||
| throw new InvalidOperationException($"{parameter.Name} is not a route paramter."); | |||
There was a problem hiding this comment.
This is worth a look. I added some tests for this scenario as well. TL;DR If you specify [FromRoute] and but didn't specify a route parameter in the template, this will throw. In theory, you could add route values dynamically via middleware so maybe it's too aggressive but I think that's pretty rare.
There was a problem hiding this comment.
Not identical, but Blazor does this today and it helps alleviate the class of issues. We can always change it to be less restrictive if we users run in to in the future (particularly given that we have an option type for configure this).
captainsafia
added
the
api-approved
| @@ -202,6 +200,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext | |||
|
|
|||
| if (parameterCustomAttributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute) | |||
| { | |||
| if (factoryContext.RouteParameters is { } routeParams && !routeParams.Contains(parameter.Name)) | |||
| { | |||
| throw new InvalidOperationException($"{parameter.Name} is not a route paramter."); | |||
There was a problem hiding this comment.
Not identical, but Blazor does this today and it helps alleviate the class of issues. We can always change it to be less restrictive if we users run in to in the future (particularly given that we have an option type for configure this).
| namespace Microsoft.AspNetCore.Http | ||
| { | ||
| /// <summary> | ||
| /// Options for controlling the behavior |
There was a problem hiding this comment.
| /// Options for controlling the behavior | |
| /// Options for controlling the behavior of <see cref="RequestDelegate" /> when created using <see cref="RequestDelegateFactory" />. |
| /// <summary> | ||
| /// Options for controlling the behavior | ||
| /// </summary> | ||
| public class RequestDelegateFactoryOptions |
There was a problem hiding this comment.
| public class RequestDelegateFactoryOptions | |
| public sealed class RequestDelegateFactoryOptions |
| { | ||
| return BindParameterFromProperty(parameter, RouteValuesExpr, parameter.Name, factoryContext); | ||
| } | ||
|
|
||
| return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); |
There was a problem hiding this comment.
Should this even attempt to bind from RouteValues anymore?
There was a problem hiding this comment.
The only scenario is if the list of routes is null. But maybe if it's not null and it's not a route, the we fallback directly to query? I can add that in rc1
amcasey
added
area-networking
Fixes #33700