From ddeec69cbc84e46d53d47d3b52b78622d77349ba Mon Sep 17 00:00:00 2001 From: Bulat Shakirzyanov <83289+avalanche123@users.noreply.github.com> Date: Tue, 11 Aug 2020 14:27:01 -0400 Subject: [PATCH] feat: error metadata handling (#33) * Serialize & deserialize gRPC errors * add metadata support and tests * add unit tests for error metadata Co-authored-by: Natalie --- lib/grpc_web/client/client_executor.rb | 24 ++++++++++++------- lib/grpc_web/server/message_serialization.rb | 16 +++++++++---- .../envoy_server_ruby_client_spec.rb | 10 ++++++-- .../ruby_server_ruby_client_spec.rb | 10 ++++++-- .../grpc_web/client/client_executor_spec.rb | 9 ++++--- .../server/message_serialization_spec.rb | 4 ++-- 6 files changed, 50 insertions(+), 23 deletions(-) diff --git a/lib/grpc_web/client/client_executor.rb b/lib/grpc_web/client/client_executor.rb index 0c7b6d6..dd5d549 100644 --- a/lib/grpc_web/client/client_executor.rb +++ b/lib/grpc_web/client/client_executor.rb @@ -14,6 +14,7 @@ class << self GRPC_STATUS_HEADER = 'grpc-status' GRPC_MESSAGE_HEADER = 'grpc-message' + GRPC_HEADERS = %W[x-grpc-web #{GRPC_STATUS_HEADER} #{GRPC_MESSAGE_HEADER}].freeze def request(uri, rpc_desc, params = {}) req_proto = rpc_desc.input.new(params) @@ -82,32 +83,37 @@ def parse_headers(header_str) end def raise_on_response_errors(resp, headers, error_unpacking_frames) + metadata = headers.reject { |key, _| GRPC_HEADERS.include?(key) } status_str = headers[GRPC_STATUS_HEADER] status_code = status_str.to_i if status_str && status_str == status_str.to_i.to_s # see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md if status_code && status_code != 0 - raise ::GRPC::BadStatus.new_status_exception(status_code, headers[GRPC_MESSAGE_HEADER]) + raise ::GRPC::BadStatus.new_status_exception( + status_code, + headers[GRPC_MESSAGE_HEADER], + metadata, + ) end case resp when Net::HTTPBadRequest # 400 - raise ::GRPC::Internal, resp.message + raise ::GRPC::Internal.new(resp.message, metadata) when Net::HTTPUnauthorized # 401 - raise ::GRPC::Unauthenticated, resp.message + raise ::GRPC::Unauthenticated.new(resp.message, metadata) when Net::HTTPForbidden # 403 - raise ::GRPC::PermissionDenied, resp.message + raise ::GRPC::PermissionDenied.new(resp.message, metadata) when Net::HTTPNotFound # 404 - raise ::GRPC::Unimplemented, resp.message + raise ::GRPC::Unimplemented.new(resp.message, metadata) when Net::HTTPTooManyRequests, # 429 Net::HTTPBadGateway, # 502 Net::HTTPServiceUnavailable, # 503 Net::HTTPGatewayTimeOut # 504 - raise ::GRPC::Unavailable, resp.message + raise ::GRPC::Unavailable.new(resp.message, metadata) else - raise ::GRPC::Unknown, resp.message unless resp.is_a?(Net::HTTPSuccess) # 200 - raise ::GRPC::Internal, resp.message if error_unpacking_frames - raise ::GRPC::Unknown, resp.message if status_code.nil? + raise ::GRPC::Unknown.new(resp.message, metadata) unless resp.is_a?(Net::HTTPSuccess) # 200 + raise ::GRPC::Internal.new(resp.message, metadata) if error_unpacking_frames + raise ::GRPC::Unknown.new(resp.message, metadata) if status_code.nil? end end end diff --git a/lib/grpc_web/server/message_serialization.rb b/lib/grpc_web/server/message_serialization.rb index 1ef51b9..c010fa2 100644 --- a/lib/grpc_web/server/message_serialization.rb +++ b/lib/grpc_web/server/message_serialization.rb @@ -59,7 +59,7 @@ def serialize_success_response(response) def serialize_error_response(response) ex = response.body if ex.is_a?(::GRPC::BadStatus) - header_str = generate_headers(ex.code, ex.details) + header_str = generate_headers(ex.code, ex.details, ex.metadata) else header_str = generate_headers( ::GRPC::Core::StatusCodes::UNKNOWN, @@ -72,13 +72,19 @@ def serialize_error_response(response) # If needed, trailers can be appended to the response as a 2nd # base64 encoded string with independent framing. - def generate_headers(status, message) - [ + def generate_headers(status, message, metadata = {}) + headers = [ "grpc-status:#{status}", "grpc-message:#{message}", 'x-grpc-web:1', - nil, # for trailing newline - ].join("\r\n") + ] + metadata.each do |name, value| + next if %w[grpc-status grpc-message x-grpc-web].include?(name) + + headers << "#{name}:#{value}" + end + headers << nil # for trailing newline + headers.join("\r\n") end end end diff --git a/spec/integration/envoy_server_ruby_client_spec.rb b/spec/integration/envoy_server_ruby_client_spec.rb index 82ee907..f1a8a68 100644 --- a/spec/integration/envoy_server_ruby_client_spec.rb +++ b/spec/integration/envoy_server_ruby_client_spec.rb @@ -33,13 +33,19 @@ let(:service) do Class.new(TestHelloService) do def say_hello(_request, _metadata = nil) - raise ::GRPC::InvalidArgument, 'Test message' + raise ::GRPC::InvalidArgument.new( + 'Test message', + 'metadata' => 'more info', + 'envoy' => 'more info', + ) end end end it 'raises an error' do - expect { subject }.to raise_error(GRPC::InvalidArgument, '3:Test message') + expect { subject }.to raise_error(GRPC::InvalidArgument, '3:Test message') do |e| + expect(e.metadata).to eq('metadata' => 'more info', 'envoy' => 'more info') + end end end diff --git a/spec/integration/ruby_server_ruby_client_spec.rb b/spec/integration/ruby_server_ruby_client_spec.rb index d0c6a34..8c878da 100644 --- a/spec/integration/ruby_server_ruby_client_spec.rb +++ b/spec/integration/ruby_server_ruby_client_spec.rb @@ -38,13 +38,19 @@ let(:service) do Class.new(TestHelloService) do def say_hello(_request, _metadata = nil) - raise ::GRPC::InvalidArgument, 'Test message' + raise ::GRPC::InvalidArgument.new( + 'Test message', + 'metadata' => 'more info', + 'envoy' => 'more info', + ) end end end it 'raises an error' do - expect { subject }.to raise_error(GRPC::InvalidArgument, '3:Test message') + expect { subject }.to raise_error(GRPC::InvalidArgument, '3:Test message') do |e| + expect(e.metadata).to eq('metadata' => 'more info', 'envoy' => 'more info') + end end end diff --git a/spec/unit/grpc_web/client/client_executor_spec.rb b/spec/unit/grpc_web/client/client_executor_spec.rb index ea39457..de9f761 100644 --- a/spec/unit/grpc_web/client/client_executor_spec.rb +++ b/spec/unit/grpc_web/client/client_executor_spec.rb @@ -134,13 +134,16 @@ { status: 200, body: - "\x80\x00\x00\x008grpc-status:#{GRPC::Core::StatusCodes::INVALID_ARGUMENT}"\ - "\r\ngrpc-message:Test message\r\nx-grpc-web:1\r\n", + "\x80\x00\x00\x00Wgrpc-status:#{GRPC::Core::StatusCodes::INVALID_ARGUMENT}"\ + "\r\ngrpc-message:Test message\r\nx-grpc-web:1"\ + "\r\nuser-role-id:123\r\nuser-id:456\r\n", } end it 'raises an error' do - expect { response }.to raise_error(GRPC::InvalidArgument) + expect { response }.to raise_error(GRPC::InvalidArgument) do |exception| + expect(exception.metadata).to eq('user-role-id' => '123', 'user-id' => '456') + end end end end diff --git a/spec/unit/grpc_web/server/message_serialization_spec.rb b/spec/unit/grpc_web/server/message_serialization_spec.rb index 45525c0..b85588c 100644 --- a/spec/unit/grpc_web/server/message_serialization_spec.rb +++ b/spec/unit/grpc_web/server/message_serialization_spec.rb @@ -115,14 +115,14 @@ end context 'when response is a GRPC::BadStatus' do - let(:body) { ::GRPC::NotFound.new('Where am I?') } + let(:body) { ::GRPC::NotFound.new('Where am I?', 'user-role-id' => '123') } it_behaves_like 'generates a body without a payload frame' it_behaves_like( 'generates a body with a header frame', expected_header_frame_body: - "grpc-status:5\r\ngrpc-message:Where am I?\r\nx-grpc-web:1\r\n", + "grpc-status:5\r\ngrpc-message:Where am I?\r\nx-grpc-web:1\r\nuser-role-id:123\r\n", ) end end