Update to rc4 of conformance suite and fix several issues (#248)

There are a handful of changes in here that go along with this update of
the conformance tests.

1. This updates the HTTP code -> RPC code mappings per latest updates to
the spec [here](connectrpc/connectrpc.com#130).

2. The newest release candidate of conformance tests really wants
clients to report "exception metadata" as trailers, and for that to be a
combined view of headers _and_ trailers for unary and client-stream
errors.

The reasoning is related to the fact that, in the gRPC and gRPC-Web
protocols, a server can choose to combine headers and trailers into a
"trailers-only" response when there are no response messages (which is
always the case for unary and client-stream error responses). So the
caller only checking headers or trailers when querying a particular key
is brittle; and the caller checking _both_ headers and trailers is
annoying and verbose. Instead, the combined metadata is reported as part
of the exception, and the caller then has one bucket of metadata to
query for a particular key, which is less error-prone.

3. Unary and client-stream operations now properly handle cases where
there is either zero or more than one response message combined with a
non-error status. They return "unimplemented" as indicated by [this
doc](https://grpc.github.io/grpc/core/md_doc_statuscodes.html) on RPC
status codes (search therein for "cardinality violation").

4. In the Connect protocol, we correctly default to the code inferred
from the HTTP status code, instead of always falling back to "unknown"
in the event that the code cannot be successfully parsed from the JSON
error body.

5. The `ConnectException` now uses a private constructor to provide a
strong invariant that the error detail parser is always non-nil if there
are non-empty error details. Previously, the old constructor allowed a
caller to provide non-empty details but a null error parser, in which
case the `unpackedDetails` method would not behave correctly. It also
had an awkward public `setErrorParser` method which was actually only
called from one place where the call was not even needed (this method
was not actually needed since it was previously declared as a `data`
class, which means it gets an automatic `copy` method that could
have been used to set the parser field).

This is technically a backwards-incompatible change since the public
constructor now has two fewer parameters. It is also no longer a `data`
class (since the automatic `copy` method would have allowed the new
invariant to be subverted -- a caller could invoke
`ex.copy(errorDetailParser = null)`).

Makefile

@@ -9,12 +9,13 @@ MAKEFLAGS += --no-print-directory
 BIN := .tmp/bin
 CACHE := .tmp/cache
 LICENSE_HEADER_YEAR_RANGE := 2022-2023
-LICENSE_HEADER_VERSION := v1.28.1
-CONFORMANCE_VERSION := v1.0.0-rc3
+LICENSE_HEADER_VERSION := v1.30.0
+CONFORMANCE_VERSION := v1.0.0-rc4
 PROTOC_VERSION ?= 25.3
 GRADLE_ARGS ?=
 PROTOC := $(BIN)/protoc
 CONNECT_CONFORMANCE := $(BIN)/connectconformance
+CONNECT_CONFORMANCE_ARGS ?= -v --mode client --trace
 
 UNAME_OS := $(shell uname -s)
 UNAME_ARCH := $(shell uname -m)
@@ -42,34 +43,34 @@ clean: ## Cleans the underlying build.
 .PHONY: runconformance
 runconformance: generate $(CONNECT_CONFORMANCE) ## Run the new conformance test suite.
 	./gradlew $(GRADLE_ARGS) conformance:client:google-java:installDist conformance:client:google-javalite:installDist
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/lite-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
 		--style suspend
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/lite-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
 		--style callback
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/lite-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
 		--style blocking
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/standard-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-java/build/install/google-java/bin/google-java \
 		--style suspend
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/standard-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-java/build/install/google-java/bin/google-java \
 		--style callback
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/standard-unary-config.yaml \
 		--known-failing @conformance/client/known-failing-unary-cases.txt -- \
 		conformance/client/google-java/build/install/google-java/bin/google-java \
 		--style blocking
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-stream-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/lite-stream-config.yaml \
 		--known-failing @conformance/client/known-failing-stream-cases.txt -- \
 		conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite
-	$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-stream-config.yaml \
+	$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/standard-stream-config.yaml \
 		--known-failing @conformance/client/known-failing-stream-cases.txt -- \
 		conformance/client/google-java/build/install/google-java/bin/google-java
 

conformance/client/google-java/src/main/kotlin/com/connectrpc/conformance/client/java/JavaHelpers.kt

@@ -251,7 +251,7 @@ class JavaHelpers {
     }
 
     private class TlsCredsImpl(
-        private val msg: com.connectrpc.conformance.v1.ClientCompatRequest.TLSCreds,
+        private val msg: com.connectrpc.conformance.v1.TLSCreds,
     ) : TlsCreds {
         override val cert: ByteString
             get() = msg.cert

conformance/client/google-javalite/src/main/kotlin/com/connectrpc/conformance/client/javalite/JavaLiteHelpers.kt

@@ -233,7 +233,7 @@ class JavaLiteHelpers {
     }
 
     private class TlsCredsImpl(
-        private val msg: com.connectrpc.conformance.v1.ClientCompatRequest.TLSCreds,
+        private val msg: com.connectrpc.conformance.v1.TLSCreds,
     ) : TlsCreds {
         override val cert: ByteString
             get() = msg.cert

conformance/client/known-failing-unary-cases.txt

@@ -1,32 +1,13 @@
-# If error body is JSON null, it is interpreted as an unknown error
-# instead of falling back to basing code on the HTTP status.
-Connect Error and End-Stream/**/error/null
-
 # Deadline headers are not currently set.
 Deadline Propagation/**
 
-# This response doesn't look like a normal Connect response, so it
-# goes through okhttp's default 408 code handling, which retries.
-# The retry triggers an error:
-#     client sent another request (#2) for the same test case
-HTTP to Connect Code Mapping/**/request-timeout
-
 # Bug: response content-type is not correctly checked
-Unexpected Responses/**/unexpected-content-type
+**/unexpected-content-type
 
 # Bug: "trailers-only" responses are not correctly identified.
 # If headers contain "grpc-status", this client assumes it is a
 # trailers-only response. However, a trailers-only response should
 # instead be identified by lack of body or HTTP trailers.
 gRPC Unexpected Responses/**/trailers-only/*
+gRPC-Web Unexpected Responses/**/trailers-only/ignore-header-if-body-present
 
-# Bug: if gRPC unary response contains zero messages or
-# more than one message, client does not complain if
-# status trailers says "ok"
-gRPC Unexpected Responses/**/unary-multiple-responses
-gRPC Unexpected Responses/**/unary-ok-but-no-response
-gRPC-Web Unexpected Responses/**/unary-ok-but-no-response
-
-# Bug: incorrect code attribution for these failures (INTERNAL instead of UNKNOWN)
-gRPC-Web Unexpected Responses/**/missing-status
-gRPC-Web Unexpected Responses/**/trailers-in-body/unary-multiple-responses

conformance/client/src/main/kotlin/com/connectrpc/conformance/client/Client.kt

@@ -385,10 +385,15 @@ class Client(
                 )
             }
             is ResponseMessage.Failure -> {
+                // TODO: report result.headers and result.trailers independently
+                //       once reference server can report send them independently.
+                //       https://github.com/connectrpc/conformance/pull/840.
+                // Until then, always report trailers as exception metadata (which
+                // may include headers).
                 ClientResponseResult(
                     headers = result.headers,
                     error = result.cause,
-                    trailers = result.trailers,
+                    trailers = result.cause.metadata,
                     numUnsentRequests = numUnsent,
                 )
             }

conformance/client/src/main/kotlin/com/connectrpc/conformance/client/adapt/ClientStreamClient.kt

@@ -103,7 +103,7 @@ abstract class ClientStreamClient<Req : MessageLite, Resp : MessageLite>(
                             return ResponseMessage.Failure(
                                 cause = connEx,
                                 headers = underlying.responseHeaders().await(),
-                                trailers = connEx.metadata,
+                                trailers = underlying.responseTrailers().await(),
                             )
                         }
                     }

library/src/main/kotlin/com/connectrpc/Code.kt

@@ -47,32 +47,24 @@ enum class Code(val codeName: String, val value: Int) {
         fun fromHTTPStatus(status: Int?): Code {
             return when (status) {
                 null -> UNKNOWN
-                400 -> INVALID_ARGUMENT
+                400 -> INTERNAL_ERROR
                 401 -> UNAUTHENTICATED
                 403 -> PERMISSION_DENIED
                 404 -> UNIMPLEMENTED
-                408 -> DEADLINE_EXCEEDED
-                409 -> ABORTED
-                412 -> FAILED_PRECONDITION
-                413 -> RESOURCE_EXHAUSTED
-                415 -> INTERNAL_ERROR
-                429 -> UNAVAILABLE
-                431 -> RESOURCE_EXHAUSTED
-                499 -> CANCELED
-                502, 503, 504 -> UNAVAILABLE
+                429, 502, 503, 504 -> UNAVAILABLE
                 else -> UNKNOWN
             }
         }
-        fun fromName(name: String?): Code {
+        fun fromName(name: String?, ifNotKnown: Code = UNKNOWN): Code {
             if (name == null) {
-                return UNKNOWN
+                return ifNotKnown
             }
             for (value in values()) {
                 if (value.codeName == name) {
                     return value
                 }
             }
-            return UNKNOWN
+            return ifNotKnown
         }
         fun fromValue(value: Int?): Code? {
             if (value == null) {

library/src/main/kotlin/com/connectrpc/ConnectErrorDetail.kt

@@ -16,14 +16,16 @@ package com.connectrpc
 
 import okio.ByteString
 
-// An ErrorDetail is a self-describing Protobuf message attached to an [*Error].
+// A ConnectErrorDetail is a self-describing Protobuf message attached to a
+// [ConnectException].
+//
 // Error details are sent over the network to clients, which can then work with
 // strongly-typed data rather than trying to parse a complex error message. For
 // example, you might use details to send a localized error message or retry
 // parameters to the client.
 //
-// The [google.golang.org/genproto/googleapis/rpc/errdetails] package contains a
-// variety of Protobuf messages commonly used as error details.
+// The [com.google.rpc](https://googleapis.github.io/googleapis/java/all/latest/apidocs/com/google/rpc/package-summary.html)
+// package contains a variety of Protobuf messages commonly used as error details.
 data class ConnectErrorDetail(
     val type: String,
     val payload: ByteString,

library/src/main/kotlin/com/connectrpc/ConnectException.kt

@@ -20,19 +20,30 @@ import kotlin.reflect.KClass
  * Typed error provided by Connect RPCs that may optionally wrap additional typed custom errors
  * using [details].
  */
-data class ConnectException(
+class ConnectException private constructor(
     // The resulting status code.
     val code: Code,
-    private val errorDetailParser: ErrorDetailParser? = null,
     // User-readable error message.
-    override val message: String? = null,
+    override val message: String?,
     // Client-side exception that occurred, resulting in the error.
-    val exception: Throwable? = null,
-    // List of typed errors that were provided by the server.
-    val details: List<ConnectErrorDetail> = emptyList(),
+    val exception: Throwable?,
     // Additional key-values that were provided by the server.
-    val metadata: Headers = emptyMap(),
+    val metadata: Headers,
+    // Optional parser for messages in details. Will be non-null if
+    // details is non-empty.
+    private val errorDetailParser: ErrorDetailParser?,
+    // List of typed errors that were provided by the server.
+    val details: List<ConnectErrorDetail>,
 ) : Exception(message, exception) {
+    /**
+     * Constructs a new ConnectException.
+     */
+    constructor (
+        code: Code,
+        message: String? = null,
+        exception: Throwable? = null,
+        metadata: Headers = emptyMap(),
+    ) : this(code, message, exception, metadata, null, emptyList())
 
     /**
      * Unpacks values from [details] and returns the first matching error, if any.
@@ -51,16 +62,31 @@ data class ConnectException(
     }
 
     /**
-     * Creates a new [ConnectException] with the specified [ErrorDetailParser].
+     * Creates a new [ConnectException] with the specified error details and
+     * accompanying (non-null) detail parser.
      */
-    fun setErrorParser(errorParser: ErrorDetailParser): ConnectException {
+    fun withErrorDetails(errorParser: ErrorDetailParser, details: List<ConnectErrorDetail>): ConnectException {
         return ConnectException(
             code,
-            errorParser,
             message,
             exception,
+            metadata,
+            errorParser,
             details,
+        )
+    }
+
+    /**
+     * Creates a new [ConnectException] with the specified metadata.
+     */
+    fun withMetadata(metadata: Headers): ConnectException {
+        return ConnectException(
+            code,
+            message,
+            exception,
             metadata,
+            errorDetailParser,
+            details,
         )
     }
 }

library/src/main/kotlin/com/connectrpc/impl/ClientOnlyStream.kt

@@ -19,7 +19,9 @@ import com.connectrpc.ClientOnlyStreamInterface
 import com.connectrpc.Code
 import com.connectrpc.ConnectException
 import com.connectrpc.Headers
+import com.connectrpc.asConnectException
 import kotlinx.coroutines.Deferred
+import kotlinx.coroutines.channels.ClosedReceiveChannelException
 
 /**
  * Concrete implementation of [ClientOnlyStreamInterface].
@@ -35,12 +37,23 @@ internal class ClientOnlyStream<Input, Output>(
         val resultChannel = messageStream.responseChannel()
         try {
             messageStream.sendClose()
-            val message = resultChannel.receive()
+            val message = resultChannel.receiveCatching()
+            if (message.isFailure) {
+                val ex = message.exceptionOrNull()
+                if (ex == null || ex is ClosedReceiveChannelException) {
+                    throw ConnectException(
+                        code = Code.UNIMPLEMENTED,
+                        message = "unary stream has no messages",
+                        exception = ex,
+                    )
+                }
+                throw asConnectException(ex)
+            }
             val additionalMessage = resultChannel.receiveCatching()
             if (additionalMessage.isSuccess) {
-                throw ConnectException(code = Code.UNKNOWN, message = "unary stream has multiple messages")
+                throw ConnectException(code = Code.UNIMPLEMENTED, message = "unary stream has multiple messages")
             }
-            return message
+            return message.getOrThrow()
         } finally {
             resultChannel.cancel()
         }

library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt

@@ -109,11 +109,10 @@ class ProtocolClient(
                     )
                     return@httpClientUnary
                 }
-                val exception = finalResponse.cause?.setErrorParser(serializationStrategy.errorDetailParser())
-                if (exception != null) {
+                if (finalResponse.cause != null) {
                     onResult(
                         ResponseMessage.Failure(
-                            exception,
+                            finalResponse.cause,
                             finalResponse.headers,
                             finalResponse.trailers,
                         ),

library/src/main/kotlin/com/connectrpc/protocols/ConnectInterceptor.kt

@@ -106,7 +106,6 @@ internal class ConnectInterceptor(
                         trailers = trailers,
                         cause = ConnectException(
                             code = Code.INTERNAL_ERROR,
-                            errorDetailParser = serializationStrategy.errorDetailParser(),
                             message = e.message,
                             exception = e,
                         ),
@@ -234,10 +233,11 @@ internal class ConnectInterceptor(
             StreamResult.Complete(
                 cause = ConnectException(
                     code = code,
-                    errorDetailParser = serializationStrategy.errorDetailParser(),
                     message = endStreamResponseJSON.error.message,
-                    details = parseErrorDetails(endStreamResponseJSON.error),
                     metadata = metadata.orEmpty(),
+                ).withErrorDetails(
+                    serializationStrategy.errorDetailParser(),
+                    parseErrorDetails(endStreamResponseJSON.error),
                 ),
             )
         }
@@ -246,23 +246,24 @@ internal class ConnectInterceptor(
     private fun parseConnectUnaryException(httpStatus: Int?, headers: Headers, source: Buffer?): ConnectException {
         val code = Code.fromHTTPStatus(httpStatus)
         if (source == null) {
-            return ConnectException(code, serializationStrategy.errorDetailParser(), "unexpected status code: $httpStatus")
+            return ConnectException(code, "unexpected status code: $httpStatus")
         }
         return source.use { bufferedSource ->
             val adapter = moshi.adapter(ErrorPayloadJSON::class.java).nonNull()
             val errorJSON = bufferedSource.readUtf8()
             val errorPayloadJSON = try {
                 adapter.fromJson(errorJSON)
             } catch (e: Exception) {
-                return ConnectException(code, serializationStrategy.errorDetailParser(), errorJSON, e)
+                return ConnectException(code, errorJSON, e)
             }
             val errorDetails = parseErrorDetails(errorPayloadJSON!!)
             ConnectException(
-                code = Code.fromName(errorPayloadJSON.code),
-                errorDetailParser = serializationStrategy.errorDetailParser(),
+                code = Code.fromName(errorPayloadJSON.code, code),
                 message = errorPayloadJSON.message,
-                details = errorDetails,
                 metadata = headers,
+            ).withErrorDetails(
+                serializationStrategy.errorDetailParser(),
+                errorDetails,
             )
         }
     }

library/src/main/kotlin/com/connectrpc/protocols/Envelope.kt

@@ -67,11 +67,9 @@ class Envelope {
             }
             val headerByte = source.readByte().toInt()
             val length = source.readInt().toLong()
-            val message = if (source.size > length) {
+            val message = if (source.size >= length) {
                 // extract relevant subset for this message
                 Buffer().write(source as Source, length)
-            } else if (source.size == length) {
-                source
             } else {
                 throw ConnectException(
                     code = Code.INTERNAL_ERROR,

library/src/main/kotlin/com/connectrpc/protocols/GRPCCompletion.kt

@@ -46,10 +46,11 @@ internal data class GRPCCompletion(
         } else {
             ConnectException(
                 code = code,
-                errorDetailParser = serializationStrategy.errorDetailParser(),
                 message = message,
-                details = errorDetails,
                 metadata = metadata,
+            ).withErrorDetails(
+                serializationStrategy.errorDetailParser(),
+                errorDetails,
             )
         }
     }