diff --git a/changelog/@unreleased/pr-1053.v2.yml b/changelog/@unreleased/pr-1053.v2.yml new file mode 100644 index 000000000..8c53ca8af --- /dev/null +++ b/changelog/@unreleased/pr-1053.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Add safelog annotations to SerializableError and RemoteException + links: + - https://github.com/palantir/conjure-java-runtime-api/pull/1053 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 ad742f519..cbdcb531d 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 @@ -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> 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; diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java index b4dffaae9..89907b4b0 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java +++ b/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 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 f45945c1a..d99064d01 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 @@ -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; @@ -130,6 +131,6 @@ public void testArgsContainsOnlyErrorInstanceId() { .containsExactlyInAnyOrder( SafeArg.of("errorInstanceId", "errorId"), SafeArg.of("errorCode", "errorCode"), - SafeArg.of("errorName", "errorName")); + UnsafeArg.of("errorName", "errorName")); } } diff --git a/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableErrorTest.java b/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableErrorTest.java index 1286be650..791c11752 100644 --- a/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableErrorTest.java +++ b/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,11 +147,11 @@ 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())) @@ -158,6 +159,27 @@ public void testSerializationContainsRedundantParameters() throws Exception { + "\"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); + } }