Translate errors from HTTP statuses #362
Conversation
This is inspired in how GRPC go approaches HTTP errors when performing GRPC requests. This provides meaningful errors in different situations. For example, AWS load balancer replies with a 504 when there are no live instances. There, a `unavailable` error is much more fitting than `internal`.
|
I didn't find where these errors are handled in Mint (if they are), so I didn't touch it. I'm not using Mint either. |
lib/grpc/rpc_error.ex
Outdated
| @@ -71,7 +71,7 @@ defmodule GRPC.RPCError do | |||
| %{error | message: error.message || Status.status_message(error.status)} | |||
| end | |||
|
|
|||
| def from_status(401), do: exception(GRPC.Status.unauthorized(), status_message(401)) | |||
| def from_status(401), do: exception(GRPC.Status.unauthenticated(), status_message(401)) | |||
There was a problem hiding this comment.
401 is unauthorized as per HTTP specs
There was a problem hiding this comment.
There was a problem hiding this comment.
I believe we should use the same verbs as HTTP if we are to do any kind of translation here
There was a problem hiding this comment.
Also, shouldn't the mint client also have similar changes?
There was a problem hiding this comment.
Hi, @polvalente. There's only so much we can do without adding new errors to the library. As the GRPC specification states only 16 error codes, what we do is only a best effort. I copied all the matching pairs from the official implementation in Go
There was a problem hiding this comment.
Hi, @polvalente. I managed to write tests, but found that adding the status handling to the mint adapter is quite complex due to current adapter structure. Can't we perhaps open an issue for a mint implementation of this feature in the future? Maybe someone with a better grasp of the mint adapter (or even future me) can take it
There was a problem hiding this comment.
I'd rather have them being feature-equivalent as one is meant to be a drop-in replacement of the other.
Can you elaborate on what made it difficult to add this kind of handling?
There was a problem hiding this comment.
Essentially, the status is received at ConnectionProcess, which is called to start the request, but the calling process reads the response from the StreamResponseProcess. Meaning I'd need to write some process coordination or rework current coordination logic. Since I don't fully understand the current architecture decisions (yet?), I found this a bit challenging.
There was a problem hiding this comment.
I believe you need to change things here https://github.com/elixir-grpc/grpc/blob/b3a7ff627ea013907a78c693016325a023a540b8/lib/grpc/client/adapters/mint.ex#L162C3-L164C6
There was a problem hiding this comment.
| client_adapters = [GRPC.Client.Adapters.Gun, GRPC.Client.Adapters.Mint] | ||
|
|
||
| for {http_code, expected_error_code} <- error_table, client_adapter <- client_adapters do | ||
| test "#{client_adapter} returns RPC Error when getting HTTP #{http_code}" do |
There was a problem hiding this comment.
Not sure where I should put these tests
|
@v0idpwn any progress here? |
|
Hi, @sleipnir. I did an attempt at implementing it on the mint adapter, but ended up hitting a wall. I'll give it another stab soonish. |
This is inspired in how GRPC go approaches HTTP errors when performing GRPC requests.
Error translation is useful in different situations. For example, AWS load balancer replies with a 504 when there are no live instances. There, a
unavailableerror is much more fitting thaninternal.For go translation table: https://github.com/grpc/grpc-go/blob/273fe145d03df516018cf4642e6987e027ffc0f5/internal/transport/http_util.go#L68