From 71fc661c5ba32987eada2f8d343776282ebdc50d Mon Sep 17 00:00:00 2001 From: Sanyia Saidova Date: Mon, 23 Mar 2026 16:01:12 +0000 Subject: [PATCH] fix(dynamodb): introducing symmetric marshalling/unmarshalling logic for time.Time The marshalling logic only formats the time to a desired standard, but does not check that the output is parseable by time.Parse. We experienced an issue where we were able to persist a time with a 5 digit year, but when trying to read the record from DynamoDB, the unmarshalling failed. Ideally we should never store or marshal a record that cannot be unmarshalled. Adding protection for this by calling the parsing logic used by the decoder within the encoder and ensuring it doesn\'t error. This adds minor overhead when encoding structs to DynamoDB\'s format, but the trade off is that this protects against inserting records which can never be unmarshalled. This is a breaking change, but open to changing this implementation so that users can add an option to invoke the check. Happy to discuss and thank you for reviewing the code in advance! --- feature/dynamodb/attributevalue/decode.go | 11 ++- .../dynamodb/attributevalue/decode_test.go | 67 +++++++++++++++++++ feature/dynamodb/attributevalue/encode.go | 12 +++- .../dynamodb/attributevalue/encode_test.go | 12 ++++ 4 files changed, 99 insertions(+), 3 deletions(-) diff --git a/feature/dynamodb/attributevalue/decode.go b/feature/dynamodb/attributevalue/decode.go index 9be77b9912a6..97a646041c59 100644 --- a/feature/dynamodb/attributevalue/decode.go +++ b/feature/dynamodb/attributevalue/decode.go @@ -202,7 +202,10 @@ func UnmarshalListOfMapsWithOptions(l []map[string]types.AttributeValue, out int type DecodeTimeAttributes struct { // Will decode S attribute values and SS attribute value elements into time.Time // - // Default string parsing format is time.RFC3339 + // Default string parsing format tries time.RFC3339Nano, time.RFC3339, + // and ISO 8601 variants with literal "Z" or no timezone. + // All input strings are validated against these formats before the + // decode function is called. S func(string) (time.Time, error) // Will decode N attribute values and NS attribute value elements into time.Time // @@ -1039,7 +1042,7 @@ func (e *UnmarshalError) Error() string { } func defaultDecodeTimeS(v string) (time.Time, error) { - t, err := time.Parse(time.RFC3339, v) + t, err := parseTimeS(v) if err != nil { return time.Time{}, &UnmarshalError{Err: err, Value: v, Type: timeType} } @@ -1049,3 +1052,7 @@ func defaultDecodeTimeS(v string) (time.Time, error) { func defaultDecodeTimeN(v string) (time.Time, error) { return decodeUnixTime(v) } + +func parseTimeS(v string) (time.Time, error) { + return time.Parse(time.RFC3339, v) +} diff --git a/feature/dynamodb/attributevalue/decode_test.go b/feature/dynamodb/attributevalue/decode_test.go index c99887eacbb2..3b42c606bb68 100644 --- a/feature/dynamodb/attributevalue/decode_test.go +++ b/feature/dynamodb/attributevalue/decode_test.go @@ -812,6 +812,22 @@ func TestUnmarshalTime_S_SS(t *testing.T) { input: "1970-01-01T00:02:03.01Z", expect: time.Unix(123, 10000000).UTC(), }, + "String RFC3339 no fractional": { + input: "1970-01-01T00:02:03Z", + expect: time.Unix(123, 0).UTC(), + }, + "String RFC3339 with offset": { + input: "1970-01-01T05:32:03+05:30", + expect: time.Unix(123, 0).In(time.FixedZone("", 5*3600+30*60)), + }, + "String DateTime literal Z": { + input: "1970-01-01T00:02:03.01Z", + expect: time.Unix(123, 10000000).UTC(), + }, + "String DateTime no timezone": { + input: "1970-01-01T00:02:03.01", + expect: time.Unix(123, 10000000).UTC(), + }, "String UnixDate": { input: "Thu Jan 1 00:02:03 UTC 1970", expect: time.Unix(123, 0).UTC(), @@ -1009,6 +1025,57 @@ func TestCustomDecodeNAndDefaultDecodeS(t *testing.T) { } } +func TestUnmarshalTime_S_RoundTrip(t *testing.T) { + type A struct { + TimeField time.Time + } + + times := []time.Time{ + time.Date(2024, 1, 15, 12, 30, 45, 0, time.UTC), + time.Date(2024, 1, 15, 12, 30, 45, 123456789, time.UTC), + time.Date(2024, 1, 15, 12, 30, 45, 100000000, time.UTC), + } + + for _, ts := range times { + t.Run(ts.String(), func(t *testing.T) { + input := A{TimeField: ts} + + av, err := Marshal(input) + if err != nil { + t.Fatalf("expect no marshal error, got %v", err) + } + + var output A + err = Unmarshal(av, &output) + if err != nil { + t.Fatalf("expect no unmarshal error, got %v", err) + } + + if !input.TimeField.Equal(output.TimeField) { + t.Errorf("expect round-trip to preserve time, got %v != %v", + input.TimeField, output.TimeField) + } + }) + } +} + +func TestUnmarshalTime_S_FiveDigitYear(t *testing.T) { + type A struct { + TimeField time.Time + } + + inputMap := &types.AttributeValueMemberM{ + Value: map[string]types.AttributeValue{ + "TimeField": &types.AttributeValueMemberS{Value: "58000-01-15T12:30:45.123456789Z"}, + }, + } + var output A + err := Unmarshal(inputMap, &output) + if err == nil { + t.Fatal("expect unmarshal error for 5-digit year, got nil") + } +} + func TestUnmarshalMap_keyTypes(t *testing.T) { type StrAlias string type IntAlias int diff --git a/feature/dynamodb/attributevalue/encode.go b/feature/dynamodb/attributevalue/encode.go index fa57dbb3dff0..daa921a0edb9 100644 --- a/feature/dynamodb/attributevalue/encode.go +++ b/feature/dynamodb/attributevalue/encode.go @@ -906,8 +906,18 @@ func (e *InvalidMarshalError) Error() string { } func defaultEncodeTime(t time.Time) (types.AttributeValue, error) { + s := t.Format(time.RFC3339Nano) + + // Validate the time string can be parsed back using the same + // function the decoder uses. We want to fail inserting data + // into the DB early long before it will be decoded. + if _, err := parseTimeS(s); err != nil { + return nil, &InvalidMarshalError{ + msg: fmt.Sprintf("time value %v does not marshal to a valid timestamp: %s", t, s), + } + } return &types.AttributeValueMemberS{ - Value: t.Format(time.RFC3339Nano), + Value: s, }, nil } diff --git a/feature/dynamodb/attributevalue/encode_test.go b/feature/dynamodb/attributevalue/encode_test.go index c7e76835bda4..8e5c7e96892e 100644 --- a/feature/dynamodb/attributevalue/encode_test.go +++ b/feature/dynamodb/attributevalue/encode_test.go @@ -292,6 +292,18 @@ func TestMarshalListOmitEmptyElem(t *testing.T) { } } +func TestMarshalTime_S_FiveDigitYear(t *testing.T) { + type A struct { + TimeField time.Time + } + + input := A{TimeField: time.Date(58000, 1, 15, 12, 30, 45, 123456789, time.UTC)} + _, err := Marshal(input) + if err == nil { + t.Fatal("expect marshal error for 5-digit year, got nil") + } +} + func TestMarshalMapOmitEmptyElem(t *testing.T) { expect := &types.AttributeValueMemberM{ Value: map[string]types.AttributeValue{