From 068c7cfc2d319db256c8316d3dfa95636b972c3c Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Fri, 21 Jun 2024 19:52:42 +0530 Subject: [PATCH] Return `dyn.InvalidValue` instead of `dyn.NilValue` when errors happen (#1514) ## Changes With https://github.com/databricks/cli/pull/1507 and https://github.com/databricks/cli/pull/1511 we are clarifying the semantics associated with `dyn.InvalidValue` and `dyn.NilValue`. An invalid value is the default zero value and is used to signals the complete absence of the value. A nil value, on the other hand, is a valid value for a piece of configuration and signals explicitly setting a key to nil in the configuration tree. In keeping with that theme, this PR returns `dyn.InvalidValue` instead of `dyn.NilValue` at error sites. This change is not expected to have a material change in behaviour and is being done to set the right convention since we have well-defined semantics associated with both `NilValue` and `InvalidValue`. ## Tests Unit tests and integration tests pass. Also manually scanned the changes and the associated call sites to verify the `NilValue` value itself was not being relied upon. --- bundle/config/generate/job.go | 2 +- bundle/config/mutator/rewrite_sync_paths.go | 6 ++--- bundle/deploy/terraform/tfdyn/rename_keys.go | 2 +- libs/dyn/merge/merge.go | 8 +++---- libs/dyn/merge/merge_test.go | 6 ++--- libs/dyn/yamlloader/loader.go | 24 ++++++++++---------- libs/dyn/yamlloader/yaml.go | 2 +- libs/dyn/yamlsaver/utils.go | 2 +- 8 files changed, 26 insertions(+), 26 deletions(-) diff --git a/bundle/config/generate/job.go b/bundle/config/generate/job.go index 469f8422..3ab5e012 100644 --- a/bundle/config/generate/job.go +++ b/bundle/config/generate/job.go @@ -17,7 +17,7 @@ func ConvertJobToValue(job *jobs.Job) (dyn.Value, error) { for _, task := range job.Settings.Tasks { v, err := convertTaskToValue(task, taskOrder) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } tasks = append(tasks, v) } diff --git a/bundle/config/mutator/rewrite_sync_paths.go b/bundle/config/mutator/rewrite_sync_paths.go index 71019023..85db7979 100644 --- a/bundle/config/mutator/rewrite_sync_paths.go +++ b/bundle/config/mutator/rewrite_sync_paths.go @@ -35,7 +35,7 @@ func (m *rewriteSyncPaths) makeRelativeTo(root string) dyn.MapFunc { dir := filepath.Dir(v.Location().File) rel, err := filepath.Rel(root, dir) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } return dyn.NewValue(filepath.Join(rel, v.MustString()), v.Location()), nil @@ -47,11 +47,11 @@ func (m *rewriteSyncPaths) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia return dyn.Map(v, "sync", func(_ dyn.Path, v dyn.Value) (nv dyn.Value, err error) { v, err = dyn.Map(v, "include", dyn.Foreach(m.makeRelativeTo(b.RootPath))) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } v, err = dyn.Map(v, "exclude", dyn.Foreach(m.makeRelativeTo(b.RootPath))) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } return v, nil }) diff --git a/bundle/deploy/terraform/tfdyn/rename_keys.go b/bundle/deploy/terraform/tfdyn/rename_keys.go index a65c9f25..650ffb89 100644 --- a/bundle/deploy/terraform/tfdyn/rename_keys.go +++ b/bundle/deploy/terraform/tfdyn/rename_keys.go @@ -28,7 +28,7 @@ func renameKeys(v dyn.Value, rename map[string]string) (dyn.Value, error) { p[0] = dyn.Key(newKey) acc, err = dyn.SetByPath(acc, p, v) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } return dyn.InvalidValue, dyn.ErrDrop } diff --git a/libs/dyn/merge/merge.go b/libs/dyn/merge/merge.go index 69ccf516..ffe000da 100644 --- a/libs/dyn/merge/merge.go +++ b/libs/dyn/merge/merge.go @@ -34,17 +34,17 @@ func merge(a, b dyn.Value) (dyn.Value, error) { switch ak { case dyn.KindMap: if bk != dyn.KindMap { - return dyn.NilValue, fmt.Errorf("cannot merge map with %s", bk) + return dyn.InvalidValue, fmt.Errorf("cannot merge map with %s", bk) } return mergeMap(a, b) case dyn.KindSequence: if bk != dyn.KindSequence { - return dyn.NilValue, fmt.Errorf("cannot merge sequence with %s", bk) + return dyn.InvalidValue, fmt.Errorf("cannot merge sequence with %s", bk) } return mergeSequence(a, b) default: if ak != bk { - return dyn.NilValue, fmt.Errorf("cannot merge %s with %s", ak, bk) + return dyn.InvalidValue, fmt.Errorf("cannot merge %s with %s", ak, bk) } return mergePrimitive(a, b) } @@ -66,7 +66,7 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) { // If the key already exists, merge the values. merged, err := merge(ov, pv) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } out.Set(pk, merged) } else { diff --git a/libs/dyn/merge/merge_test.go b/libs/dyn/merge/merge_test.go index eaaaab16..3706dbd7 100644 --- a/libs/dyn/merge/merge_test.go +++ b/libs/dyn/merge/merge_test.go @@ -76,7 +76,7 @@ func TestMergeMapsError(t *testing.T) { { out, err := Merge(v, other) assert.EqualError(t, err, "cannot merge map with string") - assert.Equal(t, dyn.NilValue, out) + assert.Equal(t, dyn.InvalidValue, out) } } @@ -151,7 +151,7 @@ func TestMergeSequencesError(t *testing.T) { { out, err := Merge(v, other) assert.EqualError(t, err, "cannot merge sequence with string") - assert.Equal(t, dyn.NilValue, out) + assert.Equal(t, dyn.InvalidValue, out) } } @@ -202,6 +202,6 @@ func TestMergePrimitivesError(t *testing.T) { { out, err := Merge(v, other) assert.EqualError(t, err, "cannot merge string with map") - assert.Equal(t, dyn.NilValue, out) + assert.Equal(t, dyn.InvalidValue, out) } } diff --git a/libs/dyn/yamlloader/loader.go b/libs/dyn/yamlloader/loader.go index 908793d5..e6a16f79 100644 --- a/libs/dyn/yamlloader/loader.go +++ b/libs/dyn/yamlloader/loader.go @@ -55,7 +55,7 @@ func (d *loader) load(node *yaml.Node) (dyn.Value, error) { case yaml.AliasNode: value, err = d.loadAlias(node, loc) default: - return dyn.NilValue, errorf(loc, "unknown node kind: %v", node.Kind) + return dyn.InvalidValue, errorf(loc, "unknown node kind: %v", node.Kind) } if err != nil { @@ -80,7 +80,7 @@ func (d *loader) loadSequence(node *yaml.Node, loc dyn.Location) (dyn.Value, err for i, n := range node.Content { v, err := d.load(n) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } acc[i] = v @@ -99,7 +99,7 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro // Assert that keys are strings if key.Kind != yaml.ScalarNode { - return dyn.NilValue, errorf(loc, "key is not a scalar") + return dyn.InvalidValue, errorf(loc, "key is not a scalar") } st := key.ShortTag() @@ -113,17 +113,17 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro merge = val continue default: - return dyn.NilValue, errorf(loc, "invalid key tag: %v", st) + return dyn.InvalidValue, errorf(loc, "invalid key tag: %v", st) } k, err := d.load(key) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } v, err := d.load(val) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } acc.Set(k, v) @@ -155,7 +155,7 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro for _, n := range mnodes { v, err := d.load(n) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } m, ok := v.AsMap() if !ok { @@ -186,12 +186,12 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error case "false": return dyn.NewValue(false, loc), nil default: - return dyn.NilValue, errorf(loc, "invalid bool value: %v", node.Value) + return dyn.InvalidValue, errorf(loc, "invalid bool value: %v", node.Value) } case "!!int": i64, err := strconv.ParseInt(node.Value, 10, 64) if err != nil { - return dyn.NilValue, errorf(loc, "invalid int value: %v", node.Value) + return dyn.InvalidValue, errorf(loc, "invalid int value: %v", node.Value) } // Use regular int type instead of int64 if possible. if i64 >= math.MinInt32 && i64 <= math.MaxInt32 { @@ -201,7 +201,7 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error case "!!float": f64, err := strconv.ParseFloat(node.Value, 64) if err != nil { - return dyn.NilValue, errorf(loc, "invalid float value: %v", node.Value) + return dyn.InvalidValue, errorf(loc, "invalid float value: %v", node.Value) } return dyn.NewValue(f64, loc), nil case "!!null": @@ -219,9 +219,9 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error return dyn.NewValue(t, loc), nil } } - return dyn.NilValue, errorf(loc, "invalid timestamp value: %v", node.Value) + return dyn.InvalidValue, errorf(loc, "invalid timestamp value: %v", node.Value) default: - return dyn.NilValue, errorf(loc, "unknown tag: %v", st) + return dyn.InvalidValue, errorf(loc, "unknown tag: %v", st) } } diff --git a/libs/dyn/yamlloader/yaml.go b/libs/dyn/yamlloader/yaml.go index a18324ff..b79b41e1 100644 --- a/libs/dyn/yamlloader/yaml.go +++ b/libs/dyn/yamlloader/yaml.go @@ -15,7 +15,7 @@ func LoadYAML(path string, r io.Reader) (dyn.Value, error) { if err == io.EOF { return dyn.NilValue, nil } - return dyn.NilValue, err + return dyn.InvalidValue, err } return newLoader(path).load(&node) diff --git a/libs/dyn/yamlsaver/utils.go b/libs/dyn/yamlsaver/utils.go index 6149491d..fa5ab08f 100644 --- a/libs/dyn/yamlsaver/utils.go +++ b/libs/dyn/yamlsaver/utils.go @@ -15,7 +15,7 @@ func ConvertToMapValue(strct any, order *Order, skipFields []string, dst map[str ref := dyn.NilValue mv, err := convert.FromTyped(strct, ref) if err != nil { - return dyn.NilValue, err + return dyn.InvalidValue, err } if mv.Kind() != dyn.KindMap {