Detect unknown fields even in known messages (#36)

* First pass, completely untested * Tests passing again * go mod tidy * Don't encode using empty message * Add some integration test coverage * Ignore protobuf generated code
bradleyjkemp · Jul 17, 2019 · ef5558f · ef5558f
1 parent b291f08
commit ef5558f
Show file tree

Hide file tree

Showing 15 changed files with 327 additions and 103 deletions.
diff --git a/.codecov.yml b/.codecov.yml
@@ -0,0 +1,2 @@
+ignore:
+  - "**/*.pb.go" # ignore protobuf generated code
diff --git a/go.sum b/go.sum
@@ -2,6 +2,7 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMT
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/DATA-DOG/go-sqlmock v1.3.3 h1:CWUqKXe0s8A2z6qCgkP4Kru7wC11YoAnoupUKFDnH08=
 github.com/DATA-DOG/go-sqlmock v1.3.3/go.mod h1:f/Ixk793poVmq4qj/V1dPUg2JEAKC73Q5eFN3EC/SaM=
+github.com/bradleyjkemp/cupaloy/v2 v2.5.0 h1:XI37Pqyl+msFaJDYL3JuPFKGUgnVxyJp+gQZQGiz2nA=
 github.com/bradleyjkemp/cupaloy/v2 v2.5.0/go.mod h1:TD5UU0rdYTbu/TtuwFuWrtiRARuN7mtRipvs/bsShSE=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
@@ -25,8 +26,6 @@ github.com/gorilla/websocket v1.4.0 h1:WDFjx/TMzVgy9VdMMQi2K2Emtwi2QcUQsztZ/zLaH
 github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ=
 github.com/improbable-eng/grpc-web v0.9.6 h1:B8FH/k5xv/vHovSt70GJHIB2/1+4plmvtfrz33ambuE=
 github.com/improbable-eng/grpc-web v0.9.6/go.mod h1:6hRR09jOEG81ADP5wCQju1z71g6OL4eEvELdran/3cs=
-github.com/jhump/protoreflect v1.4.3 h1:eUSvsY6mTZSsniEXgoFTO5vF2I5VSifUNvtKotc7cl8=
-github.com/jhump/protoreflect v1.4.3/go.mod h1:gZ3i/BeD62fjlaIL0VW4UDMT70CTX+3m4pOnAlJ0BX8=
 github.com/jhump/protoreflect v1.4.4 h1:kySdALZUh7xRtW6UoZjjHtlR8k7rLzx5EXJFRvsO5UY=
 github.com/jhump/protoreflect v1.4.4/go.mod h1:gZ3i/BeD62fjlaIL0VW4UDMT70CTX+3m4pOnAlJ0BX8=
 github.com/konsorten/go-windows-terminal-sequences v1.0.1 h1:mweAR1A6xJ3oS2pRaGiHgQ4OO8tzTaLawm8vnODuwDk=

diff --git a/grpc-dump/dump/dump.go b/grpc-dump/dump/dump.go
@@ -23,8 +23,6 @@ func Run(output io.Writer, protoRoots, protoDescriptors string, proxyConfig ...g
 		}
 		resolvers = append(resolvers, r)
 	}
-	// Always use the unknown message resolver
-	resolvers = append(resolvers, proto_decoder.NewUnknownResolver())
 
 	opts := append(proxyConfig, grpc_proxy.WithInterceptor(dumpInterceptor(output, proto_decoder.NewDecoder(resolvers...))))
 	proxy, err := grpc_proxy.New(

diff --git a/grpc-fixture/fixture/fixture_interceptor.go b/grpc-fixture/fixture/fixture_interceptor.go
@@ -55,7 +55,7 @@ func (f fixture) intercept(srv interface{}, ss grpc.ServerStream, info *grpc.Str
 			}
 
 			if !found {
-				return status.Error(codes.Unavailable, "no matching saved responses for method "+info.FullMethod)
+				return status.Errorf(codes.Unavailable, "no matching saved responses for method %s and message", info.FullMethod)
 			}
 		}
 

