diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index bef69d9c93..26b6c54fc0 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 24e5ad60fe..64a2a1a655 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 81bbaa4d53..823fb19331 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 d8fd4e178c..d9ca974867 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())