-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
closes #350: errorCode and exceptionClass to be reported always with … #369
Conversation
@@ -59,6 +56,12 @@ public CommandResult get() { | |||
} | |||
} | |||
|
|||
public CommandResult.Error getCommandResultError(String message) { | |||
Map<String, Object> fields = | |||
Map.of("errorCode", errorCode.name(), "exceptionClass", this.getClass().getSimpleName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the JsonApiException
was not including the exceptionClass
before so I added it, I guess this is also helpful information..
fields.put("errorCode", jae.getErrorCode().name()); | ||
} | ||
return new CommandResult.Error(message, fields); | ||
return ThrowableToErrorMapper.getMapperWithMessageFunction().apply(throwable, message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I adapted this, the ExceptionUtil
should not know how are CommandResult.Error
instances created, cause we have a single place for that.. Adapted for custom message..
…JsonApiException command result
fb302f2
to
661a067
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
What this PR does:
As discussed in the #350 (comment) I pushed the implementation that fixes
errorCode
missing. We still need to agree on the handling of the cause and if we do want the error array to include the cause..Which issue(s) this PR fixes:
Fixes #350
Checklist