Use canonical status code mapping from HTTP to RPC#502
Use canonical status code mapping from HTTP to RPC#502jskeet merged 1 commit intogoogleapis:mainfrom
Conversation
| HttpStatusCode.Conflict => StatusCode.FailedPrecondition, | ||
| // TODO: Others! | ||
| // All 2xx responses map to OK | ||
| 2 => StatusCode.OK, |
There was a problem hiding this comment.
+1 to this.
The Apiary libraries marks all 2xx as success, if we didn't do this here, we could get the exact same operation succeed in Apiary but "fail" in REGAPIC.
| _ => StatusCode.Internal | ||
| }, | ||
|
|
||
| // Anything else (including all 1xx and 3xx) maps to Unknown |
There was a problem hiding this comment.
We handle redirects in Apiary.
There was a problem hiding this comment.
Hmm. Do we know if they're used? (I can't remember offhand.)
Of course, the HttpClient we use under the hood could be configured to do that automatically - I suspect we don't want to do anything different at the CallInvoker level (where this is) - but we should consider it carefully. Thanks for bringing it up.
There was a problem hiding this comment.
I have no idea if they are used, tbh. I've never seen an actual redirect while debugging code, but of course, I've onle ever debugged a low percent of operations. Maybe we can at least chat to the Compute API folk and ask, or maybe to ESF?
There was a problem hiding this comment.
Any progress on the redirect issue? I would assume we'd want to handle it like Apiary and just follow the redirect. Ideally, that means HttpClient would do this and redirection codes would not show up here in practice....
There was a problem hiding this comment.
Sorry, I haven't chased the redirect issue at all. We could definitely "ask" HttpClient to do it for us, although I don't know whether it gives up after a few redirects.
There was a problem hiding this comment.
I suggest we can merge this change without worrying about redirects for now, but I'll file an issue to make sure we remember.
There was a problem hiding this comment.
Just for completeness, this is being tracked in #511.
This is now *not* 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) in some cases.
34d0e30 to
42d51cd
Compare
|
This has now been agreed internally, and is ready to merge if you're happy, @amanda-tarafa |
| _ => StatusCode.Internal | ||
| }, | ||
|
|
||
| // Anything else (including all 1xx and 3xx) maps to Unknown |
There was a problem hiding this comment.
Just for completeness, this is being tracked in #511.
This is now not consistent with the Java implementation in some cases.
This is an alternative to #490