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 <pcnoordhuis@gmail.com>

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:<eof>`
+		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" {

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")

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
 			}
 		}

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())