In connect unary protocol, fallback to code based on HTTP status if unable to deserialize it#702
In connect unary protocol, fallback to code based on HTTP status if unable to deserialize it#702
Conversation
… code from JSON error body
akshayjshah
left a comment
There was a problem hiding this comment.
Some small asks, but the approach lgtm.
| if wireErr.Code == 0 { | ||
| // code not set? default to one implied by HTTP status | ||
| wireErr.Code = connectHTTPToCode(response.StatusCode) | ||
| } |
There was a problem hiding this comment.
Can you also open a PR to add this nuance to the wire protocol docs? This feels less like a behavioral specification and more like an aspect of the wire protocol itself.
There was a problem hiding this comment.
I was in the process of adding this and then noticed this existing paragraph in the section of the spec that describes the status code of the Unary-Response production:
When reading data from the wire, client implementations must use the HTTP-to-Connect mapping to infer a Connect error code if Bare-Message is missing or malformed.
IMO, this falls into this category. So it's arguable that always defaulting to "unknown" (as was previously done) was actually a bug.
There was a problem hiding this comment.
So do you think the existing language needs an update or clarification?
|
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
<#702 (comment)>
:
> @@ -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
<#702 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHNP5TK2D5ZK7DF66GCEFTYWZ3VVAVCNFSM6AAAAABD6ETCLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJYGYYTANRYG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
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 connectrpc/conformance#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.