Add safelog annotations to SerializableError and RemoteException

Author: ash211

errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java

@@ -19,6 +19,8 @@
 import com.palantir.logsafe.Arg;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.SafeLoggable;
+import com.palantir.logsafe.Unsafe;
+import com.palantir.logsafe.UnsafeArg;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -30,12 +32,15 @@ public final class RemoteException extends RuntimeException implements SafeLogga
     private static final String ERROR_CODE = "errorCode";
     private static final String ERROR_NAME = "errorName";
 
+    @Unsafe // because errorName is unsafe
     private final String stableMessage;
+
     private final SerializableError error;
     private final int status;
     private final List<Arg<?>> args;
     // Lazily evaluated based on the stableMessage, errorInstanceId, and args.
     @SuppressWarnings("MutableException")
+    @Unsafe
     private String unsafeMessage;
 
     /** Returns the error thrown by a remote process which caused an RPC call to fail. */
@@ -56,10 +61,11 @@ public RemoteException(SerializableError error, int status) {
         this.status = status;
         this.args = Collections.unmodifiableList(Arrays.asList(
                 SafeArg.of(ERROR_INSTANCE_ID, error.errorInstanceId()),
-                SafeArg.of(ERROR_NAME, error.errorName()),
+                UnsafeArg.of(ERROR_NAME, error.errorName()),
                 SafeArg.of(ERROR_CODE, error.errorCode())));
     }
 
+    @Unsafe
     @Override
     public String getMessage() {
         // This field is not used in most environments so the cost of computation may be avoided.
@@ -71,6 +77,7 @@ public String getMessage() {
         return messageValue;
     }
 
+    @Unsafe
     private String renderUnsafeMessage() {
         StringBuilder builder = new StringBuilder()
                 .append(stableMessage)
@@ -89,6 +96,7 @@ private String renderUnsafeMessage() {
         return builder.toString();
     }
 
+    @Unsafe
     @Override
     public String getLogMessage() {
         return stableMessage;

errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java

@@ -21,6 +21,8 @@
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 import com.palantir.logsafe.Arg;
+import com.palantir.logsafe.Safe;
+import com.palantir.logsafe.Unsafe;
 import com.palantir.logsafe.exceptions.SafeIllegalStateException;
 import java.io.Serializable;
 import java.util.Map;
@@ -34,6 +36,7 @@
  * transport errors through RPC channels such as HTTP responses.
  */
 // Automatically suppressed to unblock enforcement in new code
+@Unsafe
 @SuppressWarnings("ImmutablesStyle")
 @JsonDeserialize(builder = SerializableError.Builder.class)
 @JsonSerialize(as = ImmutableSerializableError.class)
@@ -48,6 +51,7 @@ public abstract class SerializableError implements Serializable {
      * the server-side error code via {@link RemoteException#getError} and typically switch&dispatch on the error code
      * and/or name.
      */
+    @Safe
     @JsonProperty("errorCode")
     @Value.Default
     public String errorCode() {
@@ -61,6 +65,7 @@ public String errorCode() {
      * {@link ErrorType#name} and is part of the service's API surface. Clients are given access to the service-side
      * error name via {@link RemoteException#getError} and typically switch&dispatch on the error code and/or name.
      */
+    @Unsafe // because message is unsafe
     @JsonProperty("errorName")
     @Value.Default
     public String errorName() {
@@ -74,6 +79,7 @@ public String errorName() {
      * {@link #errorName}, the {@link #errorInstanceId} identifies a specific occurrence of an error, not a class of
      * errors. By convention, this field is a UUID.
      */
+    @Safe
     @JsonProperty("errorInstanceId")
     @Value.Default
     @SuppressWarnings("checkstyle:designforextension")
@@ -89,6 +95,7 @@ public String errorInstanceId() {
      *
      * @deprecated Used by the serialization-mechanism for back-compat only. Do not use.
      */
+    @Safe
     @Deprecated
     @JsonProperty(value = "exceptionClass", access = JsonProperty.Access.WRITE_ONLY)
     @Value.Auxiliary
@@ -100,6 +107,7 @@ public String errorInstanceId() {
      *
      * @deprecated Used by the serialization-mechanism for back-compat only. Do not use.
      */
+    @Unsafe
     @Deprecated
     @JsonProperty(value = "message", access = JsonProperty.Access.WRITE_ONLY)
     @Value.Auxiliary

errors/src/test/java/com/palantir/conjure/java/api/errors/RemoteExceptionTest.java

@@ -19,6 +19,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import com.palantir.logsafe.SafeArg;
+import com.palantir.logsafe.UnsafeArg;
 import org.apache.commons.lang3.SerializationUtils;
 import org.junit.jupiter.api.Test;
 
@@ -129,7 +130,7 @@ public void testArgsContainsOnlyErrorInstanceId() {
         assertThat(remoteException.getArgs())
                 .containsExactlyInAnyOrder(
                         SafeArg.of("errorInstanceId", "errorId"),
-                        SafeArg.of("errorCode", "errorCode"),
+                        UnsafeArg.of("errorCode", "errorCode"),
                         SafeArg.of("errorName", "errorName"));
     }
 }

errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableErrorTest.java

@@ -19,6 +19,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.palantir.conjure.java.api.ext.jackson.ObjectMappers;
 import com.palantir.logsafe.SafeArg;
@@ -146,18 +147,39 @@ public void forException_optionalArgValue_serializesWithToString() {
 
     @Test
     public void testSerializationContainsRedundantParameters() throws Exception {
-        assertThat(mapper.writeValueAsString(ERROR))
+        assertThat(serialize(ERROR))
                 .isEqualTo("{\"errorCode\":\"PERMISSION_DENIED\",\"errorName\":\"Product:SomethingBroke\","
                         + "\"errorInstanceId\":\"\",\"parameters\":{}}");
 
-        assertThat(mapper.writeValueAsString(SerializableError.builder()
+        assertThat(serialize(SerializableError.builder()
                         .from(ERROR)
                         .errorInstanceId("errorId")
                         .build()))
                 .isEqualTo("{\"errorCode\":\"PERMISSION_DENIED\",\"errorName\":\"Product:SomethingBroke\","
                         + "\"errorInstanceId\":\"errorId\",\"parameters\":{}}");
     }
 
+    @Test
+    public void testSerDeRoundTripDropsMessage() throws Exception {
+        SerializableError error = SerializableError.builder()
+                .from(ERROR)
+                .message("the secret is 42")
+                .build();
+
+        assertThat(error.getMessage()).hasValue("the secret is 42");
+        assertThat(deserialize(serialize(error)).getMessage()).isEmpty();
+    }
+
+    @Test
+    public void testLegacyMessageUsedAsErrorNameWhenNoErrorNameIsSet() {
+        SerializableError error = SerializableError.builder()
+                .errorCode("errorCode")
+                .message("the secret is 42")
+                .build();
+
+        assertThat(error.errorName()).isEqualTo("the secret is 42");
+    }
+
     @Test
     public void testDeserializesWhenRedundantParamerersAreGiven() throws Exception {
         String serialized =
@@ -194,4 +216,8 @@ public void testDeserializationFailsWhenNeitherErrorNameNorMessageIsSet() throws
     private static SerializableError deserialize(String serialized) throws IOException {
         return mapper.readValue(serialized, SerializableError.class);
     }
+
+    private static String serialize(SerializableError error) throws JsonProcessingException {
+        return mapper.writeValueAsString(error);
+    }
 }