Specification update for error code HTTP status #130
Merged
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
This proposes updating the ConnectRPC specification to correct the following error code to HTTP status mappings. The new status codes map closer to the intended HTTP semantics and better align with the RPC ecosystem of codes, specifically those defined in [google/protobuf/code.proto](https://buf.build/googleapis/googleapis/file/main:google/rpc/code.proto). | Code | Current HTTP Status | Proposed HTTP Status | | --- | --- | --- | | cancelled | 408 Request Timeout | 499 Client Closed Request | | deadline_exceeded | 408 Request Timeout | 504 Gateway Timeout | | failed_precondition | 412 Precondition Failed | 400 Bad Request | | unimplemented | 404 Not Found | 501 Not Implemented |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
See https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md Inferred error codes are only for clients that received an invalid RPC response. Handlers must not use this mapping to determine which status code to use. They are intentionally conservative and are not symmetric. Clients should use the inferred code to handle the error condition.
akshayjshah
approved these changes
Mar 11, 2024
jhump
added a commit
to connectrpc/connect-go
that referenced
this pull request
Mar 12, 2024
This reconciles the implementation in this repo with the recent adjustments to HTTP<->Connect code mappings (connectrpc/connectrpc.com#130). The gRPC and Connect mappings from HTTP code to RPC code are now aligned, so this unifies them into a single function.
emcfarlane
added a commit
to connectrpc/vanguard-go
that referenced
this pull request
Mar 18, 2024
See (connectrpc/connectrpc.com#130). Mapping was already incorrect. Test will come with the implementation of the conformance suite in #99 .
jhump
added a commit
to connectrpc/conformance
that referenced
this pull request
Mar 21, 2024
…nect-go (#826) This updates to the latest connect-go release and reconciles the test cases with the latest spec changes (from connectrpc/connectrpc.com#130). This also enables some other tests that were waiting on connect-go fixes and cleans up the "known failing" lists based on these fixes and the new test cases. Finally, there are some tweaks to a few gRPC test cases, related to relaxing error code assertions.
jhump
added a commit
to connectrpc/connect-kotlin
that referenced
this pull request
Apr 1, 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
to connectrpc/connect-kotlin
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)`).
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.
The RFC for this protocol change is open for voting as #148.
This PR proposes updating the Connect specification to correct the following error code to HTTP status mappings. The new status codes map closer to the intended HTTP semantics and better align with the RPC ecosystem of codes, specifically those defined in google/protobuf/code.proto.
Reasons for proposed changes
408 Request Timeout
The 408 HTTP status is not suitable for RPC errors. Current
cancelledanddeadline_exceededaren’t applicable as the request body was not timed out. Additionally, the definition defines the client MAY repeat the request which is not the intended behaviour of either code.412 Precondition Failed
The 412 HTTP status indicates that HTTP headers specified in the request code, such as
If-Unmodified-SinceorIf-None-Match, were used to conditionally apply the request. This isn’t the defined behaviour offailed_preconditionwhich states the system state is not valid for the request and is unrelated to header values.404 Not Found
The 404 HTTP status is used to indicate the resource doesn’t exist or the server doesn’t want to disclose the existence. For
unimplementeda 501 Not Implemented is more applicable as that states that it’s the server that does not support the functionality required to fulfill the request not a resource issue.499 Client Closed Request
The 499 HTTP status is a special case of the 502 Bad Gateway status code. It is adopted from the Nginx’s extended definition of 4XX errors to handle client errors. It indicates the client closed the connection whilst the server was still processing the request which aligns with the
cancelederror codes intent.Impact
The proposed changes will necessitate updates in each implementation. Fortunately, all existing ConnectRPC organization libraries decode error codes from the response body, irrespective of the HTTP status on valid payloads. This behavior ensures backward compatibility, allowing the libraries to seamlessly accommodate the newly specified HTTP statuses without any adverse effects.
Resolves #122