415 for invalid Content-Type header, remove redundant Token Filter #1298
Conversation
| if (throwable instanceof NotSupportedException | ||
| && throwable | ||
| .getMessage() | ||
| .contains("The content-type header value did not match the value")) { |
There was a problem hiding this comment.
Is this check needed? Wouldn't we always convert this exception to 415?
There was a problem hiding this comment.
Yes, let me change that.
I was just try to make sure.
| // validate the Content-Type header, 415 if failed | ||
| return ErrorCode.INVALID_CONTENT_TYPE_HEADER | ||
| .toApiException() | ||
| .getCommandResultError(Response.Status.UNSUPPORTED_MEDIA_TYPE); |
There was a problem hiding this comment.
It's too bad actual incorrect Content-Type seems not available from NotSupportedException -- if it was, would be good to include in exception message.
There was a problem hiding this comment.
Yeah, that is not included, unfortunately.
| @@ -42,6 +43,12 @@ public RestResponse<CommandResult> webApplicationExceptionMapper(WebApplicationE | |||
| if (e instanceof NotFoundException) { | |||
| return RestResponse.status(RestResponse.Status.NOT_FOUND, commandResult); | |||
| } | |||
| // Return 415 for invalid Content-Type | |||
| if (e instanceof NotSupportedException | |||
There was a problem hiding this comment.
It's odd we have to do this in 2 different places -- do you know which one is necessary for the test to pass? Is it possible to test both handlers?
There was a problem hiding this comment.
This is because our current Exception structure design.
So this failure happens before our command processing, and it is captured by this WebApplicationExceptionMapper, but we still need want to convert the Throwable to CommandResult, so the same logic goes to ThrowableToErrorMapper.
Here we just want the status 415 to be added.
Checklist