dynamodb: StringSet and StringSet decoders call unmarshaller incorrectly

[AnatolyRugalev]

Acknowledgements

• I have searched (https://github.com/aws/aws-sdk/issues?q=is%3Aissue) for past instances of this issue
• I have verified all of my SDK modules are up-to-date (you can perform a bulk update with go get -u github.com/aws/aws-sdk-go-v2/...)

Describe the bug

When unmarshaling StringSet or NumberSet values via attributevalue.Unmarshal, unmarshaller interface receives the whole set instead of individual slice items.

Unmarshaler is called here: https://github.com/aws/aws-sdk-go-v2/blob/main/feature/dynamodb/attributevalue/decode.go#L807

	for i := 0; i < v.Cap() && i < len(ss); i++ {
		if !isArray {
			v.SetLen(i + 1)
		}
		u, elem := indirect[Unmarshaler](v.Index(i), indirectOptions{})
		if u != nil {
            // This should pass `&types.AttributeValueMemberS{Value: s[i]}`
			return u.UnmarshalDynamoDBAttributeValue(&types.AttributeValueMemberSS{Value: ss})
		}
		if err := d.decodeString(ss[i], elem, tag{}); err != nil {
			return err
		}
	}

This issue is present in both dynamodb/attributevalue and dynamodbstreams/attributevalue

Regression Issue

• Select this option if this issue appears to be a regression.

Expected Behavior

Unmarshaler should receive individual set items

Current Behavior

Unmarshaler receives the whole set

Reproduction Steps

package main

import (
	"errors"
	"github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue"
	"github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
)

type myType string

type doc struct {
	Items []myType `dynamodbav:"items,stringset"`
}

func (m *myType) UnmarshalDynamoDBAttributeValue(av types.AttributeValue) error {
	switch v := av.(type) {
	case *types.AttributeValueMemberS:
		*m = myType(v.Value)
		return nil
	default:
		return errors.New("unexpected type")
	}
}

func main() {
	d := doc{
		Items: []myType{"a", "b"},
	}

	av, err := attributevalue.Marshal(d)
	if err != nil {
		panic(err)
	}

	var d2 doc
	if err := attributevalue.Unmarshal(av, &d2); err != nil {
		panic(err)
	}
}

Possible Solution

-			return u.UnmarshalDynamoDBAttributeValue(&types.AttributeValueMemberSS{Value: ss})
+ 		return u.UnmarshalDynamoDBAttributeValue(&types.AttributeValueMemberS{Value: ss[i]})

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

    github.com/aws/aws-sdk-go-v2 v1.27.1
	github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue v1.14.0
	github.com/aws/aws-sdk-go-v2/feature/dynamodb/expression v1.7.22
	github.com/aws/aws-sdk-go-v2/service/dynamodb v1.32.7

Compiler and Version used

go version go1.23.2 darwin/arm64

Operating System and version

MacOS

[RanVaknin]

Hi @AnatolyRugalev ,

Thanks for the report. Indeed looks like a bug. We will have someone review your PR.

All the best,
Ran~

[lucix-aws]

@AnatolyRugalev Is there a reason you have a marshaler implemented in this case for the string alias? All it's doing is taking the string attribute value and setting it to the receiver. The reported bug is totally valid, but if you removed the unmarshaller implementation you wouldn't hit the bad code path here.

We're not opposed to taking your patch in #2896 but it's not something we can change by default - it would have to be opt-in. It's entirely possible to write an unmarshaler that responds to this incorrect behavior (suppose your unmarshal implementation had an additional case for AttributeValueMemberSS that did something) and we can't risk breaking users who have taken a dependency on it, intentional or not.

[AnatolyRugalev]

Hey @lucix-aws ,

Unfortunately, that unmarshaller is not possible to implement. Unmarshal function's receiver type is *A, and the contents of incoming AttributeValue is supposed to be assigned to []A, which is not really possible to do from within the Unmarhsal function scope. You can only achieve that if you push that state out of scope. I would love to be proven wrong here, so if you still think that this is possible, I would like to see a code snippet that implements your idea.

Thanks for looking into this.

Edit:

Just to clarify, I can agree that it is technically possible, but it's not really feasible in a real code base. Let's assume that someone has implemented a workaround and assigns []A to a package-level variable:

var unmarshaledStringSet []myType

func (m *myType) UnmarshalDynamoDBAttributeValue(av types.AttributeValue) error {
	switch v := av.(type) {
	case *types.AttributeValueMemberSS:
        unmarhsaledStringSet = valueFromStringSet(v)
		return nil
	default:
		return errors.New("unexpected type")
	}
}

That would mean that it will be impossible to call this unmarshaler multiple times without resetting the state. If we make package level var a slice, then you won't be able to associate which set belongs to which attribute path, since attribute path is not accessible from within the unmarshaler.

This makes me to believe that the more plausible workaround is to just not to use unmarshaler for SS values and rely on pure string type, performing post-processing after the unmarhsaling.

While technically it's a breaking change, I would argue that this has never worked before, and we can safely fix it without breaking any contracts.

[lucix-aws]

this has never worked before

This is just not true though. Whether or not it worked before depends entirely on the Unmarshal implementation.

Something like your example, or even something like this,

type myType string

type doc struct {
	Items []myType `dynamodbav:"items,stringset"`
}

func (m *myType) UnmarshalDynamoDBAttributeValue(av types.AttributeValue) error {
	switch v := av.(type) {
	case *types.AttributeValueMemberS:
		*m = myType(v.Value)
		return nil
	case *types.AttributeValueMemberSS:
		// this could literally be anything, it's just an example
		*m = myType(strings.Join(v.Value, ","))
		return nil
	default:
		return errors.New("unexpected type")
	}
}

regardless of whether we consider them "valid" or "sensible" could absolutely exist in the wild. The customer could have either stumbled across this bug and tried to work around it, or they could literally be unaware that it's affecting them.

it's not really feasible in a real code base

I think we're undermining the possibility of this being depended on in general. We're not taking that risk with customer data from a tier-zero AWS service in an SDK used by tens of thousands of calling applications.

Again, your patch is fine. But it has to be opt-in. If not, we'll need to close the PR.

[lucix-aws]

#2900

[AnatolyRugalev]

Alright, I will add a flag then

[AnatolyRugalev]

FYI, the pull request has been updated: #2896