PythonMutator: support omitempty in PyDABs

bundle/config/mutator/python/python_mutator.go

@@ -241,8 +241,12 @@ func createLoadOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
 	jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs"))
 
 	return merge.OverrideVisitor{
-		VisitDelete: func(valuePath dyn.Path, left dyn.Value) error {
-			return fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
+		VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) {
+			if isOmitemptyDelete(left) {
+				return left, nil
+			}
+
+			return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
 		},
 		VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) {
 			if !valuePath.HasPrefix(jobsPath) {
@@ -274,21 +278,25 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
 	jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs"))
 
 	return merge.OverrideVisitor{
-		VisitDelete: func(valuePath dyn.Path, left dyn.Value) error {
+		VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) {
+			if isOmitemptyDelete(left) {
+				return left, nil
+			}
+
 			if !valuePath.HasPrefix(jobsPath) {
-				return fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
+				return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
 			}
 
 			deleteResource := len(valuePath) == len(jobsPath)+1
 
 			if deleteResource {
-				return fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
+				return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String())
 			}
 
 			// deleting properties is allowed because it only changes an existing resource
 			log.Debugf(ctx, "Delete value at %q", valuePath.String())
 
-			return nil
+			return dyn.NilValue, nil
 		},
 		VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) {
 			if !valuePath.HasPrefix(jobsPath) {
@@ -311,6 +319,21 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
 	}
 }
 
+func isOmitemptyDelete(left dyn.Value) bool {
+	// PyDABs output can omit empty sequences/mappings, 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.
+
+	if left.Kind() == dyn.KindMap && left.MustMap().Len() == 0 {
+		return true
+	}
+
+	if left.Kind() == dyn.KindSequence && len(left.MustSequence()) == 0 {
+		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

@@ -384,12 +384,13 @@ func TestCreateOverrideVisitor(t *testing.T) {
 
 		if tc.deletePath != nil {
 			t.Run(tc.name+"-delete", func(t *testing.T) {
-				err := visitor.VisitDelete(tc.deletePath, left)
+				out, err := visitor.VisitDelete(tc.deletePath, left)
 
 				if tc.deleteError != nil {
 					assert.Equal(t, tc.deleteError, err)
 				} else {
 					assert.NoError(t, err)
+					assert.Equal(t, dyn.NilValue, out)
 				}
 			})
 		}
@@ -409,6 +410,84 @@ func TestCreateOverrideVisitor(t *testing.T) {
 	}
 }
 
+type overrideVisitorOmitemptyTestCase struct {
+	name     string
+	path     dyn.Path
+	left     dyn.Value
+	phases   []phase
+	expected dyn.Value
+}
+
+func TestCreateOverrideVisitor_omitempty(t *testing.T) {
+	// PyDABs output can omit empty sequences/mappings, 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.
+
+	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:     "empty variables",
+			path:     dyn.MustPathFromString("variables"),
+			left:     dyn.NewValue([]dyn.Value{}, location),
+			expected: dyn.NewValue([]dyn.Value{}, location),
+			phases:   allPhases,
+		},
+		{
+			name:     "empty job clusters",
+			path:     dyn.MustPathFromString("resources.jobs.job0.job_clusters"),
+			left:     dyn.NewValue([]dyn.Value{}, location),
+			expected: dyn.NewValue([]dyn.Value{}, location),
+			phases:   allPhases,
+		},
+		{
+			name:     "non-empty job clusters",
+			path:     dyn.MustPathFromString("resources.jobs.job0.job_clusters"),
+			left:     dyn.NewValue([]dyn.Value{dyn.NewValue("abc", location)}, location),
+			expected: dyn.NilValue,
+			// deletions aren't allowed in 'load' phase
+			phases: []phase{PythonMutatorPhaseInit},
+		},
+		{
+			name:     "empty tags",
+			path:     dyn.MustPathFromString("resources.jobs.job0.tags"),
+			left:     dyn.NewValue(map[string]dyn.Value{}, location),
+			expected: dyn.NewValue(map[string]dyn.Value{}, location),
+			phases:   allPhases,
+		},
+		{
+			name: "non-empty tags",
+			path: dyn.MustPathFromString("resources.jobs.job0.tags"),
+			left: dyn.NewValue(
+				map[string]dyn.Value{"dev": dyn.NewValue("true", location)},
+				location,
+			),
+			expected: dyn.NilValue,
+			// deletions aren't allowed in 'load' phase
+			phases: []phase{PythonMutatorPhaseInit},
+		},
+	}
+
+	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)
+
+				out, err := visitor.VisitDelete(tc.path, tc.left)
+
+				assert.NoError(t, err)
+				assert.Equal(t, tc.expected, out)
+			})
+		}
+	}
+}
+
 func TestInterpreterPath(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		assert.Equal(t, "venv\\Scripts\\python3.exe", interpreterPath("venv"))

libs/dyn/merge/override.go

@@ -13,11 +13,17 @@ import (
 // For instance, it can disallow changes outside the specific path(s), or update
 // the location of the effective value.
 //
+// Values returned by 'VisitDelete', 'VisitInsert' and 'VisitUpdate' are used as
+// the final value of the node. 'VisitDelete' should return dyn.NilValue to
+// do the delete, or can return 'left' to undo it.
+//
+// TODO 'VisitDelete' and 'VisitInsert' should support dyn.NilValue as well
+//
 // '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
 type OverrideVisitor struct {
-	VisitDelete func(valuePath dyn.Path, left dyn.Value) error
+	VisitDelete func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error)
 	VisitInsert func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error)
 	VisitUpdate func(valuePath dyn.Path, left dyn.Value, right dyn.Value) (dyn.Value, error)
 }
