Skip to content

Calling 'AddJsonApi' bypass all the exception handler middlewares #501

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
mabead opened this issue Apr 27, 2019 · 8 comments
Closed

Calling 'AddJsonApi' bypass all the exception handler middlewares #501

mabead opened this issue Apr 27, 2019 · 8 comments

Comments

@mabead
Copy link

mabead commented Apr 27, 2019

Description

Before using JsonApiDotNetCore, I had an ASP.NET Core middleware that had the responsibility to reformat all exceptions that occured in my controllers. This middleware was almost identical to this one.

As soon as I started to use JsonApiDotNetCore by calling AddJsonApi, my middleware stopped working. This is because AddJsonApi registers an exception filter:

options.Filters.Add(typeof(JsonApiExceptionFilter));

This exception filter handles all exceptions. My middleware is therefore unable to detect the exception.

Is there a way to disable this exception filter or to limit this exception filter to the routes that to use JSONAPI? My routes that don't use JSONAPI should still use my exception middleware.

Environment

  • JsonApiDotNetCore Version: 3.1.0.0
@mabead
Copy link
Author

mabead commented Apr 27, 2019

IMHO, the JsonApiExceptionFilter should only handle the exceptions that occur within a controller that inherits from JsonApiController.

@wisepotato
Copy link
Contributor

I think being able to define your own exception filter is the better option here, and i get your point, but it should also happen for the mixin controller, not the derived jsonapicontroller

@mabead
Copy link
Author

mabead commented Apr 27, 2019

Providing my own exception filter sure is a powerful option. But I feel that calling AddJsonApi should not change the behavior of an existing code base (which it currently does). New users of JsonApiDotNetCore won't typically dig in the details of how this call affects the ASP.NET Core pipeline. Instead, JsonApiDotNetCore should, in my opinion, be as friction free as possible. I would suggest to add something like the following at the beginning of JsonApiExceptionFilter.OnException:

var controllerActionDescriptor = context.ActionDescriptor as ControllerActionDescriptor;
if (controllerActionDescriptor == null)
{
    return;
}

var mixinControllerType = typeof(JsonApiControllerMixin);
if (!mixinControllerType.IsAssignableFrom(controllerActionDescriptor.ControllerTypeInfo))
{
    return; // Error is not from a JSONAPI controller. Don't intervene in the process.
}

What do you think of the idea?

In the mean time, I have no choice but to manually remove the JsonApiExceptionFilter from the list of filters added to MVC.

@jaredcnance
Copy link
Contributor

Another option would be to just inspect the ContentType of the request and only handle failures for application/vnd.api+json

@maurei
Copy link
Member

maurei commented Apr 29, 2019

For v4.0 I think we could implement the proposal of either @mabead or @jaredcnance. I'm not sure which one is better

@wisepotato
Copy link
Contributor

I disagree with @mabead that jsonapi should not change existing code base, as the errors should be Json:api spec. But you should be allowed to turn them off, or atleast alter the conditions under which they are run.

@maurei
Copy link
Member

maurei commented Sep 6, 2019

As a part of fixing failing tests for #504 , I'm currently looking into this now. The official docs have the following to say about the usage of exception filters.

Use exception filters only where error handling differs based on which action method is called. For example, an app might have action methods for both API endpoints and for views/HTML. The API endpoints could return error information as JSON, while the view-based actions could return an error page as HTML.

So this suggests it would be a good practice to implement something like @mabead proposes, because with this it targets only the JsonApiDotNetCore actions. On top of that, to allow for extensibility, we could stuff this functionality in a service that we inject in the exception filter, allowing users to customize how the filter behaves without having to mess with removing and readding filters.

@maurei
Copy link
Member

maurei commented Oct 22, 2019

Custom exception handling is now supported, see #586

@maurei maurei closed this as completed Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants