Skip to content

Conversation

@marcominerva
Copy link
Contributor

Description

Refactor RequestDelegateFactory to handle parameter binding failures more flexibly:

  • Execute filter pipelines on failure if filters are present and return type is ValueTask<object?>, allowing filters to observe/modify the 400 response.
  • Short-circuit with a 400 response if no filters are present.
  • Simplify logic using Expression.Condition and Expression.Block.

Add new test cases in RouteHandlerEndpointRouteBuilderExtensionsTest.cs:

  • Validate behavior with/without filters and custom responses.
  • Ensure handlers are not executed on binding failure.

Fixes #64341

Refactor `RequestDelegateFactory` to handle parameter binding
failures more flexibly:
- Execute filter pipelines on failure if filters are present
  and return type is `ValueTask<object?>`, allowing filters
  to observe/modify the 400 response.
- Short-circuit with a 400 response if no filters are present.
- Simplify logic using `Expression.Condition` and
  `Expression.Block`.

Add new test cases in `RouteHandlerEndpointRouteBuilderExtensionsTest.cs`:
- Validate behavior with/without filters and custom responses.
- Ensure handlers are not executed on binding failure.
Copilot AI review requested due to automatic review settings November 26, 2025 11:16
@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Nov 26, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 26, 2025
Copilot finished reviewing on behalf of marcominerva November 26, 2025 11:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors parameter binding failure handling in RequestDelegateFactory to provide more flexible filter integration. The key change allows endpoint filters to observe and potentially customize 400 Bad Request responses when parameter binding fails, rather than always short-circuiting before filters run.

Key Changes:

  • Unified the parameter binding failure logic using Expression.Condition instead of separate code paths for filtered vs non-filtered scenarios
  • When filters are present and return type is ValueTask<object?>, the filter pipeline executes on binding failure, allowing filters to customize the 400 response
  • Added comprehensive test coverage for binding failure scenarios with and without filters

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Refactored CreateParamCheckingResponseWritingMethodCall to conditionally run filter pipelines on parameter binding failures, simplifying the code with a unified Expression.Condition approach
src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs Added 4 test cases validating parameter binding failure behavior: with/without filters, with custom filter responses, and with validation filters

Comment on lines +1191 to +1226
[Fact]
public async Task ParameterBindingFailure_WithAddValidation_DoesNotExecuteHandler()
{
// Arrange
var services = new ServiceCollection().AddSingleton(LoggerFactory);
services.AddValidation(); // Register validation filter factory
var serviceProvider = services.BuildServiceProvider();

var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
var handlerExecuted = false;

// Act - Binding failure happens BEFORE validation filter runs
builder.MapGet("/test/{id}", (Guid id) =>
{
handlerExecuted = true;
return 1;
});

var dataSource = GetBuilderEndpointDataSource(builder);
var endpoint = Assert.Single(dataSource.Endpoints);

var httpContext = new DefaultHttpContext
{
RequestServices = serviceProvider
};
httpContext.Request.RouteValues["id"] = "invalid-guid";

// Act
await endpoint.RequestDelegate!(httpContext);

// Assert
Assert.Equal(StatusCodes.Status400BadRequest, httpContext.Response.StatusCode);
Assert.True(httpContext.Response.Body.Length == 0, "Response body should be empty");
Assert.False(handlerExecuted, "Handler should not have been executed when parameter binding fails");
}

Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The AddValidation() extension method requires a reference to Microsoft.Extensions.Validation, but this test project (Microsoft.AspNetCore.Routing.Tests.csproj) doesn't appear to have that reference. This will cause a compilation error.

Either:

  1. Add a reference to Microsoft.Extensions.Validation in the project file, or
  2. Move this test to the Microsoft.AspNetCore.Http.Extensions.Tests project where validation testing is more appropriate, or
  3. Remove this test if validation integration testing is not needed here
Suggested change
[Fact]
public async Task ParameterBindingFailure_WithAddValidation_DoesNotExecuteHandler()
{
// Arrange
var services = new ServiceCollection().AddSingleton(LoggerFactory);
services.AddValidation(); // Register validation filter factory
var serviceProvider = services.BuildServiceProvider();
var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
var handlerExecuted = false;
// Act - Binding failure happens BEFORE validation filter runs
builder.MapGet("/test/{id}", (Guid id) =>
{
handlerExecuted = true;
return 1;
});
var dataSource = GetBuilderEndpointDataSource(builder);
var endpoint = Assert.Single(dataSource.Endpoints);
var httpContext = new DefaultHttpContext
{
RequestServices = serviceProvider
};
httpContext.Request.RouteValues["id"] = "invalid-guid";
// Act
await endpoint.RequestDelegate!(httpContext);
// Assert
Assert.Equal(StatusCodes.Status400BadRequest, httpContext.Response.StatusCode);
Assert.True(httpContext.Response.Body.Length == 0, "Response body should be empty");
Assert.False(handlerExecuted, "Handler should not have been executed when parameter binding fails");
}
// [Fact]
// public async Task ParameterBindingFailure_WithAddValidation_DoesNotExecuteHandler()
// {
// // Arrange
// var services = new ServiceCollection().AddSingleton(LoggerFactory);
// services.AddValidation(); // Register validation filter factory
// var serviceProvider = services.BuildServiceProvider();
//
// var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
// var handlerExecuted = false;
//
// // Act - Binding failure happens BEFORE validation filter runs
// builder.MapGet("/test/{id}", (Guid id) =>
// {
// handlerExecuted = true;
// return 1;
// });
//
// var dataSource = GetBuilderEndpointDataSource(builder);
// var endpoint = Assert.Single(dataSource.Endpoints);
//
// var httpContext = new DefaultHttpContext
// {
// RequestServices = serviceProvider
// };
// httpContext.Request.RouteValues["id"] = "invalid-guid";
//
// // Act
// await endpoint.RequestDelegate!(httpContext);
//
// // Assert
// Assert.Equal(StatusCodes.Status400BadRequest, httpContext.Response.StatusCode);
// Assert.True(httpContext.Response.Body.Length == 0, "Response body should be empty");
// Assert.False(handlerExecuted, "Handler should not have been executed when parameter binding fails");
// }

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the tests succeeded, so there are no compilation errors at all. In particular, this test focuses on behavior related to parameter-binding failures, so I think it makes sense to keep it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member feature-rdg

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using AddValidation causes non-validated Minimal API endpoints to continue after parsing errors

2 participants