Skip to content
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.

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

[ELIX-41] Add details field to gRPC error struct #13

Draft
wants to merge 1 commit into
base: brex-head
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
34 changes: 25 additions & 9 deletions lib/grpc/adapter/cowboy/handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ defmodule GRPC.Adapter.Cowboy.Handler do
{:cowboy_loop, req, %{pid: pid, handling_timer: timer_ref}}
else
{:error, error} ->
trailers = HTTP2.server_trailers(error.status, error.message)
trailers = HTTP2.server_trailers(error.status, error.message, error.details)
req = send_error_trailers(req, trailers)
{:ok, req, state}
end
Expand Down Expand Up @@ -255,8 +255,13 @@ defmodule GRPC.Adapter.Cowboy.Handler do
end

def info({:handling_timeout, _}, req, state = %{pid: pid}) do
error = %RPCError{status: GRPC.Status.deadline_exceeded(), message: "Deadline expired"}
trailers = HTTP2.server_trailers(error.status, error.message)
error = %RPCError{
status: GRPC.Status.deadline_exceeded(),
message: "Deadline expired",
details: []
}

trailers = HTTP2.server_trailers(error.status, error.message, error.details)
exit_handler(pid, :timeout)
req = send_error_trailers(req, trailers)
{:stop, req, state}
Expand All @@ -280,25 +285,36 @@ defmodule GRPC.Adapter.Cowboy.Handler do

# expected error raised from user to return error immediately
def info({:EXIT, pid, {%RPCError{} = error, _stacktrace}}, req, state = %{pid: pid}) do
trailers = HTTP2.server_trailers(error.status, error.message)
trailers = HTTP2.server_trailers(error.status, error.message, error.details)
exit_handler(pid, :rpc_error)
req = send_error_trailers(req, trailers)
{:stop, req, state}
end

# unknown error raised from rpc
def info({:EXIT, pid, {:handle_error, _kind}}, req, state = %{pid: pid}) do
error = %RPCError{status: GRPC.Status.unknown(), message: "Internal Server Error"}
trailers = HTTP2.server_trailers(error.status, error.message)
error = %RPCError{
status: GRPC.Status.unknown(),
message: "Internal Server Error",
details: []
}

trailers = HTTP2.server_trailers(error.status, error.message, error.details)
exit_handler(pid, :error)
req = send_error_trailers(req, trailers)
{:stop, req, state}
end

def info({:EXIT, pid, {reason, stacktrace}}, req, state = %{pid: pid}) do
Logger.error(Exception.format(:error, reason, stacktrace))
error = %RPCError{status: GRPC.Status.unknown(), message: "Internal Server Error"}
trailers = HTTP2.server_trailers(error.status, error.message)

error = %RPCError{
status: GRPC.Status.unknown(),
message: "Internal Server Error",
details: []
}

trailers = HTTP2.server_trailers(error.status, error.message, error.details)
exit_handler(pid, reason)
req = send_error_trailers(req, trailers)
{:stop, req, state}
Expand Down Expand Up @@ -428,7 +444,7 @@ defmodule GRPC.Adapter.Cowboy.Handler do

defp send_error(req, %{pid: pid}, msg) do
error = RPCError.exception(status: :internal, message: msg)
trailers = HTTP2.server_trailers(error.status, error.message)
trailers = HTTP2.server_trailers(error.status, error.message, error.details)

