dingo / api

A RESTful API package for the Laravel and Lumen frameworks.
BSD 3-Clause "New" or "Revised" License
9.33k stars 1.25k forks source link

*ResourceFailedException classes use 422 status code, should be 50x? #1688

Closed robbieaverill closed 4 years ago

robbieaverill commented 4 years ago
Q A
Bug? yes
New Feature? no
Framework Laravel
Framework version 5.8.35
Package version (v)2.3.0
PHP version 7.3.9

Actual Behaviour

When a StoreResourceFailedException, UpdateResourceFailedException, or DeleteResourceFailedException is thrown, they all use a 422 status code. This code usually indicates a validation error in the data provided by the client, while the MDN definition is more broad: "The request was well-formed but was unable to be followed due to semantic errors."

If we consider a $model->save() failure to be a semantic error then no problem, but it seems to me that these exceptions should probably use 50x status codes instead to indicate a server error.

Expected Behaviour

500 status code.

Steps to Reproduce

throw new StoreResourceFailedException('Failed to save some model');

Possible Solutions

Use 50x instead.

It's probably a breaking change for Dingo to do this though. We could safely allow the status code to be adjusted in ResourceException though, so subclasses could modify it.

Workaround is not to use Dingo exceptions and define your own. If the status code was a protected property instead of hard coded as 422, I could extend the Dingo ResourceException directly rather than replacing it.

specialtactics commented 4 years ago

I don't agree with this, 500 series http codes are generally made for web servers, and should (usually) never be thrown by your actual application.

It is commonly accepted that 422 is the most appropriate code for this scenario, and if you have a more specific scenario, there are other 4xx series codes you can use as well.

In any case, even if you personally wanted to throw a 5xx error, you still could easily do that, you don't need to use those exceptions if you don't want to - they are just there for convenience.

robbieaverill commented 4 years ago

That’s fine, thanks for the response. I was going more towards 4xx being for client errors, of which this is not one. Thanks anyway!