From b9e3c9872388d91c00b9e66219c7d8fd3abeddfd Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 3 Jul 2024 09:22:03 +0200 Subject: [PATCH] PythonMutator: support omitempty in PyDABs (#1513) ## Changes PyDABs output can omit empty sequences/mappings because we don't track them as optional. There is no semantic difference between empty and missing, which makes omitting correct. CLI detects that we falsely modify input resources by deleting all empty collections. To handle that, we extend `dyn.Override` to allow visitors to ignore certain deletes. If we see that an empty sequence or mapping is deleted, we revert such delete. ## Tests Unit tests --------- Co-authored-by: Pieter Noordhuis --- .../config/mutator/python/python_mutator.go | 29 +++++++ .../mutator/python/python_mutator_test.go | 87 +++++++++++++++++++ libs/dyn/merge/override.go | 19 +++- libs/dyn/merge/override_test.go | 29 ++++++- 4 files changed, 161 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index bef69d9c..26b6c54f 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -313,6 +313,10 @@ func createLoadOverrideVisitor(ctx context.Context) merge.OverrideVisitor { return merge.OverrideVisitor{ VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { + if isOmitemptyDelete(left) { + return merge.ErrOverrideUndoDelete + } + return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { @@ -346,6 +350,10 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { return merge.OverrideVisitor{ VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { + if isOmitemptyDelete(left) { + return merge.ErrOverrideUndoDelete + } + if !valuePath.HasPrefix(jobsPath) { return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) } @@ -382,6 +390,27 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { } } +func isOmitemptyDelete(left dyn.Value) bool { + // PyDABs can omit empty sequences/mappings in output, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before + // PyDABs deleted them. + + switch left.Kind() { + case dyn.KindMap: + return left.MustMap().Len() == 0 + + case dyn.KindSequence: + return len(left.MustSequence()) == 0 + + case dyn.KindNil: + // map/sequence can be nil, for instance, bad YAML like: `foo:` + return true + + default: + return false + } +} + // interpreterPath returns platform-specific path to Python interpreter in the virtual environment. func interpreterPath(venvPath string) string { if runtime.GOOS == "windows" { diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 24e5ad60..64a2a1a6 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -10,6 +10,8 @@ import ( "runtime" "testing" + "github.com/databricks/cli/libs/dyn/merge" + "github.com/databricks/cli/bundle/env" "github.com/stretchr/testify/require" @@ -419,6 +421,91 @@ func TestCreateOverrideVisitor(t *testing.T) { } } +type overrideVisitorOmitemptyTestCase struct { + name string + path dyn.Path + left dyn.Value + phases []phase + expectedErr error +} + +func TestCreateOverrideVisitor_omitempty(t *testing.T) { + // PyDABs can omit empty sequences/mappings in output, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before + // PyDABs deleted them. + + allPhases := []phase{PythonMutatorPhaseLoad, PythonMutatorPhaseInit} + location := dyn.Location{ + File: "databricks.yml", + Line: 10, + Column: 20, + } + + testCases := []overrideVisitorOmitemptyTestCase{ + { + // this is not happening, but adding for completeness + name: "undo delete of empty variables", + path: dyn.MustPathFromString("variables"), + left: dyn.NewValue([]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, + }, + { + name: "undo delete of empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, + }, + { + name: "allow delete of non-empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{dyn.NewValue("abc", location)}, location), + expectedErr: nil, + // deletions aren't allowed in 'load' phase + phases: []phase{PythonMutatorPhaseInit}, + }, + { + name: "undo delete of empty tags", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NewValue(map[string]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, + }, + { + name: "allow delete of non-empty tags", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NewValue( + map[string]dyn.Value{"dev": dyn.NewValue("true", location)}, + location, + ), + expectedErr: nil, + // deletions aren't allowed in 'load' phase + phases: []phase{PythonMutatorPhaseInit}, + }, + { + name: "undo delete of nil", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NilValue.WithLocation(location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, + }, + } + + for _, tc := range testCases { + for _, phase := range tc.phases { + t.Run(tc.name+"-"+string(phase), func(t *testing.T) { + visitor, err := createOverrideVisitor(context.Background(), phase) + require.NoError(t, err) + + err = visitor.VisitDelete(tc.path, tc.left) + + assert.Equal(t, tc.expectedErr, err) + }) + } + } +} + func TestLoadDiagnosticsFile_nonExistent(t *testing.T) { // this is an important behaviour, see loadDiagnosticsFile docstring _, err := loadDiagnosticsFile("non_existent_file.json") diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index 81bbaa4d..823fb193 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -1,6 +1,7 @@ package merge import ( + "errors" "fmt" "github.com/databricks/cli/libs/dyn" @@ -13,6 +14,9 @@ import ( // For instance, it can disallow changes outside the specific path(s), or update // the location of the effective value. // +// Values returned by 'VisitInsert' and 'VisitUpdate' are used as the final value +// of the node. 'VisitDelete' can return ErrOverrideUndoDelete to undo delete. +// // 'VisitDelete' is called when a value is removed from mapping or sequence // 'VisitInsert' is called when a new value is added to mapping or sequence // 'VisitUpdate' is called when a leaf value is updated @@ -22,6 +26,8 @@ type OverrideVisitor struct { VisitUpdate func(valuePath dyn.Path, left dyn.Value, right dyn.Value) (dyn.Value, error) } +var ErrOverrideUndoDelete = errors.New("undo delete operation") + // Override overrides value 'leftRoot' with 'rightRoot', keeping 'location' if values // haven't changed. Preserving 'location' is important to preserve the original source of the value // for error reporting. @@ -111,7 +117,13 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy err := visitor.VisitDelete(path, leftPair.Value) - if err != nil { + // if 'delete' was undone, add it back + if errors.Is(err, ErrOverrideUndoDelete) { + err := out.Set(leftPair.Key, leftPair.Value) + if err != nil { + return dyn.NewMapping(), err + } + } else if err != nil { return dyn.NewMapping(), err } } @@ -186,7 +198,10 @@ func overrideSequence(basePath dyn.Path, left []dyn.Value, right []dyn.Value, vi path := basePath.Append(dyn.Index(i)) err := visitor.VisitDelete(path, left[i]) - if err != nil { + // if 'delete' was undone, add it back + if errors.Is(err, ErrOverrideUndoDelete) { + values = append(values, left[i]) + } else if err != nil { return nil, err } } diff --git a/libs/dyn/merge/override_test.go b/libs/dyn/merge/override_test.go index d8fd4e17..d9ca9748 100644 --- a/libs/dyn/merge/override_test.go +++ b/libs/dyn/merge/override_test.go @@ -5,6 +5,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/databricks/cli/libs/dyn" assert "github.com/databricks/cli/libs/dyn/dynassert" ) @@ -393,6 +395,24 @@ func TestOverride_Primitive(t *testing.T) { assert.Equal(t, expected, actual) } }) + + if len(tc.state.removed) > 0 { + t.Run(tc.name+" - visitor can undo delete", func(t *testing.T) { + s, visitor := createVisitor(visitorOpts{deleteError: ErrOverrideUndoDelete}) + out, err := override(dyn.EmptyPath, tc.left, tc.right, visitor) + require.NoError(t, err) + + for _, removed := range s.removed { + expected, err := dyn.GetByPath(tc.left, dyn.MustPathFromString(removed)) + require.NoError(t, err) + + actual, err := dyn.GetByPath(out, dyn.MustPathFromString(removed)) + + assert.NoError(t, err) + assert.Equal(t, expected, actual) + } + }) + } } } } @@ -449,6 +469,7 @@ type visitorState struct { type visitorOpts struct { error error + deleteError error returnValue *dyn.Value } @@ -470,7 +491,13 @@ func createVisitor(opts visitorOpts) (*visitorState, OverrideVisitor) { VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { s.removed = append(s.removed, valuePath.String()) - return opts.error + if opts.error != nil { + return opts.error + } else if opts.deleteError != nil { + return opts.deleteError + } else { + return nil + } }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { s.added = append(s.added, valuePath.String())