exit_handler(pid, :rpc_error)
send_error_trailers(req, trailers)
Expand Down
12 changes: 10 additions & 2 deletions lib/grpc/adapter/gun.ex
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,11 @@ defmodule GRPC.Adapter.Gun do
{:error,
GRPC.RPCError.exception(
String.to_integer(headers["grpc-status"]),
headers["grpc-message"]
headers["grpc-message"],
# TODO: stop assuming details is a list of strings only
headers["grpc-details"]
|> String.split(",")
|> Enum.filter(fn x -> x != "" end)
)}
end
else
Expand All @@ -215,7 +219,11 @@ defmodule GRPC.Adapter.Gun do
{:error,
GRPC.RPCError.exception(
String.to_integer(headers["grpc-status"]),
headers["grpc-message"]
headers["grpc-message"],
# TODO: stop assuming details is a list of strings only
headers["grpc-details"]
|> String.split(",")
|> Enum.filter(fn x -> x != "" end)
)}
else
{:response, headers, :nofin}
Expand Down
31 changes: 20 additions & 11 deletions lib/grpc/error.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,30 @@ defmodule GRPC.RPCError do
# server side
raise GRPC.RPCError, status: :unknown # preferred
raise GRPC.RPCError, status: GRPC.Status.unknown, message: "error message"
raise GRPC.RPCError, status: GRPC.Status.unknown, details: [Google.Rpc.LocalizedMessage.new!(locale: “en-US”, message: “User friendly string.”)]

