Fix ErrorWriter connect GET protocol classifier#654
Conversation
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.
| 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.
|
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. |
akshayjshah
left a comment
There was a problem hiding this comment.
This change is great! Supporting GETs was an unfortunate oversight, and defaulting to JSON makes sense. A few nits about the code structure and comments, but I'd love to get this landed and released ASAP :)
`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)`.
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).