From 3245fb4ed1990ec7a9aaf947a65a31ebdb493c20 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Tue, 4 Sep 2018 16:28:13 +0100 Subject: [PATCH 1/5] Backport improved SerializableError impl --- errors/build.gradle | 1 + .../api/errors/SerializableError.java | 30 +++++++------------ .../api/errors/SerializableErrorTest.java | 25 +++------------- 3 files changed, 16 insertions(+), 40 deletions(-) diff --git a/errors/build.gradle b/errors/build.gradle index 308b991b5..67fcbe65f 100644 --- a/errors/build.gradle +++ b/errors/build.gradle @@ -7,6 +7,7 @@ dependencies { compile "com.google.code.findbugs:jsr305" compile "com.palantir.safe-logging:safe-logging" compile "javax.ws.rs:javax.ws.rs-api" + compile "com.palantir.conjure.java.api:errors" testCompile project(":extras:jackson-support") testCompile "org.assertj:assertj-core" diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java index 1684bf7e6..13d98e2b3 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java @@ -24,6 +24,7 @@ import java.io.Serializable; import java.util.Map; import java.util.Objects; +import java.util.Optional; import org.immutables.value.Value; /** @@ -45,12 +46,10 @@ public abstract class SerializableError implements Serializable { * and/or name. */ @JsonProperty("errorCode") - // TODO(rfink): errorCode and exceptionClass are mutual delagates so that they can be either set independently or - // one inherits from the other. This is quite a hack and should be removed when we remove support for the - // exceptionClass field. @Value.Default public String errorCode() { - return getExceptionClass(); + return getExceptionClass().orElseThrow(() -> new IllegalStateException( + "Expected either 'errorCode' or 'exceptionClass' to be set")); } /** @@ -59,11 +58,10 @@ public String errorCode() { * error name via {@link RemoteException#getError} and typically switch&dispatch on the error code and/or name. */ @JsonProperty("errorName") - // TODO(rfink): errorName and message are mutual delagates so that they can be either set independently or one - // inherits from the other. This is quite a hack and should be removed when we remove support for the message field. @Value.Default public String errorName() { - return getMessage(); + return getMessage().orElseThrow(() -> new IllegalStateException( + "Expected either 'errorName' or 'message' to be set")); } /** @@ -86,25 +84,19 @@ public String errorInstanceId() { * @deprecated Used by the serialization-mechanism for back-compat only. Do not use. */ @Deprecated - @Value.Default - @JsonProperty("exceptionClass") + @JsonProperty(value = "exceptionClass", access = JsonProperty.Access.WRITE_ONLY) + @Value.Auxiliary @SuppressWarnings("checkstyle:designforextension") - // TODO(rfink): Remove once all error producers have switched to errorCode. - String getExceptionClass() { - return errorCode(); - } + abstract Optional getExceptionClass(); /** * @deprecated Used by the serialization-mechanism for back-compat only. Do not use. */ @Deprecated - @Value.Default - @JsonProperty("message") + @JsonProperty(value = "message", access = JsonProperty.Access.WRITE_ONLY) + @Value.Auxiliary @SuppressWarnings("checkstyle:designforextension") - // TODO(rfink): Remove once all error producers have switched to errorName. - String getMessage() { - return errorName(); - } + abstract Optional getMessage(); /** * Creates a {@link SerializableError} representation of this exception that derives from the error code and diff --git a/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java b/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java index 1f028cd26..2dfc2563d 100644 --- a/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java +++ b/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java @@ -71,13 +71,13 @@ public void forException_arg_key_collisions_just_use_the_last_one() { @Test public void testSerializationContainsRedundantParameters() throws Exception { assertThat(mapper.writeValueAsString(ERROR)) - .isEqualTo("{\"errorCode\":\"code\",\"errorName\":\"name\",\"errorInstanceId\":\"\",\"parameters\":{}," - + "\"exceptionClass\":\"code\",\"message\":\"name\"}"); + .isEqualTo( + "{\"errorCode\":\"code\",\"errorName\":\"name\",\"errorInstanceId\":\"\",\"parameters\":{}}"); assertThat(mapper.writeValueAsString( SerializableError.builder().from(ERROR).errorInstanceId("errorId").build())) .isEqualTo("{\"errorCode\":\"code\",\"errorName\":\"name\",\"errorInstanceId\":\"errorId\"" - + ",\"parameters\":{},\"exceptionClass\":\"code\",\"message\":\"name\"}"); + + ",\"parameters\":{}}"); } @Test @@ -105,24 +105,7 @@ public void testDeserializationFailsWhenNeitherErrorNameNorMessageIsSet() throws String serialized = "{\"errorCode\":\"code\"}"; assertThatThrownBy(() -> deserialize(serialized)) .isInstanceOf(IllegalStateException.class) - .hasMessage("Cannot build SerializableError, attribute initializers form cycle[errorName, message]"); - } - - @Test - public void testDeserializesWithBackupNamesOnly() throws Exception { - String serialized = "{\"message\":\"name\",\"exceptionClass\":\"code\"}"; - assertThat(deserialize(serialized)).isEqualTo(ERROR); - } - - @Test - public void testDeserializesWithWhenObsoleteExceptionClassAndMessageAreGiven() throws Exception { - String serialized = "{\"errorCode\":\"code\",\"errorName\":\"name\",\"exceptionClass\":\"obsolete-class\"," - + "\"message\":\"obsolete-message\"}"; - assertThat(deserialize(serialized)).isEqualTo(SerializableError.builder() - .from(ERROR) - .exceptionClass("obsolete-class") - .message("obsolete-message") - .build()); + .hasMessage("Expected either 'errorName' or 'message' to be set"); } private static SerializableError deserialize(String serialized) throws IOException { From 8aae29a7c1721d73d2b8a675cab341521b0b0cfd Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Tue, 4 Sep 2018 16:44:05 +0100 Subject: [PATCH 2/5] Add asConjure methods to RemoteException, SerializableError --- .../remoting/api/errors/RemoteException.java | 5 +++++ .../remoting/api/errors/SerializableError.java | 18 ++++++++++++++++++ .../api/errors/SerializableErrorTest.java | 9 +++++++++ 3 files changed, 32 insertions(+) diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/RemoteException.java b/errors/src/main/java/com/palantir/remoting/api/errors/RemoteException.java index 3f9ee0569..aa3605a41 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/RemoteException.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/RemoteException.java @@ -44,4 +44,9 @@ public RemoteException(SerializableError error, int status) { this.error = error; this.status = status; } + + /** Converts this exception to an equivalent conjure-java-api RemoteException. */ + public com.palantir.conjure.java.api.errors.RemoteException asConjure() { + return new com.palantir.conjure.java.api.errors.RemoteException(error.asConjure(), status); + } } diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java index 13d98e2b3..b004d758e 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java @@ -115,6 +115,24 @@ public static SerializableError forException(ServiceException exception) { return builder.build(); } + public com.palantir.conjure.java.api.errors.SerializableError asConjure() { + com.palantir.conjure.java.api.errors.SerializableError.Builder builder = + com.palantir.conjure.java.api.errors.SerializableError.builder(); + + if (!getExceptionClass().isPresent()) { + builder.errorCode(errorCode()); + builder.errorName(errorName()); + } else { + // this is for back-compat, old remoting2 exceptions have 'exceptionClass' and 'message' fields. + builder.exceptionClass(getExceptionClass().get()); + builder.message(getMessage().get()); + } + + builder.errorInstanceId(errorInstanceId()); + + return builder.build(); + } + // TODO(rfink): Remove once all error producers have switched to errorCode/errorName. public static final class Builder extends ImmutableSerializableError.Builder {} diff --git a/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java b/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java index 2dfc2563d..3ff2326c1 100644 --- a/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java +++ b/errors/src/test/java/com/palantir/remoting/api/errors/SerializableErrorTest.java @@ -108,6 +108,15 @@ public void testDeserializationFailsWhenNeitherErrorNameNorMessageIsSet() throws .hasMessage("Expected either 'errorName' or 'message' to be set"); } + @Test + public void asConjure_converts_json_nicely() throws IOException { + assertThat(deserialize("{\"errorCode\":\"code\",\"errorName\":\"name\"}").asConjure()) + .isEqualTo(com.palantir.conjure.java.api.errors.SerializableError.builder() + .errorCode("code") + .errorName("name") + .build()); + } + private static SerializableError deserialize(String serialized) throws IOException { return mapper.readValue(serialized, SerializableError.class); } From 1f67aa79c063a3d8e5a1e01bb68a7ce05875a7ee Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Tue, 4 Sep 2018 16:54:33 +0100 Subject: [PATCH 3/5] Update locks --- errors/versions.lock | 72 ++++++++++++++++++++++++++++++++---- service-config/versions.lock | 8 ++-- ssl-config/versions.lock | 4 +- test-utils/versions.lock | 50 ++++++++++++++++++++++++- versions.props | 2 +- 5 files changed, 119 insertions(+), 17 deletions(-) diff --git a/errors/versions.lock b/errors/versions.lock index 6de80c4ed..e65cb9f2d 100644 --- a/errors/versions.lock +++ b/errors/versions.lock @@ -13,16 +13,44 @@ ] }, "com.fasterxml.jackson.core:jackson-databind": { - "locked": "2.9.5" + "locked": "2.9.5", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] }, "com.google.code.findbugs:jsr305": { - "locked": "3.0.1" + "locked": "3.0.1", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] + }, + "com.google.errorprone:error_prone_annotations": { + "locked": "2.1.3", + "transitive": [ + "com.palantir.safe-logging:preconditions" + ] + }, + "com.palantir.conjure.java.api:errors": { + "locked": "2.0.0-rc7" + }, + "com.palantir.safe-logging:preconditions": { + "locked": "1.5.0", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] }, "com.palantir.safe-logging:safe-logging": { - "locked": "0.1.3" + "locked": "0.1.3", + "transitive": [ + "com.palantir.conjure.java.api:errors", + "com.palantir.safe-logging:preconditions" + ] }, "javax.ws.rs:javax.ws.rs-api": { - "locked": "2.0.1" + "locked": "2.0.1", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] } }, "runtime": { @@ -39,16 +67,44 @@ ] }, "com.fasterxml.jackson.core:jackson-databind": { - "locked": "2.9.5" + "locked": "2.9.5", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] }, "com.google.code.findbugs:jsr305": { - "locked": "3.0.1" + "locked": "3.0.1", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] + }, + "com.google.errorprone:error_prone_annotations": { + "locked": "2.1.3", + "transitive": [ + "com.palantir.safe-logging:preconditions" + ] + }, + "com.palantir.conjure.java.api:errors": { + "locked": "2.0.0-rc7" + }, + "com.palantir.safe-logging:preconditions": { + "locked": "1.5.0", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] }, "com.palantir.safe-logging:safe-logging": { - "locked": "0.1.3" + "locked": "0.1.3", + "transitive": [ + "com.palantir.conjure.java.api:errors", + "com.palantir.safe-logging:preconditions" + ] }, "javax.ws.rs:javax.ws.rs-api": { - "locked": "2.0.1" + "locked": "2.0.1", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] } } } \ No newline at end of file diff --git a/service-config/versions.lock b/service-config/versions.lock index 92d6b2cdf..3587c31fa 100644 --- a/service-config/versions.lock +++ b/service-config/versions.lock @@ -35,10 +35,10 @@ ] }, "com.palantir.conjure.java.api:service-config": { - "locked": "2.0.0-rc4" + "locked": "2.0.0-rc7" }, "com.palantir.conjure.java.api:ssl-config": { - "locked": "2.0.0-rc4", + "locked": "2.0.0-rc7", "transitive": [ "com.palantir.conjure.java.api:service-config", "com.palantir.remoting-api:ssl-config" @@ -109,10 +109,10 @@ ] }, "com.palantir.conjure.java.api:service-config": { - "locked": "2.0.0-rc4" + "locked": "2.0.0-rc7" }, "com.palantir.conjure.java.api:ssl-config": { - "locked": "2.0.0-rc4", + "locked": "2.0.0-rc7", "transitive": [ "com.palantir.conjure.java.api:service-config", "com.palantir.remoting-api:ssl-config" diff --git a/ssl-config/versions.lock b/ssl-config/versions.lock index 90bcaa21d..a064a9165 100644 --- a/ssl-config/versions.lock +++ b/ssl-config/versions.lock @@ -19,7 +19,7 @@ ] }, "com.palantir.conjure.java.api:ssl-config": { - "locked": "2.0.0-rc4" + "locked": "2.0.0-rc7" } }, "runtime": { @@ -42,7 +42,7 @@ ] }, "com.palantir.conjure.java.api:ssl-config": { - "locked": "2.0.0-rc4" + "locked": "2.0.0-rc7" } } } \ No newline at end of file diff --git a/test-utils/versions.lock b/test-utils/versions.lock index 3aeb21c72..b921a04c1 100644 --- a/test-utils/versions.lock +++ b/test-utils/versions.lock @@ -15,30 +15,53 @@ "com.fasterxml.jackson.core:jackson-databind": { "locked": "2.9.5", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, "com.google.code.findbugs:jsr305": { "locked": "3.0.1", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, + "com.google.errorprone:error_prone_annotations": { + "locked": "2.1.3", + "transitive": [ + "com.palantir.safe-logging:preconditions" + ] + }, "com.google.guava:guava": { "locked": "18.0" }, + "com.palantir.conjure.java.api:errors": { + "locked": "2.0.0-rc7", + "transitive": [ + "com.palantir.remoting-api:errors" + ] + }, "com.palantir.remoting-api:errors": { "project": true }, + "com.palantir.safe-logging:preconditions": { + "locked": "1.5.0", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] + }, "com.palantir.safe-logging:safe-logging": { "locked": "0.1.3", "transitive": [ - "com.palantir.remoting-api:errors" + "com.palantir.conjure.java.api:errors", + "com.palantir.remoting-api:errors", + "com.palantir.safe-logging:preconditions" ] }, "javax.ws.rs:javax.ws.rs-api": { "locked": "2.0.1", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, @@ -81,30 +104,53 @@ "com.fasterxml.jackson.core:jackson-databind": { "locked": "2.9.5", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, "com.google.code.findbugs:jsr305": { "locked": "3.0.1", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, + "com.google.errorprone:error_prone_annotations": { + "locked": "2.1.3", + "transitive": [ + "com.palantir.safe-logging:preconditions" + ] + }, "com.google.guava:guava": { "locked": "18.0" }, + "com.palantir.conjure.java.api:errors": { + "locked": "2.0.0-rc7", + "transitive": [ + "com.palantir.remoting-api:errors" + ] + }, "com.palantir.remoting-api:errors": { "project": true }, + "com.palantir.safe-logging:preconditions": { + "locked": "1.5.0", + "transitive": [ + "com.palantir.conjure.java.api:errors" + ] + }, "com.palantir.safe-logging:safe-logging": { "locked": "0.1.3", "transitive": [ - "com.palantir.remoting-api:errors" + "com.palantir.conjure.java.api:errors", + "com.palantir.remoting-api:errors", + "com.palantir.safe-logging:preconditions" ] }, "javax.ws.rs:javax.ws.rs-api": { "locked": "2.0.1", "transitive": [ + "com.palantir.conjure.java.api:errors", "com.palantir.remoting-api:errors" ] }, diff --git a/versions.props b/versions.props index fbc0e1006..b7ad1cf5e 100644 --- a/versions.props +++ b/versions.props @@ -3,7 +3,7 @@ com.google.code.findbugs:jsr305 = 3.0.1 com.google.guava:guava = 18.0 com.palantir.safe-logging:safe-logging = 0.1.3 com.palantir.tokens:auth-tokens = 3.0.0 -com.palantir.conjure.java.api:* = 2.0.0-rc4 +com.palantir.conjure.java.api:* = 2.0.0-rc7 javax.ws.rs:javax.ws.rs-api = 2.0.1 junit:junit = 4.12 org.apache.commons:commons-lang3 = 3.6 From 19e8b2163137f7334b3b8cbb66a191c55af90238 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Tue, 4 Sep 2018 16:59:19 +0100 Subject: [PATCH 4/5] asConjure methods for ErrorType and ErrorType.Code --- .../remoting/api/errors/ErrorType.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java b/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java index 9edf1abf8..e9405ea8d 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java @@ -52,6 +52,32 @@ public enum Code { Code(Integer httpErrorCode) { this.httpErrorCode = httpErrorCode; } + + public com.palantir.conjure.java.api.errors.ErrorType.Code asConjure() { + switch (this) { + case PERMISSION_DENIED: + return com.palantir.conjure.java.api.errors.ErrorType.Code.PERMISSION_DENIED; + case INVALID_ARGUMENT: + return com.palantir.conjure.java.api.errors.ErrorType.Code.INVALID_ARGUMENT; + case NOT_FOUND: + return com.palantir.conjure.java.api.errors.ErrorType.Code.NOT_FOUND; + case CONFLICT: + return com.palantir.conjure.java.api.errors.ErrorType.Code.CONFLICT; + case REQUEST_ENTITY_TOO_LARGE: + return com.palantir.conjure.java.api.errors.ErrorType.Code.REQUEST_ENTITY_TOO_LARGE; + case FAILED_PRECONDITION: + return com.palantir.conjure.java.api.errors.ErrorType.Code.FAILED_PRECONDITION; + case INTERNAL: + return com.palantir.conjure.java.api.errors.ErrorType.Code.INTERNAL; + case TIMEOUT: + return com.palantir.conjure.java.api.errors.ErrorType.Code.TIMEOUT; + case CUSTOM_CLIENT: + return com.palantir.conjure.java.api.errors.ErrorType.Code.CUSTOM_CLIENT; + case CUSTOM_SERVER: + return com.palantir.conjure.java.api.errors.ErrorType.Code.CUSTOM_SERVER; + } + throw new UnsupportedOperationException("Unable to convert to Conjure ErrorType.Code"); + } } public static final ErrorType PERMISSION_DENIED = @@ -79,6 +105,10 @@ public enum Code { /** The HTTP error code used to convey this error to HTTP clients. */ public abstract int httpErrorCode(); + public com.palantir.conjure.java.api.errors.ErrorType asConjure() { + return com.palantir.conjure.java.api.errors.ErrorType.create(code().asConjure(), name()); + } + @Value.Check final void check() { if (!ERROR_NAME_PATTERN.matcher(name()).matches()) { From 940b5b1778fb3e5d35fb6550488a90d042dfd769 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Tue, 4 Sep 2018 17:00:38 +0100 Subject: [PATCH 5/5] Thanks checkstyle --- .../main/java/com/palantir/remoting/api/errors/ErrorType.java | 3 ++- .../com/palantir/remoting/api/errors/SerializableError.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java b/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java index e9405ea8d..e2eab2d4d 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java @@ -53,6 +53,7 @@ public enum Code { this.httpErrorCode = httpErrorCode; } + @SuppressWarnings("checkstyle:CyclomaticComplexity") public com.palantir.conjure.java.api.errors.ErrorType.Code asConjure() { switch (this) { case PERMISSION_DENIED: @@ -105,7 +106,7 @@ public com.palantir.conjure.java.api.errors.ErrorType.Code asConjure() { /** The HTTP error code used to convey this error to HTTP clients. */ public abstract int httpErrorCode(); - public com.palantir.conjure.java.api.errors.ErrorType asConjure() { + public final com.palantir.conjure.java.api.errors.ErrorType asConjure() { return com.palantir.conjure.java.api.errors.ErrorType.create(code().asConjure(), name()); } diff --git a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java index b004d758e..966a67da1 100644 --- a/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java +++ b/errors/src/main/java/com/palantir/remoting/api/errors/SerializableError.java @@ -115,7 +115,7 @@ public static SerializableError forException(ServiceException exception) { return builder.build(); } - public com.palantir.conjure.java.api.errors.SerializableError asConjure() { + public final com.palantir.conjure.java.api.errors.SerializableError asConjure() { com.palantir.conjure.java.api.errors.SerializableError.Builder builder = com.palantir.conjure.java.api.errors.SerializableError.builder();