Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service/dynamodb/dynamodbattribute: Retain backwards compatibility with ConvertTo #1978

Merged
merged 2 commits into from
Jun 11, 2018
Merged

service/dynamodb/dynamodbattribute: Retain backwards compatibility with ConvertTo #1978

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c0347e
service/dynamodb/dynamodbattribute: Retain backwards compatibility wi…
Jun 8, 2018
fcd5a40
Switch to UnmarshalError and update docs
Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions service/dynamodb/dynamodbattribute/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dynamodbattribute

import (
"encoding/base64"
"fmt"
"reflect"
"strconv"
Expand Down Expand Up @@ -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 &UnmarshalTypeError{Value: "string", Type: v.Type()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also set the Err member of UnmarshalTypeError.

https://github.com/aws/aws-sdk-go/pull/1978/files#diff-7b4de96174d722928e311dc8ec32d2fbR744

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set Err to the base64 decode error? I gave some thought to that but thought it might be confusing. This change really supports the happy path where everything lines up to support a successful upgrade. If anything isn't right, return the same error that would have been shown before if misc string data was trying to be marshalled into []byte.

I'm fine changing this as suggested, but that was my rationale behind the current design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using UnmarshalError (vs. UnmarshalTypeError) works well. This output looks good:

--- FAIL: TestUnmarshalConvertToData (0.00s)
	shared_test.go:385: case 1, expect no error, got UnmarshalError: cannot unmarshal "string" into []uint8.
		caused by: illegal base64 data at input byte 4

}
v.SetBytes(decoded)
}
case reflect.Interface:
// Ensure type aliasing is handled properly
v.Set(reflect.ValueOf(*s).Convert(v.Type()))
Expand Down
24 changes: 24 additions & 0 deletions service/dynamodb/dynamodbattribute/decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down