Closed babattles closed 2 weeks ago
lucix-aws
commented
1 month ago I agree in principle, but it would have to be either opt-in a new major version of attributevalue (and I can't say I'm inclined to do an opt-in for something this specific). This is a breaking change, and it's impossible to predict how it might affect or break existing software on upgrade.
babattles
commented
1 month ago This is a breaking change, and it's impossible to predict how it might affect or break existing software on upgrade.
I want to point out that this functionality was part of v1, and I only got here by trying to upgrade to v2.
Since the function signature of .MarshalDynamoDBAttributeValue() changed from v1 -> v2, this regression is a blocker for upgrading to v2 for code that heavily leans on custom types.
RanVaknin
commented
1 month ago Hi there,
Can confirm that this behavior has changed between v1 and v2
v1 code:
type OnlyNull struct{}
func (o OnlyNull) MarshalDynamoDBAttributeValue() (*dynamodb.AttributeValue, error) {
return &dynamodb.AttributeValue{NULL: aws.Bool(true)}, nil
}
type MyStruct struct {
Val OnlyNull `dynamodbav:",omitempty"`
}
func marshalOnlyNullStruct() {
m := MyStruct{}
marshaled, err := dynamodbattribute.MarshalMap(m)
if err != nil {
fmt.Println("Error:", err)
return
}
fmt.Println("Marshaled map:", marshaled)
}
// prints:
// Marshaled map: map[]v2 code:
type OnlyNull struct{}
func (o OnlyNull) MarshalDynamoDBAttributeValue() (types.AttributeValue, error) {
return &types.AttributeValueMemberNULL{Value: true}, nil
}
type MyStruct struct {
Val OnlyNull `dynamodbav:",omitempty"`
}
func marshalOnlyNullStruct() {
m := MyStruct{}
marshaled, err := attributevalue.MarshalMap(m)
if err != nil {
fmt.Println("Error:", err)
return
}
fmt.Println("Marshaled map:", marshaled)
}
// prints:
// Marshaled map: map[Val:0x140000200c0]I tried to root cause this and this is what I have found.
I believe that omitempty is being applied correctly, and the struct is being stripped of empty members. Instead the marshaller's isZeroValue() function fails to recognize the empty value because it does not have a case for handling empty structs:
func isZeroValue(v reflect.Value) bool {
switch v.Kind() {
case reflect.Invalid:
return true
case reflect.Array:
return v.Len() == 0
case reflect.Map, reflect.Slice:
return v.IsNil()
case reflect.String:
return v.Len() == 0
case reflect.Bool:
return !v.Bool()
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return v.Int() == 0
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
return v.Uint() == 0
case reflect.Float32, reflect.Float64:
return v.Float() == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
+ case reflect.Struct:
+ return v.NumField() == 0
}
return false
}Adding this results in:
Marshaled map: map[]Root causing and comparing aside, what @lucix-aws wrote remains. This will be a breaking change since customers might be already relying on this behavior.
Thanks, Ran~
lucix-aws
commented
1 month ago I don't think that root cause or proposed patch is right. What does the number of fields of a struct have to do with its zero-ness? The argument here is that null (and its corresponding its attribute value) is zero/empty and so should follow omitempty rules.
Regardless, the fact that it behaves the "agreed-upon" way in v1 is good enough for me.
@babattles I'll accept your previous PR provided you add it under an opt-in (for reasons previously stated).
github-actions[bot]
commented
2 weeks ago This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.
Acknowledgements
go get -u github.com/aws/aws-sdk-go-v2/...)Describe the bug
When passing a struct with a custom type field, tagged with
omitempty, that implements the Marshaler interface to .MarshalMap(), the field should be omitted from the resulting map if the custom field's.MarshalDynamoDBAttributeValue()implementation returns&types.AttributeValueMemberNULL{Value: true}, nil.Expected Behavior
omitempty should omit the custom field from the resulting map returned from
.MarshalMap()if the value is of type NULLCurrent Behavior
.MarshalMap()returns a map with a NULL field like:map[string]types.AttributeValue{"Val":(*types.AttributeValueMemberNULL)(0x1400021ea08)}Reproduction Steps
Possible Solution
Either allow NULLs to be omitted by omitempty, or provide another type that implements
types.AttributeValuethat is able to be omitted.Additional Information/Context
No response
AWS Go SDK V2 Module Versions Used
Compiler and Version used
go version go1.22.2 darwin/arm64
Operating System and version
MacOS Sonoma 14.5