From b2c03ea54cef1504c9af1b7b0d9dfabba7bff68e Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 19 Jun 2024 17:24:57 +0200 Subject: [PATCH] Use `dyn.InvalidValue` to indicate absence (#1507) ## Changes Previously, the functions `Get` and `Index` returned `dyn.NilValue` to indicate that a map key or sequence index wasn't found. This is a valid value, so we need to differentiate between actual absence and a real `dyn.NilValue`. We do this with the zero value of a `dyn.Value` (also captured in the constant `dyn.InvalidValue`). ## Tests * Unit tests. * Renamed `Get` and `Index` to find and update all call sites. --- bundle/config/mutator/environments_compat.go | 8 ++-- bundle/config/mutator/merge_job_clusters.go | 2 +- bundle/config/mutator/merge_job_tasks.go | 2 +- .../config/mutator/merge_pipeline_clusters.go | 2 +- bundle/config/root.go | 30 +++++++------ libs/dyn/convert/from_typed.go | 8 +++- libs/dyn/value.go | 8 ++-- libs/dyn/value_underlying_test.go | 42 +++++++++---------- libs/dyn/walk.go | 6 +-- libs/dyn/walk_test.go | 6 +-- 10 files changed, 62 insertions(+), 52 deletions(-) diff --git a/bundle/config/mutator/environments_compat.go b/bundle/config/mutator/environments_compat.go index cbedcaef..053fd2e3 100644 --- a/bundle/config/mutator/environments_compat.go +++ b/bundle/config/mutator/environments_compat.go @@ -32,18 +32,18 @@ func (m *environmentsToTargets) Apply(ctx context.Context, b *bundle.Bundle) dia targets := v.Get("targets") // Return an error if both "environments" and "targets" are set. - if environments != dyn.NilValue && targets != dyn.NilValue { - return dyn.NilValue, fmt.Errorf( + if environments != dyn.InvalidValue && targets != dyn.InvalidValue { + return dyn.InvalidValue, fmt.Errorf( "both 'environments' and 'targets' are specified; only 'targets' should be used: %s", environments.Location().String(), ) } // Rewrite "environments" to "targets". - if environments != dyn.NilValue && targets == dyn.NilValue { + if environments != dyn.InvalidValue && targets == dyn.InvalidValue { nv, err := dyn.Set(v, "targets", environments) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } // Drop the "environments" key. return dyn.Walk(nv, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { diff --git a/bundle/config/mutator/merge_job_clusters.go b/bundle/config/mutator/merge_job_clusters.go index 20f4efe8..ec615460 100644 --- a/bundle/config/mutator/merge_job_clusters.go +++ b/bundle/config/mutator/merge_job_clusters.go @@ -21,7 +21,7 @@ func (m *mergeJobClusters) Name() string { func (m *mergeJobClusters) jobClusterKey(v dyn.Value) string { switch v.Kind() { - case dyn.KindNil: + case dyn.KindInvalid, dyn.KindNil: return "" case dyn.KindString: return v.MustString() diff --git a/bundle/config/mutator/merge_job_tasks.go b/bundle/config/mutator/merge_job_tasks.go index 68c05383..f9a9bf71 100644 --- a/bundle/config/mutator/merge_job_tasks.go +++ b/bundle/config/mutator/merge_job_tasks.go @@ -21,7 +21,7 @@ func (m *mergeJobTasks) Name() string { func (m *mergeJobTasks) taskKeyString(v dyn.Value) string { switch v.Kind() { - case dyn.KindNil: + case dyn.KindInvalid, dyn.KindNil: return "" case dyn.KindString: return v.MustString() diff --git a/bundle/config/mutator/merge_pipeline_clusters.go b/bundle/config/mutator/merge_pipeline_clusters.go index 0b1cf898..c75f6532 100644 --- a/bundle/config/mutator/merge_pipeline_clusters.go +++ b/bundle/config/mutator/merge_pipeline_clusters.go @@ -22,7 +22,7 @@ func (m *mergePipelineClusters) Name() string { func (m *mergePipelineClusters) clusterLabel(v dyn.Value) string { switch v.Kind() { - case dyn.KindNil: + case dyn.KindInvalid, dyn.KindNil: // Note: the cluster label is optional and defaults to 'default'. // We therefore ALSO merge all clusters without a label. return "default" diff --git a/bundle/config/root.go b/bundle/config/root.go index 2bc905bd..1d56ba80 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -337,7 +337,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge `run_as`. This field must be overwritten if set, not merged. - if v := target.Get("run_as"); v != dyn.NilValue { + if v := target.Get("run_as"); v != dyn.InvalidValue { root, err = dyn.Set(root, "run_as", v) if err != nil { return err @@ -345,7 +345,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Below, we're setting fields on the bundle key, so make sure it exists. - if root.Get("bundle") == dyn.NilValue { + if root.Get("bundle") == dyn.InvalidValue { root, err = dyn.Set(root, "bundle", dyn.NewValue(map[string]dyn.Value{}, dyn.Location{})) if err != nil { return err @@ -353,7 +353,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge `mode`. This field must be overwritten if set, not merged. - if v := target.Get("mode"); v != dyn.NilValue { + if v := target.Get("mode"); v != dyn.InvalidValue { root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v) if err != nil { return err @@ -361,7 +361,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge `compute_id`. This field must be overwritten if set, not merged. - if v := target.Get("compute_id"); v != dyn.NilValue { + if v := target.Get("compute_id"); v != dyn.InvalidValue { root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("compute_id")), v) if err != nil { return err @@ -369,7 +369,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Merge `git`. - if v := target.Get("git"); v != dyn.NilValue { + if v := target.Get("git"); v != dyn.InvalidValue { ref, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git"))) if err != nil { ref = dyn.NewValue(map[string]dyn.Value{}, dyn.Location{}) @@ -382,7 +382,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // If the branch was overridden, we need to clear the inferred flag. - if branch := v.Get("branch"); branch != dyn.NilValue { + if branch := v.Get("branch"); branch != dyn.InvalidValue { out, err = dyn.SetByPath(out, dyn.NewPath(dyn.Key("inferred")), dyn.NewValue(false, dyn.Location{})) if err != nil { return err @@ -410,7 +410,7 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { // For each target, rewrite the variables block. return dyn.Map(v, "targets", dyn.Foreach(func(_ dyn.Path, target dyn.Value) (dyn.Value, error) { // Confirm it has a variables block. - if target.Get("variables") == dyn.NilValue { + if target.Get("variables") == dyn.InvalidValue { return target, nil } @@ -440,15 +440,19 @@ func validateVariableOverrides(root, target dyn.Value) (err error) { var tv map[string]variable.Variable // Collect variables from the root. - err = convert.ToTyped(&rv, root.Get("variables")) - if err != nil { - return fmt.Errorf("unable to collect variables from root: %w", err) + if v := root.Get("variables"); v != dyn.InvalidValue { + err = convert.ToTyped(&rv, v) + if err != nil { + return fmt.Errorf("unable to collect variables from root: %w", err) + } } // Collect variables from the target. - err = convert.ToTyped(&tv, target.Get("variables")) - if err != nil { - return fmt.Errorf("unable to collect variables from target: %w", err) + if v := target.Get("variables"); v != dyn.InvalidValue { + err = convert.ToTyped(&tv, v) + if err != nil { + return fmt.Errorf("unable to collect variables from target: %w", err) + } } // Check that all variables in the target exist in the root. diff --git a/libs/dyn/convert/from_typed.go b/libs/dyn/convert/from_typed.go index ae491d8a..b57d52be 100644 --- a/libs/dyn/convert/from_typed.go +++ b/libs/dyn/convert/from_typed.go @@ -172,9 +172,15 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) { out := make([]dyn.Value, src.Len()) for i := 0; i < src.Len(); i++ { v := src.Index(i) + refv := ref.Index(i) + + // Use nil reference if there is no reference for this index. + if refv == dyn.InvalidValue { + refv = dyn.NilValue + } // Convert entry taking into account the reference value (may be equal to dyn.NilValue). - nv, err := fromTyped(v.Interface(), ref.Index(i), includeZeroValuedScalars) + nv, err := fromTyped(v.Interface(), refv, includeZeroValuedScalars) if err != nil { return dyn.InvalidValue, err } diff --git a/libs/dyn/value.go b/libs/dyn/value.go index 2e8f1b9a..3d62ea1f 100644 --- a/libs/dyn/value.go +++ b/libs/dyn/value.go @@ -110,12 +110,12 @@ func (v Value) AsAny() any { func (v Value) Get(key string) Value { m, ok := v.AsMap() if !ok { - return NilValue + return InvalidValue } vv, ok := m.GetByString(key) if !ok { - return NilValue + return InvalidValue } return vv @@ -124,11 +124,11 @@ func (v Value) Get(key string) Value { func (v Value) Index(i int) Value { s, ok := v.v.([]Value) if !ok { - return NilValue + return InvalidValue } if i < 0 || i >= len(s) { - return NilValue + return InvalidValue } return s[i] diff --git a/libs/dyn/value_underlying_test.go b/libs/dyn/value_underlying_test.go index 9878cfaf..83cffb77 100644 --- a/libs/dyn/value_underlying_test.go +++ b/libs/dyn/value_underlying_test.go @@ -18,15 +18,15 @@ func TestValueUnderlyingMap(t *testing.T) { vv1, ok := v.AsMap() assert.True(t, ok) - _, ok = dyn.NilValue.AsMap() + _, ok = dyn.InvalidValue.AsMap() assert.False(t, ok) vv2 := v.MustMap() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind map, got nil", func() { - dyn.NilValue.MustMap() + assert.PanicsWithValue(t, "expected kind map, got invalid", func() { + dyn.InvalidValue.MustMap() }) } @@ -40,15 +40,15 @@ func TestValueUnderlyingSequence(t *testing.T) { vv1, ok := v.AsSequence() assert.True(t, ok) - _, ok = dyn.NilValue.AsSequence() + _, ok = dyn.InvalidValue.AsSequence() assert.False(t, ok) vv2 := v.MustSequence() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind sequence, got nil", func() { - dyn.NilValue.MustSequence() + assert.PanicsWithValue(t, "expected kind sequence, got invalid", func() { + dyn.InvalidValue.MustSequence() }) } @@ -58,15 +58,15 @@ func TestValueUnderlyingString(t *testing.T) { vv1, ok := v.AsString() assert.True(t, ok) - _, ok = dyn.NilValue.AsString() + _, ok = dyn.InvalidValue.AsString() assert.False(t, ok) vv2 := v.MustString() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind string, got nil", func() { - dyn.NilValue.MustString() + assert.PanicsWithValue(t, "expected kind string, got invalid", func() { + dyn.InvalidValue.MustString() }) } @@ -76,15 +76,15 @@ func TestValueUnderlyingBool(t *testing.T) { vv1, ok := v.AsBool() assert.True(t, ok) - _, ok = dyn.NilValue.AsBool() + _, ok = dyn.InvalidValue.AsBool() assert.False(t, ok) vv2 := v.MustBool() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind bool, got nil", func() { - dyn.NilValue.MustBool() + assert.PanicsWithValue(t, "expected kind bool, got invalid", func() { + dyn.InvalidValue.MustBool() }) } @@ -94,15 +94,15 @@ func TestValueUnderlyingInt(t *testing.T) { vv1, ok := v.AsInt() assert.True(t, ok) - _, ok = dyn.NilValue.AsInt() + _, ok = dyn.InvalidValue.AsInt() assert.False(t, ok) vv2 := v.MustInt() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind int, got nil", func() { - dyn.NilValue.MustInt() + assert.PanicsWithValue(t, "expected kind int, got invalid", func() { + dyn.InvalidValue.MustInt() }) // Test int32 type specifically. @@ -124,15 +124,15 @@ func TestValueUnderlyingFloat(t *testing.T) { vv1, ok := v.AsFloat() assert.True(t, ok) - _, ok = dyn.NilValue.AsFloat() + _, ok = dyn.InvalidValue.AsFloat() assert.False(t, ok) vv2 := v.MustFloat() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind float, got nil", func() { - dyn.NilValue.MustFloat() + assert.PanicsWithValue(t, "expected kind float, got invalid", func() { + dyn.InvalidValue.MustFloat() }) // Test float64 type specifically. @@ -148,14 +148,14 @@ func TestValueUnderlyingTime(t *testing.T) { vv1, ok := v.AsTime() assert.True(t, ok) - _, ok = dyn.NilValue.AsTime() + _, ok = dyn.InvalidValue.AsTime() assert.False(t, ok) vv2 := v.MustTime() assert.Equal(t, vv1, vv2) // Test panic. - assert.PanicsWithValue(t, "expected kind time, got nil", func() { - dyn.NilValue.MustTime() + assert.PanicsWithValue(t, "expected kind time, got invalid", func() { + dyn.InvalidValue.MustTime() }) } diff --git a/libs/dyn/walk.go b/libs/dyn/walk.go index 97b99b06..c51a11e2 100644 --- a/libs/dyn/walk.go +++ b/libs/dyn/walk.go @@ -28,7 +28,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro if err == ErrSkip { return v, nil } - return NilValue, err + return InvalidValue, err } switch v.Kind() { @@ -43,7 +43,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro continue } if err != nil { - return NilValue, err + return InvalidValue, err } out.Set(pk, nv) } @@ -57,7 +57,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro continue } if err != nil { - return NilValue, err + return InvalidValue, err } out = append(out, nv) } diff --git a/libs/dyn/walk_test.go b/libs/dyn/walk_test.go index d62b9a4d..f7222b0a 100644 --- a/libs/dyn/walk_test.go +++ b/libs/dyn/walk_test.go @@ -39,7 +39,7 @@ func (w *walkCallTracker) returnSkip(path string) { } func (w *walkCallTracker) returnDrop(path string) { - w.on(path, func(v Value) Value { return NilValue }, ErrDrop) + w.on(path, func(v Value) Value { return InvalidValue }, ErrDrop) } func (w *walkCallTracker) track(p Path, v Value) (Value, error) { @@ -148,7 +148,7 @@ func TestWalkMapError(t *testing.T) { }) out, err := Walk(value, tracker.track) assert.Equal(t, cerr, err) - assert.Equal(t, NilValue, out) + assert.Equal(t, InvalidValue, out) // The callback should have been called twice. assert.Len(t, tracker.calls, 2) @@ -239,7 +239,7 @@ func TestWalkSequenceError(t *testing.T) { }) out, err := Walk(value, tracker.track) assert.Equal(t, cerr, err) - assert.Equal(t, NilValue, out) + assert.Equal(t, InvalidValue, out) // The callback should have been called three times. assert.Len(t, tracker.calls, 3)