Merge pull request #143 from monzo/allow-use-of-legacyproto

Requests should allow encoding and decoding as legacy protobuf

Author: suhailpatel

.gitignore

@@ -1 +1,17 @@
 .DS_Store
+.AppleDouble
+.LSOverride
+.idea
+.vscode
+coverage.out
+
+# Files that might appear on external disk
+.Spotlight-V100
+.Trashes
+
+# Directories potentially created on remote AFP share
+.AppleDB
+.AppleDesktop
+Network Trash Folder
+Temporary Items
+.apdisk

legacyprototest/prototest.pb.go

@@ -0,0 +1,7 @@
+syntax = "proto3";
+option go_package = "legacyprototest";
+
+message LegacyGreeting {
+    string message = 1;
+    int32 priority = 2;
+}

legacyprototest/prototest.proto

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"strings"
 
+	legacyproto "github.com/golang/protobuf/proto"
 	"github.com/monzo/terrors"
 	"google.golang.org/protobuf/proto"
 )
@@ -97,19 +98,18 @@ func (r Request) Decode(v interface{}) error {
 	// See: https://datatracker.ietf.org/doc/html/draft-rfernando-protocol-buffers-00#section-3.2
 	// See: https://github.com/google/protorpc/blob/eb03145/python/protorpc/protobuf.py#L49-L51
 	case "application/octet-stream", "application/x-google-protobuf", "application/protobuf", "application/x-protobuf":
-		m, ok := v.(proto.Message)
-		if !ok {
+		switch m := v.(type) {
+		case proto.Message:
+			err = proto.Unmarshal(b, m)
+		case legacyproto.Message:
+			err = legacyproto.Unmarshal(b, m)
+		default:
 			return terrors.InternalService("invalid_type", "could not decode proto message", nil)
 		}
-		err = proto.Unmarshal(b, m)
 	// As older versions of typhon used json, we don't use protojson here as they are mutually exclusive standards with
 	// major differences in how they handle some types (such as Enums)
 	default:
-		m, ok := v.(proto.Message)
-		if !ok {
-			return terrors.InternalService("invalid_type", "could not decode proto message", nil)
-		}
-		err = json.Unmarshal(b, m)
+		err = json.Unmarshal(b, v)
 	}
 
 	return terrors.WrapWithCode(err, nil, terrors.ErrBadRequest)

request.go

@@ -5,14 +5,16 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"google.golang.org/protobuf/proto"
 	"io/ioutil"
 	"strings"
 	"testing"
 
+	legacyproto "github.com/golang/protobuf/proto"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 
+	"github.com/monzo/typhon/legacyprototest"
 	"github.com/monzo/typhon/prototest"
 )
 
@@ -33,6 +35,90 @@ func TestRequestDecodeCloses(t *testing.T) {
 	}
 }
 
