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 <[email protected]>
databricks · Jul 3, 2024 · b9e3c98 · b9e3c98
1 parent 5a0a6d7
commit b9e3c98
Show file tree

Hide file tree

Showing 4 changed files with 161 additions and 3 deletions.
diff --git 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:<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" {

diff --git 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
@@ -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
@@ -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())