Skip to content

Commit

Permalink
Update to conformance suite v1.0.1 (#253)
Browse files Browse the repository at this point in the history
This includes a few minor fixes, too:
1. For Connect unary RPCs, the error metadata included only headers, but
should include both headers and trailers.
2. For gRPC and gRPC-Web protocols, when a "trailers-only" response is
received (where there are only HTTP headers and no body or other
trailers, and the headers are to be interpreted as trailers), report
those headers as trailers.
  • Loading branch information
jhump authored Apr 5, 2024
1 parent 16a8b1d commit bcf420f
Show file tree
Hide file tree
Showing 7 changed files with 21 additions and 15 deletions.
5 changes: 2 additions & 3 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-rc4
CONFORMANCE_VERSION := v1.0.1
PROTOC_VERSION ?= 26.1
GRADLE_ARGS ?=
PROTOC := $(BIN)/protoc
Expand Down Expand Up @@ -39,9 +39,8 @@ buildplugin: ## Build the connect-kotlin protoc plugin.
clean: ## Cleans the underlying build.
./gradlew $(GRADLE_ARGS) clean

# TODO: remove the cross-tests and rely solely on new conformance suite
.PHONY: runconformance
runconformance: generate $(CONNECT_CONFORMANCE) ## Run the new conformance test suite.
runconformance: generate $(CONNECT_CONFORMANCE)
./gradlew $(GRADLE_ARGS) conformance:client:google-java:installDist conformance:client:google-javalite:installDist
$(CONNECT_CONFORMANCE) $(CONNECT_CONFORMANCE_ARGS) --conf conformance/client/lite-unary-config.yaml \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,15 +385,10 @@ 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.cause.metadata,
trailers = result.trailers,
numUnsentRequests = numUnsent,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ internal class ConnectInterceptor(
val exception: ConnectException?
val message: Buffer
if (response.status != 200) {
exception = parseConnectUnaryException(response.status, response.headers, responseBody)
exception = parseConnectUnaryException(response.status, responseHeaders.plus(trailers), responseBody)
// We've already read the response body to parse an error - don't read again.
message = Buffer()
} else {
Expand Down Expand Up @@ -244,7 +244,7 @@ internal class ConnectInterceptor(
}
}

private fun parseConnectUnaryException(httpStatus: Int?, headers: Headers, source: Buffer?): ConnectException {
private fun parseConnectUnaryException(httpStatus: Int?, metadata: Headers, source: Buffer?): ConnectException {
val code = Code.fromHTTPStatus(httpStatus)
if (source == null) {
return ConnectException(code, "unexpected status code: $httpStatus")
Expand All @@ -261,7 +261,7 @@ internal class ConnectInterceptor(
ConnectException(
code = Code.fromName(errorPayloadJSON.code, code),
message = errorPayloadJSON.message,
metadata = headers,
metadata = metadata,
).withErrorDetails(
serializationStrategy.errorDetailParser(),
errorDetails,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ internal data class GRPCCompletion(
val errorDetails: List<ConnectErrorDetail> = emptyList(),
// Set to either message headers (or trailers) where the gRPC status was found.
val metadata: Headers,
// If true, this status was parsed from headers, in a "trailers-only" response.
// Otherwise, the status was parsed from trailers.
val trailersOnly: Boolean = false,
// If false, this completion was synthesized and not actually present in metadata.
val present: Boolean = true,
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ internal class GRPCCompletionParser(
val statusCode: Int
val statusMetadata: Map<String, List<String>>
val statusFromHeaders = parseStatus(headers)
val trailersOnly: Boolean
if (statusFromHeaders == null) {
statusCode = parseStatus(trailers)
?: return GRPCCompletion(
Expand All @@ -46,9 +47,11 @@ internal class GRPCCompletionParser(
metadata = trailers,
)
statusMetadata = trailers
trailersOnly = false
} else {
statusCode = statusFromHeaders
statusMetadata = headers
trailersOnly = true
}
// Note: we report combined headers and trailers as exception meta, so
// caller doesn't have to check both, which is particularly important
Expand All @@ -60,6 +63,7 @@ internal class GRPCCompletionParser(
message = parseMessage(statusMetadata).utf8(),
errorDetails = connectErrorDetails(statusMetadata),
metadata = exceptionMeta,
trailersOnly = trailersOnly,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,13 @@ internal class GRPCInterceptor(
)
}
val headers = response.headers
val trailers = response.trailers
val exception = completionParser
var trailers = response.trailers
val completion = completionParser
.parse(headers, trailers)
.toConnectExceptionOrNull(serializationStrategy)
if (completion.trailersOnly) {
trailers = headers // report the headers also as trailers
}
val exception = completion.toConnectExceptionOrNull(serializationStrategy)
val message = if (exception == null) {
if (response.message.buffer.exhausted()) {
return@UnaryFunction response.clone(
Expand Down Expand Up @@ -112,6 +115,7 @@ internal class GRPCInterceptor(
response.clone(
message = message,
cause = exception,
trailers = trailers,
)
},
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ internal class GRPCWebInterceptor(
response.clone(
message = Buffer(),
cause = exception,
trailers = headers,
)
} else {
// Unpack the current message and trailers.
Expand Down

0 comments on commit bcf420f

Please sign in to comment.