Merge pull request #16980 from jackgr/force_apply

Implement conflict detection in strategic merge patch

pkg/kubectl/cmd/apply.go

@@ -151,7 +151,7 @@ func RunApply(f *cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *Ap
 		}
 
 		// Compute a three way strategic merge patch to send to server.
-		patch, err := strategicpatch.CreateThreeWayMergePatch(original, modified, current, info.VersionedObject, false)
+		patch, err := strategicpatch.CreateThreeWayMergePatch(original, modified, current, info.VersionedObject, true)
 		if err != nil {
 			format := "creating patch with:\noriginal:\n%s\nmodified:\n%s\ncurrent:\n%s\nfrom:\n%v\nfor:"
 			return cmdutil.AddSourceToErr(fmt.Sprintf(format, original, modified, current, info), info.Source, err)

pkg/util/strategicpatch/patch.go

@@ -23,6 +23,9 @@ import (
 	"sort"
 
 	forkedjson "k8s.io/kubernetes/third_party/forked/json"
+
+	"github.com/davecgh/go-spew/spew"
+	"github.com/ghodss/yaml"
 )
 
 // An alternate implementation of JSON Merge Patch
@@ -66,8 +69,8 @@ type errConflict struct {
 	message string
 }
 
-func newErrConflict(patch, current []byte) errConflict {
+func newErrConflict(patch, current string) errConflict {
-	s := fmt.Sprintf("patch:\n%s\nconflicts with current:\n%s\n", patch, current)
+	s := fmt.Sprintf("patch:\n%s\nconflicts with changes made from original to current:\n%s\n", patch, current)
 	return errConflict{s}
 }
 
@@ -331,7 +334,7 @@ loopB:
 }
 
 var errNoMergeKeyFmt = "map: %v does not contain declared merge key: %s"
-var errBadArgTypeFmt = "expected a %s, but received a %t"
+var errBadArgTypeFmt = "expected a %s, but received a %s"
 
 // Returns a (recursive) strategic merge patch that yields modified when applied to original,
 // for a pair of lists of maps with merge semantics.
@@ -354,7 +357,8 @@ loopB:
 	for ; modifiedIndex < len(modifiedSorted); modifiedIndex++ {
 		modifiedMap, ok := modifiedSorted[modifiedIndex].(map[string]interface{})
 		if !ok {
-			return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", modifiedSorted[modifiedIndex])
+			t := reflect.TypeOf(modifiedSorted[modifiedIndex])
+			return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", t.Kind().String())
 		}
 
 		modifiedValue, ok := modifiedMap[mergeKey]
@@ -365,7 +369,8 @@ loopB:
 		for ; originalIndex < len(originalSorted); originalIndex++ {
 			originalMap, ok := originalSorted[originalIndex].(map[string]interface{})
 			if !ok {
-				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", originalSorted[originalIndex])
+				t := reflect.TypeOf(originalSorted[originalIndex])
+				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", t.Kind().String())
 			}
 
 			originalValue, ok := originalMap[mergeKey]
@@ -411,7 +416,8 @@ loopB:
 		for ; originalIndex < len(originalSorted); originalIndex++ {
 			originalMap, ok := originalSorted[originalIndex].(map[string]interface{})
 			if !ok {
-				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", originalSorted[originalIndex])
+				t := reflect.TypeOf(originalSorted[originalIndex])
+				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", t.Kind().String())
 			}
 
 			originalValue, ok := originalMap[mergeKey]
@@ -444,11 +450,11 @@ func StrategicMergePatchData(original, patch []byte, dataStruct interface{}) ([]
 // by calling CreateStrategicMergePatch.
 func StrategicMergePatch(original, patch []byte, dataStruct interface{}) ([]byte, error) {
 	if original == nil {
-		original = []byte{}
+		original = []byte("{}")
 	}
 
 	if patch == nil {
-		patch = []byte{}
+		patch = []byte("{}")
 	}
 
 	originalMap := map[string]interface{}{}
@@ -926,7 +932,8 @@ func sliceElementType(slices ...[]interface{}) (reflect.Type, error) {
 
 // HasConflicts returns true if the left and right JSON interface objects overlap with
 // different values in any key. All keys are required to be strings. Since patches of the
-// same Type have congruent keys, this is valid for multiple patch types.
+// same Type have congruent keys, this is valid for multiple patch types. This method
+// supports JSON merge patch semantics.
 func HasConflicts(left, right interface{}) (bool, error) {
 	switch typedLeft := left.(type) {
 	case map[string]interface{}:
@@ -939,6 +946,7 @@ func HasConflicts(left, right interface{}) (bool, error) {
 				}
 				return HasConflicts(leftValue, rightValue)
 			}
+
 			return false, nil
 		default:
 			return true, nil
@@ -949,9 +957,11 @@ func HasConflicts(left, right interface{}) (bool, error) {
 			if len(typedLeft) != len(typedRight) {
 				return true, nil
 			}
+
 			for i := range typedLeft {
 				return HasConflicts(typedLeft[i], typedRight[i])
 			}
+
 			return false, nil
 		default:
 			return true, nil
@@ -963,14 +973,184 @@ func HasConflicts(left, right interface{}) (bool, error) {
 	}
 }
 
+// MergingMapsHaveConflicts returns true if the left and right JSON interface
+// objects overlap with different values in any key. All keys are required to be
+// strings. Since patches of the same Type have congruent keys, this is valid
+// for multiple patch types. This method supports strategic merge patch semantics.
+func MergingMapsHaveConflicts(left, right map[string]interface{}, dataStruct interface{}) (bool, error) {
+	t, err := getTagStructType(dataStruct)
+	if err != nil {
+		return true, err
+	}
+
+	return mergingMapFieldsHaveConflicts(left, right, t, "", "")
+}
+
+func mergingMapFieldsHaveConflicts(
+	left, right interface{},
+	fieldType reflect.Type,
+	fieldPatchStrategy, fieldPatchMergeKey string,
+) (bool, error) {
+	switch leftType := left.(type) {
+	case map[string]interface{}:
+		switch rightType := right.(type) {
+		case map[string]interface{}:
+			leftMarker, okLeft := leftType[directiveMarker]
+			rightMarker, okRight := rightType[directiveMarker]
+			// if one or the other has a directive marker,
+			// then we need to consider that before looking at the individual keys,
+			// since a directive operates on the whole map.
+			if okLeft || okRight {
+				// if one has a directive marker and the other doesn't,
+				// then we have a conflict, since one is deleting or replacing the whole map,
+				// and the other is doing things to individual keys.
+				if okLeft != okRight {
+					return true, nil
+				}
+
+				// if they both have markers, but they are not the same directive,
+				// then we have a conflict because they're doing different things to the map.
+				if leftMarker != rightMarker {
+					return true, nil
+				}
+			}
+
+			// Check the individual keys.
+			return mapsHaveConflicts(leftType, rightType, fieldType)
+		default:
+			return true, nil
+		}
+	case []interface{}:
+		switch rightType := right.(type) {
+		case []interface{}:
+			return slicesHaveConflicts(leftType, rightType, fieldType, fieldPatchStrategy, fieldPatchMergeKey)
+		default:
+			return true, nil
+		}
+	case string, float64, bool, int, int64, nil:
+		return !reflect.DeepEqual(left, right), nil
+	default:
+		return true, fmt.Errorf("unknown type: %v", reflect.TypeOf(left))
+	}
+}
+
+func mapsHaveConflicts(typedLeft, typedRight map[string]interface{}, structType reflect.Type) (bool, error) {
+	for key, leftValue := range typedLeft {
+		if key != directiveMarker {
+			if rightValue, ok := typedRight[key]; ok {
+				fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(structType, key)
+				if err != nil {
+					return true, err
+				}
+
+				if hasConflicts, err := mergingMapFieldsHaveConflicts(leftValue, rightValue,
+					fieldType, fieldPatchStrategy, fieldPatchMergeKey); hasConflicts {
+					return true, err
+				}
+			}
+		}
+	}
+
+	return false, nil
+}
+
+func slicesHaveConflicts(
+	typedLeft, typedRight []interface{},
+	fieldType reflect.Type,
+	fieldPatchStrategy, fieldPatchMergeKey string,
+) (bool, error) {
+	elementType, err := sliceElementType(typedLeft, typedRight)
+	if err != nil {
+		return true, err
+	}
+
+	valueType := fieldType.Elem()
+	if fieldPatchStrategy == mergeDirective {
+		// Merging lists of scalars have no conflicts by definition
+		// So we only need to check further if the elements are maps
+		if elementType.Kind() != reflect.Map {
+			return false, nil
+		}
+
+		// Build a map for each slice and then compare the two maps
+		leftMap, err := sliceOfMapsToMapOfMaps(typedLeft, fieldPatchMergeKey)
+		if err != nil {
+			return true, err
+		}
+
+		rightMap, err := sliceOfMapsToMapOfMaps(typedRight, fieldPatchMergeKey)
+		if err != nil {
+			return true, err
+		}
+
+		return mapsOfMapsHaveConflicts(leftMap, rightMap, valueType)
+	}
+
+	// Either we don't have type information, or these are non-merging lists
+	if len(typedLeft) != len(typedRight) {
+		return true, nil
+	}
+
+	// Sort scalar slices to prevent ordering issues
+	// We have no way to sort non-merging lists of maps
+	if elementType.Kind() != reflect.Map {
+		typedLeft = uniqifyAndSortScalars(typedLeft)
+		typedRight = uniqifyAndSortScalars(typedRight)
+	}
+
+	// Compare the slices element by element in order
+	// This test will fail if the slices are not sorted
+	for i := range typedLeft {
+		if hasConflicts, err := mergingMapFieldsHaveConflicts(typedLeft[i], typedRight[i], valueType, "", ""); hasConflicts {
+			return true, err
+		}
+	}
+
+	return false, nil
+}
+
+func sliceOfMapsToMapOfMaps(slice []interface{}, mergeKey string) (map[string]interface{}, error) {
+	result := make(map[string]interface{}, len(slice))
+	for _, value := range slice {
+		typedValue, ok := value.(map[string]interface{})
+		if !ok {
+			return nil, fmt.Errorf("invalid element type in merging list:%v", slice)
+		}
+
+		mergeValue, ok := typedValue[mergeKey]
+		if !ok {
+			return nil, fmt.Errorf("cannot find merge key `%s` in merging list element:%v", mergeKey, typedValue)
+		}
+
+		result[fmt.Sprintf("%s", mergeValue)] = typedValue
+	}
+
+	return result, nil
+}
+
+func mapsOfMapsHaveConflicts(typedLeft, typedRight map[string]interface{}, structType reflect.Type) (bool, error) {
+	for key, leftValue := range typedLeft {
+		if rightValue, ok := typedRight[key]; ok {
+			if hasConflicts, err := mergingMapFieldsHaveConflicts(leftValue, rightValue, structType, "", ""); hasConflicts {
+				return true, err
+			}
+		}
+	}
+
+	return false, nil
+}
+
 // CreateThreeWayMergePatch reconciles a modified configuration with an original configuration,
 // while preserving any changes or deletions made to the original configuration in the interim,
 // and not overridden by the current configuration. All three documents must be passed to the
 // method as json encoded content. It will return a strategic merge patch, or an error if any
 // of the documents is invalid, or if there are any preconditions that fail against the modified
-// configuration, or, if force is false and there are conflicts between the modified and current
+// configuration, or, if overwrite is false and there are conflicts between the modified and current
-// configurations.
+// configurations. Conflicts are defined as keys changed differently from original to modified
-func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, force bool, fns ...PreconditionFunc) ([]byte, error) {
+// than from original to current. In other words, a conflict occurs if modified changes any key
+// in a way that is different from how it is changed in current (e.g., deleting it, changing its
+// value).
+func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, overwrite bool, fns ...PreconditionFunc) ([]byte, error) {
 	originalMap := map[string]interface{}{}
 	if len(original) > 0 {
 		if err := json.Unmarshal(original, &originalMap); err != nil {
@@ -1023,8 +1203,41 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int
 		}
 	}
 
-	// TODO(jackgr): If force is false, and the patch contains any keys that are also in current,
+	// If overwrite is false, and the patch contains any keys that were changed differently,
 	// then return a conflict error.
+	if !overwrite {
+		changedMap, err := diffMaps(originalMap, currentMap, t, false, false)
+		if err != nil {
+			return nil, err
+		}
+
+		hasConflicts, err := MergingMapsHaveConflicts(patchMap, changedMap, dataStruct)
+		if err != nil {
+			return nil, err
+		}
+
+		if hasConflicts {
+			return nil, newErrConflict(toYAMLOrError(patchMap), toYAMLOrError(changedMap))
+		}
+	}
 
 	return json.Marshal(patchMap)
 }
+
+func toYAMLOrError(v interface{}) string {
+	y, err := toYAML(v)
+	if err != nil {
+		return err.Error()
+	}
+
+	return y
+}
+
+func toYAML(v interface{}) (string, error) {
+	y, err := yaml.Marshal(v)
+	if err != nil {
+		return "", fmt.Errorf("yaml marshal failed:%v\n%v\n", err, spew.Sdump(v))
+	}
+
+	return string(y), nil
+}

pkg/util/strategicpatch/patch_test.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/davecgh/go-spew/spew"
@@ -263,6 +264,17 @@ testCases:
         - 1
         - 2
         - 3
+  - description: delete map from nested map
+    original:
+      simpleMap:
+        key1: 1
+        key2: 1
+    twoWay:
+      simpleMap:
+        $patch: delete
+    modified:
+      simpleMap:
+        {}
   - description: delete all items from merging list
     original:
       mergingList:
@@ -319,6 +331,35 @@ func TestCustomStrategicMergePatch(t *testing.T) {
 //
 var createStrategicMergePatchTestCaseData = []byte(`
 testCases:
+  - description: nil original
+    twoWay:
+      name: 1
+      value: 1
+    modified:
+      name: 1
+      value: 1
+    current:
+      name: 1
+      other: a
+    threeWay:
+      value: 1
+    result:
+      name: 1
+      value: 1
+      other: a
+  - description: nil patch
+    original:
+      name: 1
+    twoWay:
+      {}
+    modified:
+      name: 1
+    current:
+      name: 1
+    threeWay:
+      {}
+    result:
+      name: 1
   - description: add field to map
     original:
       name: 1
@@ -371,6 +412,23 @@ testCases:
     result:
       value: 1
       other: a
+  - description: add field and delete field from map with conflict
+    original:
+      name: 1
+    twoWay:
+      name: null
+      value: 1
+    modified:
+      value: 1
+    current:
+      name: a
+      other: a
+    threeWay:
+      name: null
+      value: 1
+    result:
+      value: 1
+      other: a
   - description: delete field from nested map
     original:
       simpleMap:
@@ -435,6 +493,23 @@ testCases:
       value: null
     result: 
       other: a
+  - description: delete all fields from map with conflict
+    original:
+      name: 1
+      value: 1
+    twoWay:
+      name: null
+      value: null
+    modified: {}
+    current:
+      name: 1
+      value: a
+      other: a
+    threeWay:
+      name: null
+      value: null
+    result: 
+      other: a
   - description: add field and delete all fields from map
     original:
       name: 1
@@ -447,7 +522,26 @@ testCases:
       other: a
     current:
       name: 1
-      value: a
+      value: 1
+      other: a
+    threeWay:
+      name: null
+      value: null
+    result:
+      other: a
+  - description: add field and delete all fields from map with conflict
+    original:
+      name: 1
+      value: 1
+    twoWay:
+      name: null
+      value: null
+      other: a
+    modified:
+      other: a
+    current:
+      name: 1
+      value: 1
       other: b
     threeWay:
       name: null
@@ -471,6 +565,32 @@ testCases:
     current:
       nonMergingIntList:
         - 1
+        - 2
+    threeWay:
+      nonMergingIntList:
+        - 2
+        - 3
+    result:
+      nonMergingIntList:
+        - 2
+        - 3
+  - description: replace list of scalars with conflict
+    original:
+      nonMergingIntList:
+        - 1
+        - 2
+    twoWay:
+      nonMergingIntList:
+        - 2
+        - 3
+    modified:
+      nonMergingIntList:
+        - 2
+        - 3
+    current:
+      nonMergingIntList:
+        - 1
+        - 4
     threeWay:
       nonMergingIntList:
         - 2
@@ -543,6 +663,45 @@ testCases:
           other: b
         - name: 3
           value: 3
+  - description: merge lists of maps with conflict
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    twoWay:
+      mergingList:
+        - name: 3
+          value: 3
+    modified:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+        - name: 3
+          value: 3
+    current:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          value: 3
+          other: b
+    threeWay:
+      mergingList:
+        - name: 2
+          value: 2
+        - name: 3
+          value: 3
+    result:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          value: 2
+          other: b
+        - name: 3
+          value: 3
   - description: add field to map in merging list
     original:
       mergingList:
@@ -578,6 +737,45 @@ testCases:
         - name: 2
           value: 2
           other: b
+  - description: add field to map in merging list with conflict
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          value: 1
+    modified:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    current:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 3
+          value: 2
+          other: b
+    threeWay:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    result:
+      mergingList:
+        - name: 1
+          value: 1
+          other: a
+        - name: 2
+          value: 2
+        - name: 3
+          value: 2
+          other: b
   - description: add duplicate field to map in merging list
     original:
       mergingList:
@@ -612,6 +810,42 @@ testCases:
         - name: 2
           value: 2
           other: b
+  - description: add duplicate field to map in merging list with conflict
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          value: 1
+    modified:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    current:
+      mergingList:
+        - name: 1
+          value: 1
+          other: a
+        - name: 2
+          value: 3
+          other: b
+    threeWay:
+      mergingList:
+        - name: 2
+          value: 2
+    result:
+      mergingList:
+        - name: 1
+          value: 1
+          other: a
+        - name: 2
+          value: 2
+          other: b
   - description: replace map field value in merging list
     original:
       mergingList:
@@ -669,17 +903,15 @@ testCases:
     current:
       mergingList:
         - name: 1
-          value: 1
+          value: 3
           other: a
         - name: 2
-          value: b
+          value: 2
           other: b
     threeWay:
       mergingList:
         - name: 1
           value: a
-        - name: 2
-          value: 2
     result:
       mergingList:
         - name: 1
@@ -689,6 +921,31 @@ testCases:
           value: 2
           other: b
   - description: delete map from merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          $patch: delete
+    modified:
+      mergingList:
+        - name: 2
+    current:
+      mergingList:
+        - name: 1
+        - name: 2
+          other: b
+    threeWay:
+      mergingList:
+        - name: 1
+          $patch: delete
+    result:
+      mergingList:
+        - name: 2
+          other: b
+  - description: delete map from merging list with conflict
     original:
       mergingList:
         - name: 1
@@ -738,7 +995,7 @@ testCases:
       mergingList:
         - name: 2
           other: b
-  - description: delete map from merging list with conflict
+  - description: delete missing map from merging list with conflict
     original:
       mergingList:
         - name: 1
@@ -752,7 +1009,7 @@ testCases:
         - name: 2
     current:
       mergingList:
-        - name: 1
+        - name: 3
           other: a
     threeWay:
       mergingList:
@@ -762,6 +1019,8 @@ testCases:
     result:
       mergingList:
         - name: 2
+        - name: 3
+          other: a
   - description: add map and delete map from merging list
     original:
       merginglist:
@@ -779,7 +1038,6 @@ testCases:
     current:
       merginglist:
         - name: 1
-          other: a
         - name: 2
           other: b
         - name: 4
@@ -844,9 +1102,7 @@ testCases:
     current:
       mergingList:
         - name: 1
-          other: a
         - name: 2
-          other: b
     threeWay:
       mergingList:
         - name: 1
@@ -855,7 +1111,7 @@ testCases:
           $patch: delete
     result:
       mergingList: []
-  - description: delete all maps from partially empty merging list
+  - description: delete all maps from merging list with conflict
     original:
       mergingList:
         - name: 1
@@ -872,6 +1128,8 @@ testCases:
       mergingList:
         - name: 1
           other: a
+        - name: 2
+          other: b
     threeWay:
       mergingList:
         - name: 1
@@ -925,13 +1183,12 @@ testCases:
           value: 1
           other: a
         - name: 2
+          value: 2
           other: b
     threeWay:
       mergingList:
         - name: 1
           value: null
-        - name: 2
-          value: 2
     result:
       mergingList:
         - name: 1
@@ -960,6 +1217,74 @@ testCases:
         - name: 1
           value: a
           other: a
+        - name: 2
+          value: 2
+    threeWay:
+      mergingList:
+        - name: 1
+          value: null
+    result:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          value: 2
+  - description: delete missing field from map in merging list
+    original:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          value: null
+    modified:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    current:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          value: 2
+          other: b
+    threeWay:
+      mergingList:
+        - name: 1
+          value: null
+    result:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          value: 2
+          other: b
+  - description: delete missing field from map in merging list with conflict
+    original:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          value: null
+    modified:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    current:
+      mergingList:
+        - name: 1
+          other: a
+        - name: 2
+          other: b
     threeWay:
       mergingList:
         - name: 1
@@ -972,6 +1297,7 @@ testCases:
           other: a
         - name: 2
           value: 2
+          other: b
   - description: replace non merging list nested in merging list
     original:
       mergingList:
@@ -1019,6 +1345,98 @@ testCases:
               value: 1
         - name: 2
           other: b
+  - description: replace non merging list nested in merging list with value conflict
+    original:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+    modified:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+        - name: 2
+    current:
+      mergingList:
+        - name: 1
+          other: a
+          nonMergingList:
+            - name: 1
+              value: c
+        - name: 2
+          other: b
+    threeWay:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+    result:
+      mergingList:
+        - name: 1
+          other: a
+          nonMergingList:
+            - name: 1
+              value: 1
+        - name: 2
+          other: b
+  - description: replace non merging list nested in merging list with deletion conflict
+    original:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    twoWay:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+    modified:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+        - name: 2
+    current:
+      mergingList:
+        - name: 1
+          other: a
+          nonMergingList:
+            - name: 2
+              value: 2
+        - name: 2
+          other: b
+    threeWay:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+    result:
+      mergingList:
+        - name: 1
+          other: a
+          nonMergingList:
+            - name: 1
+              value: 1
+        - name: 2
+          other: b
   - description: add field to map in merging list nested in merging list
     original:
       mergingList:
@@ -1101,8 +1519,10 @@ testCases:
           mergingList:
             - name: 1
               value: a
+              other: c
             - name: 2
               value: b
+              other: d
         - name: 2
           other: b
     threeWay:
@@ -1120,8 +1540,10 @@ testCases:
           mergingList:
             - name: 1
               value: 1
+              other: c
             - name: 2
               value: 2
+              other: d
         - name: 2
           other: b
   - description: add field to map in merging list nested in merging list with deletion conflict
@@ -1155,6 +1577,7 @@ testCases:
           mergingList:
             - name: 2
               value: 2
+              other: d
         - name: 2
           other: b
     threeWay:
@@ -1172,6 +1595,7 @@ testCases:
               value: 1
             - name: 2
               value: 2
+              other: d
         - name: 2
           other: b
   - description: merge empty merging lists
@@ -1212,6 +1636,29 @@ testCases:
           other: a
         - name: 2
         - name: 3
+  - description: add map to merging list by pointer with conflict
+    original:
+      mergeItemPtr:
+        - name: 1
+    twoWay:
+      mergeItemPtr:
+        - name: 2
+    modified:
+      mergeItemPtr:
+        - name: 1
+        - name: 2
+    current:
+      mergeItemPtr:
+        - name: 3
+    threeWay:
+      mergeItemPtr:
+        - name: 1
+        - name: 2
+    result:
+      mergeItemPtr:
+        - name: 1
+        - name: 2
+        - name: 3
   - description: add field to map in merging list by pointer
     original:
       mergeItemPtr:
@@ -1267,9 +1714,72 @@ testCases:
               other: b
         - name: 2
           other: b
+  - description: add field to map in merging list by pointer with conflict
+    original:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    twoWay:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+              value: 1
+    modified:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+              value: 1
+            - name: 2
+              value: 2
+        - name: 2
+    current:
+      mergeItemPtr:
+        - name: 1
+          other: a
+          mergeItemPtr:
+            - name: 1
+              value: a
+            - name: 2
+              value: 2
+              other: b
+        - name: 2
+          other: b
+    threeWay:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+              value: 1
+    result:
+      mergeItemPtr:
+        - name: 1
+          other: a
+          mergeItemPtr:
+            - name: 1
+              value: 1
+            - name: 2
+              value: 2
+              other: b
+        - name: 2
+          other: b
 `)
 
 func TestStrategicMergePatch(t *testing.T) {
+	testStrategicMergePatchWithCustomArguments(t, "bad original",
+		"<THIS IS NOT JSON>", "{}", mergeItem, errBadJSONDoc)
+	testStrategicMergePatchWithCustomArguments(t, "bad patch",
+		"{}", "<THIS IS NOT JSON>", mergeItem, errBadJSONDoc)
+	testStrategicMergePatchWithCustomArguments(t, "bad struct",
+		"{}", "{}", []byte("<THIS IS NOT A STRUCT>"), fmt.Errorf(errBadArgTypeFmt, "struct", "slice"))
+	testStrategicMergePatchWithCustomArguments(t, "nil struct",
+		"{}", "{}", nil, fmt.Errorf(errBadArgTypeFmt, "struct", "nil"))
+
 	tc := StrategicMergePatchTestCases{}
 	err := yaml.Unmarshal(createStrategicMergePatchTestCaseData, &tc)
 	if err != nil {
@@ -1283,13 +1793,29 @@ func TestStrategicMergePatch(t *testing.T) {
 	}
 }
 
+func testStrategicMergePatchWithCustomArguments(t *testing.T, description, original, patch string, dataStruct interface{}, err error) {
+	_, err2 := StrategicMergePatch([]byte(original), []byte(patch), dataStruct)
+	if err2 != err {
+		if err2 == nil {
+			t.Errorf("expected error: %s\ndid not occur in test case: %s", err, description)
+			return
+		}
+
+		if err == nil || err2.Error() != err.Error() {
+			t.Errorf("unexpected error: %s\noccurred in test case: %s", err2, description)
+			return
+		}
+	}
+}
+
 func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
 	original, expected, modified := twoWayTestCaseToJSONOrFail(t, c)
 
 	actual, err := CreateTwoWayMergePatch(original, modified, mergeItem)
 	if err != nil {
-		t.Errorf("error: %s in test case: %s\ncannot create two way patch:%s:\n%s\n",
+		t.Errorf("error: %s\nin test case: %s\ncannot create two way patch: %s:\n%s\n",
 			err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+		return
 	}
 
 	testPatchCreation(t, expected, actual, c.Description)
@@ -1304,25 +1830,39 @@ func twoWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]
 
 func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
 	original, modified, current, expected, result := threeWayTestCaseToJSONOrFail(t, c)
-
 	actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, false)
 	if err != nil {
-		if IsConflict(err) {
+		if !IsConflict(err) {
+			t.Errorf("error: %s\nin test case: %s\ncannot create three way patch:\n%s\n",
+				err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+			return
+		}
+
+		if !strings.Contains(c.Description, "conflict") {
+			t.Errorf("unexpected conflict: %s\nin test case: %s\ncannot create three way patch:\n%s\n",
+				err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+			return
+		}
+
 		if len(c.Result) > 0 {
-				t.Errorf("error in test case: %s\nunexpected conflict occurred:\n%s\n",
+			actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, true)
-					c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+			if err != nil {
+				t.Errorf("error: %s\nin test case: %s\ncannot force three way patch application:\n%s\n",
+					err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+				return
+			}
+
+			testPatchCreation(t, expected, actual, c.Description)
+			testPatchApplication(t, current, actual, result, c.Description)
 		}
 
 		return
 	}
 
-		t.Errorf("error: %s in test case: %s\ncannot create three way patch:\n%s\n",
+	if strings.Contains(c.Description, "conflict") || len(c.Result) < 1 {
-			err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
-	}
-
-	if len(c.Result) < 1 {
 		t.Errorf("error in test case: %s\nexpected conflict did not occur:\n%s\n",
 			c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+		return
 	}
 
 	testPatchCreation(t, expected, actual, c.Description)
@@ -1340,27 +1880,31 @@ func threeWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) (
 func testPatchCreation(t *testing.T, expected, actual []byte, description string) {
 	sorted, err := sortMergeListsByName(actual, mergeItem)
 	if err != nil {
-		t.Errorf("error: %s in test case: %s\ncannot sort patch:\n%s\n",
+		t.Errorf("error: %s\nin test case: %s\ncannot sort patch:\n%s\n",
 			err, description, jsonToYAMLOrError(actual))
+		return
 	}
 
 	if !reflect.DeepEqual(sorted, expected) {
 		t.Errorf("error in test case: %s\nexpected patch:\n%s\ngot:\n%s\n",
 			description, jsonToYAMLOrError(expected), jsonToYAMLOrError(sorted))
+		return
 	}
 }
 
 func testPatchApplication(t *testing.T, original, patch, expected []byte, description string) {
 	result, err := StrategicMergePatch(original, patch, mergeItem)
 	if err != nil {
-		t.Errorf("error: %s in test case: %s\ncannot apply patch:\n%s\nto original:\n%s\n",
+		t.Errorf("error: %s\nin test case: %s\ncannot apply patch:\n%s\nto original:\n%s\n",
 			err, description, jsonToYAMLOrError(patch), jsonToYAMLOrError(original))
+		return
 	}
 
 	sorted, err := sortMergeListsByName(result, mergeItem)
 	if err != nil {
-		t.Errorf("error: %s in test case: %s\ncannot sort result object:\n%s\n",
+		t.Errorf("error: %s\nin test case: %s\ncannot sort result object:\n%s\n",
 			err, description, jsonToYAMLOrError(result))
+		return
 	}
 
 	if !reflect.DeepEqual(sorted, expected) {
@@ -1368,10 +1912,15 @@ func testPatchApplication(t *testing.T, original, patch, expected []byte, descri
 		t.Errorf(format, description,
 			jsonToYAMLOrError(original), jsonToYAMLOrError(patch),
 			jsonToYAMLOrError(expected), jsonToYAMLOrError(sorted))
+		return
 	}
 }
 
 func testObjectToJSONOrFail(t *testing.T, o map[string]interface{}, description string) []byte {
+	if o == nil {
+		return nil
+	}
+
 	j, err := toJSON(o)
 	if err != nil {
 		t.Error(err)
@@ -1379,21 +1928,13 @@ func testObjectToJSONOrFail(t *testing.T, o map[string]interface{}, description
 
 	r, err := sortMergeListsByName(j, mergeItem)
 	if err != nil {
-		t.Errorf("error: %s in test case: %s\ncannot sort object:\n%s\n", err, description, j)
+		t.Errorf("error: %s\nin test case: %s\ncannot sort object:\n%s\n", err, description, j)
+		return nil
 	}
 
 	return r
 }
 
-func toYAMLOrError(v interface{}) string {
-	y, err := toYAML(v)
-	if err != nil {
-		return err.Error()
-	}
-
-	return y
-}
-
 func jsonToYAMLOrError(j []byte) string {
 	y, err := jsonToYAML(j)
 	if err != nil {
@@ -1403,15 +1944,6 @@ func jsonToYAMLOrError(j []byte) string {
 	return string(y)
 }
 
-func toYAML(v interface{}) (string, error) {
-	y, err := yaml.Marshal(v)
-	if err != nil {
-		return "", fmt.Errorf("yaml marshal failed:%v\n%v\n", err, spew.Sdump(v))
-	}
-
-	return string(y), nil
-}
-
 func toJSON(v interface{}) ([]byte, error) {
 	j, err := json.Marshal(v)
 	if err != nil {