Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee3ee6a
in connect unary protocol, fallback to http status if can't unmarshal…
jhump Feb 21, 2024
84d7f85
review feedback: cleanup json struct tags for unmarshal-only type
jhump Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal/conformance/known-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# The current v1.0.0-rc3 of conformance suite wants to see "unknown"
# as the status for Connect unary responses where the JSON error body
# is missing the 'code' property. But we instead want clients to
# synthesize an error code from the HTTP status code. That way, if
# a proxy or middle-box happens to reply with a JSON error, but not
# a valid *Connect* error, we can use the HTTP status to derive an
# error code, just like we do when the response has an unexpected
# content type.
#
# So after we fix the tests in the conformance suite, we can remove
# these lines below.
Connect Error and End-Stream/**/error/missing-code
Connect Error and End-Stream/**/error/null
Connect Error and End-Stream/**/error/null-code
2 changes: 1 addition & 1 deletion internal/conformance/runconformance.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ $GO build -o $BINDIR/referenceclient connectrpc.com/conformance/cmd/referencecli
$GO build -o $BINDIR/referenceserver connectrpc.com/conformance/cmd/referenceserver

echo "Running conformance tests against client..."
$BINDIR/connectconformance --mode client --conf config.yaml -v --trace -- $BINDIR/referenceclient
$BINDIR/connectconformance --mode client --conf config.yaml --known-failing @known-failing.txt -v --trace -- $BINDIR/referenceclient

echo "Running conformance tests against server..."
$BINDIR/connectconformance --mode server --conf config.yaml -v --trace -- $BINDIR/referenceserver
24 changes: 24 additions & 0 deletions protocol_connect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that.

Copy link
Member Author

@jhump jhump Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

serverErr := wireErr.asError()
if serverErr == nil {
return nil
Expand Down Expand Up @@ -1245,6 +1249,26 @@ func (e *connectWireError) asError() *Error {
return err
}

func (e *connectWireError) UnmarshalJSON(data []byte) error {
// We want to be lenient if the JSON has an unrecognized or invalid code.
// So if that occurs, we leave the code unset but can still de-serialize
// the other fields from the input JSON.
var wireError struct {
Code string `json:"code"`
Message string `json:"message"`
Details []*connectWireDetail `json:"details"`
}
err := json.Unmarshal(data, &wireError)
if err != nil {
return err
}
e.Message = wireError.Message
e.Details = wireError.Details
// This will leave e.Code unset if we can't unmarshal the given string.
_ = e.Code.UnmarshalText([]byte(wireError.Code))
return nil
}

type connectEndStreamMessage struct {
Error *connectWireError `json:"error,omitempty"`
Trailer http.Header `json:"metadata,omitempty"`
Expand Down
Loading