diff --git a/integration_test/.snapshots/TestIntegration.json b/integration_test/.snapshots/TestIntegration.json
@@ -1,6 +1,6 @@
 {"service":"bradleyjkemp.github.io.TestService","method":"TestUnaryClientRequest","messages":[{"message_origin":"client","raw_message":"ChEaDUNsaWVudFJlcXVlc3QgARAB","message":{"outerValue":{"innerValue":"ClientRequest","innerNum":1},"outerNum":1},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlclJlc3BvbnNlIAIQAg==","message":{"outerValue":{"innerValue":"ServerResponse","innerNum":2},"outerNum":2},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["bradleyjkemp.github.io:444"],"content-type":["application/grpc"],"user-agent":["grpc-go/1.22.0"],"via":["HTTP/2.0 127.0.0.1:16354"]}}
 {"service":"bradleyjkemp.github.io.TestService","method":"TestUnaryClientRequest","messages":[{"message_origin":"client","raw_message":"ChEaDUNsaWVudFJlcXVlc3QgARAB","message":{"outerValue":{"innerValue":"ClientRequest","innerNum":1},"outerNum":1},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlclJlc3BvbnNlIAIQAg==","message":{"outerValue":{"innerValue":"ServerResponse","innerNum":2},"outerNum":2},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["bradleyjkemp.github.io:444"],"content-type":["application/grpc"],"user-agent":["grpc-go/1.22.0"],"via":["HTTP/2.0 127.0.0.1:16354"]}}
-{"service":"bradleyjkemp.github.io.TestService","method":"TestStreamingServerMessages","messages":[{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"1":{"3":"ServerMessage1","4":3},"2":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UyIAQQBA==","message":{"1":{"3":"ServerMessage2","4":4},"2":4},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["a-different-domain.github.io:444"],"content-type":["application/grpc"],"forwarded":["proto=https"],"user-agent":["grpc-go/1.22.0"],"via":["HTTP/2.0 127.0.0.1:16354"]}}
+{"service":"bradleyjkemp.github.io.TestService","method":"TestStreamingServerMessages","messages":[{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"outerValue":{"innerValue":"ServerMessage1","innerNum":3},"outerNum":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UyIAUQBRoORGV0ZWN0ZWQgdmFsdWU=","message":{"outerValue":{"innerValue":"ServerMessage2","innerNum":5},"outerNum":5,"3":"Detected value"},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["a-different-domain.github.io:444"],"content-type":["application/grpc"],"forwarded":["proto=https"],"user-agent":["grpc-go/1.22.0"],"via":["HTTP/2.0 127.0.0.1:16354"]}}
 {"service":"grpc.gateway.testing.EchoService","method":"Echo","messages":[{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"1":{"3":"ServerMessage1","4":3},"2":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"1":{"3":"ServerMessage1","4":3},"2":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["grpc-web.github.io"],"accept":["*/*"],"accept-encoding":["gzip, deflate, br"],"accept-language":["en-US,en;q=0.9"],"cache-control":["no-cache"],"content-type":["application/grpc+proto"],"custom-header-1":["value1"],"origin":["http://localhost:8081"],"pragma":["no-cache"],"referer":["http://localhost:8081/echotest.html"],"user-agent":["Mozilla/5.0"],"via":["HTTP/2.0 127.0.0.1:16354"],"x-grpc-web":["1"],"x-user-agent":["grpc-web-javascript/0.1"]}}
 {"service":"grpc.gateway.testing.EchoService","method":"Echo","messages":[{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"1":{"3":"ServerMessage1","4":3},"2":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"},{"message_origin":"server","raw_message":"ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==","message":{"1":{"3":"ServerMessage1","4":3},"2":3},"timestamp":"2019-06-24T19:19:46.644943+01:00"}],"metadata":{":authority":["grpc-web.github.io:1234"],"accept":["*/*"],"accept-encoding":["gzip, deflate, br"],"accept-language":["en-US,en;q=0.9"],"cache-control":["no-cache"],"content-type":["application/grpc+proto"],"custom-header-1":["value1"],"forwarded":["proto=https"],"origin":["http://localhost:8081"],"pragma":["no-cache"],"referer":["http://localhost:8081/echotest.html"],"user-agent":["Mozilla/5.0"],"via":["HTTP/2.0 127.0.0.1:16354"],"x-grpc-web":["1"],"x-user-agent":["grpc-web-javascript/0.1"]}}
 
diff --git a/integration_test/generate.go b/integration_test/generate.go
@@ -24,4 +24,15 @@ func main() {
 
 		fmt.Println(i, val, base64.StdEncoding.EncodeToString(marshalled))
 	}
+
+	marshalled, _ := proto.Marshal(&OuterWithExtra{
+		OuterValue: &Inner{
+			InnerValue: "ServerMessage2",
+			InnerNum:   int64(4 + 1),
+		},
+		OuterNum:   int64(4 + 1),
+		ExtraField: "Detected value",
+	})
+
+	fmt.Println(4, "ServerMessage2", base64.StdEncoding.EncodeToString(marshalled))
 }
diff --git a/integration_test/integration_test.go b/integration_test/integration_test.go
@@ -37,7 +37,8 @@ func TestIntegration(t *testing.T) {
 	defer func() {
 		select {
 		case err := <-errors:
-			t.Fatal("Unexpected error:", err)
+			t.Log("Unexpected error:", err)
+			t.Fail()
 		default:
 			return
 		}
@@ -47,7 +48,7 @@ func TestIntegration(t *testing.T) {
 		fixtureErr := fixture.Run(
 			protoRoots,
 			protoDescriptors,
-			".snapshots/TestIntegration.json",
+			"test-fixture.json",
 			grpc_proxy.Port(fixturePort),
 			grpc_proxy.UsingTLS(certFile, keyFile),
 		)
@@ -87,17 +88,20 @@ func TestIntegration(t *testing.T) {
 		}),
 	)
 	if replayErr != nil {
-		t.Fatal("Unexpected error:", replayErr)
+		t.Log("Unexpected error:", replayErr)
+		t.Fail()
 	}
 
 	cmd := curlCommand("http://grpc-web.github.io/grpc.gateway.testing.EchoService/Echo")
 	if out, err := cmd.CombinedOutput(); err != nil {
-		t.Fatal("Unexpected error:", err, string(out))
+		t.Log("Unexpected error:", err, string(out))
+		t.Fail()
 	}
 
 	cmd = curlCommand("https://grpc-web.github.io:1234/grpc.gateway.testing.EchoService/Echo")
 	if out, err := cmd.CombinedOutput(); err != nil {
-		t.Fatal("Unexpected error:", err, string(out))
+		t.Log("Unexpected error:", err, string(out))
+		t.Fail()
 	}
 	dumpLogSanitised := timestampRegex.ReplaceAll(dumpLog.Bytes(), []byte("\"timestamp\":\"2019-06-24T19:19:46.644943+01:00\""))
 

diff --git a/integration_test/test-dump.json b/integration_test/test-dump.json
@@ -65,7 +65,7 @@
     },
     {
       "message_origin": "server",
-      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UyIAQQBA=="
+      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UyIAUQBRoORGV0ZWN0ZWQgdmFsdWU="
     }
   ],
   "metadata": {

diff --git a/integration_test/test-fixture.json b/integration_test/test-fixture.json
@@ -0,0 +1,83 @@
+{
+  "service": "bradleyjkemp.github.io.TestService",
+  "method": "TestUnaryClientRequest",
+  "messages": [
+    {
+      "message_origin": "client",
+      "raw_message": "ChEaDUNsaWVudFJlcXVlc3QgARAB",
+      "message": {
+        "outerValue": {
+          "innerValue": "ClientRequest",
+          "innerNum": 1
+        },
+        "outerNum": 1
+      },
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    },
+    {
+      "message_origin": "server",
+      "raw_message": "ChIaDlNlcnZlclJlc3BvbnNlIAIQAg==",
+      "message": {
+        "outerValue": {
+          "innerValue": "ServerResponse",
+          "innerNum": 2
+        },
+        "outerNum": 2
+      },
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    }
+  ]
+}
+{
+  "service": "bradleyjkemp.github.io.TestService",
+  "method": "TestStreamingServerMessages",
+  "messages": [
+    {
+      "message_origin": "server",
+      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==",
+      "message": {
+        "1": {
+          "3": "ServerMessage1",
+          "4": 3
+        },
+        "2": 3
+      },
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    },
+    {
+      "message_origin": "server",
+      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UyIAUQBRoORGV0ZWN0ZWQgdmFsdWU=",
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    }
+  ]
+}
+{
+  "service": "grpc.gateway.testing.EchoService",
+  "method": "Echo",
+  "messages": [
+    {
+      "message_origin": "server",
+      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==",
+      "message": {
+        "1": {
+          "3": "ServerMessage1",
+          "4": 3
+        },
+        "2": 3
+      },
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    },
+    {
+      "message_origin": "server",
+      "raw_message": "ChIaDlNlcnZlck1lc3NhZ2UxIAMQAw==",
+      "message": {
+        "1": {
+          "3": "ServerMessage1",
+          "4": 3
+        },
+        "2": 3
+      },
+      "timestamp": "2019-06-24T19:19:46.644943+01:00"
+    }
+  ]
+}
diff --git a/integration_test/test.pb.go b/integration_test/test.pb.go
diff --git a/integration_test/test.proto b/integration_test/test.proto
@@ -11,6 +11,13 @@ message Inner {
     int64 inner_num = 4;
 }
 
+message OuterWithExtra {
+    Inner outer_value = 1;
+    int64 outer_num = 2;
+    string extra_field = 3;
+}
+
 service TestService {
     rpc TestUnaryClientRequest(Outer) returns (Outer) {};
+    rpc TestStreamingServerMessages(Outer) returns (stream Outer) {};
 }
diff --git a/internal/proto_decoder/decode.go b/internal/proto_decoder/decode.go
@@ -22,15 +22,17 @@ type MessageDecoder interface {
 }
 
 type messageDecoder struct {
-	resolvers []MessageResolver
+	resolvers    []MessageResolver
+	unknownField unknownFieldResolver
 }
 
 // Chain together a number of resolvers to decode incoming messages.
 // Resolvers are in priority order, the first to return a nil error
 // is used to decode the message.
 func NewDecoder(resolvers ...MessageResolver) *messageDecoder {
 	return &messageDecoder{
-		resolvers: resolvers,
+		resolvers:    append(resolvers, emptyResolver{}),
+		unknownField: unknownFieldResolver{},
 	}
 }
 
@@ -42,8 +44,13 @@ func (d *messageDecoder) Decode(fullMethod string, message *internal.Message) (*
 		if err != nil {
 			continue
 		}
+		// check for any unknown fields and add them to the descriptor
+		descriptor, err := d.unknownField.enrichDecodeDescriptor(descriptor, message)
+		if err != nil {
+			continue
+		}
 		dyn := dynamic.NewMessage(descriptor)
-		// now unmarshal again using the new generated message type
+		// now unmarshal using the enriched message type
 		err = proto.Unmarshal(message.RawMessage, dyn)
 		if err == nil {
 			return dyn, nil

diff --git a/internal/proto_decoder/encode.go b/internal/proto_decoder/encode.go
@@ -20,10 +20,12 @@ type MessageEncoder interface {
 
 // Chain together a number of resolvers to decode incoming messages.
 // Resolvers are in priority order, the first to return a nil error
-// is used to decode the message.
+// is used to decode the message. If no resolvers are successful,
+// a default resolver is used that always returns empty.Empty
 func NewEncoder(resolvers ...MessageResolver) *messageEncoder {
 	return &messageEncoder{
-		resolvers: resolvers,
+		resolvers: append(resolvers),
+		// TODO: include an unknown message encoder here
 	}
 }