Add `dyn.Time` to box a timestamp with its original string value (#1732)

## Changes

If not explicitly quoted, the YAML loader interprets a value like
`2024-08-29` as a timestamp. Such a value is usually intended to be a
string instead. Our normalization logic was not able to turn a time
value back into the original string.

This change boxes the time value to include its original string
representation. Normalization of one of these values into a string can
now use the original input value.

## Tests

Unit tests in `libs/dyn/convert`.

libs/dyn/convert/normalize.go

@@ -267,6 +267,8 @@ func (n normalizeOptions) normalizeString(typ reflect.Type, src dyn.Value, path
 		out = strconv.FormatInt(src.MustInt(), 10)
 	case dyn.KindFloat:
 		out = strconv.FormatFloat(src.MustFloat(), 'f', -1, 64)
+	case dyn.KindTime:
+		out = src.MustTime().String()
 	case dyn.KindNil:
 		// Return a warning if the field is present but has a null value.
 		return dyn.InvalidValue, diags.Append(nullWarning(dyn.KindString, src, path))

libs/dyn/convert/normalize_test.go

@@ -569,6 +569,14 @@ func TestNormalizeStringFromFloat(t *testing.T) {
 	assert.Equal(t, dyn.NewValue("1.2", vin.Locations()), vout)
 }
 
+func TestNormalizeStringFromTime(t *testing.T) {
+	var typ string
+	vin := dyn.NewValue(dyn.MustTime("2024-08-29"), []dyn.Location{{File: "file", Line: 1, Column: 1}})
+	vout, err := Normalize(&typ, vin)
+	assert.Empty(t, err)
+	assert.Equal(t, dyn.NewValue("2024-08-29", vin.Locations()), vout)
+}
+
 func TestNormalizeStringError(t *testing.T) {
 	var typ string
 	vin := dyn.V(map[string]dyn.Value{"an": dyn.V("error")})

libs/dyn/kind.go

@@ -2,7 +2,6 @@ package dyn
 
 import (
 	"fmt"
-	"time"
 )
 
 type Kind int
@@ -34,7 +33,7 @@ func kindOf(v any) Kind {
 		return KindInt
 	case float32, float64:
 		return KindFloat
-	case time.Time:
+	case Time:
 		return KindTime
 	case nil:
 		return KindNil

libs/dyn/merge/override_test.go

@@ -83,16 +83,16 @@ func TestOverride_Primitive(t *testing.T) {
 		{
 			name:     "time (updated)",
 			state:    visitorState{updated: []string{"root"}},
-			left:     dyn.NewValue(time.UnixMilli(10000), []dyn.Location{leftLocation}),
+			left:     dyn.NewValue(dyn.FromTime(time.UnixMilli(10000)), []dyn.Location{leftLocation}),
-			right:    dyn.NewValue(time.UnixMilli(10001), []dyn.Location{rightLocation}),
+			right:    dyn.NewValue(dyn.FromTime(time.UnixMilli(10001)), []dyn.Location{rightLocation}),
-			expected: dyn.NewValue(time.UnixMilli(10001), []dyn.Location{rightLocation}),
+			expected: dyn.NewValue(dyn.FromTime(time.UnixMilli(10001)), []dyn.Location{rightLocation}),
 		},
 		{
 			name:     "time (not updated)",
 			state:    visitorState{},
-			left:     dyn.NewValue(time.UnixMilli(10000), []dyn.Location{leftLocation}),
+			left:     dyn.NewValue(dyn.FromTime(time.UnixMilli(10000)), []dyn.Location{leftLocation}),
-			right:    dyn.NewValue(time.UnixMilli(10000), []dyn.Location{rightLocation}),
+			right:    dyn.NewValue(dyn.FromTime(time.UnixMilli(10000)), []dyn.Location{rightLocation}),
-			expected: dyn.NewValue(time.UnixMilli(10000), []dyn.Location{leftLocation}),
+			expected: dyn.NewValue(dyn.FromTime(time.UnixMilli(10000)), []dyn.Location{leftLocation}),
 		},
 		{
 			name:     "different types (updated)",

libs/dyn/time.go

@@ -0,0 +1,62 @@
+package dyn
+
+import (
+	"fmt"
+	"time"
+)
+
+// Time represents a time-like primitive value.
+//
+// It represents a timestamp and includes the original string value
+// that was parsed to create the timestamp. This makes it possible
+// to coalesce a value that YAML interprets as a timestamp back into
+// a string without losing information.
+type Time struct {
+	t time.Time
+	s string
+}
+
+// NewTime creates a new Time from the given string.
+func NewTime(str string) (Time, error) {
+	// Try a couple of layouts
+	for _, layout := range []string{
+		"2006-1-2T15:4:5.999999999Z07:00", // RCF3339Nano with short date fields.
+		"2006-1-2t15:4:5.999999999Z07:00", // RFC3339Nano with short date fields and lower-case "t".
+		"2006-1-2 15:4:5.999999999",       // space separated with no time zone
+		"2006-1-2",                        // date only
+	} {
+		t, terr := time.Parse(layout, str)
+		if terr == nil {
+			return Time{t: t, s: str}, nil
+		}
+	}
+
+	return Time{}, fmt.Errorf("invalid time value: %q", str)
+}
+
+// MustTime creates a new Time from the given string.
+// It panics if the string cannot be parsed.
+func MustTime(str string) Time {
+	t, err := NewTime(str)
+	if err != nil {
+		panic(err)
+	}
+	return t
+}
+
+// FromTime creates a new Time from the given time.Time.
+// It uses the RFC3339Nano format for its string representation.
+// This guarantees that it can roundtrip into a string without losing information.
+func FromTime(t time.Time) Time {
+	return Time{t: t, s: t.Format(time.RFC3339Nano)}
+}
+
+// Time returns the time.Time value.
+func (t Time) Time() time.Time {
+	return t.t
+}
+
+// String returns the original string value that was parsed to create the timestamp.
+func (t Time) String() string {
+	return t.s
+}

libs/dyn/time_test.go

@@ -0,0 +1,41 @@
+package dyn_test
+
+import (
+	"testing"
+	"time"
+
+	"github.com/databricks/cli/libs/dyn"
+	assert "github.com/databricks/cli/libs/dyn/dynassert"
+)
+
+func TestTimeValid(t *testing.T) {
+	for _, tc := range []string{
+		"2024-08-29",
+		"2024-01-15T12:34:56.789012345Z",
+	} {
+		tm, err := dyn.NewTime(tc)
+		if assert.NoError(t, err) {
+			assert.NotEqual(t, time.Time{}, tm.Time())
+			assert.Equal(t, tc, tm.String())
+		}
+	}
+}
+
+func TestTimeInvalid(t *testing.T) {
+	tm, err := dyn.NewTime("invalid")
+	assert.Error(t, err)
+	assert.Equal(t, dyn.Time{}, tm)
+}
+
+func TestTimeFromTime(t *testing.T) {
+	tref := time.Now()
+	t1 := dyn.FromTime(tref)
+
+	// Verify that the underlying value is the same.
+	assert.Equal(t, tref, t1.Time())
+
+	// Verify that the string representation can be used to construct the same.
+	t2, err := dyn.NewTime(t1.String())
+	assert.NoError(t, err)
+	assert.True(t, t1.Time().Equal(t2.Time()))
+}

libs/dyn/value.go

@@ -127,7 +127,8 @@ func (v Value) AsAny() any {
 	case KindFloat:
 		return v.v
 	case KindTime:
-		return v.v
+		t := v.v.(Time)
+		return t.Time()
 	default:
 		// Panic because we only want to deal with known types.
 		panic(fmt.Sprintf("invalid kind: %d", v.k))

libs/dyn/value_underlying.go

@@ -2,7 +2,6 @@ package dyn
 
 import (
 	"fmt"
-	"time"
 )
 
 // AsMap returns the underlying mapping if this value is a map,
@@ -123,14 +122,14 @@ func (v Value) MustFloat() float64 {
 
 // AsTime returns the underlying time if this value is a time,
 // the zero value and false otherwise.
-func (v Value) AsTime() (time.Time, bool) {
+func (v Value) AsTime() (Time, bool) {
-	vv, ok := v.v.(time.Time)
+	vv, ok := v.v.(Time)
 	return vv, ok
 }
 
 // MustTime returns the underlying time if this value is a time,
 // panics otherwise.
-func (v Value) MustTime() time.Time {
+func (v Value) MustTime() Time {
 	vv, ok := v.AsTime()
 	if !ok || v.k != KindTime {
 		panic(fmt.Sprintf("expected kind %s, got %s", KindTime, v.k))

libs/dyn/value_underlying_test.go

@@ -143,7 +143,7 @@ func TestValueUnderlyingFloat(t *testing.T) {
 }
 
 func TestValueUnderlyingTime(t *testing.T) {
-	v := dyn.V(time.Now())
+	v := dyn.V(dyn.FromTime(time.Now()))
 
 	vv1, ok := v.AsTime()
 	assert.True(t, ok)

libs/dyn/yamlloader/loader.go

@@ -5,7 +5,6 @@ import (
 	"math"
 	"strconv"
 	"strings"
-	"time"
 
 	"github.com/databricks/cli/libs/dyn"
 	"gopkg.in/yaml.v3"
@@ -207,17 +206,9 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error
 	case "!!null":
 		return dyn.NewValue(nil, []dyn.Location{loc}), nil
 	case "!!timestamp":
-		// Try a couple of layouts
+		t, err := dyn.NewTime(node.Value)
-		for _, layout := range []string{
+		if err == nil {
-			"2006-1-2T15:4:5.999999999Z07:00", // RCF3339Nano with short date fields.
+			return dyn.NewValue(t, []dyn.Location{loc}), nil
-			"2006-1-2t15:4:5.999999999Z07:00", // RFC3339Nano with short date fields and lower-case "t".
-			"2006-1-2 15:4:5.999999999",       // space separated with no time zone
-			"2006-1-2",                        // date only
-		} {
-			t, terr := time.Parse(layout, node.Value)
-			if terr == nil {
-				return dyn.NewValue(t, []dyn.Location{loc}), nil
-			}
 		}
 		return dyn.InvalidValue, errorf(loc, "invalid timestamp value: %v", node.Value)
 	default:

libs/dyn/yamlsaver/saver.go

@@ -129,7 +129,7 @@ func (s *saver) toYamlNodeWithStyle(v dyn.Value, style yaml.Style) (*yaml.Node,
 	case dyn.KindFloat:
 		return &yaml.Node{Kind: yaml.ScalarNode, Value: fmt.Sprint(v.MustFloat()), Style: style}, nil
 	case dyn.KindTime:
-		return &yaml.Node{Kind: yaml.ScalarNode, Value: v.MustTime().UTC().String(), Style: style}, nil
+		return &yaml.Node{Kind: yaml.ScalarNode, Value: v.MustTime().String(), Style: style}, nil
 	default:
 		// Panic because we only want to deal with known types.
 		panic(fmt.Sprintf("invalid kind: %d", v.Kind()))

libs/dyn/yamlsaver/saver_test.go

@@ -2,10 +2,10 @@ package yamlsaver
 
 import (
 	"testing"
-	"time"
 
 	"github.com/databricks/cli/libs/dyn"
 	assert "github.com/databricks/cli/libs/dyn/dynassert"
+	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v3"
 )
 
@@ -45,11 +45,14 @@ func TestMarshalBoolValue(t *testing.T) {
 }
 
 func TestMarshalTimeValue(t *testing.T) {
+	tm, err := dyn.NewTime("1970-01-01")
+	require.NoError(t, err)
+
 	s := NewSaver()
-	var timeValue = dyn.V(time.Unix(0, 0))
+	var timeValue = dyn.V(tm)
 	v, err := s.toYamlNode(timeValue)
 	assert.NoError(t, err)
-	assert.Equal(t, "1970-01-01 00:00:00 +0000 UTC", v.Value)
+	assert.Equal(t, "1970-01-01", v.Value)
 	assert.Equal(t, yaml.ScalarNode, v.Kind)
 }