Skip to content

Commit 6beda44

Browse files
shreyas-goenkashreyas-goenka
authored
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
1 parent 359f5f4 commit 6beda44

File tree

3 files changed

+241
-26
lines changed

3 files changed

+241
-26
lines changed

‎libs/dyn/convert/end_to_end_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,15 @@ func TestAdditional(t *testing.T) {
3939
})
4040
})
4141

42+
t.Run("map with empty string value", func(t *testing.T) {
43+
s := ""
44+
assertFromTypedToTypedEqual(t, Tmp{
45+
MapToPointer: map[string]*string{
46+
"key": &s,
47+
},
48+
})
49+
})
50+
4251
t.Run("map with nil value", func(t *testing.T) {
4352
assertFromTypedToTypedEqual(t, Tmp{
4453
MapToPointer: map[string]*string{

‎libs/dyn/convert/from_typed.go‎

Lines changed: 47 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,35 @@ package convert
33
import (
44
"fmt"
55
"reflect"
6+
"slices"
67

78
"github.com/databricks/cli/libs/dyn"
89
)
910

11+
type fromTypedOptions int
12+
13+
const (
14+
// Use the zero value instead of setting zero values to nil. This is useful
15+
// for types where the zero values and nil are semantically different. That is
16+
// strings, bools, ints, floats.
17+
//
18+
// Note: this is not needed for structs because dyn.NilValue is converted back
19+
// to a zero value when using the convert.ToTyped function.
20+
//
21+
// Values in maps and slices should be set to zero values, and not nil in the
22+
// dynamic representation.
23+
includeZeroValues fromTypedOptions = 1 << iota
24+
)
25+
1026
// FromTyped converts changes made in the typed structure w.r.t. the configuration value
1127
// back to the configuration value, retaining existing location information where possible.
1228
func FromTyped(src any, ref dyn.Value) (dyn.Value, error) {
29+
return fromTyped(src, ref)
30+
}
31+
32+
// Private implementation of FromTyped that allows for additional options not exposed
33+
// in the public API.
34+
func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
1335
srcv := reflect.ValueOf(src)
1436

1537
// Dereference pointer if necessary
@@ -28,13 +50,13 @@ func FromTyped(src any, ref dyn.Value) (dyn.Value, error) {
2850
case reflect.Slice:
2951
return fromTypedSlice(srcv, ref)
3052
case reflect.String:
31-
return fromTypedString(srcv, ref)
53+
return fromTypedString(srcv, ref, options...)
3254
case reflect.Bool:
33-
return fromTypedBool(srcv, ref)
55+
return fromTypedBool(srcv, ref, options...)
3456
case reflect.Int, reflect.Int32, reflect.Int64:
35-
return fromTypedInt(srcv, ref)
57+
return fromTypedInt(srcv, ref, options...)
3658
case reflect.Float32, reflect.Float64:
37-
return fromTypedFloat(srcv, ref)
59+
return fromTypedFloat(srcv, ref, options...)
3860
}
3961

4062
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) {
5274
info := getStructInfo(src.Type())
5375
for k, v := range info.FieldValues(src) {
5476
// Convert the field taking into account the reference value (may be equal to config.NilValue).
55-
nv, err := FromTyped(v.Interface(), ref.Get(k))
77+
nv, err := fromTyped(v.Interface(), ref.Get(k))
5678
if err != nil {
5779
return dyn.Value{}, err
5880
}
@@ -89,8 +111,8 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
89111
k := iter.Key().String()
90112
v := iter.Value()
91113

92-
// Convert entry taking into account the reference value (may be equal to config.NilValue).
93-
nv, err := FromTyped(v.Interface(), ref.Get(k))
114+
// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
115+
nv, err := fromTyped(v.Interface(), ref.Get(k), includeZeroValues)
94116
if err != nil {
95117
return dyn.Value{}, err
96118
}
@@ -120,8 +142,8 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
120142
for i := 0; i < src.Len(); i++ {
121143
v := src.Index(i)
122144

123-
// Convert entry taking into account the reference value (may be equal to config.NilValue).
124-
nv, err := FromTyped(v.Interface(), ref.Index(i))
145+
// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
146+
nv, err := fromTyped(v.Interface(), ref.Index(i), includeZeroValues)
125147
if err != nil {
126148
return dyn.Value{}, err
127149
}
@@ -132,7 +154,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
132154
return dyn.NewValue(out, ref.Location()), nil
133155
}
134156

135-
func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
157+
func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
136158
switch ref.Kind() {
137159
case dyn.KindString:
138160
value := src.String()
@@ -142,9 +164,9 @@ func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
142164

143165
return dyn.V(value), nil
144166
case dyn.KindNil:
145-
// This field is not set in the reference, so we only include it if it has a non-zero value.
146-
// Otherwise, we would always include all zero valued fields.
147-
if src.IsZero() {
167+
// This field is not set in the reference. We set it to nil if it's zero
168+
// valued in the typed representation and the includeZeroValues option is not set.
169+
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
148170
return dyn.NilValue, nil
149171
}
150172
return dyn.V(src.String()), nil
@@ -153,7 +175,7 @@ func fromTypedString(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
153175
return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind())
154176
}
155177

156-
func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
178+
func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
157179
switch ref.Kind() {
158180
case dyn.KindBool:
159181
value := src.Bool()
@@ -162,9 +184,9 @@ func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
162184
}
163185
return dyn.V(value), nil
164186
case dyn.KindNil:
165-
// This field is not set in the reference, so we only include it if it has a non-zero value.
166-
// Otherwise, we would always include all zero valued fields.
167-
if src.IsZero() {
187+
// This field is not set in the reference. We set it to nil if it's zero
188+
// valued in the typed representation and the includeZeroValues option is not set.
189+
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
168190
return dyn.NilValue, nil
169191
}
170192
return dyn.V(src.Bool()), nil
@@ -173,7 +195,7 @@ func fromTypedBool(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
173195
return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind())
174196
}
175197

176-
func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
198+
func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
177199
switch ref.Kind() {
178200
case dyn.KindInt:
179201
value := src.Int()
@@ -182,9 +204,9 @@ func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
182204
}
183205
return dyn.V(value), nil
184206
case dyn.KindNil:
185-
// This field is not set in the reference, so we only include it if it has a non-zero value.
186-
// Otherwise, we would always include all zero valued fields.
187-
if src.IsZero() {
207+
// This field is not set in the reference. We set it to nil if it's zero
208+
// valued in the typed representation and the includeZeroValues option is not set.
209+
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
188210
return dyn.NilValue, nil
189211
}
190212
return dyn.V(src.Int()), nil
@@ -193,7 +215,7 @@ func fromTypedInt(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
193215
return dyn.Value{}, fmt.Errorf("unhandled type: %s", ref.Kind())
194216
}
195217

196-
func fromTypedFloat(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
218+
func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
197219
switch ref.Kind() {
198220
case dyn.KindFloat:
199221
value := src.Float()
@@ -202,9 +224,9 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
202224
}
203225
return dyn.V(value), nil
204226
case dyn.KindNil:
205-
// This field is not set in the reference, so we only include it if it has a non-zero value.
206-
// Otherwise, we would always include all zero valued fields.
207-
if src.IsZero() {
227+
// This field is not set in the reference. We set it to nil if it's zero
228+
// valued in the typed representation and the includeZeroValues option is not set.
229+
if src.IsZero() && !slices.Contains(options, includeZeroValues) {
208230
return dyn.NilValue, nil
209231
}
210232
return dyn.V(src.Float()), nil

‎libs/dyn/convert/from_typed_test.go‎

Lines changed: 185 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,190 @@ func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) {
6868
assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar"))
6969
}
7070

71+
func TestFromTypedStringMapWithZeroValue(t *testing.T) {
72+
ref := dyn.NilValue
73+
src := map[string]string{
74+
"foo": "",
75+
"bar": "fuzz",
76+
}
77+
78+
nv, err := FromTyped(src, ref)
79+
require.NoError(t, err)
80+
assert.Equal(t, dyn.V(map[string]dyn.Value{
81+
"foo": dyn.V(""),
82+
"bar": dyn.V("fuzz"),
83+
}), nv)
84+
}
85+
86+
func TestFromTypedStringSliceWithZeroValue(t *testing.T) {
87+
ref := dyn.NilValue
88+
src := []string{"a", "", "c"}
89+
90+
nv, err := FromTyped(src, ref)
91+
require.NoError(t, err)
92+
assert.Equal(t, dyn.V([]dyn.Value{
93+
dyn.V("a"), dyn.V(""), dyn.V("c"),
94+
}), nv)
95+
}
96+
97+
func TestFromTypedStringStructWithZeroValue(t *testing.T) {
98+
type Tmp struct {
99+
Foo string `json:"foo"`
100+
Bar string `json:"bar"`
101+
}
102+
103+
ref := dyn.NilValue
104+
src := Tmp{
105+
Foo: "foo",
106+
Bar: "",
107+
}
108+
109+
// Note, the zero value is not included in the output.
110+
nv, err := FromTyped(src, ref)
111+
require.NoError(t, err)
112+
assert.Equal(t, dyn.V(map[string]dyn.Value{
113+
"foo": dyn.V("foo"),
114+
}), nv)
115+
}
116+
117+
func TestFromTypedBoolMapWithZeroValue(t *testing.T) {
118+
ref := dyn.NilValue
119+
src := map[string]bool{
120+
"foo": false,
121+
"bar": true,
122+
}
123+
124+
nv, err := FromTyped(src, ref)
125+
require.NoError(t, err)
126+
assert.Equal(t, dyn.V(map[string]dyn.Value{
127+
"foo": dyn.V(false),
128+
"bar": dyn.V(true),
129+
}), nv)
130+
}
131+
132+
func TestFromTypedBoolSliceWithZeroValue(t *testing.T) {
133+
ref := dyn.NilValue
134+
src := []bool{true, false, true}
135+
136+
nv, err := FromTyped(src, ref)
137+
require.NoError(t, err)
138+
assert.Equal(t, dyn.V([]dyn.Value{
139+
dyn.V(true), dyn.V(false), dyn.V(true),
140+
}), nv)
141+
}
142+
143+
func TestFromTypedBoolStructWithZeroValue(t *testing.T) {
144+
type Tmp struct {
145+
Foo bool `json:"foo"`
146+
Bar bool `json:"bar"`
147+
}
148+
149+
ref := dyn.NilValue
150+
src := Tmp{
151+
Foo: true,
152+
Bar: false,
153+
}
154+
155+
// Note, the zero value is not included in the output.
156+
nv, err := FromTyped(src, ref)
157+
require.NoError(t, err)
158+
assert.Equal(t, dyn.V(map[string]dyn.Value{
159+
"foo": dyn.V(true),
160+
}), nv)
161+
}
162+
163+
func TestFromTypedIntMapWithZeroValue(t *testing.T) {
164+
ref := dyn.NilValue
165+
src := map[string]int{
166+
"foo": 0,
167+
"bar": 1,
168+
}
169+
170+
nv, err := FromTyped(src, ref)
171+
require.NoError(t, err)
172+
assert.Equal(t, dyn.V(map[string]dyn.Value{
173+
"foo": dyn.V(int64(0)),
174+
"bar": dyn.V(int64(1)),
175+
}), nv)
176+
}
177+
178+
func TestFromTypedIntSliceWithZeroValue(t *testing.T) {
179+
ref := dyn.NilValue
180+
src := []int{1, 0, 2}
181+
182+
nv, err := FromTyped(src, ref)
183+
require.NoError(t, err)
184+
assert.Equal(t, dyn.V([]dyn.Value{
185+
dyn.V(int64(1)), dyn.V(int64(0)), dyn.V(int64(2)),
186+
}), nv)
187+
}
188+
189+
func TestFromTypedIntStructWithZeroValue(t *testing.T) {
190+
type Tmp struct {
191+
Foo int `json:"foo"`
192+
Bar int `json:"bar"`
193+
}
194+
195+
ref := dyn.NilValue
196+
src := Tmp{
197+
Foo: 1,
198+
Bar: 0,
199+
}
200+
201+
// Note, the zero value is not included in the output.
202+
nv, err := FromTyped(src, ref)
203+
require.NoError(t, err)
204+
assert.Equal(t, dyn.V(map[string]dyn.Value{
205+
"foo": dyn.V(int64(1)),
206+
}), nv)
207+
}
208+
209+
func TestFromTypedFloatMapWithZeroValue(t *testing.T) {
210+
ref := dyn.NilValue
211+
src := map[string]float64{
212+
"foo": 0.0,
213+
"bar": 1.0,
214+
}
215+
216+
nv, err := FromTyped(src, ref)
217+
require.NoError(t, err)
218+
assert.Equal(t, dyn.V(map[string]dyn.Value{
219+
"foo": dyn.V(0.0),
220+
"bar": dyn.V(1.0),
221+
}), nv)
222+
}
223+
224+
func TestFromTypedFloatSliceWithZeroValue(t *testing.T) {
225+
ref := dyn.NilValue
226+
src := []float64{1.0, 0.0, 2.0}
227+
228+
nv, err := FromTyped(src, ref)
229+
require.NoError(t, err)
230+
assert.Equal(t, dyn.V([]dyn.Value{
231+
dyn.V(1.0), dyn.V(0.0), dyn.V(2.0),
232+
}), nv)
233+
}
234+
235+
func TestFromTypedFloatStructWithZeroValue(t *testing.T) {
236+
type Tmp struct {
237+
Foo float64 `json:"foo"`
238+
Bar float64 `json:"bar"`
239+
}
240+
241+
ref := dyn.NilValue
242+
src := Tmp{
243+
Foo: 1.0,
244+
Bar: 0.0,
245+
}
246+
247+
// Note, the zero value is not included in the output.
248+
nv, err := FromTyped(src, ref)
249+
require.NoError(t, err)
250+
assert.Equal(t, dyn.V(map[string]dyn.Value{
251+
"foo": dyn.V(1.0),
252+
}), nv)
253+
}
254+
71255
func TestFromTypedMapNil(t *testing.T) {
72256
var src map[string]string = nil
73257

@@ -139,7 +323,7 @@ func TestFromTypedMapFieldWithZeroValue(t *testing.T) {
139323
nv, err := FromTyped(src, ref)
140324
require.NoError(t, err)
141325
assert.Equal(t, dyn.V(map[string]dyn.Value{
142-
"foo": dyn.NilValue,
326+
"foo": dyn.V(""),
143327
}), nv)
144328
}
145329

0 commit comments

Comments
 (0)