Fix ErrorWriter connect GET protocol classifier #654
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
ErrorWriter will now correctly classifying connect GET requests if the connect protocol version header is present or the connect protocol version is set in the URL query parameters. To handle connect GET requests without the protocol version the ErrorWriter will write errors as connect unary errors if the protocol is unknown. This ensures the error is written to the client. The connect unary error payload is always in JSON and therefore human-readable to the caller. Clients are still recommended to check `IsSupported` first to ensure the protocol will match the desired and to provide fallback to other error writers.
emcfarlane
commented
Dec 12, 2023
| @@ -394,6 +394,14 @@ func TestServer(t *testing.T) { | |||
| connect.WithSendGzip(), | |||
| ) | |||
| }) | |||
| t.Run("json_get", func(t *testing.T) { | |||
There was a problem hiding this comment.
As a test case I've extended TestServer to include an option with GET set. We exercise error writer in the middleware tests and this covers the case with ping as a GET and a large ping as a fallback to POST.
akshayjshah
reviewed
Dec 12, 2023
|
It appears this would also address #637. I think the only potentially controversial part in here is the change to always fall back to the Connect protocol's JSON error format output even when the incoming protocol cannot be determined. I'm okay with it, but am curious if @akshayjshah or @mattrobenolt have objections. |
|
I think this is fine. This behavior wouldn't affect us. I use ErrorWriter.IsSupported() to spit back a plaintext error, and we don't use GET requests for anything. |
jhump
approved these changes
Dec 14, 2023
akshayjshah
reviewed
Dec 14, 2023
akshayjshah
approved these changes
Dec 14, 2023
bufdev
approved these changes
Dec 22, 2023
emcfarlane
added a commit
to emcfarlane/connect-go
that referenced
this pull request
Jan 1, 2024
`ErrorWriter` will now correctly classifying connect GET requests if the connect protocol version header is present or the connect protocol version is set in the URL query parameters. To handle connect GET requests without the protocol version the ErrorWriter will write errors as connect unary errors if the protocol is unknown. This ensures the error is always written to the client. The connect unary error payload is always in JSON and therefore makes a nice human-readable format for the caller. Clients are still recommended to check `IsSupported` first to ensure the protocol will match the desired and to provide fallback to other error writers if desired. Fix for connectrpc#637 . Defaults to connect unary errors. To support other fallbacks use the `(*ErrorWriter).IsSupported()` check before calling `(*ErrorWriter).Write(err)`.
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.
ErrorWriterwill now correctly classifying connect GET requests if the connect protocol version header is present or the connect protocol version is set in the URL query parameters. To handle connect GET requests without the protocol version the ErrorWriter will write errors as connect unary errors if the protocol is unknown. This ensures the error is always written to the client. The connect unary error payload is always in JSON and therefore makes a nice human-readable format for the caller. Clients are still recommended to checkIsSupportedfirst to ensure the protocol will match the desired and to provide fallback to other error writers if desired.Fix for #637 . Defaults to connect unary errors. To support other fallbacks use the
(*ErrorWriter).IsSupported()check before calling(*ErrorWriter).Write(err).