Closed brunolins16 closed 5 days ago
I like the dictionary because it gives the ability to manipulate the data (CRUD). IMO Map
isn't a clear name, I like MapStatusCode
I like the dictionary because it gives the ability to manipulate the data (CRUD). IMO
Map
isn't a clear name, I likeMapStatusCode
I like it as well :) and using a dictionary
might be common pattern in ASP.NET Core already, right. Do you feel we need to do anything regards to the key
type (System.Type
)? Maybe introduce some generic type (struct ExceptionMapKey<T> where T: Exception
)
MapStatusCode
sounds good and is what I'm already using.
However, it feels very limiting to restrict mapping from exceptions to status codes. Maybe this is just a first step? In my middleware, status code mapping is just a few higher level convenience methods that boil down to Func<HttpContext, Exception, ProblemDetails>
registrations. These can also be paired with Func<HttpContext, Exception, bool>
predicates to allow conditional mapping. It's an infinitely more flexible solution that can support a lot of additional scenarios.
What's the rationale behind this limitation? Just keeping it simple?
@khellang Thanks for the feedback. Do you have any usage telemetry of the convenience methods x underlying func lists?
The ExceptionHandlerMiddleware
provides a mechanism for customization already, but this mechanism is more complex than it need to be when we want to do simple configurations, like Map to a status Code
, Ignore an exception
, etc. The rationale behind my proposal (that needs more discussion yet) is that we should provide convenient simple options that play well with the default handler. These scenarios you mentioned are great additions, but I feel like they are already cover by:
ExceptionHandlerOptions.ExceptionHandler
or;UseExceptionHandler(IApplicationBuilder, Action<IApplicationBuilder>)
or;ProblemDetailsOptions.CustomizeProblemDetails
(when you want customization for the generated PD) or;IProblemDetailsWriter
custom implementationsThanks for contacting us.
We're moving this issue to the .NET 8 Planning
milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
Having the option to create typed ProblemDetails is quite useful in OpenApi where you can clearly define a possible list of "Problems" and additional information as per rfc7807 standard, without having to overwrite default ExceptionHandler behavior.
One existing example would be to generate Microsoft.AspNetCore.Http.HttpValidationProblemDetails and include an Errors dictionary when there is a FluentValidation exception.
So yes, having Func<HttpContext, Exception, ProblemDetails> would be ideal :)
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:
API Review notes:
AllowStatusCode404Response
?
ExceptionHandler
request delegate?
ExceptionHandler
isn't setFunc<Exception, int>
would allow users to look at the exception and provide the status code they want, and not inadvertently write to the response if we passed in HttpContext, this helps with the ProblemDetails handling in the middleware
API Approved!
namespace Microsoft.AspNetCore.Builder;
public class ExceptionHandlerOptions
{
+ public Func<Exception, int>? StatusCodeSelector { get; set; }
}
If nobody is planning to this yet. Can I work on it?
I don't anyone is working on it yet @brunolins16 - it's all yours :)
I don't anyone is working on it yet @brunolins16 - it's all yours :)
Thanks. I will have a PR later this week.
What's the status on this? Is there a chance this will be released in the next few weeks?
If the work to do this change hasn't already been merged into the release/8.0 branch (which I would assume not if this issue is still open with no updates since April), then it's too late for .NET 8.
@brunolins16 are you still working on this? Otherwise, I would be happy to provide a PR 😊
Background and Motivation
In .NET 7 the
ExceptionHandlerMiddleware
was updated to generate aProblem Details
when theIProblemDetailsService
is registered.While this change allows a consistent
ProblemDetails
for the application the default handler forExceptionHandlerMiddleware
always sets the response status code to500
and changing to produce a different Status Code requires a custom implementation (as mentioned here #43831).Proposed API
The community
ProblemDetails
middleware (https://github.com/khellang/Middleware) has a capability to map an Exception to Status Code when producing the payload. Since theExceptionHandlerMiddleware
is the built-in feature to handle exception, my suggestion is to add a similar feature to allow mappingexception type
Xstatus code
every time an exception is handled.Usage Examples
Configuring the
ExceptionHandlerOptions
Setting
ExceptionHandlerOptions
directly to the middlewareAlternative Designs
Alternative names
public void AddMap<TException>(int statusCode) where TException : Exception {}
public void MapStatusCode<TException>(int statusCode) where TException : Exception {}
public void MapToStatusCode<TException>(int statusCode) where TException : Exception {}
Using a property instead
The disadvantage of this design is the
Dictionary
potentially accepts any type, not necessarily anException
type.cc @khellang