From 6beda4405e732ff4d97bc2874e161eceff07890e Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 31 Jan 2024 19:55:13 +0530 Subject: [PATCH] Fix dynamic representation of zero values in maps and slices (#1154) ## Changes In the dynamic configuration, the nil value (dyn.NilValue) denotes a value that should not be serialized, ie a value being nil is the same as it not existing in the first place. This is not true for zero values in maps and slices. This PR fixes the conversion from typed values to dyn.Value, to treat zero values in maps and slices as zero and not nil. ## Tests Unit tests --- libs/dyn/convert/end_to_end_test.go | 9 ++ libs/dyn/convert/from_typed.go | 72 +++++++---- libs/dyn/convert/from_typed_test.go | 186 +++++++++++++++++++++++++++- 3 files changed, 241 insertions(+), 26 deletions(-) diff --git a/libs/dyn/convert/end_to_end_test.go b/libs/dyn/convert/end_to_end_test.go index fbb84336..7c048136 100644 --- a/libs/dyn/convert/end_to_end_test.go +++ b/libs/dyn/convert/end_to_end_test.go @@ -39,6 +39,15 @@ func TestAdditional(t *testing.T) { }) }) + t.Run("map with empty string value", func(t *testing.T) { + s := "" + assertFromTypedToTypedEqual(t, Tmp{ + MapToPointer: map[string]*string{ + "key": &s, + }, + }) + }) + t.Run("map with nil value", func(t *testing.T) { assertFromTypedToTypedEqual(t, Tmp{ MapToPointer: map[string]*string{ diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go index 0659d1cd..75f1c721 100644 --- a/libs/dyn/convert/from_typed.go +++ b/libs/dyn/convert/from_typed.go @@ -3,13 +3,35 @@ package convert import ( "fmt" "reflect" + "slices" "github.com/databricks/cli/libs/dyn" ) +type fromTypedOptions int + +const ( + // Use the zero value instead of setting zero values to nil. This is useful + // for types where the zero values and nil are semantically different. That is + // strings, bools, ints, floats. + // + // Note: this is not needed for structs because dyn.NilValue is converted back + // to a zero value when using the convert.ToTyped function. + // + // Values in maps and slices should be set to zero values, and not nil in the + // dynamic representation. + includeZeroValues fromTypedOptions = 1 << iota +) + // FromTyped converts changes made in the typed structure w.r.t. the configuration value // back to the configuration value, retaining existing location information where possible. func FromTyped(src any, ref dyn.Value) (dyn.Value, error) { + return fromTyped(src, ref) +} + +// Private implementation of FromTyped that allows for additional options not exposed +// in the public API. +func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { srcv := reflect.ValueOf(src) // Dereference pointer if necessary @@ -28,13 +50,13 @@ func FromTyped(src any, ref dyn.Value) (dyn.Value, error) { case reflect.Slice: return fromTypedSlice(srcv, ref) case reflect.String: - return fromTypedString(srcv, ref) + return fromTypedString(srcv, ref, options...) case reflect.Bool: - return fromTypedBool(srcv, ref) + return fromTypedBool(srcv, ref, options...) case reflect.Int, reflect.Int32, reflect.Int64: - return fromTypedInt(srcv, ref) + return fromTypedInt(srcv, ref, options...) case reflect.Float32, reflect.Float64: - return fromTypedFloat(srcv, ref) + return fromTypedFloat(srcv, ref, options...) } return dyn.NilValue, fmt.Errorf("unsupported type: %s", srcv.Kind()) @@ -52,7 +74,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) { info := getStructInfo(src.Type()) for k, v := range info.FieldValues(src) { // Convert the field taking into account the reference value (may be equal to config.NilValue). - nv, err := FromTyped(v.Interface(), ref.Get(k)) + nv, err := fromTyped(v.Interface(), ref.Get(k)) if err != nil { return dyn.Value{}, err } @@ -89,8 +111,8 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) { k := iter.Key().String() v := iter.Value() - // Convert entry taking into account the reference value (may be equal to config.NilValue). - nv, err := FromTyped(v.Interface(), ref.Get(k)) + // Convert entry taking into account the reference value (may be equal to dyn.NilValue). + nv, err := fromTyped(v.Interface(), ref.Get(k), includeZeroValues) if err != nil { return dyn.Value{}, err } @@ -120,8 +142,8 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { for i := 0; i < src.Len(); i++ { v := src.Index(i) - // Convert entry taking into account the reference value (may be equal to config.NilValue). - nv, err := FromTyped(v.Interface(), ref.Index(i)) + // Convert entry taking into account the reference value (may be equal to dyn.NilValue). + nv, err := fromTyped(v.Interface(), ref.Index(i), includeZeroValues) if err != nil { return dyn.Value{}, err } @@ -132,7 +154,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { return dyn.NewValue(out, ref.Location()), nil } -func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) { +func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { switch ref.Kind() { case dyn.KindString: value := src.String() @@ -142,9 +164,9 @@ func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) { return dyn.V(value), nil case dyn.KindNil: - // This field is not set in the reference, so we only include it if it has a non-zero value. - // Otherwise, we would always include all zero valued fields. - if src.IsZero() { + // This field is not set in the reference. We set it to nil if it's zero + // valued in the typed representation and the includeZeroValues option is not set. + if src.IsZero() && !slices.Contains(options, includeZeroValues) { return dyn.NilValue, nil } return dyn.V(src.String()), nil @@ -153,7 +175,7 @@ func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) { return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) } -func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) { +func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { switch ref.Kind() { case dyn.KindBool: value := src.Bool() @@ -162,9 +184,9 @@ func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) { } return dyn.V(value), nil case dyn.KindNil: - // This field is not set in the reference, so we only include it if it has a non-zero value. - // Otherwise, we would always include all zero valued fields. - if src.IsZero() { + // This field is not set in the reference. We set it to nil if it's zero + // valued in the typed representation and the includeZeroValues option is not set. + if src.IsZero() && !slices.Contains(options, includeZeroValues) { return dyn.NilValue, nil } return dyn.V(src.Bool()), nil @@ -173,7 +195,7 @@ func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) { return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) } -func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) { +func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { switch ref.Kind() { case dyn.KindInt: value := src.Int() @@ -182,9 +204,9 @@ func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) { } return dyn.V(value), nil case dyn.KindNil: - // This field is not set in the reference, so we only include it if it has a non-zero value. - // Otherwise, we would always include all zero valued fields. - if src.IsZero() { + // This field is not set in the reference. We set it to nil if it's zero + // valued in the typed representation and the includeZeroValues option is not set. + if src.IsZero() && !slices.Contains(options, includeZeroValues) { return dyn.NilValue, nil } return dyn.V(src.Int()), nil @@ -193,7 +215,7 @@ func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) { return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind()) } -func fromTypedFloat(src reflect.Value, ref dyn.Value) (dyn.Value, error) { +func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) { switch ref.Kind() { case dyn.KindFloat: value := src.Float() @@ -202,9 +224,9 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value) (dyn.Value, error) { } return dyn.V(value), nil case dyn.KindNil: - // This field is not set in the reference, so we only include it if it has a non-zero value. - // Otherwise, we would always include all zero valued fields. - if src.IsZero() { + // This field is not set in the reference. We set it to nil if it's zero + // valued in the typed representation and the includeZeroValues option is not set. + if src.IsZero() && !slices.Contains(options, includeZeroValues) { return dyn.NilValue, nil } return dyn.V(src.Float()), nil diff --git a/libs/dyn/convert/from_typed_test.go b/libs/dyn/convert/from_typed_test.go index 0e9b9c7c..d7fa60bb 100644 --- a/libs/dyn/convert/from_typed_test.go +++ b/libs/dyn/convert/from_typed_test.go @@ -68,6 +68,190 @@ func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) { assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar")) } +func TestFromTypedStringMapWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := map[string]string{ + "foo": "", + "bar": "fuzz", + } + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(""), + "bar": dyn.V("fuzz"), + }), nv) +} + +func TestFromTypedStringSliceWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := []string{"a", "", "c"} + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V([]dyn.Value{ + dyn.V("a"), dyn.V(""), dyn.V("c"), + }), nv) +} + +func TestFromTypedStringStructWithZeroValue(t *testing.T) { + type Tmp struct { + Foo string `json:"foo"` + Bar string `json:"bar"` + } + + ref := dyn.NilValue + src := Tmp{ + Foo: "foo", + Bar: "", + } + + // Note, the zero value is not included in the output. + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V("foo"), + }), nv) +} + +func TestFromTypedBoolMapWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := map[string]bool{ + "foo": false, + "bar": true, + } + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(false), + "bar": dyn.V(true), + }), nv) +} + +func TestFromTypedBoolSliceWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := []bool{true, false, true} + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V([]dyn.Value{ + dyn.V(true), dyn.V(false), dyn.V(true), + }), nv) +} + +func TestFromTypedBoolStructWithZeroValue(t *testing.T) { + type Tmp struct { + Foo bool `json:"foo"` + Bar bool `json:"bar"` + } + + ref := dyn.NilValue + src := Tmp{ + Foo: true, + Bar: false, + } + + // Note, the zero value is not included in the output. + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(true), + }), nv) +} + +func TestFromTypedIntMapWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := map[string]int{ + "foo": 0, + "bar": 1, + } + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(int64(0)), + "bar": dyn.V(int64(1)), + }), nv) +} + +func TestFromTypedIntSliceWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := []int{1, 0, 2} + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V([]dyn.Value{ + dyn.V(int64(1)), dyn.V(int64(0)), dyn.V(int64(2)), + }), nv) +} + +func TestFromTypedIntStructWithZeroValue(t *testing.T) { + type Tmp struct { + Foo int `json:"foo"` + Bar int `json:"bar"` + } + + ref := dyn.NilValue + src := Tmp{ + Foo: 1, + Bar: 0, + } + + // Note, the zero value is not included in the output. + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(int64(1)), + }), nv) +} + +func TestFromTypedFloatMapWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := map[string]float64{ + "foo": 0.0, + "bar": 1.0, + } + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(0.0), + "bar": dyn.V(1.0), + }), nv) +} + +func TestFromTypedFloatSliceWithZeroValue(t *testing.T) { + ref := dyn.NilValue + src := []float64{1.0, 0.0, 2.0} + + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V([]dyn.Value{ + dyn.V(1.0), dyn.V(0.0), dyn.V(2.0), + }), nv) +} + +func TestFromTypedFloatStructWithZeroValue(t *testing.T) { + type Tmp struct { + Foo float64 `json:"foo"` + Bar float64 `json:"bar"` + } + + ref := dyn.NilValue + src := Tmp{ + Foo: 1.0, + Bar: 0.0, + } + + // Note, the zero value is not included in the output. + nv, err := FromTyped(src, ref) + require.NoError(t, err) + assert.Equal(t, dyn.V(map[string]dyn.Value{ + "foo": dyn.V(1.0), + }), nv) +} + func TestFromTypedMapNil(t *testing.T) { var src map[string]string = nil @@ -139,7 +323,7 @@ func TestFromTypedMapFieldWithZeroValue(t *testing.T) { nv, err := FromTyped(src, ref) require.NoError(t, err) assert.Equal(t, dyn.V(map[string]dyn.Value{ - "foo": dyn.NilValue, + "foo": dyn.V(""), }), nv) }