Skip to content

RequestDelegateFactory should throw BadHttpRequestException when in development to make diagnosing handler issues easier #35858

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

Closed
DamianEdwards opened this issue Aug 27, 2021 · 4 comments · Fixed by #36004
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 27, 2021

When using the new "Map*" methods to build minimal APIs via the RouteDelegateFactory, it can be difficult to diagnose the cause of 400 and 415 responses that are implicitly returned when the mapped route handler's parameters cannot be satisfied from the request, either via the implicit parameter binding behavior or TryParse' and 'BindAsync methods on the parameter target types.

It would be helpful if when in development (i.e. Environment.IsDevelopment() == true) instead of implicitly returning the response with a 4xx status code, a BadHttpRequestException was thrown instead, with the relevant status code set and an appropriate exception message added, making it clear why the exception/status code is being throw/set. With the DeveloperExceptionPageMiddleware on (which it is by default in development in .NET 6 when using WebApplication) the exception message and stack trace will be returned in the response body, making it much clearer why the invocation of the route handler failed.

This behavior should be controllable via a new options type RouteHandlerOptions with boolean property ThrowOnBadRequest that is configured by default to be true when Environment.IsDevelopmet() is true. The default configuration would be achieved via an internal type implementing IConfigureOptions<RouteHandlerOptions> that is registered in DI when IServiceCollection.AddRouting() is called.

Related:

Proposed APIs

namespace Microsoft.AspNetCore.Routing
{
+    public class RouteHandlerOptions
+    {
+        public bool ThrowOnBadRequest { get; set; }
+    }
}

namespace Microsoft.AspNetCore.Http
{
    public class RequestDelegateFactoryOptions
    {
+        public bool ThrowOnBadRequest { get; init; }
    }
}

Example usage

var builder = WebApplication.CreateBuilder(args);

// Enable exceptions for implicit 4xx responses in custom "LocalDev" environment
var isDev = app.Environment.IsDevelopment() || app.Environment.IsEnvironemnt("LocalDev");
builder.Services.Configure<RouteHandlerOptions>(options => options.ThrowOnBadRequest = isDev);

var app = builder.Build();
@DamianEdwards DamianEdwards added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Aug 27, 2021
@DamianEdwards DamianEdwards added this to the 6.0-rc2 milestone Aug 27, 2021
@DamianEdwards DamianEdwards changed the title RouteDelegateFactory should throw BadHttpRequestException when in development to make diagnosing handler issues easier RequestDelegateFactory should throw BadHttpRequestException when in development to make diagnosing handler issues easier Aug 27, 2021
@martincostello
Copy link
Member

This would definitely be helpful. I was working on something today with an rc.1 build and a couple of times managed to forget to make an optional parameter nullable (string vs string?) and had to resort to putting the log level up to Debug on RequestDelegateFactory to see where I’d gone wrong.

@DamianEdwards
Copy link
Member Author

@martincostello yep same here. We discussed changing the log message level dynamically but @halter73's suggestion of throwing ultimately led to this design which I think is much nicer.

@rafikiassumani-msft rafikiassumani-msft added enhancement This issue represents an ask for new feature or an enhancement to an existing one Priority:1 Work that is critical for the release, but we could probably ship without labels Aug 30, 2021
@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 1, 2021
@ghost
Copy link

ghost commented Sep 1, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@ghost
Copy link

ghost commented Sep 2, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Sep 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2021
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants