DynamoDB AttributeValue's MarshallMap() Does Not Support time.Time Correctly

kmacmcfarlane commented 1 year ago

Describe the bug

I am seeing that it's not possible to use omitempty on time.Time struct fields with attributevalue.MarshalMap() from github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue.

This behavior was observed on the latest version: github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue v1.12.3

Expected Behavior

When MarshalMap() is supplied with a struct containing a time.Time field with the tag omitempty whos value is the zero value, the output map should not contain a key for that struct field.

Current Behavior

Output attribute value map includes a key and value for the field that should be omitted.

Reproduction Steps

repro_test.go

package service

import (
    "github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue"
    . "github.com/onsi/ginkgo/v2"
    . "github.com/onsi/gomega"
    "time"
)

var _ = Describe("Serialize Struct", func() {

    Context("Struct with omitempty on a time.Time field", func() {
        It("Does not serialize the field", func() {

            e := struct {
                Created time.Time `dynamodbav:"created,omitempty"`
            }{
                Created: time.Time{},
            }
            av, err := attributevalue.MarshalMap(e)
            Ω(err).ShouldNot(HaveOccurred())

            Ω(av).ShouldNot(HaveKey("created"))
        })
    })
})

service_test_suite.go

package service_test

import (
    "testing"

    . "github.com/onsi/ginkgo/v2"
    . "github.com/onsi/gomega"
)

func TestService(t *testing.T) {
    RegisterFailHandler(Fail)
    RunSpecs(t, "Service Suite")
}

Possible Solution

As far as I've been able to reason by stepping through the repro above in a debugger, there should probably be a check for fieldTag.OmitEmpty in this code block in encode.go (just like the fieldTag.AsUnixTime tag is checked).

https://github.com/aws/aws-sdk-go-v2/blob/3bd97c063d962a34ca496720a3ce00ef4affe5fd/feature/dynamodb/attributevalue/encode.go#L478

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

        github.com/aws/aws-lambda-go v1.34.1
        github.com/aws/aws-sdk-go v1.44.128
        github.com/aws/aws-sdk-go-v2 v1.23.1
        github.com/aws/aws-sdk-go-v2/config v1.17.10
        github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue v1.12.3
        github.com/aws/aws-sdk-go-v2/feature/dynamodb/expression v1.4.35
        github.com/aws/aws-sdk-go-v2/service/dynamodb v1.25.3
        github.com/aws/aws-sdk-go-v2/service/sfn v1.19.7
        github.com/aws/aws-xray-sdk-go v1.8.0
        github.com/awslabs/aws-lambda-go-api-proxy v0.13.3
        github.com/aws/aws-sdk-go-v2/credentials v1.12.23 // indirect
        github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.12.19 // indirect
        github.com/aws/aws-sdk-go-v2/internal/configsources v1.2.4 // indirect
        github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.5.4 // indirect
        github.com/aws/aws-sdk-go-v2/internal/ini v1.3.26 // indirect
        github.com/aws/aws-sdk-go-v2/service/cloudwatchevents v1.9.2 // indirect
        github.com/aws/aws-sdk-go-v2/service/dynamodbstreams v1.17.3 // indirect
        github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.10.1 // indirect
        github.com/aws/aws-sdk-go-v2/service/internal/endpoint-discovery v1.8.4 // indirect
        github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.19 // indirect
        github.com/aws/aws-sdk-go-v2/service/kms v1.18.5 // indirect
        github.com/aws/aws-sdk-go-v2/service/sns v1.20.1 // indirect
        github.com/aws/aws-sdk-go-v2/service/sso v1.11.25 // indirect
        github.com/aws/aws-sdk-go-v2/service/ssooidc v1.13.8 // indirect
        github.com/aws/aws-sdk-go-v2/service/sts v1.17.1 // indirect
        github.com/aws/smithy-go v1.17.0 // indirect

Compiler and Version used

go version go1.20.6 linux/amd64

Operating System and version

Fedora Linux 37

RanVaknin commented 12 months ago

Hi @kmacmcfarlane ,

The Dynamodb marshallar is built to account for Dynamodb native types (BOOL,SS,N,S,L, etc..) time.Time is not one of them. Thats why there is no out of the box functionality to support this. I have converted this to a feature request and added it to our backlog until it gets prioritized.

In the meantime, as a workaround, you can handle your time.Time data by converting it into a string or an epoch number, both of which are supported by the encoder. For example, you can format the time.Time as an ISO 8601 string, or you can convert it to a UNIX timestamp (epoch number). Something like

epochNumber := yourTime.Unix()

Thanks, Ran~

kmacmcfarlane commented 12 months ago

@RanVaknin This is surprising, since the godocs indicate time.RFC3339Nano is the default encoding for time.Time in Marshal() and the EncoderOptions docs indicate if you don't supply a EncodeTime func it will use time.RFC3339Nano as well. If I call MarshalMapWithOptions() with default options, I would expect it to encode time.Time fields as time.RFC3339Nano strings.

Further the docs state

MarshalMapWithOptions is an alias for MarshalWithOptions func which marshals Go value type to a map of AttributeValues. If the in parameter does not serialize to a map, an empty AttributeValue map will be returned.

It feels like it shouldn't behave any different than a regular Marshal() which is why I think it's a bug.

aws / aws-sdk-go-v2