Skip to content

Commit

Permalink
Relaxes assertions on query parameters (#771)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jhump
jhump authored Feb 7, 2024
1 parent 9613811 commit 6a5203a
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 95 deletions.
16 changes: 8 additions & 8 deletions internal/app/connectconformance/results.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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()))
}
}
Expand Down
6 changes: 3 additions & 3 deletions internal/app/connectconformance/results_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`,
},
},
}
Expand Down
31 changes: 15 additions & 16 deletions internal/app/connectconformance/test_case_library.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package connectconformance

import (
"encoding/base64"
"errors"
"fmt"
"math"
Expand Down Expand Up @@ -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},
Expand Down
38 changes: 0 additions & 38 deletions internal/app/connectconformance/test_case_library_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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)
}
33 changes: 3 additions & 30 deletions internal/codec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package internal

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand All @@ -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}}
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 6a5203a

Please sign in to comment.