Custom Marshaler returning NULL should be omitted from .MarshalMap() with omitempty tag

[babattles]

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 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 NULL

Current Behavior

.MarshalMap() returns a map with a NULL field like: map[string]types.AttributeValue{"Val":(*types.AttributeValueMemberNULL)(0x1400021ea08)}

Reproduction Steps

import (
	"testing"

	"github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue"
	"github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

type OnlyNull struct{}

func (o OnlyNull) MarshalDynamoDBAttributeValue() (types.AttributeValue, error) {
	return &types.AttributeValueMemberNULL{Value: true}, nil
}

func TestMaybe(t *testing.T) {
	t.Parallel()

	type MyStruct struct {
		Val OnlyNull `dynamodbav:",omitempty"`
	}

	// Test Fails
	t.Run("omitted", func(t *testing.T) {
		t.Parallel()

		o := MyStruct{}
		val, err := attributevalue.MarshalMap(o)
		require.NoError(t, err)
		assert.Empty(t, val)
	})
}

Possible Solution

Either allow NULLs to be omitted by omitempty, or provide another type that implements types.AttributeValue that is able to be omitted.

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue v1.14.10
github.com/aws/aws-sdk-go-v2/service/dynamodb v1.34.4
github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 // indirect
github.com/aws/aws-sdk-go-v2/credentials v1.17.27 // indirect
github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.17.10 // indirect
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect
github.com/aws/aws-sdk-go-v2/internal/v4a v1.3.15 // indirect
github.com/aws/aws-sdk-go-v2/service/dynamodbstreams v1.22.3 // indirect
github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect
github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.3.17 // indirect
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 // indirect
github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.15 // indirect
github.com/aws/aws-sdk-go-v2/service/s3 v1.58.3 // indirect
github.com/aws/smithy-go v1.20.3 // indirect

Compiler and Version used

go version go1.22.2 darwin/arm64

Operating System and version

MacOS Sonoma 14.5

[lucix-aws]

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]

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]

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]

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]

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.