In Connect unary protocol, fallback to code based on HTTP status if u…

…nable to deserialize it (#702)

The spec states the following:

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.

However, connect-go was only considering the message to be
malformed if json.Unmarshal failed. Because Go's encoding/json
package is fairly lenient in many ways, there were many kinds of
malformed responses that would be accepted, such as the
"code" property being absent or null. In these cases, connect-go
was always falling back to using "unknown" as the code, instead
of using the HTTP-to-Connect mapping as required by the spec.
This addresses that and reconciles connect-go's behavior with
the specification.

internal/conformance/known-failing.txt

@@ -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

internal/conformance/runconformance.sh

@@ -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

protocol_connect.go

@@ -547,6 +547,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)
+		}
 		serverErr := wireErr.asError()
 		if serverErr == nil {
 			return nil
@@ -1233,6 +1237,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"`