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

In connect unary protocol, fallback to code based on HTTP status if unable to deserialize it #702

Closed jhump closed 4 months ago

jhump commented 4 months ago

The conformance tests, as of v1.0.0-rc3, currently require behavior that matches the current behavior of connect-go: if the error JSON body in a Connect unary response can be unmarshaled but is missing the code, it defaults to using "unknown" as the code.

However, in the Connect unary format (and not any of the others), we also have a non-200 HTTP status that could be used to determine a potentially better default than "unknown". So this PR changes it so that connect-go uses the HTTP status in this case, instead of always using "unknown". (Also see https://github.com/connectrpc/conformance/issues/810.)

It also updates the JSON unmarshaling of the error body so it accepts a "code" property with an invalid value. In this case, the invalid value is discarded, and due to the change described above, the code will instead be computed from the HTTP status. But this lenience allows us to capture the message and details, in the event that the JSON contains these other properties but with (for example) a misspelled code string.

jhump commented 4 months ago

cc @srikrsna-buf

akshayjshah commented 4 months ago

Huh, I suppose that’s clear enough then!

On Tue, Mar 5, 2024 at 6:13 PM Joshua Humphries @.***> wrote:

@.**** commented on this pull request.

In protocol_connect.go https://github.com/connectrpc/connect-go/pull/702#discussion_r1513725408 :

@@ -559,6 +559,10 @@ func (cc connectUnaryClientConn) validateResponse(response http.Response) *Err errors.New(response.Status), ) }

  • if wireErr.Code == 0 {
  • // code not set? default to one implied by HTTP status
  • wireErr.Code = connectHTTPToCode(response.StatusCode)
  • }

So do you think the existing language needs an update or clarification?

— Reply to this email directly, view it on GitHub https://github.com/connectrpc/connect-go/pull/702#discussion_r1513725408, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHNP5TK2D5ZK7DF66GCEFTYWZ3VVAVCNFSM6AAAAABD6ETCLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJYGYYTANRYG4 . You are receiving this because your review was requested.Message ID: @.***>