Merge pull request #13007 from jackgr/diff_and_patch

Auto commit by PR queue bot

pkg/util/strategicpatch/patch.go

@@ -31,28 +31,357 @@ import (
 // lists should be merged or replaced.
 //
 // For more information, see the PATCH section of docs/api-conventions.md.
-func StrategicMergePatchData(original, patch []byte, dataStruct interface{}) ([]byte, error) {
+//
-	var o map[string]interface{}
+// Some of the content of this package was borrowed with minor adaptations from
-	err := json.Unmarshal(original, &o)
+// evanphx/json-patch and openshift/origin.
+
+const specialKey = "$patch"
+const specialValue = "delete"
+
+var errBadJSONDoc = fmt.Errorf("Invalid JSON document")
+var errNoListOfLists = fmt.Errorf("Lists of lists are not supported")
+
+// CreateStrategicMergePatch creates a patch that can be passed to StrategicMergePatch.
+// The original and modified documents must be passed to the method as json encoded content.
+// It will return a mergeable json document with differences from original to modified, or an error
+// if either of the two documents is invalid.
+func CreateStrategicMergePatch(original, modified []byte, dataStruct interface{}) ([]byte, error) {
+	originalMap := map[string]interface{}{}
+	err := json.Unmarshal(original, &originalMap)
+	if err != nil {
+		return nil, errBadJSONDoc
+	}
+
+	modifiedMap := map[string]interface{}{}
+	err = json.Unmarshal(modified, &modifiedMap)
+	if err != nil {
+		return nil, errBadJSONDoc
+	}
+
+	t, err := getTagStructType(dataStruct)
 	if err != nil {
 		return nil, err
 	}
 
-	var p map[string]interface{}
+	patchMap, err := diffMaps(originalMap, modifiedMap, t, false, false)
-	err = json.Unmarshal(patch, &p)
 	if err != nil {
 		return nil, err
 	}
 
-	t := reflect.TypeOf(dataStruct)
+	return json.Marshal(patchMap)
+}
+
+// Returns a (recursive) strategic merge patch that yields modified when applied to original.
+func diffMaps(original, modified map[string]interface{}, t reflect.Type, ignoreAdditions, ignoreChangesAndDeletions bool) (map[string]interface{}, error) {
+	patch := map[string]interface{}{}
 	if t.Kind() == reflect.Ptr {
 		t = t.Elem()
 	}
-	if t.Kind() != reflect.Struct {
+
-		return nil, fmt.Errorf("strategic merge patch needs a struct, %s received instead", t.Kind().String())
+	for key, modifiedValue := range modified {
+		originalValue, ok := original[key]
+		// value was added
+		if !ok {
+			if !ignoreAdditions {
+				patch[key] = modifiedValue
+			}
+
+			continue
+		}
+
+		if key == specialKey {
+			originalString, ok := originalValue.(string)
+			if !ok {
+				return nil, fmt.Errorf("invalid value for special key: %s", specialKey)
+			}
+
+			modifiedString, ok := modifiedValue.(string)
+			if !ok {
+				return nil, fmt.Errorf("invalid value for special key: %s", specialKey)
+			}
+
+			if modifiedString != originalString {
+				patch[key] = modifiedValue
+			}
+
+			continue
+		}
+
+		if !ignoreChangesAndDeletions {
+			// If types have changed, replace completely
+			if reflect.TypeOf(originalValue) != reflect.TypeOf(modifiedValue) {
+				patch[key] = modifiedValue
+				continue
+			}
+		}
+
+		// Types are the same, compare values
+		switch originalValueTyped := originalValue.(type) {
+		case map[string]interface{}:
+			modifiedValueTyped := modifiedValue.(map[string]interface{})
+			fieldType, _, _, err := forkedjson.LookupPatchMetadata(t, key)
+			if err != nil {
+				return nil, err
+			}
+
+			patchValue, err := diffMaps(originalValueTyped, modifiedValueTyped, fieldType, ignoreAdditions, ignoreChangesAndDeletions)
+			if err != nil {
+				return nil, err
+			}
+
+			if len(patchValue) > 0 {
+				patch[key] = patchValue
+			}
+
+			continue
+		case []interface{}:
+			modifiedValueTyped := modifiedValue.([]interface{})
+			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, key)
+			if err != nil {
+				return nil, err
+			}
+
+			if fieldPatchStrategy == "merge" {
+				patchValue, err := diffLists(originalValueTyped, modifiedValueTyped, fieldType.Elem(), fieldPatchMergeKey, ignoreAdditions, ignoreChangesAndDeletions)
+				if err != nil {
+					return nil, err
+				}
+
+				if len(patchValue) > 0 {
+					patch[key] = patchValue
+				}
+
+				continue
+			}
+		}
+
+		if !ignoreChangesAndDeletions {
+			if !reflect.DeepEqual(originalValue, modifiedValue) {
+				patch[key] = modifiedValue
+			}
+		}
 	}
 
-	result, err := mergeMap(o, p, t)
+	if !ignoreChangesAndDeletions {
+		// Now add all deleted values as nil
+		for key := range original {
+			_, found := modified[key]
+			if !found {
+				patch[key] = nil
+			}
+		}
+	}
+
+	return patch, nil
+}
+
+// Returns a (recursive) strategic merge patch that yields modified when applied to original,
+// for a pair of lists with merge semantics.
+func diffLists(original, modified []interface{}, t reflect.Type, mergeKey string, ignoreAdditions, ignoreChangesAndDeletions bool) ([]interface{}, error) {
+	if len(original) == 0 {
+		if len(modified) == 0 || ignoreAdditions {
+			return nil, nil
+		}
+
+		return modified, nil
+	}
+
+	elementType, err := sliceElementType(original, modified)
+	if err != nil {
+		return nil, err
+	}
+
+	var patch []interface{}
+
+	// If the elements are not maps...
+	if elementType.Kind() == reflect.Map {
+		patch, err = diffListsOfMaps(original, modified, t, mergeKey, ignoreAdditions, ignoreChangesAndDeletions)
+	} else {
+		patch, err = diffListsOfScalars(original, modified, ignoreAdditions)
+	}
+
+	if err != nil {
+		return nil, err
+	}
+
+	return patch, nil
+}
+
+// Returns a (recursive) strategic merge patch that yields modified when applied to original,
+// for a pair of lists of scalars with merge semantics.
+func diffListsOfScalars(original, modified []interface{}, ignoreAdditions bool) ([]interface{}, error) {
+	if len(modified) == 0 {
+		// There is no need to check the length of original because there is no way to create
+		// a patch that deletes a scalar from a list of scalars with merge semantics.
+		return nil, nil
+	}
+
+	patch := []interface{}{}
+
+	originalScalars := uniqifyAndSortScalars(original)
+	modifiedScalars := uniqifyAndSortScalars(modified)
+	originalIndex, modifiedIndex := 0, 0
+
+loopB:
+	for ; modifiedIndex < len(modifiedScalars); modifiedIndex++ {
+		for ; originalIndex < len(originalScalars); originalIndex++ {
+			originalString := fmt.Sprintf("%v", original[originalIndex])
+			modifiedString := fmt.Sprintf("%v", modified[modifiedIndex])
+			if originalString >= modifiedString {
+				if originalString != modifiedString {
+					if !ignoreAdditions {
+						patch = append(patch, modified[modifiedIndex])
+					}
+				}
+
+				continue loopB
+			}
+			// There is no else clause because there is no way to create a patch that deletes
+			// a scalar from a list of scalars with merge semantics.
+		}
+
+		break
+	}
+
+	if !ignoreAdditions {
+		// Add any remaining items found only in modified
+		for ; modifiedIndex < len(modifiedScalars); modifiedIndex++ {
+			patch = append(patch, modified[modifiedIndex])
+		}
+	}
+
+	return patch, nil
+}
+
+var errNoMergeKeyFmt = "map: %v does not contain declared merge key: %s"
+var errBadArgTypeFmt = "expected a %s, but received a %t"
+
+// Returns a (recursive) strategic merge patch that yields modified when applied to original,
+// for a pair of lists of maps with merge semantics.
+func diffListsOfMaps(original, modified []interface{}, t reflect.Type, mergeKey string, ignoreAdditions, ignoreChangesAndDeletions bool) ([]interface{}, error) {
+	patch := make([]interface{}, 0)
+
+	originalSorted, err := sortMergeListsByNameArray(original, t, mergeKey, false)
+	if err != nil {
+		return nil, err
+	}
+
+	modifiedSorted, err := sortMergeListsByNameArray(modified, t, mergeKey, false)
+	if err != nil {
+		return nil, err
+	}
+
+	originalIndex, modifiedIndex := 0, 0
+
+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])
+		}
+
+		modifiedValue, ok := modifiedMap[mergeKey]
+		if !ok {
+			return nil, fmt.Errorf(errNoMergeKeyFmt, modifiedMap, mergeKey)
+		}
+
+		for ; originalIndex < len(originalSorted); originalIndex++ {
+			originalMap, ok := originalSorted[originalIndex].(map[string]interface{})
+			if !ok {
+				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", originalSorted[originalIndex])
+			}
+
+			originalValue, ok := originalMap[mergeKey]
+			if !ok {
+				return nil, fmt.Errorf(errNoMergeKeyFmt, originalMap, mergeKey)
+			}
+
+			// Assume that the merge key values are comparable strings
+			originalString := fmt.Sprintf("%v", originalValue)
+			modifiedString := fmt.Sprintf("%v", modifiedValue)
+			if originalString >= modifiedString {
+				if originalString == modifiedString {
+					patchValue, err := diffMaps(originalMap, modifiedMap, t, ignoreAdditions, ignoreChangesAndDeletions)
+					if err != nil {
+						return nil, err
+					}
+
+					originalIndex++
+					if len(patchValue) > 0 {
+						patchValue[mergeKey] = modifiedValue
+						patch = append(patch, patchValue)
+					}
+				} else if !ignoreAdditions {
+					patch = append(patch, modifiedMap)
+				}
+
+				continue loopB
+			}
+
+			if !ignoreChangesAndDeletions {
+				patch = append(patch, map[string]interface{}{mergeKey: originalValue, specialKey: specialValue})
+			}
+		}
+
+		break
+	}
+
+	if !ignoreChangesAndDeletions {
+		// Delete any remaining items found only in original
+		for ; originalIndex < len(originalSorted); originalIndex++ {
+			originalMap, ok := originalSorted[originalIndex].(map[string]interface{})
+			if !ok {
+				return nil, fmt.Errorf(errBadArgTypeFmt, "map[string]interface{}", originalSorted[originalIndex])
+			}
+
+			originalValue, ok := originalMap[mergeKey]
+			if !ok {
+				return nil, fmt.Errorf(errNoMergeKeyFmt, originalMap, mergeKey)
+			}
+
+			patch = append(patch, map[string]interface{}{mergeKey: originalValue, specialKey: specialValue})
+		}
+	}
+
+	if !ignoreAdditions {
+		// Add any remaining items found only in modified
+		for ; modifiedIndex < len(modifiedSorted); modifiedIndex++ {
+			patch = append(patch, modified[modifiedIndex])
+		}
+	}
+
+	return patch, nil
+}
+
+// StrategicMergePatchData applies a patch using strategic merge patch semantics.
+// Deprecated: StrategicMergePatchData is deprecated. Use the synonym StrategicMergePatch,
+// instead, which follows the naming convention of evanphx/json-patch.
+func StrategicMergePatchData(original, patch []byte, dataStruct interface{}) ([]byte, error) {
+	return StrategicMergePatch(original, patch, dataStruct)
+}
+
+// StrategicMergePatch applies a strategic merge patch. The patch and the original document
+// must be json encoded content. A patch can be created from an original and a modified document
+// by calling CreateStrategicMergePatch.
+func StrategicMergePatch(original, patch []byte, dataStruct interface{}) ([]byte, error) {
+	originalMap := map[string]interface{}{}
+	err := json.Unmarshal(original, &originalMap)
+	if err != nil {
+		return nil, errBadJSONDoc
+	}
+
+	patchMap := map[string]interface{}{}
+	err = json.Unmarshal(patch, &patchMap)
+	if err != nil {
+		return nil, errBadJSONDoc
+	}
+
+	t, err := getTagStructType(dataStruct)
+	if err != nil {
+		return nil, err
+	}
+
+	result, err := mergeMap(originalMap, patchMap, t)
 	if err != nil {
 		return nil, err
 	}
@@ -60,7 +389,20 @@ func StrategicMergePatchData(original, patch []byte, dataStruct interface{}) ([]
 	return json.Marshal(result)
 }
 
-const specialKey = "$patch"
+func getTagStructType(dataStruct interface{}) (reflect.Type, error) {
+	t := reflect.TypeOf(dataStruct)
+	if t.Kind() == reflect.Ptr {
+		t = t.Elem()
+	}
+
+	if t.Kind() != reflect.Struct {
+		return nil, fmt.Errorf(errBadArgTypeFmt, "struct", t.Kind().String())
+	}
+
+	return t, nil
+}
+
+var errBadPatchTypeFmt = "unknown patch type: %s in map: %v"
 
 // Merge fields from a patch map into the original map. Note: This may modify
 // both the original map and the patch because getting a deep copy of a map in
@@ -74,7 +416,8 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type) (map[strin
 			delete(patch, specialKey)
 			return patch, nil
 		}
-		return nil, fmt.Errorf("unknown patch type found: %s", v)
+
+		return nil, fmt.Errorf(errBadPatchTypeFmt, v, patch)
 	}
 
 	// nil is an accepted value for original to simplify logic in other places.
