diff --git a/changelog/@unreleased/pr-633.v2.yml b/changelog/@unreleased/pr-633.v2.yml new file mode 100644 index 000000000..c4f22ced8 --- /dev/null +++ b/changelog/@unreleased/pr-633.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: RemoteException.getMessage (unsafe message) includes SerializableError parameters. RemoteException.getLogMessage (safe) is not impacted. + links: + - https://github.com/palantir/conjure-java-runtime-api/pull/633 diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java index 205be7334..83551c8d3 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java @@ -26,11 +26,13 @@ public final class RemoteException extends RuntimeException implements SafeLoggable { private static final long serialVersionUID = 1L; - private final String message; private final String stableMessage; private final SerializableError error; private final int status; private final List> args; + // Lazily evaluated based on the stableMessage, errorInstanceId, and args. + @SuppressWarnings("MutableException") + private String unsafeMessage; /** Returns the error thrown by a remote process which caused an RPC call to fail. */ public SerializableError getError() { @@ -44,9 +46,8 @@ public int getStatus() { public RemoteException(SerializableError error, int status) { this.stableMessage = error.errorCode().equals(error.errorName()) - ? String.format("RemoteException: %s", error.errorCode()) - : String.format("RemoteException: %s (%s)", error.errorCode(), error.errorName()); - this.message = this.stableMessage + " with instance ID " + error.errorInstanceId(); + ? "RemoteException: " + error.errorCode() + : "RemoteException: " + error.errorCode() + " (" + error.errorName() + ")"; this.error = error; this.status = status; this.args = Collections.singletonList(SafeArg.of("errorInstanceId", error.errorInstanceId())); @@ -54,7 +55,31 @@ public RemoteException(SerializableError error, int status) { @Override public String getMessage() { - return message; + // This field is not used in most environments so the cost of computation may be avoided. + String messageValue = unsafeMessage; + if (messageValue == null) { + messageValue = renderUnsafeMessage(); + unsafeMessage = messageValue; + } + return messageValue; + } + + private String renderUnsafeMessage() { + StringBuilder builder = new StringBuilder() + .append(stableMessage) + .append(" with instance ID ") + .append(error.errorInstanceId()); + if (!error.parameters().isEmpty()) { + builder.append(": {"); + error.parameters() + .forEach((name, unsafeValue) -> + builder.append(name).append('=').append(unsafeValue).append(", ")); + // remove the trailing space + builder.setLength(builder.length() - 1); + // replace the trailing comma with a close curly brace + builder.setCharAt(builder.length() - 1, '}'); + } + return builder.toString(); } @Override diff --git a/errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java b/errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java index 8cc9994df..8d5e403d6 100644 --- a/errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java +++ b/errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java @@ -47,7 +47,7 @@ public void testJavaSerialization() { } @Test - public void testSuperMessage() { + public void testUnsafeMessage_differentCodeAndName() { SerializableError error = new SerializableError.Builder() .errorCode("errorCode") .errorName("errorName") @@ -55,8 +55,11 @@ public void testSuperMessage() { .build(); assertThat(new RemoteException(error, 500).getMessage()) .isEqualTo("RemoteException: errorCode (errorName) with instance ID errorId"); + } - error = new SerializableError.Builder() + @Test + public void testUnsafeMessage_sameCodeAndName() { + SerializableError error = new SerializableError.Builder() .errorCode("errorCode") .errorName("errorCode") .errorInstanceId("errorId") @@ -65,6 +68,31 @@ public void testSuperMessage() { .isEqualTo("RemoteException: errorCode with instance ID errorId"); } + @Test + public void testUnsafeMessage_oneParameter() { + SerializableError error = new SerializableError.Builder() + .errorCode("errorCode") + .errorName("errorName") + .errorInstanceId("errorId") + .putParameters("foo", "bar") + .build(); + assertThat(new RemoteException(error, 500).getMessage()) + .isEqualTo("RemoteException: errorCode (errorName) with instance ID errorId: {foo=bar}"); + } + + @Test + public void testUnsafeMessage_multipleParameters() { + SerializableError error = new SerializableError.Builder() + .errorCode("errorCode") + .errorName("errorName") + .errorInstanceId("errorId") + .putParameters("a", "b") + .putParameters("c", "d") + .build(); + assertThat(new RemoteException(error, 500).getMessage()) + .isEqualTo("RemoteException: errorCode (errorName) with instance ID errorId: {a=b, c=d}"); + } + @Test public void testLogMessageMessage() { SerializableError error = new SerializableError.Builder() @@ -77,7 +105,19 @@ public void testLogMessageMessage() { } @Test - public void testArgsIsEmpty() { + public void testLogMessageMessageDoesNotIncludeParameters() { + SerializableError error = new SerializableError.Builder() + .errorCode("errorCode") + .errorName("errorName") + .errorInstanceId("errorId") + .putParameters("param", "value") + .build(); + RemoteException remoteException = new RemoteException(error, 500); + assertThat(remoteException.getLogMessage()).isEqualTo("RemoteException: errorCode (errorName)"); + } + + @Test + public void testArgsContainsOnlyErrorInstanceId() { RemoteException remoteException = new RemoteException( new SerializableError.Builder() .errorCode("errorCode")