Add more status code handling for the REST adapter#490
Add more status code handling for the REST adapter#490jskeet wants to merge 1 commit intogoogleapis:mainfrom
Conversation
amanda-tarafa
left a comment
There was a problem hiding this comment.
LGTM as is, although with a couple of alternatives.
+1 having these be the same for all languages.
| HttpStatusCode.Ambiguous => StatusCode.Unknown, | ||
| HttpStatusCode.BadGateway => StatusCode.InvalidArgument, | ||
| HttpStatusCode.BadRequest => StatusCode.InvalidArgument, | ||
| // Note: this is ambiguous between FailedPrecondition, Aborted or Unavailable. |
There was a problem hiding this comment.
Should we go with Aborted so that clients maybe retry (Aborted is the one that's usually OK to retry).
On the other hand if it's really a FailedPrecondition (as in it's an erroneous argument) retrying won't solve it. But still client code should retry only a few times, etc.
So, I would favour giving client code the opportunity to recover by retrying.
There was a problem hiding this comment.
Java uses different error codes based on the message. (Not entirely happy with that, really.) We'll see what consensus we get. I've made a note in the code.
| HttpStatusCode.Created => StatusCode.OK, // In the 200 range | ||
| HttpStatusCode.ExpectationFailed => StatusCode.FailedPrecondition, | ||
| HttpStatusCode.Forbidden => StatusCode.PermissionDenied, | ||
| // Note: Found and Redirect are both 302 |
There was a problem hiding this comment.
Is this note here out of place, or is there something missing?
There was a problem hiding this comment.
Ah, Redirect is done, maybe add that to the note.
There was a problem hiding this comment.
I've reorganized everything - it should be clearer now.
| // Note: Found and Redirect are both 302 | ||
| HttpStatusCode.GatewayTimeout => StatusCode.DeadlineExceeded, | ||
| HttpStatusCode.Gone => StatusCode.NotFound, | ||
| HttpStatusCode.HttpVersionNotSupported => StatusCode.Unknown, |
There was a problem hiding this comment.
Should this one be InvalidArgument?
There was a problem hiding this comment.
I generally expect InvalidArgument to mean that there's something the caller can do about it - and that wouldn't be the case here.
There was a problem hiding this comment.
(Mind you, it's not the case in various other things either...)
| HttpStatusCode.HttpVersionNotSupported => StatusCode.Unknown, | ||
| HttpStatusCode.InternalServerError => StatusCode.Internal, | ||
| HttpStatusCode.LengthRequired => StatusCode.InvalidArgument, | ||
| HttpStatusCode.MethodNotAllowed => StatusCode.InvalidArgument, |
There was a problem hiding this comment.
Unimplemented? Although I'm fine with InvalidArgument as well.
|
|
||
| HttpStatusCode.Accepted => StatusCode.OK, | ||
| HttpStatusCode.Ambiguous => StatusCode.Unknown, | ||
| HttpStatusCode.BadGateway => StatusCode.InvalidArgument, |
There was a problem hiding this comment.
This doesn't seem consistent with https://grpc.github.io/grpc/core/md_doc_http-grpc-status-mapping.html
There was a problem hiding this comment.
That table seems to be losing a lot of information though, and isn't really aimed at our use case, IMO. Mapping almost everything to unknown seems like a bad idea to me.
I'm happy to revisit specific ones like BadGateway (and Unavailable seems fine for that) but I don't think we should follow that table for everything. Where there's a really clear mapping (e.g. BadRequest to InvalidArgument) I don't see why we'd want to lose that information.
There was a problem hiding this comment.
I tend to agree with you, but I'm worried about us (Google) being inconsistent. Noah also mentioned that there are more such maps internally, so it would be good to check those--again, for consistency. (I think the mapping you've put in here is certainly very reasonable)
There was a problem hiding this comment.
Right. I'll leave this PR open, and we can discuss internally.
This should really be standardized across implementations. This is still up for debate, but it's now consistent with [the Java implementation](https://github.com/googleapis/gax-java/blob/main/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonStatusCode.java) where feasible: - Java throws a separate exception on 411 - Java uses the error message to determine the RPC code for a few cases Using the error message would be feasible (as we have already read it by this point) but it feels brittle.
5ac87c6 to
76160cd
Compare
|
I've now updated this (completely changing the layout, in a way that I hope makes it easier to read). @vchudnov-g: Are you happy to circulate this to everyone else who should be aware of it, so we can make this canonical? (I'm not suggesting it's already "right" - but that we make it right, then rely on this.) |
|
@jskeet Yes, I'll spread the word on this so we can get agreement. |
|
Closing in favour of #502. |
This should really be standardized across implementations.
This is still up for debate, but it's now consistent with the Java implementation where feasible:
Using the error message would be feasible (as we have already read it by this point) but it feels brittle.