search

connectrpc / vanguard-go

Support REST, gRPC, gRPC-Web, and Connect clients with one server.
https://pkg.go.dev/connectrpc.com/vanguard
Apache License 2.0
236 stars 14 forks source link

Update connect-go dependency to latest #126

Closed jhump closed 6 months ago

jhump commented 6 months ago

Some of the updated behavior in connect-go tickled some bugs in vanguard, which were caught by the tests.

  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 fixes are making 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".
jhump commented 6 months ago

@emcfarlane, this fixes a few bugs in Vanguard that were tickled by behavior changes in the newer connect-go.

Akshay already approved, but I wanted you to take a look before I merge.

After this is merged, I will cut a new release of Vanguard. We've got a few other fixes and improvements that aren't yet release, so now is a great time! This update also (indirectly) updates the golang.org/x/net dependency, which makes sure that consumers of this repo won't be vulnerable to CVE-2023-45288.

jhump commented 6 months ago

@emcfarlane, actually, I just realized it's pretty late in Switzerland and it's a Friday evening. So I'll go ahead merge, but if you have any questions or feedback, go ahead and leave comments. I can open a follow-up PR if there's anything that needs to be addressed.