Make ApiUsageException return a more appropriate response code #4396
Conversation
| } | ||
|
|
||
| public ApiUsageException(Throwable cause) | ||
| { | ||
| super(cause.getMessage() == null ? cause.toString() : cause.getMessage(), cause); | ||
| this(cause.getMessage() == null ? cause.toString() : cause.getMessage(), cause); |
There was a problem hiding this comment.
I think we should just pass cause.getMessage() here, though I see this old code that I appear to have committed.
| */ | ||
| public class ApiUsageException extends RuntimeException implements SkipMothershipLogging | ||
| public class ApiUsageException extends BadRequestException implements SkipMothershipLogging |
There was a problem hiding this comment.
I think defaulting to a 422 seems best, and cases that should return a 404 can be switched to use NotFoundException.
There was a problem hiding this comment.
What a mess. A lot of usages of ApiUsageException do indeed seem equally or better served by NotFoundException or ValidationException.
I like the move to 4xx responses. I do not like using 400 BAD_REQUEST for any request that is basically well formatted but we can't handle for an explainable reason (e.g. Validation, NotFound, Unauthorized).
Separately, but not a topic for today. If I could design this all over again, I would use 200 {success:false} for most of our error cases and reserve non 200 responses for actual protocol errors (e.g. this isn't an legal end point so NOT_FOUND, rather than I can't find that rowid).
There was a problem hiding this comment.
I disagree. I think we should try to respond with the appropriate HTTP codes as intended. For example, 404:
404 doesn't indicate that the end point is illegal but rather that the resource was not found. Not finding the thing with "rowId" is precisely the case it is intended to cover.
There was a problem hiding this comment.
Thank you for taking a look at this. When I have some spare cycles, I'll take a crack at switching ApiUsageException to return a 422 and switching around some inappropriate usages.
There was a problem hiding this comment.
I'm a bit ambivalent about the 404 case. I still think that generally any interaction where "I got your request", "I understood your request", "Here's my response to your request" doesn't require out of band error handling.
There was a problem hiding this comment.
Certainly agree that the "out of band error handling" is not the ideal way to process this kind of thing.
There was a problem hiding this comment.
The latest wording for a 400 error is a little less specific to malformed requests: something that is perceived to be a client error -- compared to the original wording: The request could not be understood by the server due to malformed syntax.
422 was added as a WebDAV specific response code but has since been pulled into the core HTTP standard.
That being said, I'm inclined to move forward with 422 for ApiUsageException. That's what the GitHub API returns for requests that it understands but have some semantic error (e.g. missing a required field).
Rationale
The description on
ApiUsageExceptionindicate that it should be used when the client makes an invalid request. A500response is not appropriate for this sort of error.ExceptionUtil.handleExceptionwill detect this exception and return400status but not all code paths will pass through that.Looking at usages of
ApiUsageException; some should be404s and many others should be422. Changing them all to4xxresponses seem like a step in the right direction.Related Pull Requests
Changes
ApiUsageExceptionproduce a 400 response