@@ -111,11 +117,16 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy
 		if _, ok := rightMapping.GetPair(leftPair.Key); !ok {
 			path := basePath.Append(dyn.Key(leftPair.Key.MustString()))
 
-			err := visitor.VisitDelete(path, leftPair.Value)
+			deleteOut, err := visitor.VisitDelete(path, leftPair.Value)
 
 			if err != nil {
 				return dyn.NewMapping(), err
 			}
+
+			// if 'delete' was undone, add it back
+			if deleteOut != dyn.NilValue {
+				out.Set(leftPair.Key, deleteOut)
+			}
 		}
 	}
 
@@ -186,11 +197,16 @@ func overrideSequence(basePath dyn.Path, left []dyn.Value, right []dyn.Value, vi
 	} else if len(left) > len(right) {
 		for i := minLen; i < len(left); i++ {
 			path := basePath.Append(dyn.Index(i))
-			err := visitor.VisitDelete(path, left[i])
+			out, err := visitor.VisitDelete(path, left[i])
 
 			if err != nil {
 				return nil, err
 			}
+
+			// if 'delete' was undone, add it back
+			if out != dyn.NilValue {
+				values = append(values, out)
+			}
 		}
 	}
 

libs/dyn/merge/override_test.go

@@ -392,6 +392,13 @@ func TestOverride_Primitive(t *testing.T) {
 					assert.NoError(t, err)
 					assert.Equal(t, expected, actual)
 				}
+
+				for _, removed := range s.removed {
+					actual, err := dyn.GetByPath(out, dyn.MustPathFromString(removed))
+
+					assert.NoError(t, err)
+					assert.Equal(t, expected, actual)
+				}
 			})
 		}
 	}
@@ -467,10 +474,16 @@ func createVisitor(opts visitorOpts) (*visitorState, OverrideVisitor) {
 				return right, nil
 			}
 		},
-		VisitDelete: func(valuePath dyn.Path, left dyn.Value) error {
+		VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) {
 			s.removed = append(s.removed, valuePath.String())
 
-			return opts.error
+			if opts.error != nil {
+				return dyn.InvalidValue, opts.error
+			} else if opts.returnValue != nil {
+				return *opts.returnValue, nil
+			} else {
+				return dyn.NilValue, nil
+			}
 		},
 		VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) {
 			s.added = append(s.added, valuePath.String())