Skip to content

Translate errors from HTTP statuses #362

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Translate errors from HTTP statuses #362

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7904b1
Translate errors from HTTP statuses
v0idpwn May 3, 2024
ba0d3d3
fixfix
v0idpwn May 3, 2024
ed52bce
fix: unauthorized -> unauthenticated
v0idpwn May 3, 2024
26fee70
Add test
v0idpwn May 4, 2024
cf8763c
s/from_status/from_http_status/
v0idpwn May 4, 2024
0280a41
format
v0idpwn May 4, 2024
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
12 changes: 2 additions & 10 deletions lib/grpc/client/adapters/gun.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,7 @@ defmodule GRPC.Client.Adapters.Gun do
)}
end
else
{:error,
GRPC.RPCError.exception(
GRPC.Status.internal(),
"status got is #{status} instead of 200"
)}
{:error, GRPC.RPCError.from_status(status)}
end

{:response, :nofin, status, headers} ->
Expand All @@ -266,11 +262,7 @@ defmodule GRPC.Client.Adapters.Gun do
{:response, headers, :nofin}
end
else
{:error,
GRPC.RPCError.exception(
GRPC.Status.internal(),
"status got is #{status} instead of 200"
)}
{:error, GRPC.RPCError.from_status(status)}
end

{:data, :fin, data} ->
Expand Down
17 changes: 17 additions & 0 deletions lib/grpc/rpc_error.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,23 @@ defmodule GRPC.RPCError do
%{error | message: error.message || Status.status_message(error.status)}
end

def from_status(401), do: exception(GRPC.Status.unauthenticated(), status_message(401))
Copy link
Contributor

Choose a reason for hiding this comment

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

401 is unauthorized as per HTTP specs

Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.mozilla.org/pt-BR/docs/Web/HTTP/Status/401

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should use the same verbs as HTTP if we are to do any kind of translation here

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, shouldn't the mint client also have similar changes?

Copy link
Contributor Author

@v0idpwn v0idpwn May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@v0idpwn v0idpwn May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you need to change things here https://github.com/elixir-grpc/grpc/blob/b3a7ff627ea013907a78c693016325a023a540b8/lib/grpc/client/adapters/mint.ex#L162C3-L164C6

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe here: https://github.com/elixir-grpc/grpc/blob/b3a7ff627ea013907a78c693016325a023a540b8/lib/grpc/client/adapters/mint.ex#L162C3-L164C6

def from_status(403), do: exception(GRPC.Status.permission_denied(), status_message(403))
def from_status(404), do: exception(GRPC.Status.unimplemented(), status_message(404))
def from_status(429), do: exception(GRPC.Status.unavailable(), status_message(429))

def from_status(status) when status in [502, 503, 504] do
exception(GRPC.Status.unavailable(), status_message(status))
end

def from_status(status) do
exception(GRPC.Status.internal(), status_message(status))
end

defp status_message(status) do
"got http status #{status} instead of 200"
end

defp parse_args([], acc), do: acc

defp parse_args([{:status, status} | t], acc) when is_integer(status) do
Expand Down