From 6a5203a2c7137462e1dd50a578fa21e4797e8857 Mon Sep 17 00:00:00 2001 From: Joshua Humphries <2035234+jhump@users.noreply.github.com> Date: Wed, 7 Feb 2024 11:04:50 -0500 Subject: [PATCH] Relaxes assertions on query parameters (#771) Previously, computing the expected response would add an expected query parameter for the `message` param, with an encoded value. But the problem is that not all clients would necessarily encode the message in exactly the same way, and that's okay. Some will choose to base64-encode (which is usually more efficient with binary payloads). Also, there's no _canonical_ protobuf binary format, so there could be subtle differences in the byte output, even for equivalent messages. Finally, there's no _canonical_ encoding for compressed payloads -- different default compression settings or other subtle differences in implementation can lead to more than one valid compressed encoding of the same data. --- internal/app/connectconformance/results.go | 16 ++++---- .../app/connectconformance/results_test.go | 6 +-- .../connectconformance/test_case_library.go | 31 ++++++++------- .../test_case_library_test.go | 38 ------------------- internal/codec.go | 33 ++-------------- 5 files changed, 29 insertions(+), 95 deletions(-) diff --git a/internal/app/connectconformance/results.go b/internal/app/connectconformance/results.go index 35103848..c7cf0db6 100644 --- a/internal/app/connectconformance/results.go +++ b/internal/app/connectconformance/results.go @@ -159,6 +159,9 @@ func (r *testResults) failed(testCase string, err *conformancev1.ClientErrorResu func (r *testResults) assert(testCase string, expected, actual *conformancev1.ClientResponseResult) { var errs multiErrors + errs = append(errs, checkError(expected.Error, actual.Error)...) + errs = append(errs, checkPayloads(expected.Payloads, actual.Payloads)...) + // TODO - This check is for trailers-only and should really only apply to gRPC and gRPC-Web protocols. // Previously, it checked for error != nil, which is compliant with gRPC. But gRPC-Web does trailers-only // responses with no errors also. @@ -188,9 +191,6 @@ func (r *testResults) assert(testCase string, expected, actual *conformancev1.Cl errs = append(errs, checkHeaders("response trailers", expected.ResponseTrailers, actual.ResponseTrailers)...) } - errs = append(errs, checkPayloads(expected.Payloads, actual.Payloads)...) - errs = append(errs, checkError(expected.Error, actual.Error)...) - // If client didn't provide actual raw error, we skip this check. if expected.ConnectErrorRaw != nil && actual.ConnectErrorRaw != nil { diff := cmp.Diff(expected.ConnectErrorRaw, actual.ConnectErrorRaw, protocmp.Transform()) @@ -403,12 +403,12 @@ func checkRequestInfo(expected, actual *conformancev1.ConformancePayload_Request if actual == nil || actual.TimeoutMs == nil { errs = append(errs, fmt.Errorf("server did not echo back a timeout but one was expected (%d ms)", expected.GetTimeoutMs())) } else { - max := expected.GetTimeoutMs() - min := max - timeoutCheckGracePeriodMillis - if min < 0 { - min = 0 + maxAllowed := expected.GetTimeoutMs() + minAllowed := maxAllowed - timeoutCheckGracePeriodMillis + if minAllowed < 0 { + minAllowed = 0 } - if actual.GetTimeoutMs() > max || actual.GetTimeoutMs() < min { + if actual.GetTimeoutMs() > maxAllowed || actual.GetTimeoutMs() < minAllowed { errs = append(errs, fmt.Errorf("server echoed back a timeout (%d ms) that did not match expected (%d ms)", actual.GetTimeoutMs(), expected.GetTimeoutMs())) } } diff --git a/internal/app/connectconformance/results_test.go b/internal/app/connectconformance/results_test.go index a59a2ac2..64b17a04 100644 --- a/internal/app/connectconformance/results_test.go +++ b/internal/app/connectconformance/results_test.go @@ -671,15 +671,15 @@ func TestResults_Assert_ReportsAllErrors(t *testing.T) { }`, // It tries to describe everything wrong, all in one shot. expectedErrors: []string{ - `actual response headers missing "abc"`, - `actual response trailers missing "xyz"`, + `expecting an error but received none`, `expecting 3 response messages but instead got 1`, `response #1: expecting data 69b71d79f821, got d76df8`, `actual request headers missing "foo"`, `server did not echo back a timeout but one was expected (12345 ms)`, `expecting 1 request messages to be described but instead got 2`, `request #1: did not survive round-trip`, - `expecting an error but received none`, + `actual response headers missing "abc"`, + `actual response trailers missing "xyz"`, }, }, } diff --git a/internal/app/connectconformance/test_case_library.go b/internal/app/connectconformance/test_case_library.go index e673a25f..533b288b 100644 --- a/internal/app/connectconformance/test_case_library.go +++ b/internal/app/connectconformance/test_case_library.go @@ -15,7 +15,6 @@ package connectconformance import ( - "encoding/base64" "errors" "fmt" "math" @@ -524,28 +523,28 @@ func populateExpectedUnaryResponse(testCase *conformancev1.TestCase) error { // If this is a GET test, then the request should be marshalled and in the query params if testCase.Request.UseGetHttpMethod { - isJSON := testCase.Request.Codec == conformancev1.Codec_CODEC_JSON - // Build a codec based on what is used in the request - codec := internal.NewCodec(isJSON) - reqAsBytes, err := codec.MarshalStable(definer) - if err != nil { - return err - } - var value string var encoding string - if isJSON { - value = string(reqAsBytes) + if testCase.Request.Codec == conformancev1.Codec_CODEC_JSON { encoding = "json" } else { - value = base64.RawURLEncoding.EncodeToString(reqAsBytes) encoding = "proto" } + // We intentionally exclude the "message" query param: the RPC response, which + // echos back the request data, will suffice to check that. Also, we can't + // indicate a specific sequence of bytes to expect for "message" because there + // isn't a *canonical* encoding for protobuf nor is there necessarily a + // canonical encoding of compressed messages. So we just verify that it has + // the required "encoding" parameter and then will check the echoed-back request + // to verify that the message was otherwise correctly encoded (by virtue of the + // fact that it could be correctly decoded by the server). + // + // We also exclude the "base64" parameter, allowing implementations flexibility + // on when to use it. It's mainly valuable to use for binary data, where the + // alternative (to just rely on the percent-encoding of URL query strings) will + // be more verbose and lead to a larger request line (potentially one that is + // too long to fit in a URI). reqInfo.ConnectGetInfo = &conformancev1.ConformancePayload_ConnectGetInfo{ QueryParams: []*conformancev1.Header{ - { - Name: "message", - Value: []string{value}, - }, { Name: "encoding", Value: []string{encoding}, diff --git a/internal/app/connectconformance/test_case_library_test.go b/internal/app/connectconformance/test_case_library_test.go index 04d4a0c2..219cdcf5 100644 --- a/internal/app/connectconformance/test_case_library_test.go +++ b/internal/app/connectconformance/test_case_library_test.go @@ -15,11 +15,9 @@ package connectconformance import ( - "encoding/base64" "sort" "testing" - "connectrpc.com/conformance/internal" "connectrpc.com/conformance/internal/app/connectconformance/testsuites" conformancev1 "connectrpc.com/conformance/internal/gen/proto/go/connectrpc/conformance/v1" "connectrpc.com/connect" @@ -922,10 +920,6 @@ func TestPopulateExpectedResponse(t *testing.T) { Requests: asAnySlice(t, unarySuccessReq), ConnectGetInfo: &conformancev1.ConformancePayload_ConnectGetInfo{ QueryParams: []*conformancev1.Header{ - { - Name: "message", - Value: []string{marshalToString(t, true, unarySuccessReq)}, - }, { Name: "encoding", Value: []string{"json"}, @@ -961,10 +955,6 @@ func TestPopulateExpectedResponse(t *testing.T) { Requests: asAnySlice(t, unarySuccessReq), ConnectGetInfo: &conformancev1.ConformancePayload_ConnectGetInfo{ QueryParams: []*conformancev1.Header{ - { - Name: "message", - Value: []string{marshalToString(t, false, unarySuccessReq)}, - }, { Name: "encoding", Value: []string{"proto"}, @@ -1000,10 +990,6 @@ func TestPopulateExpectedResponse(t *testing.T) { Requests: asAnySlice(t, unaryErrorReq), ConnectGetInfo: &conformancev1.ConformancePayload_ConnectGetInfo{ QueryParams: []*conformancev1.Header{ - { - Name: "message", - Value: []string{marshalToString(t, true, unaryErrorReq)}, - }, { Name: "encoding", Value: []string{"json"}, @@ -1038,10 +1024,6 @@ func TestPopulateExpectedResponse(t *testing.T) { Requests: asAnySlice(t, unaryErrorReq), ConnectGetInfo: &conformancev1.ConformancePayload_ConnectGetInfo{ QueryParams: []*conformancev1.Header{ - { - Name: "message", - Value: []string{marshalToString(t, false, unaryErrorReq)}, - }, { Name: "encoding", Value: []string{"proto"}, @@ -1791,23 +1773,3 @@ func asAnySlice(t *testing.T, msgs ...proto.Message) []*anypb.Any { } return arr } - -// marshalToString marshals the given proto message to a string mirroring the -// logic that Connect specifies for GET requests. -// If asJSON is true, the message is first marshalled to JSON and the bytes are -// then converted to a string. -// If asJSON is false, the message is marshalled to binary and the bytes are then -// base64-encoded as a string. -func marshalToString(t *testing.T, asJSON bool, msg proto.Message) string { - t.Helper() - codec := internal.NewCodec(asJSON) - - bytes, err := codec.MarshalStable(msg) - require.NoError(t, err) - - if asJSON { - return string(bytes) - } - - return base64.RawURLEncoding.EncodeToString(bytes) -} diff --git a/internal/codec.go b/internal/codec.go index b3cafb57..7a86b2d9 100644 --- a/internal/codec.go +++ b/internal/codec.go @@ -15,7 +15,6 @@ package internal import ( - "bytes" "encoding/json" "errors" "fmt" @@ -38,15 +37,14 @@ type StreamEncoder interface { Encode(msg proto.Message) error } -// codec describes anything that can marshal and unmarshal proto messages. -type codec interface { +// Codec describes anything that can marshal and unmarshal proto messages. +type Codec interface { NewDecoder(io.Reader) StreamDecoder NewEncoder(io.Writer) StreamEncoder - MarshalStable(msg proto.Message) ([]byte, error) } // NewCodec returns a new Codec. -func NewCodec(json bool) codec { +func NewCodec(json bool) Codec { if json { return &jsonCodec{MarshalOptions: protojson.MarshalOptions{Multiline: true}} } @@ -112,22 +110,6 @@ func (j *jsonEncoder) Encode(msg proto.Message) error { return nil } -// This function is lifted from connect-go since this is the logic it uses -// to stably marshal a request message into JSON for putting into query params -// of GET requests. -// See https://github.com/connectrpc/connect-go/blob/main/codec.go -func (c *jsonCodec) MarshalStable(msg proto.Message) ([]byte, error) { - messageJSON, err := c.Marshal(msg) - if err != nil { - return nil, fmt.Errorf("failed to marshal message to JSON: %w", err) - } - compactedJSON := bytes.NewBuffer(messageJSON[:0]) - if err = json.Compact(compactedJSON, messageJSON); err != nil { - return nil, fmt.Errorf("failed to compact marshaled JSON: %w", err) - } - return compactedJSON.Bytes(), nil -} - // protoCodec marshals and unmarshals the Protobuf binary format. type protoCodec struct { proto.MarshalOptions @@ -148,15 +130,6 @@ func (c *protoCodec) NewEncoder(out io.Writer) StreamEncoder { } } -// This function is lifted from connect-go since this is the logic it uses -// to stably marshal a request message into binary for putting into query params -// of GET requests. -// See https://github.com/connectrpc/connect-go/blob/main/codec.go -func (c *protoCodec) MarshalStable(msg proto.Message) ([]byte, error) { - options := proto.MarshalOptions{Deterministic: true} - return options.Marshal(msg) -} - type protoDecoder struct { opts proto.UnmarshalOptions in io.Reader