Remove errorType from ServiceException args #133
Conversation
| * Get Args of this service exception excluding injected/generated Args like errorType. | ||
| * @return parameters passed to this ServiceException at construction time | ||
| */ | ||
| public List<Arg<?>> getParameters() { |
There was a problem hiding this comment.
was this released? is someone using this?
There was a problem hiding this comment.
This was released, but it is exactly the same as getArgs. But we probably need to keep it to avoid breaks. I'll keep it and mark it as deprecated.
There was a problem hiding this comment.
not probably, but absolutely.
| * Get Args of this service exception excluding injected/generated Args like errorType. | ||
| * @return parameters passed to this ServiceException at construction time | ||
| */ | ||
| public List<Arg<?>> getParameters() { |
There was a problem hiding this comment.
it's a break to remove this, can you deprecate it instead and just have it return args?
| isNotNull(); | ||
|
|
||
| // fetch getParameters here to exclude any generated/injected args like 'errorType' | ||
| AssertableArgs actualArgs = new AssertableArgs(actual.getParameters()); |
There was a problem hiding this comment.
keep this as it is, or we're going to have issues with mistmatching errors / test-utils versions once again :(
|
|
||
| /** A {@link ServiceException} thrown in server-side code to indicate server-side {@link ErrorType error states}. */ | ||
| public final class ServiceException extends RuntimeException implements SafeLoggable { | ||
| public static final String ERROR_TYPE_ARG_NAME = "errorType"; |
There was a problem hiding this comment.
also an API break, but I don't think anyone should have relied on this so should be fine (and luckily test-utils didn't rely on it either)
3 participants
Revert of #92
Now that palantir/conjure-java-runtime#827 has merged and we are logging the error name in the exception mapper, we no longer need the injected arg here.
With the injected args, the
ServiceExceptionMapperlogs would have both aerrorNameparam and athrowable0_errorTypeparam which is confusing and unnecessary.Also note that this parameter has never been logged properly (see #130), so this does not cause a regression in capability.