Skip to content

Commit

Permalink
Update connect-go dependency to latest (#126)
Browse files Browse the repository at this point in the history
Some of the updated behavior in connect-go tickled some bugs in
vanguard, which were caught by the tests. These issues are fixed
in this change:

1. When the `envelopingReader` was transforming an un-enveloped body
(for Connect unary) to an enveloped one, it would include an extra,
second, invalid envelope(!!). This wasn't previously caught because the
older version of connect-go only tried to read a single request for
unary RPCs and then effectively ignored the rest of the body. The newer
connect-go validates that the client isn't erroneously sending more than
one message, which brought this bug to light.
2. The "content-type" response header was not being correctly set when
translating a Connect unary error (JSON response body) to gRPC. This
wasn't previously caught because the older version of connect-go failed
to validate the response content-type. The newer version does validate
the content-type, so complains when it is empty since it should always
be set for gRPC and gRPC-Web responses (even if the body is empty).
3. If the "content-length" header was set, the vanguard transcoder was
failing to correctly enforce the max buffer size limit. This wasn't
previously caught because the older connect-go never set the
"content-length" header; but now it will be set for unary and
server-stream operations, which tickled this bug. Luckily, we had a test
case to catch it.

The other changes necessary to get tests to pass were changes to make
vanguard's test code more robust:
1. It was using `assert.Equal` on a `*connect.Error`, which may contain
`proto.Message` instances if there are error details, which do not
correctly compare with `assert.Error`. I've extract the comparison to a
helper that checks all of the error properties and uses `protocmp` to
compare the message values.
2. It was assuming a content-type of "application/grpc+proto" for gRPC
requests with a proto codec, but the newer connect-go version will now
shorten this to simply "application/grpc". So the assertions here were
relaxed accordingly.
  • Loading branch information
jhump authored May 17, 2024
1 parent b8455b9 commit 8dca306
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 7 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ require (
buf.build/gen/go/connectrpc/eliza/connectrpc/go v1.11.1-20230822171018-8b8b971d6fde.1
buf.build/gen/go/connectrpc/eliza/grpc/go v1.3.0-20230822171018-8b8b971d6fde.1
buf.build/gen/go/connectrpc/eliza/protocolbuffers/go v1.31.0-20230822171018-8b8b971d6fde.1
connectrpc.com/connect v1.14.0
connectrpc.com/connect v1.16.2
connectrpc.com/grpcreflect v1.2.0
github.com/google/go-cmp v0.5.9
github.com/stretchr/testify v1.8.4
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,8 @@ cloud.google.com/go/workflows v1.6.0/go.mod h1:6t9F5h/unJz41YqfBmqSASJSXccBLtD1V
cloud.google.com/go/workflows v1.7.0/go.mod h1:JhSrZuVZWuiDfKEFxU0/F1PQjmpnpcoISEXH2bcHC3M=
cloud.google.com/go/workflows v1.8.0/go.mod h1:ysGhmEajwZxGn1OhGOGKsTXc5PyxOc0vfKf5Af+to4M=
cloud.google.com/go/workflows v1.9.0/go.mod h1:ZGkj1aFIOd9c8Gerkjjq7OW7I5+l6cSvT3ujaO/WwSA=
connectrpc.com/connect v1.14.0 h1:PDS+J7uoz5Oui2VEOMcfz6Qft7opQM9hPiKvtGC01pA=
connectrpc.com/connect v1.14.0/go.mod h1:uoAq5bmhhn43TwhaKdGKN/bZcGtzPW1v+ngDTn5u+8s=
connectrpc.com/connect v1.16.2 h1:ybd6y+ls7GOlb7Bh5C8+ghA6SvCBajHwxssO2CGFjqE=
connectrpc.com/connect v1.16.2/go.mod h1:n2kgwskMHXC+lVqb18wngEpF95ldBHXjZYJussz5FRc=
connectrpc.com/grpcreflect v1.2.0 h1:Q6og1S7HinmtbEuBvARLNwYmTbhEGRpHDhqrPNlmK+U=
connectrpc.com/grpcreflect v1.2.0/go.mod h1:nwSOKmE8nU5u/CidgHtPYk1PFI3U9ignz7iDMxOYkSY=
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
Expand Down
1 change: 1 addition & 0 deletions protocol_grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ func grpcAddRequestMeta(contentTypePrefix string, meta requestMeta, headers http

func grpcAddResponseMeta(contentTypePrefix string, meta responseMeta, headers http.Header) int {
if meta.end != nil {
headers.Set("Content-Type", contentTypePrefix+meta.codec)
grpcWriteEndToTrailers(meta.end, headers)
return http.StatusOK
}
Expand Down
27 changes: 25 additions & 2 deletions protocol_grpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import (
"unicode/utf8"

"connectrpc.com/connect"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/wrapperspb"
)

Expand All @@ -45,7 +47,7 @@ func TestGRPCErrorWriter(t *testing.T) {
assert.Empty(t, rec.Body.Bytes())

got := grpcExtractErrorFromTrailer(rec.Header())
assert.Equal(t, cerr, got)
compareErrors(t, cerr, got)

// Now again, but this time an error with details
errDetail, err := connect.NewErrorDetail(&wrapperspb.StringValue{Value: "foo"})
Expand All @@ -60,7 +62,7 @@ func TestGRPCErrorWriter(t *testing.T) {
assert.Empty(t, rec.Body.Bytes())

got = grpcExtractErrorFromTrailer(rec.Header())
assert.Equal(t, cerr, got)
compareErrors(t, cerr, got)
}

func TestGRPCEncodeTimeoutQuick(t *testing.T) {
Expand Down Expand Up @@ -144,3 +146,24 @@ func TestGRPCEncodeTimeout(t *testing.T) {
timeout = grpcEncodeTimeout(-1 * time.Hour)
assert.Equal(t, "0n", timeout)
}

func compareErrors(t *testing.T, got, want *connect.Error) {
t.Helper()
assert.Equal(t, want.Code(), got.Code(), "wrong code")
assert.Equal(t, want.Message(), got.Message(), "wrong message")
wantDetails := want.Details()
gotDetails := got.Details()
if !assert.Len(t, wantDetails, len(gotDetails), "wrong number of details") {
return
}
for i, wantDetail := range wantDetails {
gotDetail := gotDetails[i]
if assert.Equal(t, wantDetail.Type(), gotDetail.Type(), "wrong detail type at index %d", i) {
wantedMsg, err := wantDetail.Value()
require.NoError(t, err, "failed to deserialize wanted detail at index %d", i)
gotMsg, err := gotDetail.Value()
require.NoError(t, err, "failed to deserialize got detail at index %d", i)
require.Empty(t, cmp.Diff(wantedMsg, gotMsg, protocmp.Transform()))
}
}
}
10 changes: 10 additions & 0 deletions transcoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -854,8 +854,18 @@ func (r *envelopingReader) prepareNext() error {
r.envRemain = 0
return nil
case r.rw.op.clientEnveloper == nil:
if r.current != nil {
// If there is no enveloping, the entire body is part of a single
// message. And we've already prepared that message once. So there
// is no more.
return io.EOF
}
env.compressed = r.rw.op.client.reqCompression != nil
if r.rw.op.contentLen != -1 {
limit := int64(r.rw.op.methodConf.maxMsgBufferBytes)
if r.rw.op.contentLen > limit {
return bufferLimitError(limit)
}
r.current = &hardLimitReader{r: r.r, rw: r.rw, limit: r.rw.op.contentLen, makeError: contentLengthError}
env.length = uint32(r.rw.op.contentLen)
} else {
Expand Down
12 changes: 10 additions & 2 deletions vanguard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -916,13 +916,21 @@ func protocolAssertMiddleware(
var wantHdr map[string][]string
switch protocol {
case ProtocolGRPC:
wantContentType := []string{"application/grpc+" + codec}
if codec == CodecProto {
wantContentType = append(wantContentType, "application/grpc")
}
wantHdr = map[string][]string{
"Content-Type": {"application/grpc+" + codec},
"Content-Type": wantContentType,
"Grpc-Encoding": allowedCompression,
}
case ProtocolGRPCWeb:
wantContentType := []string{"application/grpc-web+" + codec}
if codec == CodecProto {
wantContentType = append(wantContentType, "application/grpc-web")
}
wantHdr = map[string][]string{
"Content-Type": {"application/grpc-web+" + codec},
"Content-Type": wantContentType,
"Grpc-Encoding": allowedCompression,
}
case ProtocolConnect:
Expand Down

0 comments on commit 8dca306

Please sign in to comment.