Repository: yarpc/yab
Pull request #345 (2021-07-21 11:39:06)

Fixed: Maintain protobuf reflection connection until end of request
This change avoids early closure of protobuf reflection connection, allowing correct parsing of protobuf messages containing `Anytype` fields in the response or error details.

Before:
```
> yab test-service test_service.Test/Unary -r '{}'
Failed while parsing response: unknown message type "test_service.Test.FooAny"
```

After:
```
> yab test-service test_service.Test/Unary -r '{}'
{
  "body": {
    "any" : {
      "value": 123
    }
  }
}
```

codecov[bot]: # [Codecov](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc) Report
> Merging [#345](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc) (2b70418) into [dev](https://codecov.io/gh/yarpc/yab/commit/2f7d2420877e28f8dba8854e1cf01044f4c8cdb0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc) (2f7d242) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/yarpc/yab/pull/345/graphs/tree.svg?width=650&height=150&src=pr&token=yEgrqUTCuh&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc)](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc)

```diff
@@            Coverage Diff             @@
##              dev     #345      +/-   ##
==========================================
+ Coverage   90.30%   90.31%   +0.01%     
==========================================
  Files          54       54              
  Lines        2712     2715       +3     
==========================================
+ Hits         2449     2452       +3     
  Misses        184      184              
  Partials       79       79              
```


| [Impacted Files](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc) | Coverage Δ | |
|---|---|---|
| [request.go](https://codecov.io/gh/yarpc/yab/pull/345/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc#diff-cmVxdWVzdC5nbw==) | `97.46% <ø> (-0.04%)` | :arrow_down: |
| [encoding/protobuf.go](https://codecov.io/gh/yarpc/yab/pull/345/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc#diff-ZW5jb2RpbmcvcHJvdG9idWYuZ28=) | `93.89% <100.00%> (+0.09%)` | :arrow_up: |
| [main.go](https://codecov.io/gh/yarpc/yab/pull/345/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc#diff-bWFpbi5nbw==) | `91.66% <100.00%> (+0.07%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc). Last update [2f7d242...2b70418](https://codecov.io/gh/yarpc/yab/pull/345?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ffc48e9..0607936 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,7 +2,8 @@ Changelog
 =========
 
 # Unreleased (0.21.0)
-* Nothing yet
+* Fix parsing of protobuf responses/error-details containing Any type fields, by maintaining
+  reflection server connection until end of the request.
 
 # 0.20.0 (2021-05-18)
 * Add `stream-delay-close-send` option which delays client send stream closure.
diff --git a/encoding/protobuf.go b/encoding/protobuf.go
index 2ac9dc3..2dbf950 100644
--- a/encoding/protobuf.go
+++ b/encoding/protobuf.go
@@ -25,7 +25,7 @@ type protoSerializer struct {
 	serviceName string
 	methodName  string
 	method      *desc.MethodDescriptor
-	anyResolver jsonpb.AnyResolver
+	anyResolver anyResolver
 }
 
 // bytesMsg wraps a raw byte slice for serialization purposes. Especially
@@ -264,6 +264,12 @@ func (p protoStreamRequestReader) NextBody() ([]byte, error) {
 	return p.proto.encode(body)
 }
 
+// Close stops the protobuf reflection/file based descriptor provider.
+func (p protoSerializer) Close() error {
+	p.anyResolver.source.Close()
+	return nil
+}
+
 func splitMethod(fullMethod string) (svc, method string, err error) {
 	parts := strings.Split(fullMethod, "/")
 	switch len(parts) {
diff --git a/go.mod b/go.mod
index bc5b4c1..28b32ea 100644
--- a/go.mod
+++ b/go.mod
@@ -43,7 +43,7 @@ require (
 	go.uber.org/zap v1.10.0
 	golang.org/x/net v0.0.0-20190926025831-c00fd9afed17
 	golang.org/x/text v0.3.1-0.20180511172408-5c1cf69b5978 // indirect
-	google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8 // indirect
+	google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8
 	google.golang.org/grpc v1.24.0
 	gopkg.in/yaml.v2 v2.2.2
 )
diff --git a/main.go b/main.go
index 58a0584..7f0e3b1 100644
--- a/main.go
+++ b/main.go
@@ -329,6 +329,9 @@ func runWithOptions(opts Options, out output, logger *zap.Logger) {
 	if err != nil {
 		out.Fatalf("Failed while parsing input: %v\n", err)
 	}
+	if serializerWithClose, ok := serializer.(io.Closer); ok {
+		defer serializerWithClose.Close()
+	}
 
 	tracer, closer := getTracer(opts, out)
 	if closer != nil {
diff --git a/request.go b/request.go
index 5ab7905..ecc7445 100644
--- a/request.go
+++ b/request.go
@@ -122,9 +122,6 @@ func NewSerializer(opts Options, resolved resolvedProtocolEncoding) (encoding.Se
 			return nil, err
 		}
 
-		// The descriptor is only used in the New function, so it's safe to defer Close.
-		defer descSource.Close()
-
 		return encoding.NewProtobuf(opts.ROpts.Procedure, descSource)
 	}
 


Test patch:
diff --git a/integration_test.go b/integration_test.go
index 2ce1dba..d90d7f2 100644
--- a/integration_test.go
+++ b/integration_test.go
@@ -34,6 +34,7 @@ import (
 	"time"
 
 	"github.com/yarpc/yab/testdata/gen-go/integration"
+	testdataany "github.com/yarpc/yab/testdata/protobuf/any"
 	"github.com/yarpc/yab/testdata/protobuf/simple"
 	yintegration "github.com/yarpc/yab/testdata/yarpc/integration"
 	"github.com/yarpc/yab/testdata/yarpc/integration/fooserver"
@@ -54,9 +55,9 @@ import (
 	yhttp "go.uber.org/yarpc/transport/http"
 	ytchan "go.uber.org/yarpc/transport/tchannel"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/reflection"
 	rpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha"
-	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
@@ -383,7 +384,7 @@ func (s *simpleService) Baz(c context.Context, in *simple.Foo) (*simple.Foo, err
 
 	if in.Test == -1 {
 		st := status.New(codes.InvalidArgument, "invalid username")
-		st, err := st.WithDetails(in)
+		st, err := st.WithDetails(in, &testdataany.FooAny{Value: 123})
 		if err != nil {
 			// If this errored, it will always error
 			// here, so better panic so we can figure
@@ -1212,6 +1213,11 @@ func TestGRPCReflectionSource(t *testing.T) {
       "type.googleapis.com/Foo": {
         "test": -1
       }
+    },
+    {
+      "type.googleapis.com/FooAny": {
+        "value": 123
+      }
     }
   ]
 }