Wrap basic types in a struct (tendermint#270)

* First stab on tendermint#204: disallow direct encoding of basic types * fix some tests and disallow defined types on decoding (but allow interfaces on decoding) * fix remaining tests and introduce a new error type (instead of panicing) * remove code: condition is always 'false' because 'err' is always 'nil' * WIP: add `(field_number << 3) | wire_type` with field_number == 1 if the passed type is not a struct to simulate protobuf message {} Still missing: - proper table driven tests - decoding (will basically be skipping over the first bytes if non struct type is passed) * remove unused type aliases (use types defined in tests package instead) * Some cleanup: - table driven tests - use helper method `writeFieldIfNotEmpty` where it makes sense - remove a bunch of comments & debug output * Added a more complex test case (currently fails), some lengthy debugging session revealed another minor difference between amino and proto3 (see the two added FIXMEs); also: - renamed some proto3 types - some minor cleanup * Do not prepend with list of structs with field tag (will be handled internally) * WIP: First stab on decoding -> a few failing tests * Fix one of the failing tests: TestBasicTypes * Fix all remaining tests * Fix unfinished comment & remove debug output Signed-off-by: Ismail Khoffi <[email protected]> * remove unsed var Signed-off-by: Ismail Khoffi <[email protected]> * delete debug output and comment Signed-off-by: Ismail Khoffi <[email protected]> * delete debug output and obsolete TODOs and some minot cleanups Signed-off-by: Ismail Khoffi <[email protected]> * Add another test case (type PrimitivesStructAr [2]PrimitivesStruct) and fix implementation: - Additionally to the fieldnum and wiretag we need a size prefix (not bare) in case the typ3 isn't of ByteLength. - add helper methods as Bez suggested: check if struct and check if pointer to struct - fix tests due to changes Signed-off-by: Ismail Khoffi <[email protected]>
mesg-foundation · May 28, 2019 · 0b520c1 · 0b520c1
1 parent 716e340
commit 0b520c1
Show file tree

Hide file tree

Showing 17 changed files with 682 additions and 197 deletions.
diff --git a/amino.go b/amino.go
@@ -2,21 +2,27 @@ package amino
 
 import (
 	"bytes"
-	"encoding/binary"
-	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"reflect"
 	"time"
+
+	"encoding/binary"
+	"encoding/json"
+
+	"github.com/pkg/errors"
 )
 
-//----------------------------------------
-// Global methods for global sealed codec.
-var gcdc *Codec
+var (
+	// Global methods for global sealed codec.
+	gcdc *Codec
+
+	// we use this time to init. a zero value (opposed to reflect.Zero which gives time.Time{} / 01-01-01 00:00:00)
+	zeroTime time.Time
 
-// we use this time to init. a zero value (opposed to reflect.Zero which gives time.Time{} / 01-01-01 00:00:00)
-var zeroTime time.Time
+	// NotPointerErr is thrown when you call a method that expects a pointer, e.g. Unmarshal
+	NotPointerErr = errors.New("expected a pointer")
+)
 
 const (
 	unixEpochStr = "1970-01-01 00:00:00 +0000 UTC"
@@ -203,21 +209,43 @@ func (cdc *Codec) MarshalBinaryBare(o interface{}) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	err = cdc.encodeReflectBinary(buf, info, rv, FieldOptions{BinFieldNum: 1}, true)
-	if err != nil {
-		return nil, err
+	// in the case of of a repeated struct (e.g. type Alias []SomeStruct),
+	// we do not need to prepend with `(field_number << 3) | wire_type` as this
+	// would need to be done for each struct and not only for the first.
+	if rv.Kind() != reflect.Struct && !isStructOrRepeatedStruct(info) {
+		writeEmpty := false
+		typ3 := typeToTyp3(info.Type, FieldOptions{})
+		bare := typ3 != Typ3_ByteLength
+		if err := cdc.writeFieldIfNotEmpty(buf, 1, info, FieldOptions{}, FieldOptions{}, rv, writeEmpty, bare); err != nil {
+			return nil, err
+		}
+		bz = buf.Bytes()
+	} else {
+		err = cdc.encodeReflectBinary(buf, info, rv, FieldOptions{BinFieldNum: 1}, true)
+		if err != nil {
+			return nil, err
+		}
+		bz = buf.Bytes()
 	}
-	bz = buf.Bytes()
-
 	// If registered concrete, prepend prefix bytes.
 	if info.Registered {
+		// TODO: https://github.com/tendermint/go-amino/issues/267
+		//return MarshalBinaryBare(RegisteredAny{
+		//	AminoPreOrDisfix: info.Prefix.Bytes(),
+		//	Value: bz,
+		//})
 		pb := info.Prefix.Bytes()
 		bz = append(pb, bz...)
 	}
 
 	return bz, nil
 }
 
+//type RegisteredAny struct {
+//	AminoPreOrDisfix []byte
+//	Value []byte
+//}
+
 // Panics if error.
 func (cdc *Codec) MustMarshalBinaryBare(o interface{}) []byte {
 	bz, err := cdc.MarshalBinaryBare(o)
@@ -322,16 +350,18 @@ func (cdc *Codec) UnmarshalBinaryBare(bz []byte, ptr interface{}) error {
 
 	rv := reflect.ValueOf(ptr)
 	if rv.Kind() != reflect.Ptr {
-		panic("Unmarshal expects a pointer")
+		return NotPointerErr
 	}
 	rv = rv.Elem()
 	rt := rv.Type()
 	info, err := cdc.getTypeInfo_wlock(rt)
 	if err != nil {
 		return err
 	}
+
 	// If registered concrete, consume and verify prefix bytes.
 	if info.Registered {
+		// TODO: https://github.com/tendermint/go-amino/issues/267
 		pb := info.Prefix.Bytes()
 		if len(bz) < 4 {
 			return fmt.Errorf("UnmarshalBinaryBare expected to read prefix bytes %X (since it is registered concrete) but got %X", pb, bz)
@@ -340,17 +370,97 @@ func (cdc *Codec) UnmarshalBinaryBare(bz []byte, ptr interface{}) error {
 		}
 		bz = bz[4:]
 	}
+	// Only add length prefix if we have another typ3 then Typ3_ByteLength.
+	// Default is non-length prefixed:
+	bare := true
+	var nWrap int
+	isKnownType := (info.Type.Kind() != reflect.Map) && (info.Type.Kind() != reflect.Func)
+	if !isStructOrRepeatedStruct(info) &&
+		!isPointerToStructOrToRepeatedStruct(rv, rt) &&
+		len(bz) > 0 &&
+		(rv.Kind() != reflect.Interface) &&
+		isKnownType {
+		fnum, typ, nFnumTyp3, err := decodeFieldNumberAndTyp3(bz)
+		if err != nil {
+			return errors.Wrap(err, "could not decode field number and type")
+		}
+		if fnum != 1 {
+			return fmt.Errorf("expected field number: 1; got: %v", fnum)
+		}
+		typWanted := typeToTyp3(info.Type, FieldOptions{})
+		if typ != typWanted {
+			return fmt.Errorf("expected field type %v for # %v of %v, got %v",
+				typWanted, fnum, info.Type, typ)
+		}
+
+		slide(&bz, &nWrap, nFnumTyp3)
+		bare = typeToTyp3(info.Type, FieldOptions{}) != Typ3_ByteLength
+	}
+
 	// Decode contents into rv.
-	n, err := cdc.decodeReflectBinary(bz, info, rv, FieldOptions{BinFieldNum: 1}, true)
+	n, err := cdc.decodeReflectBinary(bz, info, rv, FieldOptions{BinFieldNum: 1}, bare)
 	if err != nil {
-		return fmt.Errorf("unmarshal to %v failed after %d bytes (%v): %X", info.Type, n, err, bz)
+		return fmt.Errorf(
+			"unmarshal to %v failed after %d bytes (%v): %X",
+			info.Type,
+			n+nWrap,
+			err,
+			bz,
+		)
 	}
 	if n != len(bz) {
-		return fmt.Errorf("unmarshal to %v didn't read all bytes. Expected to read %v, only read %v: %X", info.Type, len(bz), n, bz)
+		return fmt.Errorf(
+			"unmarshal to %v didn't read all bytes. Expected to read %v, only read %v: %X",
+			info.Type,
+			len(bz),
+			n+nWrap,
+			bz,
+		)
 	}
+
 	return nil
 }
 
+func isStructOrRepeatedStruct(info *TypeInfo) bool {
+	if info.Type.Kind() == reflect.Struct {
+		return true
+	}
+	isRepeatedStructAr := info.Type.Kind() == reflect.Array && info.Type.Elem().Kind() == reflect.Struct
+	isRepeatedStructSl := info.Type.Kind() == reflect.Slice && info.Type.Elem().Kind() == reflect.Struct
+	return isRepeatedStructAr || isRepeatedStructSl
+}
+
+func isPointerToStructOrToRepeatedStruct(rv reflect.Value, rt reflect.Type) bool {
+	if rv.Kind() == reflect.Struct {
+		return true
+	}
+
+	drv, isPtr, isNil := derefPointers(rv)
+	if isPtr && drv.Kind() == reflect.Struct {
+		return true
+	}
+
+	if isPtr && isNil {
+		rt := derefType(rt)
+		if rt.Kind() == reflect.Struct {
+			return true
+		}
+		return rt.Kind() == reflect.Slice && rt.Elem().Kind() == reflect.Struct ||
+			rt.Kind() == reflect.Array && rt.Elem().Kind() == reflect.Struct
+	}
+	isRepeatedStructSl := isPtr && drv.Kind() == reflect.Slice && drv.Elem().Kind() == reflect.Struct
+	isRepeatedStructAr := isPtr && drv.Kind() == reflect.Array && drv.Elem().Kind() == reflect.Struct
+	return isRepeatedStructAr || isRepeatedStructSl
+}
+
+func derefType(rt reflect.Type) (drt reflect.Type) {
+	drt = rt
+	for drt.Kind() == reflect.Ptr {
+		drt = drt.Elem()
+	}
+	return
+}
+
 // Panics if error.
 func (cdc *Codec) MustUnmarshalBinaryBare(bz []byte, ptr interface{}) {
 	err := cdc.UnmarshalBinaryBare(bz, ptr)
@@ -373,7 +483,6 @@ func (cdc *Codec) MarshalJSON(o interface{}) ([]byte, error) {
 
 	// Write the disfix wrapper if it is a registered concrete type.
 	if info.Registered {
-		// Part 1:
 		err = writeStr(w, _fmt(`{"type":"%s","value":`, info.Name))
 		if err != nil {
 			return nil, err
@@ -387,10 +496,6 @@ func (cdc *Codec) MarshalJSON(o interface{}) ([]byte, error) {
 
 	// disfix wrapper continued...
 	if info.Registered {
-		// Part 2:
-		if err != nil {
-			return nil, err
-		}
 		err = writeStr(w, `}`)
 		if err != nil {
 			return nil, err

diff --git a/amino_test.go b/amino_test.go
@@ -60,40 +60,44 @@ func TestUnmarshalBinaryReader(t *testing.T) {
 	assert.Equal(t, s, s2)
 }
 
+type stringWrapper struct {
+	S string
+}
+
 func TestUnmarshalBinaryReaderSize(t *testing.T) {
 	var cdc = amino.NewCodec()
 
-	var s1 string = "foo"
+	s1 := stringWrapper{"foo"}
 	b, err := cdc.MarshalBinaryLengthPrefixed(s1)
 	assert.Nil(t, err)
 	t.Logf("MarshalBinaryLengthPrefixed(s) -> %X", b)
 
-	var s2 string
+	var s2 stringWrapper
 	var n int64
 	n, err = cdc.UnmarshalBinaryLengthPrefixedReader(bytes.NewBuffer(b), &s2, 0)
 	assert.Nil(t, err)
 	assert.Equal(t, s1, s2)
-	frameLengthBytes, msgLengthBytes := 1, 1
-	assert.Equal(t, frameLengthBytes+msgLengthBytes+len(s1), int(n))
+	frameLengthBytes, msgLengthBytes, embedOverhead := 1, 1, 1
+	assert.Equal(t, frameLengthBytes+msgLengthBytes+embedOverhead+len(s1.S), int(n))
 }
 
 func TestUnmarshalBinaryReaderSizeLimit(t *testing.T) {
 	var cdc = amino.NewCodec()
 
-	var s1 string = "foo"
+	s1 := stringWrapper{"foo"}
 	b, err := cdc.MarshalBinaryLengthPrefixed(s1)
 	assert.Nil(t, err)
 	t.Logf("MarshalBinaryLengthPrefixed(s) -> %X", b)
 
-	var s2 string
+	var s2 stringWrapper
 	var n int64
 	n, err = cdc.UnmarshalBinaryLengthPrefixedReader(bytes.NewBuffer(b), &s2, int64(len(b)-1))
 	assert.NotNil(t, err, "insufficient limit should lead to failure")
 	n, err = cdc.UnmarshalBinaryLengthPrefixedReader(bytes.NewBuffer(b), &s2, int64(len(b)))
 	assert.Nil(t, err, "sufficient limit should not cause failure")
 	assert.Equal(t, s1, s2)
-	frameLengthBytes, msgLengthBytes := 1, 1
-	assert.Equal(t, frameLengthBytes+msgLengthBytes+len(s1), int(n))
+	frameLengthBytes, msgLengthBytes, embedOverhead := 1, 1, 1
+	assert.Equal(t, frameLengthBytes+msgLengthBytes+embedOverhead+len(s1.S), int(n))
 }
 
 func TestUnmarshalBinaryReaderTooLong(t *testing.T) {
@@ -125,7 +129,7 @@ func TestUnmarshalBinaryBufferedWritesReads(t *testing.T) {
 	var buf = bytes.NewBuffer(nil)
 
 	// Write 3 times.
-	var s1 string = "foo"
+	s1 := stringWrapper{"foo"}
 	_, err := cdc.MarshalBinaryLengthPrefixedWriter(buf, s1)
 	assert.Nil(t, err)
 	_, err = cdc.MarshalBinaryLengthPrefixedWriter(buf, s1)
@@ -134,7 +138,7 @@ func TestUnmarshalBinaryBufferedWritesReads(t *testing.T) {
 	assert.Nil(t, err)
 
 	// Read 3 times.
-	var s2 string
+	s2 := stringWrapper{}
 	_, err = cdc.UnmarshalBinaryLengthPrefixedReader(buf, &s2, 0)
 	assert.Nil(t, err)
 	assert.Equal(t, s1, s2)

diff --git a/binary-decode.go b/binary-decode.go
@@ -558,6 +558,11 @@ func (cdc *Codec) decodeReflectBinaryByteSlice(bz []byte, info *TypeInfo, rv ref
 	if ert.Kind() != reflect.Uint8 {
 		panic("should not happen")
 	}
+	// If len(bz) == 0 the code below will err
+	if len(bz) == 0 {
+		rv.Set(info.ZeroValue)
+		return 0, nil
+	}
 
 	// Read byte-length prefixed byteslice.
 	var byteslice, _n = []byte(nil), int(0)

diff --git a/binary-encode.go b/binary-encode.go
@@ -296,7 +296,7 @@ func (cdc *Codec) encodeReflectBinaryList(w io.Writer, info *TypeInfo, rv reflec
 				return
 			}
 		}
-	} else {
+	} else { // typ3 == Typ3_ByteLength
 		// NOTE: ert is for the element value, while einfo.Type is dereferenced.
 		isErtStructPointer := ert.Kind() == reflect.Ptr && einfo.Type.Kind() == reflect.Struct
 
@@ -413,27 +413,12 @@ func (cdc *Codec) encodeReflectBinaryStruct(w io.Writer, info *TypeInfo, rv refl
 					return
 				}
 			} else {
-				lBeforeKey := buf.Len()
-				// Write field key (number and type).
-				err = encodeFieldNumberAndTyp3(buf, field.BinFieldNum, typeToTyp3(finfo.Type, field.FieldOptions))
-				if err != nil {
-					return
-				}
-				lBeforeValue := buf.Len()
-
-				// Write field value from rv.
-				err = cdc.encodeReflectBinary(buf, finfo, dfrv, field.FieldOptions, false)
+				// write empty if explicitly set or if this is a pointer:
+				writeEmpty := fopts.WriteEmpty || frvIsPtr
+				err = cdc.writeFieldIfNotEmpty(buf, field.BinFieldNum, finfo, fopts, field.FieldOptions, dfrv, writeEmpty, false)
 				if err != nil {
 					return
 				}
-				lAfterValue := buf.Len()
-
-				if !frvIsPtr && !fopts.WriteEmpty && lBeforeValue == lAfterValue-1 && buf.Bytes()[buf.Len()-1] == 0x00 {
-					// rollback typ3/fieldnum and last byte if
-					// not a pointer and empty:
-					buf.Truncate(lBeforeKey)
-				}
-
 			}
 		}
 	}
@@ -469,3 +454,36 @@ func encodeFieldNumberAndTyp3(w io.Writer, num uint32, typ Typ3) (err error) {
 	_, err = w.Write(buf[0:n])
 	return
 }
+
+func (cdc *Codec) writeFieldIfNotEmpty(
+	buf *bytes.Buffer,
+	fieldNum uint32,
+	finfo *TypeInfo,
+	structsFopts FieldOptions, // the wrapping struct's FieldOptions if any
+	fieldOpts FieldOptions, // the field's FieldOptions
+	derefedVal reflect.Value,
+	isWriteEmpty bool,
+	bare bool,
+) error {
+	lBeforeKey := buf.Len()
+	// Write field key (number and type).
+	err := encodeFieldNumberAndTyp3(buf, fieldNum, typeToTyp3(finfo.Type, fieldOpts))
+	if err != nil {
+		return err
+	}
+	lBeforeValue := buf.Len()
+
+	// Write field value from rv.
+	err = cdc.encodeReflectBinary(buf, finfo, derefedVal, fieldOpts, bare)
+	if err != nil {
+		return err
+	}
+	lAfterValue := buf.Len()
+
+	if !isWriteEmpty && lBeforeValue == lAfterValue-1 && buf.Bytes()[buf.Len()-1] == 0x00 {
+		// rollback typ3/fieldnum and last byte if
+		// not a pointer and empty:
+		buf.Truncate(lBeforeKey)
+	}
+	return nil
+}