service/dynamodb/dynamodbattribute: Retain backwards compatibility with ConvertTo #1978
Conversation
…th ConvertTo data
There was a problem hiding this comment.
Looks good thanks for putting this change together and identifying the compatibility issue.
The only concern I have is that this change would allow a user to read an item previously marshaled with ConvertTo, and then If the user uses the Marshal function to marshal back out the Go type to a DynamoDB item the value will be stored in the B field instead of the S field of the DynamoDB Attribute value record. An app updating DynamoDB items which only uses the new Marshal functions, might experience this issue.
Given that this is addictive functionality that never existed before the concern probably is not that significant. If we can document this in the package doc.go bottom section I think it will help at clarify the migration from ConvertTo to Marshal
| if v.Type() == byteSliceType { | ||
| decoded, err := base64.StdEncoding.DecodeString(*s) | ||
| if err != nil { | ||
| return &UnmarshalTypeError{Value: "string", Type: v.Type()} |
There was a problem hiding this comment.
This should also set the Err member of UnmarshalTypeError.
https://github.com/aws/aws-sdk-go/pull/1978/files#diff-7b4de96174d722928e311dc8ec32d2fbR744
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
I can definitely add some comments to doc.go. I think the change is narrowly enough scoped that it shouldn't cause new problems, and will allow much easier upgrades like we're facing. The key warning will be that data written using Marshal is not going to necessarily be readable by the deprecated functions. One still needs to think through the broader upgrade path, but at least this backwards compatibility should ease the implementation burden somewhat. |
|
Thanks for making the updates @kalafut . The changes look good. I'll get this merged in and it will be included in the SDK's next release. |
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. We ran into this issue in our most recent Vault release: hashicorp/vault#4721
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.