# client side
{:error, error} = Your.Stub.unary_call(channel, request)
"""

defexception [:status, :message]
@type t :: %__MODULE__{status: GRPC.Status.t(), message: String.t()}
defexception [:status, :message, :details]
@type t :: %__MODULE__{status: GRPC.Status.t(), message: String.t(), details: [any()]}

alias GRPC.Status

@spec exception(Status.t(), String.t()) :: t
@spec exception(Status.t(), String.t(), [any()]) :: t
def new(status) when is_atom(status) do
exception(status: status)
end

def exception(args) when is_list(args) do
error = parse_args(args, %__MODULE__{})

if error.message do
error
else
Map.put(error, :message, status_message(error.status))
cond do
error.message && error.details -> error
error.details -> Map.put(error, :message, status_message(error.status))
error.message -> Map.put(error, :details, [])
true -> error |> Map.put(:message, status_message(error.status)) |> Map.put(:details, [])
end
end

Expand All @@ -47,12 +49,19 @@ defmodule GRPC.RPCError do
parse_args(t, acc)
end

def exception(status, message) when is_atom(status) do
%GRPC.RPCError{status: apply(GRPC.Status, status, []), message: message}
defp parse_args([{:details, details} | t], acc) do
acc = %{acc | details: details}
parse_args(t, acc)
end

def exception(status, message, details \\ [])

def exception(status, message, details) when is_atom(status) do
%GRPC.RPCError{status: apply(GRPC.Status, status, []), message: message, details: details}
end

def exception(status, message) when is_integer(status) do
%GRPC.RPCError{status: status, message: message}
def exception(status, message, details) when is_integer(status) do
%GRPC.RPCError{status: status, message: message, details: details}
end

defp status_message(1), do: "The operation was cancelled (typically by the caller)"
Expand Down
11 changes: 10 additions & 1 deletion lib/grpc/stub.ex
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,16 @@ defmodule GRPC.Stub do
if status == GRPC.Status.ok() do
:ok
else
{:error, %GRPC.RPCError{status: status, message: trailers["grpc-message"]}}
{:error,
%GRPC.RPCError{
status: status,
message: trailers["grpc-message"],
# TODO: stop assuming details is a list of strings only
details:
trailers["grpc-details"]
|> String.split(",")
|> Enum.filter(fn x -> x != "" end)
}}
end
end

Expand Down
8 changes: 5 additions & 3 deletions lib/grpc/transport/http2.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ defmodule GRPC.Transport.HTTP2 do
%{"content-type" => "application/grpc+#{codec.name}"}
end

@spec server_trailers(integer, String.t()) :: map
def server_trailers(status \\ Status.ok(), message \\ "") do
@spec server_trailers(integer, String.t(), [any()]) :: map
def server_trailers(status \\ Status.ok(), message \\ "", details \\ []) do
# TODO: stop assuming details is a list of String.t() only
%{
"grpc-status" => Integer.to_string(status),
"grpc-message" => message
"grpc-message" => message,
"grpc-details" => details |> Enum.join(",")
Copy link
Author

@rishab-brex rishab-brex Oct 19, 2021

Choose a reason for hiding this comment

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

we need to be able to encode the details field (a list of of type [any()]) to a string and back without knowing the types. is there a way to do this without knowing the type of struct/string/integer/etc. to decode to?

}
end

Expand Down
71 changes: 67 additions & 4 deletions test/grpc/integration/server_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,64 @@ defmodule GRPC.Integration.ServerTest do
end
end

defmodule RichErrorErrorServer do
use GRPC.Server, service: Helloworld.Greeter.Service

def say_hello(%{name: "string_errors"}, _stream) do
raise GRPC.RPCError,
status: GRPC.Status.unauthenticated(),
message: "string_errors",
details: ["hello", "world"]
end

def say_hello(%{name: "number_errors"}, _stream) do
raise GRPC.RPCError,
status: GRPC.Status.unauthenticated(),
message: "number_errors",
details: [1, 2, 3, 4, 5]
end

def say_hello(%{name: "rich_errors"}, _stream) do
raise GRPC.RPCError,
status: GRPC.Status.unauthenticated(),
message: "rich_errors",
details: [Helloworld.HelloReply.new(message: "hello world")]
end
end

test "error details work" do
run_server([RichErrorErrorServer], fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}")

req = Helloworld.HelloRequest.new(name: "string_errors")
{:error, reply} = channel |> Helloworld.Greeter.Stub.say_hello(req)

assert %GRPC.RPCError{
status: GRPC.Status.unauthenticated(),
message: "string_errors",
details: ["hello", "world"]
Copy link
Author

Choose a reason for hiding this comment

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

this test works

} == reply

req = Helloworld.HelloRequest.new(name: "number_errors")
{:error, reply} = channel |> Helloworld.Greeter.Stub.say_hello(req)

assert %GRPC.RPCError{
status: GRPC.Status.unauthenticated(),
message: "number_errors",
details: [1, 2, 3, 4, 5]
} == reply

req = Helloworld.HelloRequest.new(name: "rich_errors")
{:error, reply} = channel |> Helloworld.Greeter.Stub.say_hello(req)

assert %GRPC.RPCError{
status: GRPC.Status.unauthenticated(),
message: "rich_errors",
details: [%Helloworld.HelloReply{message: "hello world"}]
} == reply
end)
end

test "multiple servers works" do
run_server([FeatureServer, HelloServer], fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}")
Expand All @@ -109,7 +167,8 @@ defmodule GRPC.Integration.ServerTest do

assert %GRPC.RPCError{
status: GRPC.Status.unauthenticated(),
message: "Please authenticate"
message: "Please authenticate",
details: []
} == reply
end)
end
Expand All @@ -120,7 +179,11 @@ defmodule GRPC.Integration.ServerTest do
req = Helloworld.HelloRequest.new(name: "unknown error")

assert {:error,
%GRPC.RPCError{message: "Internal Server Error", status: GRPC.Status.unknown()}} ==
%GRPC.RPCError{
message: "Internal Server Error",
status: GRPC.Status.unknown(),
details: []
}} ==
channel |> Helloworld.Greeter.Stub.say_hello(req)
end)
end
Expand All @@ -129,7 +192,7 @@ defmodule GRPC.Integration.ServerTest do
run_server([FeatureErrorServer], fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}")
rect = Routeguide.Rectangle.new()
error = %GRPC.RPCError{message: "Please authenticate", status: 16}
error = %GRPC.RPCError{message: "Please authenticate", status: 16, details: []}
assert {:error, ^error} = channel |> Routeguide.RouteGuide.Stub.list_features(rect)
end)
end
Expand All @@ -148,7 +211,7 @@ defmodule GRPC.Integration.ServerTest do
run_server([TimeoutServer], fn port ->
{:ok, channel} = GRPC.Stub.connect("localhost:#{port}")
rect = Routeguide.Rectangle.new()
error = %GRPC.RPCError{message: "Deadline expired", status: 4}
error = %GRPC.RPCError{message: "Deadline expired", status: 4, details: []}

assert {:error, ^error} =
channel |> Routeguide.RouteGuide.Stub.list_features(rect, timeout: 500)
Expand Down