Skip to content

Commit

Permalink
Update to v1.0.0-rc3 of the conformance suite (#226)
Browse files Browse the repository at this point in the history
These new test cases found some uncaught-exception bugs that are fixed
in here. It also found some other small bugs in handling
anomalous/erroneous responses. These are not high priority, so instead
of trying to fix all of them, I've documented them all in the known
failing config files.

Release notes for connectrpc/conformance v1.0.0-rc3:
https://github.com/connectrpc/conformance/releases/tag/v1.0.0-rc3
  • Loading branch information
jhump authored Feb 23, 2024
1 parent db3e4da commit bb0db27
Show file tree
Hide file tree
Showing 10 changed files with 118 additions and 53 deletions.
25 changes: 16 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.28.1
CONFORMANCE_VERSION := v1.0.0-rc2
CONFORMANCE_VERSION := v1.0.0-rc3
PROTOC_VERSION ?= 25.3
GRADLE_ARGS ?=
PROTOC := $(BIN)/protoc
Expand Down Expand Up @@ -46,34 +46,34 @@ runconformance: runcrosstests runconformancenew
runconformancenew: 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 -- \
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 -- \
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 -- \
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 -- \
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 -- \
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 -- \
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 -- \
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 -- \
conformance/client/google-java/build/install/google-java/bin/google-java

.PHONY: runcrosstests
Expand Down Expand Up @@ -125,13 +125,20 @@ $(CONNECT_CONFORMANCE): $(CONNECT_CONFORMANCE_DOWNLOAD)

.PHONY: generate
generate: $(PROTOC) buildplugin generateconformance generateexamples ## Generate proto files for the entire project.
rm -rf protoc-gen-connect-kotlin/build/generated/sources/bufgen || true
buf generate --template protoc-gen-connect-kotlin/buf.gen.yaml -o protoc-gen-connect-kotlin protoc-gen-connect-kotlin/proto
rm -rf extensions/google-java/build/generated/sources/bufgen || true
rm -rf extensions/google-javalite/build/generated/sources/bufgen || true
buf generate --template extensions/buf.gen.yaml -o extensions buf.build/googleapis/googleapis
make licenseheaders

.PHONY: generateconformance
generateconformance: $(PROTOC) buildplugin ## Generate protofiles for conformance tests.
rm -rf conformance/google-java/build/generated/sources/bufgen || true
rm -rf conformance/google-javalite/build/generated/sources/bufgen || true
buf generate --template conformance/buf.gen.yaml -o conformance conformance/proto
rm -rf conformance/client/google-java/build/generated/sources/bufgen || true
rm -rf conformance/client/google-javalite/build/generated/sources/bufgen || true
buf generate --template conformance/buf.gen.yaml -o conformance/client buf.build/connectrpc/conformance:$(CONFORMANCE_VERSION)

.PHONY: generateexamples
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.connectrpc.conformance.v1.ClientCompatRequest.Cancel.CancelTimingCase
import com.connectrpc.conformance.v1.ClientErrorResult
import com.connectrpc.conformance.v1.ClientResponseResult
import com.connectrpc.conformance.v1.ClientStreamResponse
import com.connectrpc.conformance.v1.Code
import com.connectrpc.conformance.v1.Codec
import com.connectrpc.conformance.v1.Compression
import com.connectrpc.conformance.v1.ConfigProto
Expand Down Expand Up @@ -137,7 +138,7 @@ class JavaHelpers {

private fun toProtoError(ex: ConnectException): Error {
return Error.newBuilder()
.setCode(ex.code.value)
.setCode(toProtoCode(ex.code))
.setMessage(ex.message ?: ex.code.codeName)
.addAllDetails(
ex.details.map {
Expand All @@ -150,6 +151,10 @@ class JavaHelpers {
.build()
}

private fun toProtoCode(code: com.connectrpc.Code): Code {
return Code.forNumber(code.value) ?: Code.CODE_UNKNOWN
}

private fun toTypeUrl(typeName: String): String {
return if (typeName.contains('/')) typeName else TYPE_URL_PREFIX + typeName
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.connectrpc.conformance.v1.ClientCompatRequest.Cancel.CancelTimingCase
import com.connectrpc.conformance.v1.ClientErrorResult
import com.connectrpc.conformance.v1.ClientResponseResult
import com.connectrpc.conformance.v1.ClientStreamResponse
import com.connectrpc.conformance.v1.Code
import com.connectrpc.conformance.v1.Codec
import com.connectrpc.conformance.v1.Compression
import com.connectrpc.conformance.v1.ConformancePayload
Expand Down Expand Up @@ -129,7 +130,7 @@ class JavaLiteHelpers {

private fun toProtoError(ex: ConnectException): Error {
return Error.newBuilder()
.setCode(ex.code.value)
.setCode(toProtoCode(ex.code))
.setMessage(ex.message ?: ex.code.codeName)
.addAllDetails(
ex.details.map {
Expand All @@ -142,6 +143,10 @@ class JavaLiteHelpers {
.build()
}

private fun toProtoCode(code: com.connectrpc.Code): Code {
return Code.forNumber(code.value) ?: Code.CODE_UNKNOWN
}

private fun toTypeUrl(typeName: String): String {
return if (typeName.contains('/')) typeName else TYPE_URL_PREFIX + typeName
}
Expand Down
9 changes: 7 additions & 2 deletions conformance/client/known-failing-stream-cases.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,10 @@
# RPC deadlines, but that is not enforced when the request
# body is duplex. So timeouts don't currently work with
# bidi streams.
Timeouts/HTTPVersion:2/**/bidi half duplex timeout
Timeouts/HTTPVersion:2/**/bidi full duplex timeout
Timeouts/HTTPVersion:2/**/bidi-stream/**

# Deadline headers are not currently set.
Deadline Propagation/**

# Bug: incorrect code attribution for these failures (UNKNOWN instead of INTERNAL)
Connect Unexpected Responses/**/unexpected-stream-codec
35 changes: 32 additions & 3 deletions conformance/client/known-failing-unary-cases.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,32 @@
# Test runner is overly strict in asserting the contents
# of the message query param, so these tests currently fail.
Idempotency/**
# 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

# 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/*

# 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 @@ -14,13 +14,13 @@

package com.connectrpc.conformance.client

import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.Headers
import com.connectrpc.ProtocolClientConfig
import com.connectrpc.RequestCompression
import com.connectrpc.ResponseMessage
import com.connectrpc.SerializationStrategy
import com.connectrpc.asConnectException
import com.connectrpc.compression.GzipCompressionPool
import com.connectrpc.conformance.client.adapt.AnyMessage
import com.connectrpc.conformance.client.adapt.BidiStreamClient
Expand Down Expand Up @@ -228,17 +228,8 @@ class Client(
}
} catch (ex: Throwable) {
if (!sent) {
val connEx = if (ex is ConnectException) {
ex
} else {
ConnectException(
code = Code.UNKNOWN,
message = ex.message,
exception = ex,
)
}
return ClientResponseResult(
error = connEx,
error = asConnectException(ex),
numUnsentRequests = 1,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
package com.connectrpc.conformance.client.adapt

import com.connectrpc.ClientOnlyStreamInterface
import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.Headers
import com.connectrpc.ResponseMessage
import com.connectrpc.asConnectException
import com.google.protobuf.MessageLite
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.channels.ReceiveChannel
Expand Down Expand Up @@ -99,16 +98,12 @@ abstract class ClientStreamClient<Req : MessageLite, Resp : MessageLite>(
headers = underlying.responseHeaders().await(),
trailers = underlying.responseTrailers().await(),
)
} catch (e: Exception) {
val connectException = if (e is ConnectException) {
e
} else {
ConnectException(code = Code.UNKNOWN, exception = e)
}
} catch (ex: Exception) {
val connEx = asConnectException(ex)
return ResponseMessage.Failure(
cause = connectException,
cause = connEx,
headers = underlying.responseHeaders().await(),
trailers = connectException.metadata,
trailers = connEx.metadata,
)
}
}
Expand Down
13 changes: 13 additions & 0 deletions library/src/main/kotlin/com/connectrpc/ConnectException.kt
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,16 @@ data class ConnectException(
)
}
}

/**
* Returns a ConnectException for the given cause. If ex is a ConnectException
* then it is returned. Otherwise, it is wrapped in a ConnectException with
* the given code.
*/
fun asConnectException(ex: Throwable, code: Code = Code.UNKNOWN): ConnectException {
return if (ex is ConnectException) {
ex
} else {
ConnectException(code = code, exception = ex)
}
}
40 changes: 29 additions & 11 deletions library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import com.connectrpc.ServerOnlyStreamInterface
import com.connectrpc.StreamResult
import com.connectrpc.StreamType
import com.connectrpc.UnaryBlockingCall
import com.connectrpc.asConnectException
import com.connectrpc.http.Cancelable
import com.connectrpc.http.HTTPClientInterface
import com.connectrpc.http.HTTPRequest
import com.connectrpc.http.HTTPResponse
import com.connectrpc.http.UnaryHTTPRequest
import com.connectrpc.http.dispatchIn
import com.connectrpc.http.transform
Expand Down Expand Up @@ -94,7 +96,20 @@ class ProtocolClient(
val unaryFunc = config.createInterceptorChain()
val finalRequest = unaryFunc.requestFunction(unaryRequest)
val cancelable = httpClient.unary(finalRequest) httpClientUnary@{ httpResponse ->
val finalResponse = unaryFunc.responseFunction(httpResponse)
val finalResponse: HTTPResponse
try {
finalResponse = unaryFunc.responseFunction(httpResponse)
} catch (ex: Throwable) {
val connEx = asConnectException(ex)
onResult(
ResponseMessage.Failure(
connEx,
emptyMap(),
connEx.metadata,
),
)
return@httpClientUnary
}
val exception = finalResponse.cause?.setErrorParser(serializationStrategy.errorDetailParser())
if (exception != null) {
onResult(
Expand All @@ -110,10 +125,10 @@ class ProtocolClient(
val responseMessage: Output
try {
responseMessage = responseCodec.deserialize(finalResponse.message)
} catch (e: Exception) {
} catch (ex: Exception) {
onResult(
ResponseMessage.Failure(
ConnectException(code = Code.INTERNAL_ERROR, exception = e),
asConnectException(ex, Code.INTERNAL_ERROR),
finalResponse.headers,
finalResponse.trailers,
),
Expand All @@ -129,8 +144,16 @@ class ProtocolClient(
)
}
return cancelable
} catch (e: Exception) {
throw e
} catch (ex: Exception) {
val connEx = asConnectException(ex)
onResult(
ResponseMessage.Failure(
connEx,
emptyMap(),
connEx.metadata,
),
)
return { }
}
}

Expand Down Expand Up @@ -228,12 +251,7 @@ class ProtocolClient(
try {
streamResult = streamFunc.streamResultFunction(initialResult)
} catch (ex: Throwable) {
val connEx = if (ex is ConnectException) {
ex
} else {
ConnectException(code = Code.UNKNOWN, exception = ex)
}
streamResult = StreamResult.Complete(connEx)
streamResult = StreamResult.Complete(asConnectException(ex))
}
when (streamResult) {
is StreamResult.Headers -> {
Expand Down
7 changes: 2 additions & 5 deletions okhttp/src/main/kotlin/com/connectrpc/okhttp/OkHttpStream.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package com.connectrpc.okhttp
import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.StreamResult
import com.connectrpc.asConnectException
import com.connectrpc.http.HTTPMethod
import com.connectrpc.http.HTTPRequest
import com.connectrpc.http.Stream
Expand Down Expand Up @@ -134,11 +135,7 @@ private class ResponseCallback(
// This is the final chance to notify trailers to the consumer.
val connectEx = when (exception) {
null -> null
is ConnectException -> exception
else -> ConnectException(
code = codeFromException(call.isCanceled(), exception),
exception = exception,
)
else -> asConnectException(exception, codeFromException(call.isCanceled(), exception))
}
val finalResult = StreamResult.Complete<Buffer>(
trailers = response.safeTrailers() ?: emptyMap(),
Expand Down

0 comments on commit bb0db27

Please sign in to comment.