+func TestRequestDecodeJSONStruct(t *testing.T) {
+	req := NewRequest(nil, "GET", "/", nil)
+	b := []byte("{\"message\":\"Hello world!\"}\n")
+	r := newDoneReader(ioutil.NopCloser(bytes.NewReader(b)), -1)
+	req.Body = r
+
+	g := &struct {
+		Message string `json:"message"`
+	}{}
+	err := req.Decode(g)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g.Message)
+}
+
+func TestRequestDecodeProto(t *testing.T) {
+	generateRequest := func() Request {
+		req := NewRequest(nil, "GET", "/", nil)
+		b, _ := proto.Marshal(&prototest.Greeting{Message: "Hello world!"})
+		r := newDoneReader(ioutil.NopCloser(bytes.NewReader(b)), -1)
+		req.Header.Set("Content-Type", "application/protobuf")
+		req.Body = r
+		return req
+	}
+
+	req1 := generateRequest()
+	g1 := &prototest.Greeting{}
+	err := req1.Decode(g1)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g1.Message)
+
+	req2 := generateRequest()
+	g2 := &legacyprototest.LegacyGreeting{}
+	err = req2.Decode(g2)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g2.Message)
+}
+
+func TestRequestDecodeProtoMaskingAsJSON(t *testing.T) {
+	req := NewRequest(nil, "GET", "/", nil)
+	b := []byte("{\"message\":\"Hello world!\"}\n")
+	r := newDoneReader(ioutil.NopCloser(bytes.NewReader(b)), -1)
+	req.Body = r
+
+	g := &prototest.Greeting{}
+	err := req.Decode(g)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g.Message)
+}
+
+func TestRequestDecodeLegacyProto(t *testing.T) {
+	generateRequest := func() Request {
+		req := NewRequest(nil, "GET", "/", nil)
+		b, _ := legacyproto.Marshal(&legacyprototest.LegacyGreeting{Message: "Hello world!"})
+		r := newDoneReader(ioutil.NopCloser(bytes.NewReader(b)), -1)
+		req.Header.Set("Content-Type", "application/protobuf")
+		req.Body = r
+		return req
+	}
+
+	req1 := generateRequest()
+	g1 := &prototest.Greeting{}
+	err := req1.Decode(g1)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g1.Message)
+
+	req2 := generateRequest()
+	g2 := &legacyprototest.LegacyGreeting{}
+	err = req2.Decode(g2)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g2.Message)
+}
+
+func TestRequestDecodeLegacyProtoMaskingAsJSON(t *testing.T) {
+	req := NewRequest(nil, "GET", "/", nil)
+	b := []byte("{\"message\":\"Hello world!\"}\n")
+	r := newDoneReader(ioutil.NopCloser(bytes.NewReader(b)), -1)
+	req.Body = r
+
+	g := &legacyprototest.LegacyGreeting{}
+	err := req.Decode(g)
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello world!", g.Message)
+}
+
 // TestRequestEncodeReader verifies that passing an io.Reader to request.Encode() uses it properly as the body, and
 // does not attempt to encode it as JSON
 func TestRequestEncodeReader(t *testing.T) {
@@ -88,7 +174,7 @@ func TestRequestEncodeProtobuf(t *testing.T) {
 }
 
 func TestRequestEncodeJSON(t *testing.T) {
-	message := map[string]interface{} {
+	message := map[string]interface{}{
 		"foo": "bar",
 		"bar": 3,
 	}
@@ -118,7 +204,6 @@ func TestRequestSetMetadata(t *testing.T) {
 	assert.Equal(t, []string{"data"}, req.Request.Header["meta"])
 }
 
-
 func jsonStreamMarshal(v interface{}) ([]byte, error) {
 	var buffer bytes.Buffer
 	writer := bufio.NewWriter(&buffer)

request_test.go

@@ -5,6 +5,8 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	legacyproto "github.com/golang/protobuf/proto"
+	"github.com/monzo/typhon/legacyprototest"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -218,6 +220,44 @@ func TestResponseDecodeProtobufWithAltType(t *testing.T) {
 	assert.EqualValues(t, 1, gout.Priority)
 }
 
+// TestResponseDecodeLegacyProtobuf verifies decoding of a legacy protobuf message
+func TestResponseDecodeLegacyProtobuf(t *testing.T) {
+	t.Parallel()
+
+	g := &legacyprototest.LegacyGreeting{
+		Message:  "Hello world!",
+		Priority: 1,
+	}
+	b, _ := legacyproto.Marshal(g)
+	rsp := NewResponse(Request{})
+	rsp.Body = ioutil.NopCloser(bytes.NewReader(b))
+	rsp.Header.Set("Content-Type", "application/protobuf")
+
+	gout := &legacyprototest.LegacyGreeting{}
+	assert.NoError(t, rsp.Decode(gout))
+	assert.Equal(t, "Hello world!", gout.Message)
+	assert.EqualValues(t, 1, gout.Priority)
+}
+
+// TestResponseDecodeLegacyProtobufWithAltType verifies decoding of a legacy protobuf message
+func TestResponseDecodeLegacyProtobufWithAltType(t *testing.T) {
+	t.Parallel()
+
+	g := &legacyprototest.LegacyGreeting{
+		Message:  "Hello world!",
+		Priority: 1,
+	}
+	b, _ := legacyproto.Marshal(g)
+	rsp := NewResponse(Request{})
+	rsp.Body = ioutil.NopCloser(bytes.NewReader(b))
+	rsp.Header.Set("Content-Type", "application/x-protobuf")
+
+	gout := &prototest.Greeting{}
+	assert.NoError(t, rsp.Decode(gout))
+	assert.Equal(t, "Hello world!", gout.Message)
+	assert.EqualValues(t, 1, gout.Priority)
+}
+
 // rc is a helper type used in tests involving a generic io.ReadCloser
 type rc struct {
 	strings.Reader