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

Revise code mappings per latest changes to spec #706

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Revise code mappings per latest changes to spec #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions connect_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1731,9 +1731,9 @@ func TestConnectHTTPErrorCodes(t *testing.T) {
assert.NotNil(t, err)
assert.Nil(t, connectResp)
}
t.Run("CodeCanceled-408", func(t *testing.T) {
t.Run("CodeCanceled-499", func(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeCanceled, 408)
checkHTTPStatus(t, connect.CodeCanceled, 499)
})
t.Run("CodeUnknown-500", func(t *testing.T) {
t.Parallel()
Expand All @@ -1743,9 +1743,9 @@ func TestConnectHTTPErrorCodes(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeInvalidArgument, 400)
})
t.Run("CodeDeadlineExceeded-408", func(t *testing.T) {
t.Run("CodeDeadlineExceeded-504", func(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeDeadlineExceeded, 408)
checkHTTPStatus(t, connect.CodeDeadlineExceeded, 504)
})
t.Run("CodeNotFound-404", func(t *testing.T) {
t.Parallel()
Expand All @@ -1763,9 +1763,9 @@ func TestConnectHTTPErrorCodes(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeResourceExhausted, 429)
})
t.Run("CodeFailedPrecondition-412", func(t *testing.T) {
t.Run("CodeFailedPrecondition-400", func(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeFailedPrecondition, 412)
checkHTTPStatus(t, connect.CodeFailedPrecondition, 400)
})
t.Run("CodeAborted-409", func(t *testing.T) {
t.Parallel()
Expand All @@ -1775,9 +1775,9 @@ func TestConnectHTTPErrorCodes(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeOutOfRange, 400)
})
t.Run("CodeUnimplemented-404", func(t *testing.T) {
t.Run("CodeUnimplemented-501", func(t *testing.T) {
t.Parallel()
checkHTTPStatus(t, connect.CodeUnimplemented, 404)
checkHTTPStatus(t, connect.CodeUnimplemented, 501)
})
t.Run("CodeInternal-500", func(t *testing.T) {
t.Parallel()
Expand Down
2 changes: 1 addition & 1 deletion handler_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func TestHandler_ServeHTTP(t *testing.T) {
resp, err := client.Do(req)
assert.Nil(t, err)
defer resp.Body.Close()
assert.Equal(t, resp.StatusCode, http.StatusNotFound)
assert.Equal(t, resp.StatusCode, http.StatusNotImplemented)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one case where this mapping makes a lot more sense: when the server receives an unrecognized content-encoding, it returns an "unimplemented" status error. And 501 makes way more sense than 404. (Ideally it would be 415, but let's take what we can get 😆)


type errorMessage struct {
Code string `json:"code,omitempty"`
Expand Down
15 changes: 14 additions & 1 deletion internal/conformance/known-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,17 @@
# 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
Connect Error and End-Stream/**/error/null-code

# The current v1.0.0-rc3 of conformance suite has expectations based
# on the old mappings of HTTP to RPC code. The mappings were revised
# in the spec (https://github.com/connectrpc/connectrpc.com/pull/130),
# and this repo implements those new mappings. So test cases based on
# the old mappings fail for right now.
Connect Unexpected Responses/**/unexpected-error-body
HTTP to Connect Code Mapping/**/bad-request
HTTP to Connect Code Mapping/**/conflict
HTTP to Connect Code Mapping/**/payload-too-large
HTTP to Connect Code Mapping/**/precondition-failed
HTTP to Connect Code Mapping/**/request-header-fields-too-large
HTTP to Connect Code Mapping/**/request-timeout
25 changes: 25 additions & 0 deletions protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,3 +396,28 @@ func canonicalizeContentTypeSlow(contentType string) string {
}
return mime.FormatMediaType(base, params)
}

func httpToCode(httpCode int) Code {
// https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
// Note that this is NOT the inverse of the gRPC-to-HTTP or Connect-to-HTTP
// mappings.

// Literals are easier to compare to the specification (vs named
// constants).
switch httpCode {
case 400:
return CodeInternal
case 401:
return CodeUnauthenticated
case 403:
return CodePermissionDenied
case 404:
return CodeUnimplemented
case 429:
return CodeUnavailable
case 502, 503, 504:
return CodeUnavailable
default:
return CodeUnknown
}
}
49 changes: 8 additions & 41 deletions protocol_connect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,13 +543,13 @@ func (cc *connectUnaryClientConn) validateResponse(response *http.Response) *Err
var wireErr connectWireError
if err := unmarshaler.UnmarshalFunc(&wireErr, json.Unmarshal); err != nil {
return NewError(
connectHTTPToCode(response.StatusCode),
httpToCode(response.StatusCode),
errors.New(response.Status),
)
}
if wireErr.Code == 0 {
// code not set? default to one implied by HTTP status
wireErr.Code = connectHTTPToCode(response.StatusCode)
wireErr.Code = httpToCode(response.StatusCode)
}
serverErr := wireErr.asError()
if serverErr == nil {
Expand Down Expand Up @@ -652,7 +652,7 @@ func (cc *connectStreamingClientConn) onRequestSend(fn func(*http.Request)) {

func (cc *connectStreamingClientConn) validateResponse(response *http.Response) *Error {
if response.StatusCode != http.StatusOK {
return errorf(connectHTTPToCode(response.StatusCode), "HTTP status %v", response.Status)
return errorf(httpToCode(response.StatusCode), "HTTP status %v", response.Status)
}
if err := connectValidateStreamResponseContentType(
cc.codec.Name(),
Expand Down Expand Up @@ -1267,13 +1267,13 @@ func connectCodeToHTTP(code Code) int {
// it easier to compare this function to the Connect specification.
switch code {
case CodeCanceled:
return 408
return 499
case CodeUnknown:
return 500
case CodeInvalidArgument:
return 400
case CodeDeadlineExceeded:
return 408
return 504
case CodeNotFound:
return 404
case CodeAlreadyExists:
Expand All @@ -1283,13 +1283,13 @@ func connectCodeToHTTP(code Code) int {
case CodeResourceExhausted:
return 429
case CodeFailedPrecondition:
return 412
return 400
case CodeAborted:
return 409
case CodeOutOfRange:
return 400
case CodeUnimplemented:
return 404
return 501
case CodeInternal:
return 500
case CodeUnavailable:
Expand All @@ -1303,39 +1303,6 @@ func connectCodeToHTTP(code Code) int {
}
}

func connectHTTPToCode(httpCode int) Code {
// As above, literals are easier to compare to the specificaton (vs named
// constants).
switch httpCode {
case 400:
return CodeInvalidArgument
case 401:
return CodeUnauthenticated
case 403:
return CodePermissionDenied
case 404:
return CodeUnimplemented
case 408:
return CodeDeadlineExceeded
case 409:
return CodeAborted
case 412:
return CodeFailedPrecondition
case 413:
return CodeResourceExhausted
case 415:
return CodeInternal
case 429:
return CodeUnavailable
case 431:
return CodeResourceExhausted
case 502, 503, 504:
return CodeUnavailable
default:
return CodeUnknown
}
}

func connectCodecFromContentType(streamType StreamType, contentType string) string {
if streamType == StreamTypeUnary {
return strings.TrimPrefix(contentType, connectUnaryContentTypePrefix)
Expand Down Expand Up @@ -1393,7 +1360,7 @@ func connectValidateUnaryResponseContentType(
return nil
}
return NewError(
connectHTTPToCode(statusCode),
httpToCode(statusCode),
errors.New(statusMsg),
)
}
Expand Down
6 changes: 3 additions & 3 deletions protocol_connect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,19 +243,19 @@ func TestConnectValidateUnaryResponseContentType(t *testing.T) {
codecName: codecNameProto,
statusCode: http.StatusNotFound,
responseContentType: "application/proto",
expectCode: connectHTTPToCode(http.StatusNotFound),
expectCode: httpToCode(http.StatusNotFound),
},
{
codecName: codecNameJSON,
statusCode: http.StatusBadRequest,
responseContentType: "some/garbage",
expectCode: connectHTTPToCode(http.StatusBadRequest),
expectCode: httpToCode(http.StatusBadRequest),
},
{
codecName: codecNameJSON,
statusCode: http.StatusTooManyRequests,
responseContentType: "some/garbage",
expectCode: connectHTTPToCode(http.StatusTooManyRequests),
expectCode: httpToCode(http.StatusTooManyRequests),
},
}
for _, testCase := range testCases {
Expand Down
23 changes: 1 addition & 22 deletions protocol_grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ func grpcValidateResponse(
codecName string,
) *Error {
if response.StatusCode != http.StatusOK {
return errorf(grpcHTTPToCode(response.StatusCode), "HTTP status %v", response.Status)
return errorf(httpToCode(response.StatusCode), "HTTP status %v", response.Status)
}
if err := grpcValidateResponseContentType(
web,
Expand All @@ -678,27 +678,6 @@ func grpcValidateResponse(
return nil
}

func grpcHTTPToCode(httpCode int) Code {
// https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md
// Note that this is not just the inverse of the gRPC-to-HTTP mapping.
switch httpCode {
case 400:
return CodeInternal
case 401:
return CodeUnauthenticated
case 403:
return CodePermissionDenied
case 404:
return CodeUnimplemented
case 429:
return CodeUnavailable
case 502, 503, 504:
return CodeUnavailable
default:
return CodeUnknown
}
}

// The gRPC wire protocol specifies that errors should be serialized using the
// binary Protobuf format, even if the messages in the request/response stream
// use a different codec. Consequently, this function needs a Protobuf codec to
Expand Down
Loading