PythonMutator: support omitempty in PyDABs

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.
+
+	if left.Kind() == dyn.KindMap && left.MustMap().Len() == 0 {
+		return true
+	}
+
+	if left.Kind() == dyn.KindSequence && len(left.MustSequence()) == 0 {
+		return true
+	}
+
+	// map/sequence can be nil, for instance, bad YAML like: `foo:<eof>`
+	if left.Kind() == dyn.KindNil {
+		return true
+	}
+
+	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,14 @@ 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 +199,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())