service/dynamodb/dynamodbattribute: Retain backwards compatibility wi…

…th ConvertTo (#1978)

This PR adds special handling if string data is the source for a byte slice field. Previously, an Unmarshalling error would be returned. Now, if the string is valid base64 data--as it would be if it came from Convert*--then it will be decoded as base64 and saved as bytes.

The sdk docs explain how the Convert* functions are deprecated and (Un)marshal should be used instead. But Unmarshalling data originally created with Convert* can have issues which I've not seen mentioned in the docs.

service/dynamodb/dynamodbattribute/decode.go

@@ -1,6 +1,7 @@
 package dynamodbattribute
 
 import (
+	"encoding/base64"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -538,6 +539,16 @@ func (d *Decoder) decodeString(s *string, v reflect.Value, fieldTag tag) error {
 	switch v.Kind() {
 	case reflect.String:
 		v.SetString(*s)
+	case reflect.Slice:
+		// To maintain backwards compatibility with the ConvertFrom family of methods
+		// which converted []byte into base64-encoded strings if the input was typed
+		if v.Type() == byteSliceType {
+			decoded, err := base64.StdEncoding.DecodeString(*s)
+			if err != nil {
+				return &UnmarshalError{Err: err, Value: "string", Type: v.Type()}
+			}
+			v.SetBytes(decoded)
+		}
 	case reflect.Interface:
 		// Ensure type aliasing is handled properly
 		v.Set(reflect.ValueOf(*s).Convert(v.Type()))

service/dynamodb/dynamodbattribute/decode_test.go

@@ -234,6 +234,30 @@ func TestUnmarshalListError(t *testing.T) {
 	}
 }
 
+func TestUnmarshalConvertToData(t *testing.T) {
+	type T struct {
+		Int       int
+		Str       string
+		ByteSlice []byte
+		StrSlice  []string
+	}
+
+	exp := T{
+		Int:       42,
+		Str:       "foo",
+		ByteSlice: []byte{42, 97, 83},
+		StrSlice:  []string{"cat", "dog"},
+	}
+	av, err := ConvertToMap(exp)
+	if err != nil {
+		t.Fatalf("expect no error, got %v", err)
+	}
+
+	var act T
+	err = UnmarshalMap(av, &act)
+	assertConvertTest(t, 0, act, exp, err, nil)
+}
+
 func TestUnmarshalMapShared(t *testing.T) {
 	for i, c := range sharedMapTestCases {
 		err := UnmarshalMap(c.in, c.actual)

service/dynamodb/dynamodbattribute/doc.go

@@ -81,7 +81,7 @@
 // The ConvertTo, ConvertToList, ConvertToMap, ConvertFrom, ConvertFromMap
 // and ConvertFromList methods have been deprecated. The Marshal and Unmarshal
 // functions should be used instead. The ConvertTo|From marshallers do not
-// support BinarySet, NumberSet, nor StringSets, and will incorrect marshal
+// support BinarySet, NumberSet, nor StringSets, and will incorrectly marshal
 // binary data fields in structs as base64 strings.
 //
 // The Marshal and Unmarshal functions correct this behavior, and removes
@@ -91,5 +91,11 @@
 // replaced with have been replaced with dynamodbattribute.Marshaler and
 // dynamodbattribute.Unmarshaler interfaces.
 //
+// The Unmarshal functions are backwards compatible with data marshalled by
+// ConvertTo*, but the reverse is not true: objects marshalled using Marshal
+// are not necessarily usable by ConvertFrom*. This backward compatibility is
+// intended to assist with incremental upgrading of data following a switch
+// away from the Convert* family of functions.
+//
 // `time.Time` is marshaled as RFC3339 format.
 package dynamodbattribute