Indicate client-vs-server error types #14
Conversation
| public static final ErrorType INVALID_ARGUMENT = createInternal(Code.INVALID_ARGUMENT, "InvalidArgument"); | ||
| public static final ErrorType FAILED_PRECONDITION = createInternal(Code.FAILED_PRECONDITION, "FailedPrecondition"); | ||
| public static final ErrorType CLIENT_ERROR_INVALID_ARGUMENT = | ||
| createInternal(Code.CLIENT_ERROR_INVALID_ARGUMENT, "InvalidArgument"); |
There was a problem hiding this comment.
do you want to update the upper camel case names as well? (InvalidArgument -> ClientErrorInvalidArgument)
| public static final ErrorType CLIENT_ERROR_INVALID_ARGUMENT = | ||
| createInternal(Code.CLIENT_ERROR_INVALID_ARGUMENT, "InvalidArgument"); | ||
| public static final ErrorType SERVER_ERROR_FAILED_PRECONDITION = | ||
| createInternal(Code.SERVER_ERROR_FAILED_PRECONDITION, "FailedPrecondition"); |
There was a problem hiding this comment.
same here -- FailedPrecondition -> ServerErrorFailedPrecondition
|
ping - really really want to release http-remoting-api with RemoteException |
| CLIENT_ERROR_INVALID_ARGUMENT(400), | ||
| SERVER_ERROR_FAILED_PRECONDITION(500), | ||
| INTERNAL(500), | ||
| CUSTOM(null /* unused */); |
There was a problem hiding this comment.
is there some way for the author of a custom error to indicate that it's a client error instead of a server error?
There was a problem hiding this comment.
For custom errors:
- the error code is always CUSTOM
- the error name is any CamelCase string (so it can include the prefix ServerError... of course)
- the HTTP error code is either 400 (clientish) or 500 (serverish)
There was a problem hiding this comment.
happy with that, @markelliot ?
There was a problem hiding this comment.
It seems like it'd be useful to be able to pattern match on client and server errors more explicitly
There was a problem hiding this comment.
What do you mean? Looks like this PR suggest prefixing with "ClientError" or "ServerError" to differentiate / be able to pattern match
There was a problem hiding this comment.
I generally don't really want consumer of this to worry about HTTP codes.
There was a problem hiding this comment.
Yes, plan if to have such scaffolding, but align pretty closely with the error codes. If you feel strongly that client-vs-server is the most important top-level distinction, then I'm happy to add this in a way that is easy to digest for clients.
There was a problem hiding this comment.
I guess part of my flag is that if you don't want the consumer to worry about HTTP codes we probably ought to make it reasonably transparent to the producer, too, and that a producer should most likely say something like clientError(String name, int httpErrorCode) instead of custom(String name, int httpErrorCode), and we'll forcibly check that a clientError is a 400 range code, and a serverError is a 500 range code.
There was a problem hiding this comment.
here's another attempt.
|
ping. this is blocking releasing RemoteException which I want on a daily basis |
|
Agreed.
…On Thu, Jul 6, 2017 at 10:42 AM Mark Elliot ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In errors/src/main/java/com/palantir/remoting/api/errors/ErrorType.java
<https://github.com/palantir/http-remoting-api/pull/14#discussion_r125967568>
:
> @@ -32,8 +32,8 @@
public enum Code {
UNKNOWN(500),
PERMISSION_DENIED(403),
- INVALID_ARGUMENT(400),
- FAILED_PRECONDITION(400),
+ CLIENT_ERROR_INVALID_ARGUMENT(400),
+ SERVER_ERROR_FAILED_PRECONDITION(500),
INTERNAL(500),
CUSTOM(null /* unused */);
I guess part of my flag is that if you don't want the consumer to worry
about HTTP codes we probably ought to make it reasonably transparent to the
producer, too, and that a producer should most likely say something like clientError(String
name, int httpErrorCode) instead of custom(String name, int httpErrorCode),
and we'll forcibly check that a clientError is a 400 range code, and a
serverError is a 500 range code.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/palantir/http-remoting-api/pull/14#discussion_r125967568>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGOdwRvDaLaGin0-vsLwdEjK0QTVq4Phks5sLRxwgaJpZM4OK00d>
.
|
|
@uschi2000 all good? |
|
@uschi2000 ping |
| 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 " |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
@uschi2000 Why is FAILED_PRECONDITION changed to a server error? Isn't the standard 412? |
|
this is explicitly for a failing server-side precondition. |
|
Is there a standard for which error type to use for a client side precondition check failure? INVALID_ARGUMENT makes sense in most cases? |
|
Yep, INVALID_ARG is the one.
|
No description provided.