Update to rc4 of conformance suite and fix several issues #248
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhump
force-pushed
the
jh/update-to-conformance-rc4
branch
from
955909b
to
aefd8e3
Compare
|
cc: @drice-buf (just an FYI; no need to review if you are busy) |
pkwarren
approved these changes
Apr 1, 2024
… controlled via env var
jhump
added a commit
that referenced
this pull request
Apr 2, 2024
We are hoping to cut a _final_ v1.0.0 of the conformance suite this week. It has a couple of minor changes from rc4. I wanted to make sure they wouldn't cause issues (particularly, [this pending PR](connectrpc/conformance#840), which could possibly impact some of the logic I just added in #248 related to reporting trailers and merging headers+trailers in error metadata). And... this ended up exposing some issues. For one, I had not updated the Connect stream protocol in the same way as the gRPC-Web and gRPC protocols regarding merging of headers+trailers in metadata. I didn't notice in the previous PR because it was actually the "trailers only" responses that tickled test failures related to this (the connect-go reference server will only use "trailers only" responses for gRPC-Web, not for other protocols). For two, this code path was not correctly setting the trailers of the `StreamResult.Completed` object. Oops 🤦. After I fixed that, I just happened to observe a conformance test flake that caused the client to hang. It turned out to be due to uncaught exception while trying to drain the response body in a unary RPC. So that's what warranted the other changes in here, adding `try/catch` to the unary flow of `ConnectOkHttpClient` and extracting the `safeTrailers` helper so it can also be used from that unary flow. While making the above change, I greatly simplified the `safeTrailers` helper because it seemed to be incorrect: it was almost always computing empty trailers in these unary flows (which causes issues for gRPC calls, which rely on trailers to convey the status code). After re-reading the code, it's not clear why it was wrong, and I didn't have the patience to try to put it in a debugger to step through it and understand it. I just simplified it, removing the questionable conditional, and that fixed it.
pkwarren
pushed a commit
that referenced
this pull request
Apr 19, 2024
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)`).
pkwarren
pushed a commit
that referenced
this pull request
Apr 19, 2024
We are hoping to cut a _final_ v1.0.0 of the conformance suite this week. It has a couple of minor changes from rc4. I wanted to make sure they wouldn't cause issues (particularly, [this pending PR](connectrpc/conformance#840), which could possibly impact some of the logic I just added in #248 related to reporting trailers and merging headers+trailers in error metadata). And... this ended up exposing some issues. For one, I had not updated the Connect stream protocol in the same way as the gRPC-Web and gRPC protocols regarding merging of headers+trailers in metadata. I didn't notice in the previous PR because it was actually the "trailers only" responses that tickled test failures related to this (the connect-go reference server will only use "trailers only" responses for gRPC-Web, not for other protocols). For two, this code path was not correctly setting the trailers of the `StreamResult.Completed` object. Oops 🤦. After I fixed that, I just happened to observe a conformance test flake that caused the client to hang. It turned out to be due to uncaught exception while trying to drain the response body in a unary RPC. So that's what warranted the other changes in here, adding `try/catch` to the unary flow of `ConnectOkHttpClient` and extracting the `safeTrailers` helper so it can also be used from that unary flow. While making the above change, I greatly simplified the `safeTrailers` helper because it seemed to be incorrect: it was almost always computing empty trailers in these unary flows (which causes issues for gRPC calls, which rely on trailers to convey the status code). After re-reading the code, it's not clear why it was wrong, and I didn't have the patience to try to put it in a debugger to step through it and understand it. I just simplified it, removing the questionable conditional, and that fixed it.
rebello95
added a commit
to connectrpc/connect-swift
that referenced
this pull request
Jun 8, 2024
This PR includes updates to fix all outstanding conformance test failures and removes the corresponding opt-outs. All changes are validated by the conformance test suite. Many of these fixes are similar to connectrpc/connect-kotlin#248 and connectrpc/connect-kotlin#274. Resolves #268. Resolves #269. Resolves #270. This should also be one of the final changes before v1.0 #222. --------- Signed-off-by: Michael Rebello <me@michaelrebello.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a handful of changes in here that go along with this update of the conformance tests.
This updates the HTTP code -> RPC code mappings per latest updates to the spec here.
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.
Unary and client-stream operations now properly handle cases where there is either zero or more than one response message combined with an non-error status. They return "unimplemented" as indicated by this doc on RPC status codes (search therein for "cardinality violation").
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.
The
ConnectExceptionnow 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 theunpackedDetailsmethod would not behave correctly. It also had an awkward publicsetErrorParsermethod which was actually only called from one place and was not even needed (and was technically not needed since it was previously declared as adataclass, which means it gets an automaticcopymethod 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
dataclass (since the automaticcopymethod would have allowed the new invariant to be subverted -- a caller could invokeex.copy(errorDetailParser = null)).