Hi @AnatolyRugalev ,

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

All the best,
Ran~

@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.

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.

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.

#2900

Alright, I will add a flag then

FYI, the pull request has been updated: #2896