Skip to content

Commit

Permalink
Fix conversion of zero valued scalar pointers to a dynamic value (#1433)
Browse files Browse the repository at this point in the history
## Changes
This PR also fixes empty values variable overrides using the --var flag.
Now, using `--var="my_variable="` will set the value of `my_variable` to
the empty string instead of ignoring the flag altogether.

## Tests
The change using a unit test. Manually verified the `--var` flag works
now.
  • Loading branch information
shreyas-goenka authored May 21, 2024
1 parent 3f8036f commit c503264
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 14 deletions.
45 changes: 45 additions & 0 deletions libs/dyn/convert/end_to_end_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,49 @@ func TestAdditional(t *testing.T) {
SliceOfPointer: []*string{nil},
})
})

t.Run("pointer to a empty string", func(t *testing.T) {
s := ""
assertFromTypedToTypedEqual(t, &s)
})

t.Run("nil pointer", func(t *testing.T) {
var s *string
assertFromTypedToTypedEqual(t, s)
})

t.Run("pointer to struct with scalar values", func(t *testing.T) {
s := ""
type foo struct {
A string `json:"a"`
B int `json:"b"`
C bool `json:"c"`
D *string `json:"d"`
}
assertFromTypedToTypedEqual(t, &foo{
A: "a",
B: 1,
C: true,
D: &s,
})
assertFromTypedToTypedEqual(t, &foo{
A: "",
B: 0,
C: false,
D: nil,
})
})

t.Run("map with scalar values", func(t *testing.T) {
assertFromTypedToTypedEqual(t, map[string]string{
"a": "a",
"b": "b",
"c": "",
})
assertFromTypedToTypedEqual(t, map[string]int{
"a": 1,
"b": 0,
"c": 2,
})
})
}
42 changes: 28 additions & 14 deletions libs/dyn/convert/from_typed.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,22 @@ import (
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.
// If this flag is set, zero values for scalars (strings, bools, ints, floats)
// would resolve to corresponding zero values in the dynamic representation.
// Otherwise, zero values for scalars resolve to dyn.NilValue.
//
// Note: this is not needed for structs because dyn.NilValue is converted back
// to a zero value when using the convert.ToTyped function.
// This flag exists to reconcile the default values for scalars in a Go struct
// being zero values with zero values in a dynamic representation. In a Go struct,
// zero values are the same as the values not being set at all. This is not the case
// in the dynamic representation.
//
// Values in maps and slices should be set to zero values, and not nil in the
// dynamic representation.
includeZeroValues fromTypedOptions = 1 << iota
// If a scalar value in a typed Go struct is zero, in the dynamic representation
// we would set it to dyn.NilValue, i.e. equivalent to the value not being set at all.
//
// If a scalar value in a Go map, slice or pointer is set to zero, we will set it
// to the zero value in the dynamic representation, and not dyn.NilValue. This is
// equivalent to the value being intentionally set to zero.
includeZeroValuedScalars fromTypedOptions = 1 << iota
)

// FromTyped converts changes made in the typed structure w.r.t. the configuration value
Expand All @@ -41,6 +47,14 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value,
return dyn.NilValue, nil
}
srcv = srcv.Elem()

// If a pointer to a scalar type points to a zero value, we should include
// that zero value in the dynamic representation.
// This is because by default a pointer is nil in Go, and it not being nil
// indicates its value was intentionally set to zero.
if !slices.Contains(options, includeZeroValuedScalars) {
options = append(options, includeZeroValuedScalars)
}
}

switch srcv.Kind() {
Expand Down Expand Up @@ -129,7 +143,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
}

// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
nv, err := fromTyped(v.Interface(), refv, includeZeroValues)
nv, err := fromTyped(v.Interface(), refv, includeZeroValuedScalars)
if err != nil {
return dyn.InvalidValue, err
}
Expand Down Expand Up @@ -160,7 +174,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
v := src.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)
nv, err := fromTyped(v.Interface(), ref.Index(i), includeZeroValuedScalars)
if err != nil {
return dyn.InvalidValue, err
}
Expand All @@ -183,7 +197,7 @@ func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
case dyn.KindNil:
// 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) {
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
return dyn.NilValue, nil
}
return dyn.V(src.String()), nil
Expand All @@ -203,7 +217,7 @@ func fromTypedBool(src reflect.Value, ref dyn.Value, options ...fromTypedOptions
case dyn.KindNil:
// 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) {
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
return dyn.NilValue, nil
}
return dyn.V(src.Bool()), nil
Expand All @@ -228,7 +242,7 @@ func fromTypedInt(src reflect.Value, ref dyn.Value, options ...fromTypedOptions)
case dyn.KindNil:
// 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) {
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
return dyn.NilValue, nil
}
return dyn.V(src.Int()), nil
Expand All @@ -253,7 +267,7 @@ func fromTypedFloat(src reflect.Value, ref dyn.Value, options ...fromTypedOption
case dyn.KindNil:
// 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) {
if src.IsZero() && !slices.Contains(options, includeZeroValuedScalars) {
return dyn.NilValue, nil
}
return dyn.V(src.Float()), nil
Expand Down

0 comments on commit c503264

Please sign in to comment.