ServiceException#getLogMessage does not include args (#12)

Author: uschi2000

errors/src/main/java/com/palantir/remoting/api/errors/ServiceException.java

@@ -32,6 +32,7 @@ public final class ServiceException extends RuntimeException implements SafeLogg
 
     private final String errorId = UUID.randomUUID().toString();
     private final String safeMessage;
+    private final String noArgsMessage;
 
     /**
      * Creates a new exception for the given error. All {@link com.palantir.logsafe.SafeArg safe} parameters are
@@ -48,12 +49,13 @@ public ServiceException(ErrorType errorType, @Nullable Throwable cause, Arg<?>..
 
     private ServiceException(ErrorType errorType, @Nullable Throwable cause, List<Arg<?>> args) {
         // TODO(rfink): Memoize formatting?
-        super(safeMessage(errorType, args), cause);
+        super(renderSafeMessage(errorType, args), cause);
 
         this.errorType = errorType;
         // Note that instantiators cannot mutate List<> args since it comes through copyToList in all code paths.
         this.args = Collections.unmodifiableList(args);
-        this.safeMessage = safeMessage(errorType, args);
+        this.safeMessage = renderSafeMessage(errorType, args);
+        this.noArgsMessage = renderNoArgsMessage(errorType);
     }
 
     /** The {@link ErrorType} that gave rise to this exception. */
@@ -68,18 +70,18 @@ public String getErrorId() {
 
     @Override
     public String getMessage() {
+        // Including safe args here since any logger not configured with safe-logging will log this message.
         return safeMessage;
     }
 
     @Override
     public String getLogMessage() {
-        return safeMessage;
+        // Not returning safe args here since the safe-logging framework will log this message + args explicitly.
+        return noArgsMessage;
     }
 
-    private static String safeMessage(ErrorType errorType, List<Arg<?>> args) {
-        String message = errorType.code().name().equals(errorType.name())
-                ? String.format("ServiceException: %s", errorType.code())
-                : String.format("ServiceException: %s (%s)", errorType.code(), errorType.name());
+    private static String renderSafeMessage(ErrorType errorType, List<Arg<?>> args) {
+        String message = renderNoArgsMessage(errorType);
 
         if (args.isEmpty()) {
             return message;
@@ -102,6 +104,12 @@ private static String safeMessage(ErrorType errorType, List<Arg<?>> args) {
         return builder.toString();
     }
 
+    private static String renderNoArgsMessage(ErrorType errorType) {
+        return errorType.code().name().equals(errorType.name())
+                ? String.format("ServiceException: %s", errorType.code())
+                : String.format("ServiceException: %s (%s)", errorType.code(), errorType.name());
+    }
+
     @Override
     public List<Arg<?>> getArgs() {
         return args;

errors/src/test/java/com/palantir/remoting/api/errors/ServiceExceptionTest.java

@@ -30,16 +30,15 @@ public final class ServiceExceptionTest {
     private static final String EXPECTED_ERROR_MSG = "ServiceException: CUSTOM (MyDesc)";
 
     @Test
-    public void testExceptionMessagesContainsSafeArgsOnly() {
+    public void testExceptionMessageContainsNoArgs_safeLogMessageContainsSafeArgsOnly() {
         Arg<?>[] args = {
                 SafeArg.of("arg1", "foo"),
                 UnsafeArg.of("arg2", 2),
                 UnsafeArg.of("arg3", null)};
         ServiceException ex = new ServiceException(ERROR, args);
 
-        String expectedMessage = EXPECTED_ERROR_MSG + ": {arg1=foo}";
-        assertThat(ex.getMessage()).isEqualTo(expectedMessage);
-        assertThat(ex.getLogMessage()).isEqualTo(expectedMessage);
+        assertThat(ex.getLogMessage()).isEqualTo(EXPECTED_ERROR_MSG);
+        assertThat(ex.getMessage()).isEqualTo(EXPECTED_ERROR_MSG + ": {arg1=foo}");
     }
 
     @Test