Merge pull request #2206 from loft-sh/fix-op-patching-adds

fix: op patching to not add elements if key already exists

Author: FabianKramm

e2e/tests/config/config.go

@@ -244,6 +244,51 @@ var _ = DevSpaceDescribe("config", func() {
 		framework.ExpectEqual(latestConfig.Deployments["test2"].Name, "test2")
 	})
 
+	ginkgo.It("should not be able to add in patch if key already exists", func() {
+		tempDir, err := framework.CopyToTempDir("tests/config/testdata/patch-add-dont-overwrite-existing-key")
+		framework.ExpectNoError(err)
+		defer framework.CleanupTempDir(initialDir, tempDir)
+
+		configBuffer := &bytes.Buffer{}
+		printCmd := &cmd.PrintCmd{
+			GlobalFlags: &flags.GlobalFlags{
+				ConfigPath: "devspace.yaml",
+				Profiles:   []string{"deploy"},
+			},
+			Out:      configBuffer,
+			SkipInfo: true,
+		}
+
+		err = printCmd.Run(f)
+		framework.ExpectError(err)
+	})
+
+	ginkgo.It("should be able to add in patch if key does not already exists", func() {
+		tempDir, err := framework.CopyToTempDir("tests/config/testdata/patch-add-dont-overwrite-existing-key")
+		framework.ExpectNoError(err)
+		defer framework.CleanupTempDir(initialDir, tempDir)
+
+		configBuffer := &bytes.Buffer{}
+		printCmd := &cmd.PrintCmd{
+			GlobalFlags: &flags.GlobalFlags{
+				ConfigPath: "devspace.yaml",
+				Profiles:   []string{"patch-ok"},
+			},
+			Out:      configBuffer,
+			SkipInfo: true,
+		}
+
+		err = printCmd.Run(f)
+		framework.ExpectNoError(err)
+
+		latestConfig := &latest.Config{}
+		err = yaml.Unmarshal(configBuffer.Bytes(), latestConfig)
+		framework.ExpectNoError(err)
+
+		// validate config
+		framework.ExpectEqual(string(latestConfig.Images["importme"].RebuildStrategy), "ignoreContextChanges")
+	})
+
 	ginkgo.It("should load profile cached and uncached", func() {
 		tempDir, err := framework.CopyToTempDir("tests/config/testdata/profile")
 		framework.ExpectNoError(err)

e2e/tests/config/testdata/patch-add-dont-overwrite-existing-key/devspace.yaml

@@ -0,0 +1,16 @@
+version: v1beta11
+images:
+  importme:
+    image: golang:1.17
+    context: ../..
+profiles:
+  - name: deploy
+    patches:
+      - op: add
+        path: images.importme.context
+        value: ../..
+  - name: patch-ok
+    patches:
+      - op: add
+        path: images.importme.rebuildStrategy
+        value: ignoreContextChanges

pkg/devspace/config/loader/patch/operation.go

@@ -2,7 +2,6 @@ package patch
 
 import (
 	"fmt"
-
 	"github.com/vmware-labs/yaml-jsonpath/pkg/yamlpath"
 	yaml "gopkg.in/yaml.v3"
 )
@@ -35,38 +34,63 @@ func (op *Operation) Perform(doc *yaml.Node) error {
 		return err
 	}
 
-	if len(matches) == 0 {
-		if op.Op == opAdd {
-			matches, err = getParents(doc, op.Path)
-			if err != nil {
-				return fmt.Errorf("could not add using path: %s", op.Path)
+	if len(matches) == 0 && op.Op != opAdd {
+		return fmt.Errorf("%s operation does not apply: doc is missing path: %s", op.Op, op.Path)
+	}
+
+	// function that will actually perform the patch operation
+	var opFunc func(parent *yaml.Node, match *yaml.Node)
+
+	switch op.Op {
+	case opAdd:
+		opFunc = op.add
+
+		if len(matches) > 0 {
+			if matches[0].Kind == yaml.MappingNode || matches[0].Kind == yaml.SequenceNode {
+				break
 			}
+		}
+
+		originalMatches := matches
 
-			parentPath := op.Path.getParentPath()
-			propertName := op.Path.getChildName()
-			if op.Value != nil {
-				propertyValue := op.Value.Content[0]
-				op.Value = createMappingNode(propertName, propertyValue)
+		matches, err = getParents(doc, op.Path)
+		if err != nil {
+			return fmt.Errorf("could not add using path: %s", op.Path)
+		}
+
+		if len(matches) > 0 && len(originalMatches) > 0 {
+			if matches[0].Kind == yaml.SequenceNode {
+				matches = originalMatches
+				break
 			}
-			op.Path = OpPath(parentPath)
-		} else {
-			return fmt.Errorf("%s operation does not apply: doc is missing path: %s", op.Op, op.Path)
+
+			// we are trying to overwrite an existing key in a map, don't do that!
+			return fmt.Errorf(
+				"attempting add operation for non array/object path '%s' which already exists",
+				op.Path,
+			)
 		}
+
+		parentPath := op.Path.getParentPath()
+		propertyName := op.Path.getChildName()
+		if op.Value != nil {
+			propertyValue := op.Value.Content[0]
+			op.Value = createMappingNode(propertyName, propertyValue)
+		}
+		op.Path = OpPath(parentPath)
+
+	case opRemove:
+		opFunc = op.remove
+	case opReplace:
+		opFunc = op.replace
+	default:
+		return fmt.Errorf("unexpected op: %s", op.Op)
 	}
 
 	for _, match := range matches {
 		parent := find(doc, containsChild(match))
 
-		switch op.Op {
-		case opAdd:
-			op.add(parent, match)
-		case opRemove:
-			op.remove(parent, match)
-		case opReplace:
-			op.replace(parent, match)
-		default:
-			return fmt.Errorf("unexpected op: %s", op.Op)
-		}
+		opFunc(parent, match)
 	}
 
 	return nil