From f4fd1e529c251be8dc18aab02f51a2e99ff0048e Mon Sep 17 00:00:00 2001 From: Rishab Jaggi Date: Tue, 19 Oct 2021 10:42:58 -0400 Subject: [PATCH] Add details field to gRPC error struct Adds a details field of type [any()] to the error struct and updates the transport layer to properly encode + decode them. --- lib/grpc/adapter/cowboy/handler.ex | 34 +++++++++---- lib/grpc/adapter/gun.ex | 12 ++++- lib/grpc/error.ex | 31 +++++++----- lib/grpc/stub.ex | 11 ++++- lib/grpc/transport/http2.ex | 8 +-- test/grpc/integration/server_test.exs | 71 +++++++++++++++++++++++++-- 6 files changed, 137 insertions(+), 30 deletions(-) diff --git a/lib/grpc/adapter/cowboy/handler.ex b/lib/grpc/adapter/cowboy/handler.ex index f281f843..176ceded 100644 --- a/lib/grpc/adapter/cowboy/handler.ex +++ b/lib/grpc/adapter/cowboy/handler.ex @@ -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 @@ -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} @@ -280,7 +285,7 @@ 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} @@ -288,8 +293,13 @@ defmodule GRPC.Adapter.Cowboy.Handler do # 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} @@ -297,8 +307,14 @@ defmodule GRPC.Adapter.Cowboy.Handler do 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} @@ -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) diff --git a/lib/grpc/adapter/gun.ex b/lib/grpc/adapter/gun.ex index bd7234aa..f2737789 100644 --- a/lib/grpc/adapter/gun.ex +++ b/lib/grpc/adapter/gun.ex @@ -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 @@ -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} diff --git a/lib/grpc/error.ex b/lib/grpc/error.ex index c6aff4dc..17c16407 100644 --- a/lib/grpc/error.ex +++ b/lib/grpc/error.ex @@ -5,17 +5,18 @@ 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 @@ -23,10 +24,11 @@ defmodule GRPC.RPCError do 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 @@ -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)" diff --git a/lib/grpc/stub.ex b/lib/grpc/stub.ex index b77bb5fa..5c59a865 100644 --- a/lib/grpc/stub.ex +++ b/lib/grpc/stub.ex @@ -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 diff --git a/lib/grpc/transport/http2.ex b/lib/grpc/transport/http2.ex index a5cbf248..a3eddb1c 100644 --- a/lib/grpc/transport/http2.ex +++ b/lib/grpc/transport/http2.ex @@ -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(",") } end diff --git a/test/grpc/integration/server_test.exs b/test/grpc/integration/server_test.exs index 3960bf10..baa693fe 100644 --- a/test/grpc/integration/server_test.exs +++ b/test/grpc/integration/server_test.exs @@ -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"] + } == 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}") @@ -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 @@ -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 @@ -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 @@ -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)