Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ 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
CONNECT_CONFORMANCE := $(BIN)/connectconformance
CONNECT_CONFORMANCE_ARGS ?= -v --mode client --trace

UNAME_OS := $(shell uname -s)
UNAME_ARCH := $(shell uname -m)
Expand Down Expand Up @@ -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

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
Loading