search

connectrpc / connect-go

The Go implementation of Connect: Protobuf RPC that works.
https://connectrpc.com
Apache License 2.0
2.76k stars 91 forks source link

Fix ErrorWriter connect GET protocol classifier #654

Closed emcfarlane closed 6 months ago

emcfarlane commented 6 months ago

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 #637 . Defaults to connect unary errors. To support other fallbacks use the (*ErrorWriter).IsSupported() check before calling (*ErrorWriter).Write(err).

jhump commented 6 months ago

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.

mattrobenolt commented 6 months ago

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.