Handle non-200 status codes more gracefully (#1613)

Motivation:

Accepted RPCs should have a 200 HTTP response status code. gRPC knows
how to handle some non-200 status codes and can synthesize a gRPC status
from them. At the moment they are treated as errors and later converted
to a gRPC status. However this means that only the gRPC status sees the
error as a status: other response parts (e.g. the response stream) see
an error relating to invalid HTTP response codes rather than the
synthesized status.

Modifications:

- Handle non-200 HTTP status codes more gracefully by turning them into
  a gRPC status where possible.

Results:

- Non-200 status codes are more readily converted to a gRPC status.

Author: glbrntt

Sources/GRPC/GRPCClientChannelHandler.swift

@@ -374,8 +374,6 @@ extension GRPCClientChannelHandler: ChannelInboundHandler {
             return GRPCError.InvalidContentType(contentType).captureContext()
           case let .invalidHTTPStatus(status):
             return GRPCError.InvalidHTTPStatus(status).captureContext()
-          case let .invalidHTTPStatusWithGRPCStatus(status):
-            return GRPCError.InvalidHTTPStatusWithGRPCStatus(status).captureContext()
           case .invalidState:
             return GRPCError.InvalidState("parsing end-of-stream trailers").captureContext()
           }

Sources/GRPC/GRPCClientStateMachine.swift

@@ -41,10 +41,6 @@ enum ReceiveEndOfResponseStreamError: Error, Equatable {
   /// The HTTP response status from the server was not 200 OK.
   case invalidHTTPStatus(String?)
 
-  /// The HTTP response status from the server was not 200 OK but the "grpc-status" header contained
-  /// a valid value.
-  case invalidHTTPStatusWithGRPCStatus(GRPCStatus)
-
   /// An invalid state was encountered. This is a serious implementation error.
   case invalidState
 }
@@ -742,7 +738,7 @@ extension GRPCClientStateMachine.State {
   private func readStatusCode(from trailers: HPACKHeaders) -> GRPCStatus.Code? {
     return trailers.first(name: GRPCHeaderName.statusCode)
       .flatMap(Int.init)
-      .flatMap(GRPCStatus.Code.init)
+      .flatMap({ GRPCStatus.Code(rawValue: $0) })
   }
 
   private func readStatusMessage(from trailers: HPACKHeaders) -> String? {
@@ -764,18 +760,22 @@ extension GRPCClientStateMachine.State {
     //
     // See: https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
     let statusHeader = trailers.first(name: ":status")
-    guard let status = statusHeader.flatMap(Int.init).map({ HTTPResponseStatus(statusCode: $0) })
-    else {
+    let httpResponseStatus = statusHeader.flatMap(Int.init).map {
+      HTTPResponseStatus(statusCode: $0)
+    }
+
+    guard let httpResponseStatus = httpResponseStatus else {
       return .failure(.invalidHTTPStatus(statusHeader))
     }
 
-    guard status == .ok else {
-      if let code = self.readStatusCode(from: trailers) {
-        let message = self.readStatusMessage(from: trailers)
-        return .failure(.invalidHTTPStatusWithGRPCStatus(.init(code: code, message: message)))
-      } else {
-        return .failure(.invalidHTTPStatus(statusHeader))
-      }
+    guard httpResponseStatus == .ok else {
+      // Non-200 response. If there's a 'grpc-status' message we should use that otherwise try
+      // to create one from the HTTP status code.
+      let grpcStatusCode = self.readStatusCode(from: trailers)
+        ?? GRPCStatus.Code(httpStatus: Int(httpResponseStatus.code))
+        ?? .unknown
+      let message = self.readStatusMessage(from: trailers)
+      return .success(GRPCStatus(code: grpcStatusCode, message: message))
     }
 
     // Only validate the content-type header if it's present. This is a small deviation from the

Sources/GRPC/GRPCError.swift

@@ -229,7 +229,7 @@ public enum GRPCError {
 
     public func makeGRPCStatus() -> GRPCStatus {
       return GRPCStatus(
-        code: .init(httpStatus: self.status),
+        code: .init(httpStatus: self.status) ?? .unknown,
         message: self.description,
         cause: self
       )
@@ -342,21 +342,29 @@ extension GRPCErrorProtocol {
 extension GRPCStatus.Code {
   /// The gRPC status code associated with the given HTTP status code. This should only be used if
   /// the RPC did not return a 'grpc-status' trailer.
-  internal init(httpStatus: String?) {
+  internal init?(httpStatus codeString: String?) {
+    if let code = codeString.flatMap(Int.init) {
+      self.init(httpStatus: code)
+    } else {
+      return nil
+    }
+  }
+
+  internal init?(httpStatus: Int) {
     /// See: https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
     switch httpStatus {
-    case "400":
+    case 400:
       self = .internalError
-    case "401":
+    case 401:
       self = .unauthenticated
-    case "403":
+    case 403:
       self = .permissionDenied
-    case "404":
+    case 404:
       self = .unimplemented
-    case "429", "502", "503", "504":
+    case 429, 502, 503, 504:
       self = .unavailable
     default:
-      self = .unknown
+      return nil
     }
   }
 }

Tests/GRPCTests/GRPCClientStateMachineTests.swift

@@ -1097,14 +1097,8 @@ extension GRPCClientStateMachineTests {
       ":status": "418",
       "grpc-status": "5",
     ]
-    stateMachine.receiveEndOfResponseStream(trailers).assertFailure { error in
-      XCTAssertEqual(
-        error,
-        .invalidHTTPStatusWithGRPCStatus(GRPCStatus(
-          code: GRPCStatus.Code(rawValue: 5)!,
-          message: nil
-        ))
-      )
+    stateMachine.receiveEndOfResponseStream(trailers).assertSuccess { status in
+      XCTAssertEqual(status.code.rawValue, 5)
     }
   }
 
@@ -1115,8 +1109,8 @@ extension GRPCClientStateMachineTests {
     ))
 
     let trailers: HPACKHeaders = [":status": "418"]
-    stateMachine.receiveEndOfResponseStream(trailers).assertFailure { error in
-      XCTAssertEqual(error, .invalidHTTPStatus("418"))
+    stateMachine.receiveEndOfResponseStream(trailers).assertSuccess { status in
+      XCTAssertEqual(status.code, .unknown)
     }
   }
 }

Tests/GRPCTests/GRPCStatusCodeTests.swift

@@ -117,14 +117,74 @@ class GRPCStatusCodeTests: GRPCTestCase {
 
     self.sendRequestHead()
     let headerFramePayload = HTTP2Frame.FramePayload.headers(.init(headers: headers))
-    XCTAssertThrowsError(try self.channel.writeInbound(headerFramePayload)) { error in
-      guard let withContext = error as? GRPCError.WithContext,
-            let invalidHTTPStatus = withContext.error as? GRPCError.InvalidHTTPStatusWithGRPCStatus
-      else {
-        XCTFail("Unexpected error: \(error)")
-        return
+    try self.channel.writeInbound(headerFramePayload)
+
+    let responsePart1 = try XCTUnwrap(
+      self.channel.readInbound(as: _GRPCClientResponsePart<ByteBuffer>.self)
+    )
+
+    switch responsePart1 {
+    case .trailingMetadata:
+      ()
+    case .initialMetadata, .message, .status:
+      XCTFail("Unexpected response part \(responsePart1)")
+    }
+
+    let responsePart2 = try XCTUnwrap(
+      self.channel.readInbound(as: _GRPCClientResponsePart<ByteBuffer>.self)
+    )
+
+    switch responsePart2 {
+    case .initialMetadata, .message, .trailingMetadata:
+      XCTFail("Unexpected response part \(responsePart2)")
+    case let .status(actual):
+      XCTAssertEqual(actual.code, status.code)
+      XCTAssertEqual(actual.message, status.message)
+    }
+  }
+
+  func testNon200StatusCodesAreConverted() throws {
+    let tests: [(Int, GRPCStatus.Code)] = [
+      (400, .internalError),
+      (401, .unauthenticated),
+      (403, .permissionDenied),
+      (404, .unimplemented),
+      (429, .unavailable),
+      (502, .unavailable),
+      (503, .unavailable),
+      (504, .unavailable),
+    ]
+
+    for (httpStatusCode, grpcStatusCode) in tests {
+      let headers: HPACKHeaders = [":status": "\(httpStatusCode)"]
+
+      self.setUp()
+      self.sendRequestHead()
+      let headerFramePayload = HTTP2Frame.FramePayload
+        .headers(.init(headers: headers, endStream: true))
+      try self.channel.writeInbound(headerFramePayload)
+
+      let responsePart1 = try XCTUnwrap(
+        self.channel.readInbound(as: _GRPCClientResponsePart<ByteBuffer>.self)
+      )
+
+      switch responsePart1 {
+      case .trailingMetadata:
+        ()
+      case .initialMetadata, .message, .status:
+        XCTFail("Unexpected response part \(responsePart1)")
+      }
+
+      let responsePart2 = try XCTUnwrap(
+        self.channel.readInbound(as: _GRPCClientResponsePart<ByteBuffer>.self)
+      )
+
+      switch responsePart2 {
+      case .initialMetadata, .message, .trailingMetadata:
+        XCTFail("Unexpected response part \(responsePart2)")
+      case let .status(actual):
+        XCTAssertEqual(actual.code, grpcStatusCode)
       }
-      XCTAssertEqual(invalidHTTPStatus.makeGRPCStatus(), status)
     }
   }