@@ -92,29 +435,32 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type) (map[strin
 			if _, ok := original[k]; ok {
 				delete(original, k)
 			}
+
 			continue
 		}
+
 		_, ok := original[k]
 		if !ok {
 			// If it's not in the original document, just take the patch value.
 			original[k] = patchV
 			continue
 		}
+
 		// If the data type is a pointer, resolve the element.
 		if t.Kind() == reflect.Ptr {
 			t = t.Elem()
 		}
 
 		// If they're both maps or lists, recurse into the value.
-		// First find the fieldPatchStrategy and fieldPatchMergeKey.
-		fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
-		if err != nil {
-			return nil, err
-		}
-
 		originalType := reflect.TypeOf(original[k])
 		patchType := reflect.TypeOf(patchV)
 		if originalType == patchType {
+			// First find the fieldPatchStrategy and fieldPatchMergeKey.
+			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+			if err != nil {
+				return nil, err
+			}
+
 			if originalType.Kind() == reflect.Map && fieldPatchStrategy != "replace" {
 				typedOriginal := original[k].(map[string]interface{})
 				typedPatch := patchV.(map[string]interface{})
@@ -123,8 +469,10 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type) (map[strin
 				if err != nil {
 					return nil, err
 				}
+
 				continue
 			}
+
 			if originalType.Kind() == reflect.Slice && fieldPatchStrategy == "merge" {
 				elemType := fieldType.Elem()
 				typedOriginal := original[k].([]interface{})
@@ -134,6 +482,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type) (map[strin
 				if err != nil {
 					return nil, err
 				}
+
 				continue
 			}
 		}
@@ -154,15 +503,13 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 	if len(original) == 0 && len(patch) == 0 {
 		return original, nil
 	}
+
 	// All the values must be of the same type, but not a list.
 	t, err := sliceElementType(original, patch)
 	if err != nil {
-		return nil, fmt.Errorf("types of list elements need to be the same, type: %s: %v",
+		return nil, err
-			elemType.Kind().String(), err)
-	}
-	if t.Kind() == reflect.Slice {
-		return nil, fmt.Errorf("not supporting merging lists of lists yet")
 	}
+
 	// If the elements are not maps, merge the slices of scalars.
 	if t.Kind() != reflect.Map {
 		// Maybe in the future add a "concat" mode that doesn't
@@ -182,10 +529,14 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 		typedV := v.(map[string]interface{})
 		patchType, ok := typedV[specialKey]
 		if ok {
-			if patchType == "delete" {
+			if patchType == specialValue {
 				mergeValue, ok := typedV[mergeKey]
 				if ok {
-					_, originalKey, found := findMapInSliceBasedOnKeyValue(original, mergeKey, mergeValue)
+					_, originalKey, found, err := findMapInSliceBasedOnKeyValue(original, mergeKey, mergeValue)
+					if err != nil {
+						return nil, err
+					}
+
 					if found {
 						// Delete the element at originalKey.
 						original = append(original[:originalKey], original[originalKey+1:]...)
@@ -199,7 +550,7 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 			} else if patchType == "merge" {
 				return nil, fmt.Errorf("merging lists cannot yet be specified in the patch")
 			} else {
-				return nil, fmt.Errorf("unknown patch type found: %s", patchType)
+				return nil, fmt.Errorf(errBadPatchTypeFmt, patchType, typedV)
 			}
 		} else {
 			patchWithoutSpecialElements = append(patchWithoutSpecialElements, v)
@@ -218,11 +569,16 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 		typedV := v.(map[string]interface{})
 		mergeValue, ok := typedV[mergeKey]
 		if !ok {
-			return nil, fmt.Errorf("all list elements need the merge key %s", mergeKey)
+			return nil, fmt.Errorf(errNoMergeKeyFmt, typedV, mergeKey)
 		}
+
 		// If we find a value with this merge key value in original, merge the
 		// maps. Otherwise append onto original.
-		originalMap, originalKey, found := findMapInSliceBasedOnKeyValue(original, mergeKey, mergeValue)
+		originalMap, originalKey, found, err := findMapInSliceBasedOnKeyValue(original, mergeKey, mergeValue)
+		if err != nil {
+			return nil, err
+		}
+
 		if found {
 			var mergedMaps interface{}
 			var err error
@@ -231,6 +587,7 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 			if err != nil {
 				return nil, err
 			}
+
 			original[originalKey] = mergedMaps
 		} else {
 			original = append(original, v)
@@ -240,16 +597,21 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 	return original, nil
 }
 
-// This panics if any element of the slice is not a map.
+// This method no longer panics if any element of the slice is not a map.
-func findMapInSliceBasedOnKeyValue(m []interface{}, key string, value interface{}) (map[string]interface{}, int, bool) {
+func findMapInSliceBasedOnKeyValue(m []interface{}, key string, value interface{}) (map[string]interface{}, int, bool, error) {
 	for k, v := range m {
-		typedV := v.(map[string]interface{})
+		typedV, ok := v.(map[string]interface{})
+		if !ok {
+			return nil, 0, false, fmt.Errorf("value for key %v is not a map.", k)
+		}
+
 		valueToMatch, ok := typedV[key]
 		if ok && valueToMatch == value {
-			return typedV, k, true
+			return typedV, k, true, nil
 		}
 	}
-	return nil, 0, false
+
+	return nil, 0, false, nil
 }
 
 // This function takes a JSON map and sorts all the lists that should be merged
@@ -274,43 +636,46 @@ func sortMergeListsByName(mapJSON []byte, dataStruct interface{}) ([]byte, error
 func sortMergeListsByNameMap(s map[string]interface{}, t reflect.Type) (map[string]interface{}, error) {
 	newS := map[string]interface{}{}
 	for k, v := range s {
-		fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+		if k != specialKey {
-		if err != nil {
+			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
-			return nil, err
-		}
-		// If v is a map or a merge slice, recurse.
-		if typedV, ok := v.(map[string]interface{}); ok {
-			var err error
-			v, err = sortMergeListsByNameMap(typedV, fieldType)
 			if err != nil {
 				return nil, err
 			}
-		} else if typedV, ok := v.([]interface{}); ok {
+
-			if fieldPatchStrategy == "merge" {
+			// If v is a map or a merge slice, recurse.
+			if typedV, ok := v.(map[string]interface{}); ok {
 				var err error
-				v, err = sortMergeListsByNameArray(typedV, fieldType.Elem(), fieldPatchMergeKey)
+				v, err = sortMergeListsByNameMap(typedV, fieldType)
 				if err != nil {
 					return nil, err
 				}
+			} else if typedV, ok := v.([]interface{}); ok {
+				if fieldPatchStrategy == "merge" {
+					var err error
+					v, err = sortMergeListsByNameArray(typedV, fieldType.Elem(), fieldPatchMergeKey, true)
+					if err != nil {
+						return nil, err
+					}
+				}
 			}
 		}
+
 		newS[k] = v
 	}
+
 	return newS, nil
 }
 
-func sortMergeListsByNameArray(s []interface{}, elemType reflect.Type, mergeKey string) ([]interface{}, error) {
+func sortMergeListsByNameArray(s []interface{}, elemType reflect.Type, mergeKey string, recurse bool) ([]interface{}, error) {
 	if len(s) == 0 {
 		return s, nil
 	}
+
 	// We don't support lists of lists yet.
 	t, err := sliceElementType(s)
 	if err != nil {
 		return nil, err
 	}
-	if t.Kind() == reflect.Slice {
-		return nil, fmt.Errorf("not supporting lists of lists yet")
-	}
 
 	// If the elements are not maps...
 	if t.Kind() != reflect.Map {
@@ -319,15 +684,20 @@ func sortMergeListsByNameArray(s []interface{}, elemType reflect.Type, mergeKey
 	}
 
 	// Elements are maps - if one of the keys of the map is a map or a
-	// list, we need to recurse into it.
+	// list, we may need to recurse into it.
 	newS := []interface{}{}
 	for _, elem := range s {
-		typedElem := elem.(map[string]interface{})
+		if recurse {
-		newElem, err := sortMergeListsByNameMap(typedElem, elemType)
+			typedElem := elem.(map[string]interface{})
-		if err != nil {
+			newElem, err := sortMergeListsByNameMap(typedElem, elemType)
-			return nil, err
+			if err != nil {
+				return nil, err
+			}
+
+			newS = append(newS, newElem)
+		} else {
+			newS = append(newS, elem)
 		}
-		newS = append(newS, newElem)
 	}
 
 	// Sort the maps.
@@ -336,19 +706,32 @@ func sortMergeListsByNameArray(s []interface{}, elemType reflect.Type, mergeKey
 }
 
 func sortMapsBasedOnField(m []interface{}, fieldName string) []interface{} {
-	mapM := []map[string]interface{}{}
+	mapM := mapSliceFromSlice(m)
-	for _, v := range m {
-		mapM = append(mapM, v.(map[string]interface{}))
-	}
 	ss := SortableSliceOfMaps{mapM, fieldName}
 	sort.Sort(ss)
-	newM := []interface{}{}
+	newS := sliceFromMapSlice(ss.s)
-	for _, v := range ss.s {
+	return newS
-		newM = append(newM, v)
+}
+
+func mapSliceFromSlice(m []interface{}) []map[string]interface{} {
+	newM := []map[string]interface{}{}
+	for _, v := range m {
+		vt := v.(map[string]interface{})
+		newM = append(newM, vt)
 	}
+
 	return newM
 }
 
+func sliceFromMapSlice(s []map[string]interface{}) []interface{} {
+	newS := []interface{}{}
+	for _, v := range s {
+		newS = append(newS, v)
+	}
+
+	return newS
+}
+
 type SortableSliceOfMaps struct {
 	s []map[string]interface{}
 	k string // key to sort on
@@ -391,6 +774,7 @@ func uniqifyScalars(s []interface{}) []interface{} {
 			}
 		}
 	}
+
 	return s
 }
 
@@ -415,7 +799,7 @@ func (ss SortableSliceOfScalars) Swap(i, j int) {
 }
 
 // Returns the type of the elements of N slice(s). If the type is different,
-// returns an error.
+// another slice or undefined, returns an error.
 func sliceElementType(slices ...[]interface{}) (reflect.Type, error) {
 	var prevType reflect.Type
 	for _, s := range slices {
@@ -424,16 +808,22 @@ func sliceElementType(slices ...[]interface{}) (reflect.Type, error) {
 			currentType := reflect.TypeOf(v)
 			if prevType == nil {
 				prevType = currentType
+				// We don't support lists of lists yet.
+				if prevType.Kind() == reflect.Slice {
+					return nil, errNoListOfLists
+				}
 			} else {
 				if prevType != currentType {
-					return nil, fmt.Errorf("at least two types found: %s and %s", prevType, currentType)
+					return nil, fmt.Errorf("list element types are not identical: %v", fmt.Sprint(slices))
 				}
 				prevType = currentType
 			}
 		}
 	}
+
 	if prevType == nil {
-		return nil, fmt.Errorf("no elements in any given slices")
+		return nil, fmt.Errorf("no elements in any of the given slices")
 	}
+
 	return prevType, nil
 }

pkg/util/strategicpatch/patch_test.go

@@ -26,19 +26,26 @@ import (
 	"github.com/ghodss/yaml"
 )
 
-type TestCases struct {
+type StrategicMergePatchTestCases struct {
-	StrategicMergePatchCases []StrategicMergePatchCase
+	TestCases []StrategicMergePatchTestCase
-	SortMergeListTestCases   []SortMergeListCase
 }
 
-type StrategicMergePatchCase struct {
+type SortMergeListTestCases struct {
+	TestCases []SortMergeListTestCase
+}
+
+type StrategicMergePatchTestCaseData struct {
+	Original map[string]interface{}
+	Patch    map[string]interface{}
+	Modified map[string]interface{}
+}
+
+type StrategicMergePatchTestCase struct {
 	Description string
-	Patch       map[string]interface{}
+	StrategicMergePatchTestCaseData
-	Original    map[string]interface{}
-	Result      map[string]interface{}
 }
 
-type SortMergeListCase struct {
+type SortMergeListTestCase struct {
 	Description string
 	Original    map[string]interface{}
 	Sorted      map[string]interface{}
@@ -47,6 +54,7 @@ type SortMergeListCase struct {
 type MergeItem struct {
 	Name              string
 	Value             string
+	Other             string
 	MergingList       []MergeItem `patchStrategy:"merge" patchMergeKey:"name"`
 	NonMergingList    []MergeItem
 	MergingIntList    []int `patchStrategy:"merge"`
@@ -55,220 +63,10 @@ type MergeItem struct {
 	SimpleMap         map[string]string
 }
 
-var testCaseData = []byte(`
+// These are test cases for SortMergeList, used to assert that it (recursively)
-strategicMergePatchCases:
+// sorts both merging and non merging lists correctly.
-  - description: add new field
+var sortMergeListTestCaseData = []byte(`
-    original:
+testCases:
-      name: 1
-    patch:
-      value: 1
-    result:
-      name: 1
-      value: 1
-  - description: remove field and add new field
-    original:
-      name: 1
-    patch:
-      name: null
-      value: 1
-    result:
-      value: 1
-  - description: merge arrays of scalars
-    original:
-      mergingIntList:
-        - 1
-        - 2
-    patch:
-      mergingIntList:
-        - 2
-        - 3
-    result:
-      mergingIntList:
-        - 1
-        - 2
-        - 3
-  - description: replace arrays of scalars
-    original:
-      nonMergingIntList:
-        - 1
-        - 2
-    patch:
-      nonMergingIntList:
-        - 2
-        - 3
-    result:
-      nonMergingIntList:
-        - 2
-        - 3
-  - description: update param of list that should be merged but had element added serverside
-    original:
-      mergingList:
-        - name: 1
-          value: 1
-        - name: 2
-          value: 2
-    patch:
-      mergingList:
-        - name: 1
-          value: a
-    result:
-      mergingList:
-        - name: 1
-          value: a
-        - name: 2
-          value: 2
-  - description: delete field when field is nested in a map
-    original:
-      simpleMap:
-        key1: 1
-        key2: 1
-    patch:
-      simpleMap:
-        key2: null
-    result:
-      simpleMap:
-        key1: 1
-  - description: update nested list when nested list should not be merged
-    original:
-      mergingList:
-        - name: 1
-          nonMergingList:
-            - name: 1
-            - name: 2
-              value: 2
-        - name: 2
-    patch:
-      mergingList:
-        - name: 1
-          nonMergingList:
-            - name: 1
-              value: 1
-    result:
-      mergingList:
-        - name: 1
-          nonMergingList:
-            - name: 1
-              value: 1
-        - name: 2
-  - description: update nested list when nested list should be merged
-    original:
-      mergingList:
-        - name: 1
-          mergingList:
-            - name: 1
-            - name: 2
-              value: 2
-        - name: 2
-    patch:
-      mergingList:
-        - name: 1
-          mergingList:
-            - name: 1
-              value: 1
-    result:
-      mergingList:
-        - name: 1
-          mergingList:
-            - name: 1
-              value: 1
-            - name: 2
-              value: 2
-        - name: 2
-  - description: update map when map should be replaced
-    original:
-      name: 1
-      value: 1
-    patch:
-      value: 1
-      $patch: replace
-    result:
-      value: 1
-  - description: merge empty merge lists
-    original:
-      mergingList: []
-    patch:
-      mergingList: []
-    result:
-      mergingList: []
-  - description: delete others in a map
-    original:
-      name: 1
-      value: 1
-    patch:
-      $patch: replace
-    result: {}
-  - description: delete item from a merge list
-    original:
-      mergingList:
-        - name: 1
-        - name: 2
-    patch:
-      mergingList:
-        - $patch: delete
-          name: 1
-    result:
-      mergingList:
-        - name: 2
-  - description: add and delete item from a merge list
-    original:
-      merginglist:
-        - name: 1
-        - name: 2
-    patch:
-      merginglist:
-        - name: 3
-        - $patch: delete
-          name: 1
-    result:
-      merginglist:
-        - name: 2
-        - name: 3
-  - description: delete all items from a merge list
-    original:
-      mergingList:
-        - name: 1
-        - name: 2
-    patch:
-      mergingList:
-        - $patch: replace
-    result:
-      mergingList: []
-  - description: add new field inside pointers
-    original:
-      mergeItemPtr:
-        - name: 1
-    patch:
-      mergeItemPtr:
-        - name: 2
-    result:
-      mergeItemPtr:
-        - name: 1
-        - name: 2
-  - description: update nested pointers
-    original:
-      mergeItemPtr:
-        - name: 1
-          mergeItemPtr:
-            - name: 1
-            - name: 2
-              value: 2
-        - name: 2
-    patch:
-      mergeItemPtr:
-        - name: 1
-          mergeItemPtr:
-            - name: 1
-              value: 1
-    result:
-      mergeItemPtr:
-        - name: 1
-          mergeItemPtr:
-            - name: 1
-              value: 1
-            - name: 2
-              value: 2
-        - name: 2
-sortMergeListTestCases:
   - description: sort one list of maps
     original:
       mergingList:
@@ -327,7 +125,7 @@ sortMergeListTestCases:
             - name: 1
             - name: 2
             - name: 3
-  - description: merging list should NOT sort when nested in a non merging list
+  - description: merging list should NOT sort when nested in non merging list
     original:
       nonMergingList:
         - name: 2
@@ -350,7 +148,7 @@ sortMergeListTestCases:
           mergingList:
             - name: 2
             - name: 1
-  - description: sort a very nested list of maps
+  - description: sort very nested list of maps
     fieldTypes:
     original:
       mergingList:
@@ -410,7 +208,7 @@ sortMergeListTestCases:
             - 1
             - 2
             - 3
-  - description: sort one pointer of maps
+  - description: sort merging list by pointer
     original:
       mergeItemPtr:
         - name: 1
@@ -423,81 +221,546 @@ sortMergeListTestCases:
         - name: 3
 `)
 
-func TestStrategicMergePatch(t *testing.T) {
-	tc := TestCases{}
-	err := yaml.Unmarshal(testCaseData, &tc)
-	if err != nil {
-		t.Errorf("can't unmarshal test cases: %v", err)
-		return
-	}
-
-	var e MergeItem
-	for _, c := range tc.StrategicMergePatchCases {
-		result, err := StrategicMergePatchData(toJSON(c.Original), toJSON(c.Patch), e)
-		if err != nil {
-			t.Errorf("error patching: %v:\noriginal:\n%s\npatch:\n%s",
-				err, toYAML(c.Original), toYAML(c.Patch))
-		}
-
-		// Sort the lists that have merged maps, since order is not significant.
-		result, err = sortMergeListsByName(result, e)
-		if err != nil {
-			t.Errorf("error sorting result object: %v", err)
-		}
-		cResult, err := sortMergeListsByName(toJSON(c.Result), e)
-		if err != nil {
-			t.Errorf("error sorting result object: %v", err)
-		}
-
-		if !reflect.DeepEqual(result, cResult) {
-			t.Errorf("patching failed: %s\noriginal:\n%s\npatch:\n%s\nexpected result:\n%s\ngot result:\n%s",
-				c.Description, toYAML(c.Original), toYAML(c.Patch), jsonToYAML(cResult), jsonToYAML(result))
-		}
-	}
-}
-
 func TestSortMergeLists(t *testing.T) {
-	tc := TestCases{}
+	tc := SortMergeListTestCases{}
-	err := yaml.Unmarshal(testCaseData, &tc)
+	err := yaml.Unmarshal(sortMergeListTestCaseData, &tc)
 	if err != nil {
 		t.Errorf("can't unmarshal test cases: %v", err)
 		return
 	}
 
 	var e MergeItem
-	for _, c := range tc.SortMergeListTestCases {
+	for _, c := range tc.TestCases {
-		sorted, err := sortMergeListsByName(toJSON(c.Original), e)
+		sorted, err := sortMergeListsByName(toJSONOrFail(c.Original, t), e)
 		if err != nil {
 			t.Errorf("sort arrays returned error: %v", err)
 		}
 
-		if !reflect.DeepEqual(sorted, toJSON(c.Sorted)) {
+		if !reflect.DeepEqual(sorted, toJSONOrFail(c.Sorted, t)) {
-			t.Errorf("sorting failed: %s\ntried to sort:\n%s\nexpected:\n%s\ngot:\n%s",
+			t.Errorf("sorting failed: %v\ntried to sort:\n%vexpected:\n%vgot:\n%v",
-				c.Description, toYAML(c.Original), toYAML(c.Sorted), jsonToYAML(sorted))
+				c.Description, toYAMLOrError(c.Original), toYAMLOrError(c.Sorted), jsonToYAMLOrError(sorted))
 		}
 	}
 }
 
-func toYAML(v interface{}) string {
+// These are test cases for StrategicMergePatch that cannot be generated using
-	y, err := yaml.Marshal(v)
+// CreateStrategicMergePatch because it doesn't use the replace directive, generate
+// duplicate integers for a merging list patch, or generate empty merging lists.
+var customStrategicMergePatchTestCaseData = []byte(`
+testCases:
+  - description: unique scalars when merging lists
+    original:
+      mergingIntList:
+        - 1
+        - 2
+    patch:
+      mergingIntList:
+        - 2
+        - 3
+    modified:
+      mergingIntList:
+        - 1
+        - 2
+        - 3
+  - description: delete all items from merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - $patch: replace
+    modified:
+      mergingList: []
+  - description: merge empty merging lists
+    original:
+      mergingList: []
+    patch:
+      mergingList: []
+    modified:
+      mergingList: []
+  - description: delete all keys from map
+    original:
+      name: 1
+      value: 1
+    patch:
+      $patch: replace
+    modified: {}
+  - description: add key and delete all keys from map
+    original:
+      name: 1
+      value: 1
+    patch:
+      other: a
+      $patch: replace
+    modified:
+      other: a
+`)
+
+func TestCustomStrategicMergePatch(t *testing.T) {
+	tc := StrategicMergePatchTestCases{}
+	err := yaml.Unmarshal(customStrategicMergePatchTestCaseData, &tc)
 	if err != nil {
-		panic(fmt.Sprintf("yaml marshal failed: %v", err))
+		t.Errorf("can't unmarshal test cases: %v", err)
+		return
 	}
+
+	for _, c := range tc.TestCases {
+		cOriginal, cPatch, cModified := testCaseToJSONOrFail(t, c)
+		testPatchApplication(t, cOriginal, cPatch, cModified, c.Description)
+	}
+}
+
+func testCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte) {
+	var e MergeItem
+	cOriginal := toJSONOrFail(c.Original, t)
+	cPatch, err := sortMergeListsByName(toJSONOrFail(c.Patch, t), e)
+	if err != nil {
+		t.Errorf("error:%v sorting patch object:\n%v", err, c.Patch)
+	}
+
+	cModified, err := sortMergeListsByName(toJSONOrFail(c.Modified, t), e)
+	if err != nil {
+		t.Errorf("error: %v sorting modified object:\n%v", err, c.Modified)
+	}
+
+	return cOriginal, cPatch, cModified
+}
+
+func testPatchApplication(t *testing.T, cOriginal, cPatch, cModified []byte, description string) {
+	var e MergeItem
+	modified, err := StrategicMergePatch(cOriginal, cPatch, e)
+	if err != nil {
+		t.Errorf("error applying patch: %v:\noriginal:\n%vpatch:\n%v",
+			err, jsonToYAMLOrError(cOriginal), jsonToYAMLOrError(cPatch))
+	}
+
+	// Sort the lists that have merged maps, since order is not significant.
+	modified, err = sortMergeListsByName(modified, e)
+	if err != nil {
+		t.Errorf("error: %v sorting modified object:\n%v", err, modified)
+	}
+
+	if !reflect.DeepEqual(modified, cModified) {
+		t.Errorf("patch application failed: %v\noriginal:\n%vpatch:\n%vexpected modified:\n%vgot modified:\n%v",
+			description, jsonToYAMLOrError(cOriginal), jsonToYAMLOrError(cPatch),
+			jsonToYAMLOrError(cModified), jsonToYAMLOrError(modified))
+	}
+}
+
+// These are test cases for CreateStrategicMergePatch, used to assert that it
+// generates the correct patch for a given outcome. They are also test cases for
+// StrategicMergePatch, used to assert that applying a patch yields the correct
+// outcome.
+var createStrategicMergePatchTestCaseData = []byte(`
+testCases:
+  - description: add field to map
+    original:
+      name: 1
+    patch:
+      value: 1
+    modified:
+      name: 1
+      value: 1
+  - description: add field and delete field from map
+    original:
+      name: 1
+    patch:
+      name: null
+      value: 1
+    modified:
+      value: 1
+  - description: delete field from nested map
+    original:
+      simpleMap:
+        key1: 1
+        key2: 1
+    patch:
+      simpleMap:
+        key2: null
+    modified:
+      simpleMap:
+        key1: 1
+  - description: delete all fields from map
+    original:
+      name: 1
+      value: 1
+    patch:
+      name: null
+      value: null
+    modified: {}
+  - description: add field and delete all fields from map
+    original:
+      name: 1
+      value: 1
+    patch:
+      other: a
+      name: null
+      value: null
+    modified:
+      other: a
+  - description: replace list of scalars
+    original:
+      nonMergingIntList:
+        - 1
+        - 2
+    patch:
+      nonMergingIntList:
+        - 2
+        - 3
+    modified:
+      nonMergingIntList:
+        - 2
+        - 3
+  - description: merge lists of scalars
+    original:
+      mergingIntList:
+        - 1
+        - 2
+    patch:
+      mergingIntList:
+        - 3
+    modified:
+      mergingIntList:
+        - 1
+        - 2
+        - 3
+  - description: merge lists of maps
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    patch:
+      mergingList:
+        - name: 3
+          value: 3
+    modified:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+        - name: 3
+          value: 3
+  - description: add field to map in merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    patch:
+      mergingList:
+        - name: 1
+          value: 1
+    modified:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+  - description: add duplicate field to map in merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+    patch:
+      mergingList:
+        - name: 1
+          value: 1
+    modified:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+  - description: replace map field value in merging list
+    original:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    patch:
+      mergingList:
+        - name: 1
+          value: a
+    modified:
+      mergingList:
+        - name: 1
+          value: a
+        - name: 2
+          value: 2
+  - description: delete map from merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          $patch: delete
+    modified:
+      mergingList:
+        - name: 2
+  - description: delete missing map from merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          $patch: delete
+    modified:
+      mergingList:
+        - name: 2
+  - description: add map and delete map from merging list
+    original:
+      merginglist:
+        - name: 1
+        - name: 2
+    patch:
+      merginglist:
+        - name: 1
+          $patch: delete
+        - name: 3
+    modified:
+      merginglist:
+        - name: 2
+        - name: 3
+  - description: delete all maps from merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          $patch: delete
+        - name: 2
+          $patch: delete
+    modified:
+      mergingList: []
+  - description: delete all maps from partially empty merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          $patch: delete
+        - name: 2
+          $patch: delete
+    modified:
+      mergingList: []
+  - description: delete all maps from empty merging list
+    original:
+      mergingList:
+        - name: 1
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          $patch: delete
+        - name: 2
+          $patch: delete
+    modified:
+      mergingList: []
+  - description: delete field from map in merging list
+    original:
+      mergingList:
+        - name: 1
+          value: 1
+        - name: 2
+          value: 2
+    patch:
+      mergingList:
+        - name: 1
+          value: null
+    modified:
+      mergingList:
+        - name: 1
+        - name: 2
+          value: 2
+  - description: replace non merging list nested in merging list
+    original:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+    modified:
+      mergingList:
+        - name: 1
+          nonMergingList:
+            - name: 1
+              value: 1
+        - name: 2
+  - description: add field to map in merging list nested in merging list
+    original:
+      mergingList:
+        - name: 1
+          mergingList:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    patch:
+      mergingList:
+        - name: 1
+          mergingList:
+            - name: 1
+              value: 1
+    modified:
+      mergingList:
+        - name: 1
+          mergingList:
+            - name: 1
+              value: 1
+            - name: 2
+              value: 2
+        - name: 2
+  - description: merge empty merging lists
+    original:
+      mergingList: []
+    patch:
+      {}
+    modified:
+      mergingList: []
+    current:
+      mergingList: []
+    result:
+      mergingList: []
+  - description: add map to merging list by pointer
+    original:
+      mergeItemPtr:
+        - name: 1
+    patch:
+      mergeItemPtr:
+        - name: 2
+    modified:
+      mergeItemPtr:
+        - name: 1
+        - name: 2
+  - description: add field to map in merging list by pointer
+    original:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+            - name: 2
+              value: 2
+        - name: 2
+    patch:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+              value: 1
+    modified:
+      mergeItemPtr:
+        - name: 1
+          mergeItemPtr:
+            - name: 1
+              value: 1
+            - name: 2
+              value: 2
+        - name: 2
+`)
+
+func TestStrategicMergePatch(t *testing.T) {
+	tc := StrategicMergePatchTestCases{}
+	err := yaml.Unmarshal(createStrategicMergePatchTestCaseData, &tc)
+	if err != nil {
+		t.Errorf("can't unmarshal test cases: %v", err)
+		return
+	}
+
+	var e MergeItem
+	for _, c := range tc.TestCases {
+		cOriginal, cPatch, cModified := testCaseToJSONOrFail(t, c)
+
+		// Test patch generation
+		patch, err := CreateStrategicMergePatch(cOriginal, cModified, e)
+		if err != nil {
+			t.Errorf("error generating patch: %s:\n%v", err, toYAMLOrError(c.StrategicMergePatchTestCaseData))
+		}
+
+		// Sort the lists that have merged maps, since order is not significant.
+		patch, err = sortMergeListsByName(patch, e)
+		if err != nil {
+			t.Errorf("error: %s sorting patch object:\n%v", err, patch)
+		}
+
+		if !reflect.DeepEqual(patch, cPatch) {
+			t.Errorf("patch generation failed:\n%vgot patch:\n%v", toYAMLOrError(c.StrategicMergePatchTestCaseData), jsonToYAMLOrError(patch))
+		}
+
+		// Test patch application
+		testPatchApplication(t, cOriginal, cPatch, cModified, c.Description)
+	}
+}
+
+func toYAMLOrError(v interface{}) string {
+	y, err := toYAML(v)
+	if err != nil {
+		return err.Error()
+	}
+
+	return y
+}
+
+func toJSONOrFail(v interface{}, t *testing.T) []byte {
+	theJSON, err := toJSON(v)
+	if err != nil {
+		t.Error(err)
+	}
+
+	return theJSON
+}
+
+func jsonToYAMLOrError(j []byte) string {
+	y, err := jsonToYAML(j)
+	if err != nil {
+		return err.Error()
+	}
+
 	return string(y)
 }
 
-func toJSON(v interface{}) []byte {
+func toYAML(v interface{}) (string, error) {
-	j, err := json.Marshal(v)
+	y, err := yaml.Marshal(v)
 	if err != nil {
-		panic(fmt.Sprintf("json marshal failed: %s", spew.Sdump(v)))
+		return "", fmt.Errorf("yaml marshal failed: %v\n%v", err, spew.Sdump(v))
 	}
-	return j
+
+	return string(y), nil
 }
 
-func jsonToYAML(j []byte) []byte {
+func toJSON(v interface{}) ([]byte, error) {
+	j, err := json.Marshal(v)
+	if err != nil {
+		return nil, fmt.Errorf("json marshal failed: %v\n%v", err, spew.Sdump(v))
+	}
+
+	return j, nil
+}
+
+func jsonToYAML(j []byte) ([]byte, error) {
 	y, err := yaml.JSONToYAML(j)
 	if err != nil {
-		panic(fmt.Sprintf("json to yaml failed: %v", err))
+		return nil, fmt.Errorf("json to yaml failed: %v\n%v", err, j)
 	}
-	return y
+
+	return y, nil
 }