diff --git a/Libraries/Connect/Internal/Interceptors/ConnectInterceptor.swift b/Libraries/Connect/Internal/Interceptors/ConnectInterceptor.swift index db195260..c76a312f 100644 --- a/Libraries/Connect/Internal/Interceptors/ConnectInterceptor.swift +++ b/Libraries/Connect/Internal/Interceptors/ConnectInterceptor.swift @@ -176,7 +176,8 @@ extension ConnectInterceptor: StreamInterceptor { code: code, error: ConnectError.from( code: code, - headers: self.streamResponseHeaders.value ?? [:], + headers: self.streamResponseHeaders.value, + trailers: trailers, source: nil ), trailers: trailers diff --git a/Libraries/Connect/Internal/Interceptors/GRPCWebInterceptor.swift b/Libraries/Connect/Internal/Interceptors/GRPCWebInterceptor.swift index 15282132..cdece53c 100644 --- a/Libraries/Connect/Internal/Interceptors/GRPCWebInterceptor.swift +++ b/Libraries/Connect/Internal/Interceptors/GRPCWebInterceptor.swift @@ -57,13 +57,16 @@ extension GRPCWebInterceptor: UnaryInterceptor { } guard let responseData = response.message, !responseData.isEmpty else { - let code = response.headers.grpcStatus() ?? response.code + let (grpcCode, connectError) = ConnectError.parseGRPCHeaders( + response.headers, + trailers: response.trailers + ) proceed(HTTPResponse( - code: code, + code: grpcCode, headers: response.headers, message: response.message, trailers: response.trailers, - error: ConnectError.fromGRPCTrailers(response.headers, code: code), + error: connectError, tracingInfo: response.tracingInfo )) return @@ -147,7 +150,7 @@ extension GRPCWebInterceptor: StreamInterceptor { // Headers-only response. proceed(.complete( code: grpcCode, - error: ConnectError.fromGRPCTrailers(headers, code: grpcCode), + error: ConnectError.parseGRPCHeaders(nil, trailers: headers).error, trailers: headers )) } else { @@ -166,13 +169,15 @@ extension GRPCWebInterceptor: StreamInterceptor { let isTrailers = 0b10000000 & headerByte != 0 if isTrailers { let trailers = try Trailers.fromGRPCHeadersBlock(unpackedData) - let grpcCode = trailers.grpcStatus() ?? .unknown + let (grpcCode, error) = ConnectError.parseGRPCHeaders( + self.streamResponseHeaders.value, trailers: trailers + ) if grpcCode == .ok { proceed(.complete(code: .ok, error: nil, trailers: trailers)) } else { proceed(.complete( code: grpcCode, - error: ConnectError.fromGRPCTrailers(trailers, code: grpcCode), + error: error, trailers: trailers )) } @@ -222,10 +227,10 @@ private extension Trailers { private extension HTTPResponse { func withHandledGRPCWebTrailers(_ trailers: Trailers, message: Data?) -> Self { - let grpcStatus = trailers.grpcStatus() ?? .unknown - if grpcStatus == .ok { + let (grpcCode, error) = ConnectError.parseGRPCHeaders(self.headers, trailers: trailers) + if grpcCode == .ok { return HTTPResponse( - code: grpcStatus, + code: grpcCode, headers: self.headers, message: message, trailers: trailers, @@ -234,11 +239,11 @@ private extension HTTPResponse { ) } else { return HTTPResponse( - code: grpcStatus, + code: grpcCode, headers: self.headers, message: message, trailers: trailers, - error: ConnectError.fromGRPCTrailers(trailers, code: grpcStatus), + error: error, tracingInfo: self.tracingInfo ) } diff --git a/Libraries/Connect/PackageInternal/ConnectError+GRPC.swift b/Libraries/Connect/PackageInternal/ConnectError+GRPC.swift index 323f58a6..945007f2 100644 --- a/Libraries/Connect/PackageInternal/ConnectError+GRPC.swift +++ b/Libraries/Connect/PackageInternal/ConnectError+GRPC.swift @@ -18,24 +18,39 @@ extension ConnectError { /// This should not be considered part of Connect's public/stable interface, and is subject /// to change. When the compiler supports it, this should be package-internal. /// - /// Creates an error using gRPC trailers. + /// Parses gRPC headers and/or trailers to obtain the status and any potential error. /// - /// - parameter trailers: The trailers (or headers, for gRPC-Web) from which to parse the error. - /// - parameter code: The status code received from the server. + /// - parameter headers: Headers received from the server. + /// - parameter trailers: Trailers received from the server. Note that this could be trailers + /// passed in the headers block for gRPC-Web. /// - /// - returns: An error, if the status indicated an error. - public static func fromGRPCTrailers(_ trailers: Trailers, code: Code) -> Self? { - if code == .ok { - return nil + /// - returns: A tuple containing the gRPC status code and an optional error. + public static func parseGRPCHeaders( + _ headers: Headers?, trailers: Trailers? + ) -> (grpcCode: Code, error: ConnectError?) { + // "Trailers-only" responses can be sent in the headers or trailers block. + // Check for a valid gRPC status in the headers first, then in the trailers. + guard let grpcCode = headers?.grpcStatus() ?? trailers?.grpcStatus() else { + return (.unknown, ConnectError( + code: .unknown, message: "RPC response missing status", exception: nil, + details: [], metadata: [:] + )) } - return .init( - code: code, - message: trailers.grpcMessage(), + if grpcCode == .ok { + return (.ok, nil) + } + + // Combine headers + trailers into metadata to make error parsing easier for consumers, + // since gRPC can include error information in either headers or trailers. + let metadata = (headers ?? [:]).merging(trailers ?? [:]) { $1 } + return (grpcCode, .init( + code: grpcCode, + message: metadata.grpcMessage(), exception: nil, - details: trailers.connectErrorDetailsFromGRPC(), - metadata: trailers - ) + details: metadata.connectErrorDetailsFromGRPC(), + metadata: metadata + )) } } diff --git a/Libraries/Connect/Public/Implementation/Clients/ProtocolClient.swift b/Libraries/Connect/Public/Implementation/Clients/ProtocolClient.swift index 4847530a..f54d043e 100644 --- a/Libraries/Connect/Public/Implementation/Clients/ProtocolClient.swift +++ b/Libraries/Connect/Public/Implementation/Clients/ProtocolClient.swift @@ -428,6 +428,7 @@ private extension ResponseMessage where Output: ProtobufMessage { ?? ConnectError.from( code: response.code, headers: response.headers, + trailers: response.trailers, source: response.message ) self.init( diff --git a/Libraries/Connect/Public/Interfaces/ConnectError.swift b/Libraries/Connect/Public/Interfaces/ConnectError.swift index a5d6bbeb..507b61e8 100644 --- a/Libraries/Connect/Public/Interfaces/ConnectError.swift +++ b/Libraries/Connect/Public/Interfaces/ConnectError.swift @@ -110,26 +110,34 @@ extension ConnectError: Swift.Decodable { } extension ConnectError { - public static func from(code: Code, headers: Headers, source: Data?) -> Self { - let headers = headers.reduce(into: Headers(), { headers, current in - headers[current.key.lowercased()] = current.value - }) + public static func from( + code: Code, headers: Headers?, trailers: Trailers?, source: Data? + ) -> Self { + // Combine headers + trailers into metadata to make error parsing easier for consumers, + // since gRPC can include error information in either headers or trailers. + var metadata = Headers() + for (headerName, headerValue) in headers ?? [:] { + metadata[headerName.lowercased()] = headerValue + } + for (trailerName, trailerValue) in trailers ?? [:] { + metadata[trailerName.lowercased()] = trailerValue + } guard let source = source else { return .init( code: code, message: "empty error message from source", exception: nil, - details: [], metadata: headers + details: [], metadata: metadata ) } do { var connectError = try Foundation.JSONDecoder().decode(ConnectError.self, from: source) - connectError.metadata = headers + connectError.metadata = metadata return connectError } catch let error { return .init( code: code, message: String(data: source, encoding: .utf8), - exception: error, details: [], metadata: headers + exception: error, details: [], metadata: metadata ) } } diff --git a/Libraries/ConnectNIO/Internal/GRPCInterceptor.swift b/Libraries/ConnectNIO/Internal/GRPCInterceptor.swift index 006f8839..24444db0 100644 --- a/Libraries/ConnectNIO/Internal/GRPCInterceptor.swift +++ b/Libraries/ConnectNIO/Internal/GRPCInterceptor.swift @@ -59,8 +59,9 @@ extension GRPCInterceptor: UnaryInterceptor { return } - let (grpcCode, connectError) = self.grpcResult( - fromHeaders: response.headers, trailers: response.trailers + let (grpcCode, connectError) = ConnectError.parseGRPCHeaders( + response.headers, + trailers: response.trailers ) guard grpcCode == .ok, let rawData = response.message, !rawData.isEmpty else { proceed(HTTPResponse( @@ -154,8 +155,9 @@ extension GRPCInterceptor: StreamInterceptor { return } - let (grpcCode, connectError) = self.grpcResult( - fromHeaders: self.streamResponseHeaders.value, trailers: trailers + let (grpcCode, connectError) = ConnectError.parseGRPCHeaders( + self.streamResponseHeaders.value, + trailers: trailers ) if grpcCode == .ok { proceed(.complete( @@ -172,20 +174,6 @@ extension GRPCInterceptor: StreamInterceptor { } } } - - private func grpcResult( - fromHeaders headers: Headers?, trailers: Trailers? - ) -> (code: Code, error: ConnectError?) { - // "Trailers-only" responses can be sent in the headers or trailers block. - // Check for a valid gRPC status in the headers first, then in the trailers. - if let headers = headers, let grpcCode = headers.grpcStatus() { - return (grpcCode, .fromGRPCTrailers(headers, code: grpcCode)) - } else if let trailers = trailers, let grpcCode = trailers.grpcStatus() { - return (grpcCode, .fromGRPCTrailers(trailers, code: grpcCode)) - } else { - return (.unknown, nil) - } - } } private final class Locked: @unchecked Sendable { diff --git a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/ConnectErrorTests.swift b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/ConnectErrorTests.swift index 79de55cd..b29e9542 100644 --- a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/ConnectErrorTests.swift +++ b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/ConnectErrorTests.swift @@ -54,6 +54,7 @@ final class ConnectErrorTests: XCTestCase { XCTAssertEqual(error.unpackedDetails(), [expectedDetails1, expectedDetails2]) XCTAssertTrue(error.metadata.isEmpty) } + func testDeserializingErrorUsingHelperFunctionLowercasesHeaderKeys() throws { let expectedDetails = Connectrpc_Conformance_V1_RawHTTPRequest.with { $0.uri = "a/b/c" } let errorData = try self.errorData(expectedDetails: [expectedDetails]) @@ -63,6 +64,7 @@ final class ConnectErrorTests: XCTestCase { "sOmEkEy": ["foo"], "otherKey1": ["BAR", "bAz"], ], + trailers: nil, source: errorData ) XCTAssertEqual(error.code, .unavailable) // Respects the code from the error body @@ -73,6 +75,33 @@ final class ConnectErrorTests: XCTestCase { XCTAssertEqual(error.metadata, ["somekey": ["foo"], "otherkey1": ["BAR", "bAz"]]) } + func testDeserializingErrorUsingHelperFunctionCombinesHeadersAndTrailers() throws { + let expectedDetails = Connectrpc_Conformance_V1_RawHTTPRequest.with { $0.uri = "a/b/c" } + let errorData = try self.errorData(expectedDetails: [expectedDetails]) + let error = ConnectError.from( + code: .aborted, + headers: [ + "duPlIcaTedKey": ["headers"], + "otherKey1": ["BAR", "bAz"], + ], + trailers: [ + "duPlIcaTedKey": ["trailers"], + "anOthErKey": ["foo"], + ], + source: errorData + ) + XCTAssertEqual(error.code, .unavailable) // Respects the code from the error body + XCTAssertEqual(error.message, "overloaded: back off and retry") + XCTAssertNil(error.exception) + XCTAssertEqual(error.details.count, 1) + XCTAssertEqual(error.unpackedDetails(), [expectedDetails]) + XCTAssertEqual(error.metadata, [ + "duplicatedkey": ["trailers"], + "otherkey1": ["BAR", "bAz"], + "anotherkey": ["foo"], + ]) + } + func testDeserializingSimpleError() throws { let errorDictionary = [ "code": "unavailable", diff --git a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorChainIterationTests.swift b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorChainIterationTests.swift index a582b82d..8052da86 100644 --- a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorChainIterationTests.swift +++ b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorChainIterationTests.swift @@ -104,7 +104,9 @@ final class InterceptorChainIterationTests: XCTestCase { chain.executeInterceptorsAndStopOnFailure( [ { _, proceed in - proceed(.failure(.from(code: .unknown, headers: Headers(), source: nil))) + proceed(.failure(.from( + code: .unknown, headers: nil, trailers: nil, source: nil + ))) }, { value, proceed in proceed(.success(value + "b")) }, ], @@ -141,7 +143,9 @@ final class InterceptorChainIterationTests: XCTestCase { chain.executeLinkedInterceptorsAndStopOnFailure( [ { _, proceed in - proceed(.failure(.from(code: .unknown, headers: Headers(), source: nil))) + proceed(.failure(.from( + code: .unknown, headers: nil, trailers: nil, source: nil + ))) }, { value, proceed in proceed(.success(value + "2")) }, ], @@ -168,7 +172,9 @@ final class InterceptorChainIterationTests: XCTestCase { firstInFirstOut: true, initial: "", transform: { _, proceed in - proceed(.failure(.from(code: .unknown, headers: Headers(), source: nil))) + proceed(.failure(.from( + code: .unknown, headers: nil, trailers: nil, source: nil + ))) }, then: [ { value, proceed in proceed(.success(value + 1)) }, @@ -192,7 +198,9 @@ final class InterceptorChainIterationTests: XCTestCase { transform: { value1, proceed in proceed(.success(Int(value1)!)) }, then: [ { _, proceed in - proceed(.failure(.from(code: .unknown, headers: Headers(), source: nil))) + proceed(.failure(.from( + code: .unknown, headers: nil, trailers: nil, source: nil + ))) }, { value, proceed in proceed(.success(value + 3)) }, ], diff --git a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorIntegrationTests.swift b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorIntegrationTests.swift index 71b44818..723eaacd 100644 --- a/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorIntegrationTests.swift +++ b/Tests/UnitTests/ConnectLibraryTests/ConnectTests/InterceptorIntegrationTests.swift @@ -374,7 +374,7 @@ extension StepTrackingInterceptor: UnaryInterceptor { ) { self.trackStep(.unaryRequest(id: self.id)) if self.failOutboundRequests { - proceed(.failure(.from(code: .aborted, headers: Headers(), source: nil))) + proceed(.failure(.from(code: .aborted, headers: nil, trailers: nil, source: nil))) } else if self.requestDelay != .never { DispatchQueue.global().asyncAfter(deadline: .now() + self.requestDelay) { proceed(.success(request)) @@ -420,7 +420,7 @@ extension StepTrackingInterceptor: StreamInterceptor { ) { self.trackStep(.streamStart(id: self.id)) if self.failOutboundRequests { - proceed(.failure(.from(code: .aborted, headers: Headers(), source: nil))) + proceed(.failure(.from(code: .aborted, headers: nil, trailers: nil, source: nil))) } else if self.requestDelay != .never { DispatchQueue.global().asyncAfter(deadline: .now() + self.requestDelay) { proceed(.success(request))