Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
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.

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

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
24 changes: 24 additions & 0 deletions test/grpc/integration/stub_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,30 @@ defmodule GRPC.Integration.StubTest do
end)
end

error_table = [
{401, GRPC.Status.unauthenticated()},
{403, GRPC.Status.permission_denied()},
{404, GRPC.Status.unimplemented()},
{429, GRPC.Status.unavailable()},
{502, GRPC.Status.unavailable()},
{503, GRPC.Status.unavailable()},
{504, GRPC.Status.unavailable()},
# General case
{518, GRPC.Status.internal()}
]

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where I should put these tests

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine

run_error_server(unquote(http_code), fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}", adapter: unquote(client_adapter))
req = %Helloworld.HelloRequest{name: "GRPC"}
{:error, %GRPC.RPCError{status: unquote(expected_error_code)}} = Helloworld.Greeter.Stub.say_hello(channel, req)
end)
end
end

test "you can disconnect stubs" do
run_server(HelloServer, fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}")
Expand Down
28 changes: 28 additions & 0 deletions test/support/integration_test_case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,34 @@ defmodule GRPC.Integration.TestCase do
end
end


defmodule ErrorHandler do
@behaviour :cowboy_handler

@impl :cowboy_handler
def init(req, error_code) do
req = :cowboy_req.reply(error_code, %{"content-type" => "text/plain"}, "", req)

{:ok, req, error_code}
end
end

def run_error_server(error_code, func, port \\ 0) do
dispatch = :cowboy_router.compile([
({:_, [{:_, ErrorHandler, error_code}]})
])

{:ok, _} = :cowboy.start_clear("error_server", [port: port], %{env: %{dispatch: dispatch}})
port = :ranch.get_port("error_server")

try do
func.(port)
:ok
after
:ok = :cowboy.stop_listener("error_server")
end
end

def run_server(servers, func, port \\ 0, opts \\ []) do
{:ok, _pid, port} = GRPC.Server.start(servers, port, opts)

Expand Down