framework icon indicating copy to clipboard operation
framework copied to clipboard

[11.x] Improve API error handling

Open tontonsb opened this issue 1 year ago • 7 comments

I've come across two common issues when working with API backends and testing the endpoints in browser or just not knowing to specify the Accept: application/json header. Most projects work around this by adding some middleware that either adds the header to all incoming API requests or sets a forceJson flag on their custom request.

This PR proposes to remedy these issues by taking a "only redirect if there's a reasonable redirect target" approach.

Unauthenticated user

When hitting an endpoint that requires authentication, but failing the auth, user should receive an authentication error. Instead they receive a 500 error with Route [login] not defined. message.

Proposed solution: only redirect to route('login') if the route exists. Otherwise fall back to JSON response.

Backwards compatibility? Technically it is a breaking change, but I can't imagine a legit scenario where someone would rely on their auth errors being redirected to non-existant route, catching the particular error and doing some useful reporting with it.

Validation error

When getting some request params wrong, users should see validation errors. Instead users that are hitting the endpoint in their browserse receive a redirect to /. Because no "previous" route exists and even if it did, Laravel wouldn't know it as the session is not enabled on API routes.

Proposed solution: only redirect to "previous" route if it exists in the session. Do not use url()->previous() as

  • falling back to url('/') for all validation errors is unlikely to be useful in real usecases (although common in automated tests)
  • redirecting to referrer would be useless as they wouldn't receive the error bag or error code in any way

Backwards compatibility?

  1. This will require to update web (non-json) validation tests that expect a 302 redirect now. Two options:
    • Expect 422 instead of 302, i.e. change assertRedirect or assertStatus(302) to assertInvalid or assertStatus(422).
    • Set up the session with the previous URL as it would be in a real life workflow. Add session()->setPreviousUrl($prevUrl); to the test before hitting the endpoint that should redirect you back.
  2. This would also break the workflow if someone has set their index page as a dedicated error displayer. I don't know if someone actually does that. There are multiple ways to restore the previous behaviour, e.g. overriding the handler's invalid method or catching the exceptions in handler (and maybe just specifying ->redirectTo('/') on them which is still respected).

tontonsb avatar Jun 02 '23 06:06 tontonsb

I faced the same issue recently, since my app did not have any route named login.

Laravel core must not auto assume that the this route exists in Skelton

ankurk91 avatar Jun 02 '23 13:06 ankurk91

Going to revisit this a bit later. I'm currently doing some heavy work on the skeleton, exception handler, etc.

taylorotwell avatar Jun 07 '23 14:06 taylorotwell

Personally I think the api route group could just have a middleware applied that enforces JSON responses rather than all of these attempts to handle various different scenarios.

martinbean avatar Sep 15 '23 11:09 martinbean

Personally I think the api route group could just have a middleware applied that enforces JSON responses rather than all of these attempts to handle various different scenarios.

Yeah this seems good to have even if this PR is merged. Maybe a part of the default API middleware group…

trevorgehman avatar Sep 20 '23 00:09 trevorgehman

An idea for a middle ground between a 500 response and returning JSON when it may not be acceptable could be to return no content at all, but still return the appropriate status code.

For example, a request that is required to be authenticated but is not could have this behaviour:

  • If json is expected, return a 401 response with a JSON body
  • If json is not expected and a login route exists, redirect to it
  • If json is not expected but there is no login route, just return an empty response with a 401 status code

This way, a proper status code is returned without returning JSON when it may not be acceptable (although, is that really that common?)

irealworlds avatar Dec 26 '23 15:12 irealworlds

REST API routes imply that they will be accessed exclusively programmatically with very primitive HTTP client. API routes should always return responses in raw form, for example in JSON. No redirects and responses in HTML - all of the above is necessary only for web browsers.

In Laravel, it is still a problem to achieve this behavior due to global route processing. For example, middleware does not work for 404 page. To process API routes, I even had to make a separate entry point into the application - api.php instead of index.php.

TheAndrey avatar Dec 26 '23 15:12 TheAndrey

The Authenticate middleware explicitly checks if a JSON response is requested, does it not? In this case laravel does not redirect.

I just use the api middleware group and list all middlewares I need, haven't had an issue with responses.

dennisprudlo avatar Dec 26 '23 16:12 dennisprudlo