Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions errors/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,33 @@ public enum Code {
Code(Integer httpErrorCode) {
this.httpErrorCode = httpErrorCode;
}

@SuppressWarnings("checkstyle:CyclomaticComplexity")
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 =
Expand Down Expand Up @@ -79,6 +106,10 @@ public enum Code {
/** The HTTP error code used to convey this error to HTTP clients. */
public abstract int httpErrorCode();

public final 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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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"));
}

/**
Expand All @@ -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"));
}

/**
Expand All @@ -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<String> 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<String> getMessage();

/**
* Creates a {@link SerializableError} representation of this exception that derives from the error code and
Expand All @@ -123,6 +115,24 @@ public static SerializableError forException(ServiceException exception) {
return builder.build();
}

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();

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 {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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\":{}}");
Copy link
Contributor

@qinfchen qinfchen Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a break, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test just captures exhaustive behaviour, but I don't think this is a real use case. In practise, as long as you can deserialize both the old remoting2 and newer remoting3 JSON, I think it's fine.


assertThat(mapper.writeValueAsString(
SerializableError.builder().from(ERROR).errorInstanceId("errorId").build()))
.isEqualTo("{\"errorCode\":\"code\",\"errorName\":\"name\",\"errorInstanceId\":\"errorId\""
+ ",\"parameters\":{},\"exceptionClass\":\"code\",\"message\":\"name\"}");
+ ",\"parameters\":{}}");
}

@Test
Expand Down Expand Up @@ -105,24 +105,16 @@ public void testDeserializationFailsWhenNeitherErrorNameNorMessageIsSet() throws
String serialized = "{\"errorCode\":\"code\"}";
assertThatThrownBy(() -> deserialize(serialized))
.isInstanceOf(IllegalStateException.class)
.hasMessage("Cannot build SerializableError, attribute initializers form cycle[errorName, message]");
.hasMessage("Expected either 'errorName' or 'message' to be set");
}

@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());
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 {
Expand Down
72 changes: 64 additions & 8 deletions errors/versions.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -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"
]
}
}
}
8 changes: 4 additions & 4 deletions service-config/versions.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions ssl-config/versions.lock
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
]
},
"com.palantir.conjure.java.api:ssl-config": {
"locked": "2.0.0-rc4"
"locked": "2.0.0-rc7"
}
},
"runtime": {
Expand All @@ -42,7 +42,7 @@
]
},
"com.palantir.conjure.java.api:ssl-config": {
"locked": "2.0.0-rc4"
"locked": "2.0.0-rc7"
}
}
}
Loading