Skip to content

Indicate client-vs-server error types #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 13, 2017
Merged

Indicate client-vs-server error types #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59f3d8b
Indicate client-vs-server error types
Jun 30, 2017
69f427d
update names
Jun 30, 2017
3f5ea94
Merge branch 'develop' into rfink/client-vs-server-error
uschi2000 Jun 30, 2017
46cf392
differentiate between client and server errors more explicitly
Jul 11, 2017
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
43 changes: 22 additions & 21 deletions errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
import org.immutables.value.Value;

/**
* Represents errors by code and name. {@link ErrorType} instance are meant to be compile-time constants in
* the sense that the name should not contain information that is available at runtime only.
* Represents errors by code and name. {@link ErrorType} instance are meant to be compile-time constants in the sense
* that the name should not contain information that is available at runtime only. By convention, and in alignment with
* the HTTP specification, errors associated with a {@code 4xx} HTTP status code are client errors, and errors
* associated with a {@code 5xx} status are server errors.
*/
@Value.Immutable
@ImmutablesStyle
Expand All @@ -30,12 +32,12 @@ public abstract class ErrorType {
private static final Pattern UPPER_CAMEL_CASE = Pattern.compile("([A-Z][a-z]+)+");

public enum Code {
UNKNOWN(500),
PERMISSION_DENIED(403),
INVALID_ARGUMENT(400),
FAILED_PRECONDITION(400),
FAILED_PRECONDITION(500),
INTERNAL(500),
CUSTOM(null /* unused */);
CUSTOM_CLIENT(400),
CUSTOM_SERVER(500);

private final Integer httpErrorCode; // boxed so that we fail loudly if someone accesses CUSTOM.httpErrorCode

Expand All @@ -44,7 +46,6 @@ public enum Code {
}
}

public static final ErrorType UNKNOWN = createInternal(Code.UNKNOWN, "Unknown");
public static final ErrorType PERMISSION_DENIED = createInternal(Code.PERMISSION_DENIED, "PermissionDenied");
public static final ErrorType INVALID_ARGUMENT = createInternal(Code.INVALID_ARGUMENT, "InvalidArgument");
public static final ErrorType FAILED_PRECONDITION = createInternal(Code.FAILED_PRECONDITION, "FailedPrecondition");
Expand All @@ -70,27 +71,27 @@ final void check() {
}

/**
* Creates a new error type with the given name and HTTP error code, and error type{@link Code#CUSTOM}.
* Allowed error codes are {@code 400 BAD REQUEST} and {@code 500 INTERNAL SERVER ERROR}.
* Creates a new error type with code {@link Code#CUSTOM_CLIENT} and the given name.
*/
public static ErrorType custom(String name, int httpErrorCode) {
if (httpErrorCode != 400 && httpErrorCode != 500) {
throw new IllegalArgumentException("CUSTOM ErrorTypes must have HTTP error code 400 or 500");
}
return ImmutableErrorType.builder()
.code(Code.CUSTOM)
.name(name)
.httpErrorCode(httpErrorCode)
.build();
public static ErrorType client(String name) {
return createInternal(Code.CUSTOM_CLIENT, name);
}

/**
* Creates a new error type with code {@link Code#CUSTOM_SERVER} and the given name.
*/
public static ErrorType server(String name) {
return createInternal(Code.CUSTOM_SERVER, name);
}

/**
* Constructs an {@link ErrorType} with the given error {@link Code} and name. Cannot use the {@link
* Code#CUSTOM} error code, see {@link #custom} instead.
* Code#CUSTOM_CLIENT} or {@link Code#CUSTOM_SERVER} error codes, see {@link #client} and {@link #server} instead.
*/
public static ErrorType of(Code code, String name) {
if (code == Code.CUSTOM) {
throw new IllegalArgumentException("Use the custom() method to construct ErrorTypes with code CUSTOM");
public static ErrorType create(Code code, String name) {
if (code == Code.CUSTOM_CLIENT || code == Code.CUSTOM_SERVER) {
throw new IllegalArgumentException("Use the client() or server() methods to construct "
Copy link

Choose a reason for hiding this comment

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

So, explicitly, custom client/server errors now requires using exactly 400 and 500?

Fine with this, but slightly different than discussed. Previously had assumed we'd just keep CUSTOM as an error code, teach RemoteException to have isClientError and isServerError accessors, add these client/server methods and validate in client the numeric code was 400 range and in server the numeric code was 500 range.

Cool either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to whitelist the available HTTP codes since certain ones (e.g., 429, 503) will trigger under-the-cover retry/failover behavior. Simplest whitelist here is 400/500, but can always expand later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the receiving end can support three modes:

  • 4xx vs 5xx --> isClientError vs isServerError
  • switch on error code (PERMISSION_DENIED, INVALID_ARGUMENT, etc. a fixed list)
  • switch on error name (application-specific, e.g., DatasetNotFound)

+ "ErrorTypes with code CUSTOM_CLIENT or CUSTOM_SERVER");
}
return createInternal(code, name);
}
Expand Down
44 changes: 22 additions & 22 deletions errors/src/test/java/com/palantir/remoting/api/errors/ErrorTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,56 +25,56 @@ public final class ErrorTypeTest {

@Test
public void testNameMustBeCamelCase() throws Exception {
assertThatThrownBy(() -> ErrorType.of(ErrorType.Code.FAILED_PRECONDITION, "foo"))
assertThatThrownBy(() -> ErrorType.create(ErrorType.Code.FAILED_PRECONDITION, "foo"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("ErrorType names must be UpperCamelCase: foo");

assertThatThrownBy(() -> ErrorType.custom("foo", 400))
assertThatThrownBy(() -> ErrorType.client("foo"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("ErrorType names must be UpperCamelCase: foo");
assertThatThrownBy(() -> ErrorType.custom("fooBar", 400))
assertThatThrownBy(() -> ErrorType.client("fooBar"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("ErrorType names must be UpperCamelCase: fooBar");
assertThatThrownBy(() -> ErrorType.custom("", 400))
assertThatThrownBy(() -> ErrorType.client(""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("ErrorType names must be UpperCamelCase: ");
}

@Test
public void testDefaultErrorTypeHttpErrorCodes() throws Exception {
assertThat(ErrorType.UNKNOWN.httpErrorCode()).isEqualTo(500);
assertThat(ErrorType.PERMISSION_DENIED.httpErrorCode()).isEqualTo(403);
assertThat(ErrorType.INVALID_ARGUMENT.httpErrorCode()).isEqualTo(400);
assertThat(ErrorType.FAILED_PRECONDITION.httpErrorCode()).isEqualTo(400);
assertThat(ErrorType.FAILED_PRECONDITION.httpErrorCode()).isEqualTo(500);
assertThat(ErrorType.INTERNAL.httpErrorCode()).isEqualTo(500);
}

@Test
public void testCustomErrors() throws Exception {
ErrorType custom400 = ErrorType.custom("MyDesc", 400);
assertThat(custom400.code()).isEqualTo(ErrorType.Code.CUSTOM);
assertThat(custom400.httpErrorCode()).isEqualTo(400);
assertThat(custom400.name()).isEqualTo("MyDesc");
ErrorType customClient = ErrorType.client("MyDesc");
assertThat(customClient.code()).isEqualTo(ErrorType.Code.CUSTOM_CLIENT);
assertThat(customClient.httpErrorCode()).isEqualTo(400);
assertThat(customClient.name()).isEqualTo("MyDesc");

ErrorType custom500 = ErrorType.custom("MyDesc", 500);
assertThat(custom500.code()).isEqualTo(ErrorType.Code.CUSTOM);
assertThat(custom500.httpErrorCode()).isEqualTo(500);
assertThat(custom500.name()).isEqualTo("MyDesc");

assertThatThrownBy(() -> ErrorType.custom("MyDesc", 403))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("CUSTOM ErrorTypes must have HTTP error code 400 or 500");
ErrorType customServer = ErrorType.server("MyDesc");
assertThat(customServer.code()).isEqualTo(ErrorType.Code.CUSTOM_SERVER);
assertThat(customServer.httpErrorCode()).isEqualTo(500);
assertThat(customServer.name()).isEqualTo("MyDesc");
}

@Test
public void testCanCreateNewErrorTypes() throws Exception {
ErrorType error = ErrorType.of(ErrorType.Code.FAILED_PRECONDITION, "MyDesc");
ErrorType error = ErrorType.create(ErrorType.Code.FAILED_PRECONDITION, "MyDesc");
assertThat(error.code()).isEqualTo(ErrorType.Code.FAILED_PRECONDITION);
assertThat(error.httpErrorCode()).isEqualTo(400);
assertThat(error.httpErrorCode()).isEqualTo(500);
assertThat(error.name()).isEqualTo("MyDesc");

assertThatThrownBy(() -> ErrorType.of(ErrorType.Code.CUSTOM, "MyDesc"))
assertThatThrownBy(() -> ErrorType.create(ErrorType.Code.CUSTOM_CLIENT, "MyDesc"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Use the client() or server() methods to construct ErrorTypes with code CUSTOM_CLIENT "
+ "or CUSTOM_SERVER");
assertThatThrownBy(() -> ErrorType.create(ErrorType.Code.CUSTOM_SERVER, "MyDesc"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Use the custom() method to construct ErrorTypes with code CUSTOM");
.hasMessage("Use the client() or server() methods to construct ErrorTypes with code CUSTOM_CLIENT "
+ "or CUSTOM_SERVER");
}
}
4 changes: 2 additions & 2 deletions errors/src/test/java/com/palantir/remoting/api/errors/ServiceExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@

public final class ServiceExceptionTest {

private static final ErrorType ERROR = ErrorType.custom("MyDesc", 400);
private static final String EXPECTED_ERROR_MSG = "ServiceException: CUSTOM (MyDesc)";
private static final ErrorType ERROR = ErrorType.client("MyDesc");
private static final String EXPECTED_ERROR_MSG = "ServiceException: CUSTOM_CLIENT (MyDesc)";

@Test
public void testExceptionMessageContainsNoArgs_safeLogMessageContainsSafeArgsOnly() {
Expand Down