Refactoring to allow fixing #1216 by making JsonApiException accept HTTP status code, propagate
#1217
Conversation
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/CommandResult.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/CommandResult.java
Outdated
Show resolved
Hide resolved
| @@ -23,6 +23,8 @@ public class JsonApiException extends RuntimeException implements Supplier<Comma | |||
|
|
|||
| private final ErrorCode errorCode; | |||
|
|
|||
| private final Response.Status httpStatus; | |||
There was a problem hiding this comment.
Will try propagating HTTP response code along with the exception so it's neither lost nor need to be carried separately.
Yeah. If only it actually worked... having hard time figuring out where the status code is lost. :-/ |
tatu-at-datastax
changed the title
tatu-at-datastax
changed the title
JsonApiException accept HTTP status code, propagate
| @@ -188,11 +190,21 @@ public JsonApiException toApiException(String format, Object... args) { | |||
| return new JsonApiException(this, message + ": " + String.format(format, args)); | |||
| } | |||
|
|
|||
| public JsonApiException toApiException( | |||
There was a problem hiding this comment.
Factory method for passing HTTP status code along: needs to be the first argument due to varargs needed to pass format String arguments.
| public JsonApiException toApiException(Throwable cause, String format, Object... args) { | ||
| return new JsonApiException(this, message + ": " + String.format(format, args), cause); | ||
| } | ||
|
|
||
| public JsonApiException toApiException() { | ||
| return new JsonApiException(this, message); | ||
| } | ||
|
|
||
| public JsonApiException toApiException(Response.Status httpStatus) { |
There was a problem hiding this comment.
Alternate factory method when the default non-templatted ErrorCode message is used.
| @@ -219,7 +223,7 @@ private static CommandResult.Error handleJsonProcessingException( | |||
| .toApiException( | |||
| "\"%s\" not one of known fields (%s) at '%s'", | |||
| upe.getPropertyName(), knownDesc, upe.getPathReference()) | |||
| .getCommandResultError(Response.Status.OK); | |||
| .getCommandResultError(); | |||
There was a problem hiding this comment.
so this defaults to 200?
There was a problem hiding this comment.
Yes; it uses Response.Status from JsonApiException, which in turn defaults to Response.Status.OK (unless constructor defines something else: in this case it's called via ErrorCode.toApiException() without specifying override).
Code is quite complicated with back and forth; here I just want to remove use of getCommandResultError() override and use one associated with JsonApiException
What this PR does:
Changes plumbing wrt
ErrorCodeandJsonApiExceptionto allow passing HTTP response status along with exception, to indicate status other than 200 as necessary.Meant to help resolve #1216 but does not quite achieve that.
Which issue(s) this PR fixes:
N/A
Checklist