Skip to content

Commit

Permalink
update to rc4 of conformance suite and fix several issues
Browse files Browse the repository at this point in the history
  • Loading branch information
jhump committed Apr 1, 2024
1 parent 195b457 commit aefd8e3
Show file tree
Hide file tree
Showing 19 changed files with 207 additions and 126 deletions.
18 changes: 9 additions & 9 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ BIN := .tmp/bin
CACHE := .tmp/cache
LICENSE_HEADER_YEAR_RANGE := 2022-2023
LICENSE_HEADER_VERSION := v1.30.0
CONFORMANCE_VERSION := v1.0.0-rc3
CONFORMANCE_VERSION := v1.0.0-rc4
PROTOC_VERSION ?= 26.1
GRADLE_ARGS ?=
PROTOC := $(BIN)/protoc
Expand Down Expand Up @@ -43,34 +43,34 @@ clean: ## Cleans the underlying build.
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt --trace -- \
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 \
--known-failing @conformance/client/known-failing-stream-cases.txt -- \
--known-failing @conformance/client/known-failing-stream-cases.txt --trace -- \
conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-stream-config.yaml \
--known-failing @conformance/client/known-failing-stream-cases.txt -- \
--known-failing @conformance/client/known-failing-stream-cases.txt --trace -- \
conformance/client/google-java/build/install/google-java/bin/google-java

ifeq ($(UNAME_OS),Darwin)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 2 additions & 21 deletions conformance/client/known-failing-unary-cases.txt
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
)
}
}
Expand Down
18 changes: 5 additions & 13 deletions library/src/main/kotlin/com/connectrpc/Code.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 5 additions & 3 deletions library/src/main/kotlin/com/connectrpc/ConnectErrorDetail.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
46 changes: 36 additions & 10 deletions library/src/main/kotlin/com/connectrpc/ConnectException.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
)
}
}
Expand Down
19 changes: 16 additions & 3 deletions library/src/main/kotlin/com/connectrpc/impl/ClientOnlyStream.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand All @@ -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()
}
Expand Down
5 changes: 2 additions & 3 deletions library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ internal class ConnectInterceptor(
trailers = trailers,
cause = ConnectException(
code = Code.INTERNAL_ERROR,
errorDetailParser = serializationStrategy.errorDetailParser(),
message = e.message,
exception = e,
),
Expand Down Expand Up @@ -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),
),
)
}
Expand All @@ -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,
)
}
}
Expand Down
4 changes: 1 addition & 3 deletions library/src/main/kotlin/com/connectrpc/protocols/Envelope.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}
}
Expand Down
Loading

0 comments on commit aefd8e3

Please sign in to comment.