diff --git a/pkg/kubectl/cmd/apply.go b/pkg/kubectl/cmd/apply.go index da3f68b1f29..a13165fc823 100644 --- a/pkg/kubectl/cmd/apply.go +++ b/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) diff --git a/pkg/util/strategicpatch/patch.go b/pkg/util/strategicpatch/patch.go index 1d3f77e2fb7..effb8f9ae60 100644 --- a/pkg/util/strategicpatch/patch.go +++ b/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 { - s := fmt.Sprintf("patch:\n%s\nconflicts with current:\n%s\n", patch, current) +func newErrConflict(patch, current string) errConflict { + 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 -// configurations. -func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, force bool, fns ...PreconditionFunc) ([]byte, error) { +// configuration, or, if overwrite is false and there are conflicts between the modified and current +// configurations. Conflicts are defined as keys changed differently from original to modified +// 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 +} diff --git a/pkg/util/strategicpatch/patch_test.go b/pkg/util/strategicpatch/patch_test.go index 3fff3e52f6d..753d0d72451 100644 --- a/pkg/util/strategicpatch/patch_test.go +++ b/pkg/util/strategicpatch/patch_test.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "reflect" + "strings" "testing" "github.com/davecgh/go-spew/spew" @@ -230,7 +231,7 @@ func TestSortMergeLists(t *testing.T) { tc := SortMergeListTestCases{} err := yaml.Unmarshal(sortMergeListTestCaseData, &tc) if err != nil { - t.Errorf("can't unmarshal test cases:%s\n", err) + t.Errorf("can't unmarshal test cases: %s\n", err) return } @@ -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: @@ -302,7 +314,7 @@ func TestCustomStrategicMergePatch(t *testing.T) { tc := StrategicMergePatchTestCases{} err := yaml.Unmarshal(customStrategicMergePatchTestCaseData, &tc) if err != nil { - t.Errorf("can't unmarshal test cases:%v\n", err) + t.Errorf("can't unmarshal test cases: %v\n", err) return } @@ -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,13 +1714,76 @@ 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", + "", "{}", mergeItem, errBadJSONDoc) + testStrategicMergePatchWithCustomArguments(t, "bad patch", + "{}", "", mergeItem, errBadJSONDoc) + testStrategicMergePatchWithCustomArguments(t, "bad struct", + "{}", "{}", []byte(""), 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 { - t.Errorf("can't unmarshal test cases:%s\n", err) + t.Errorf("can't unmarshal test cases: %s\n", err) return } @@ -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 len(c.Result) > 0 { - t.Errorf("error in test case: %s\nunexpected conflict occurred:\n%s\n", - c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData)) - } - + 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 } - t.Errorf("error: %s in test case: %s\ncannot create three way patch:\n%s\n", - err, c.Description, toYAMLOrError(c.StrategicMergePatchTestCaseData)) + 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 { + actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, true) + 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 } - if len(c.Result) < 1 { + if strings.Contains(c.Description, "conflict") || 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,19 +1944,10 @@ 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 { - return nil, fmt.Errorf("json marshal failed:%v\n%v\n", err, spew.Sdump(v)) + return nil, fmt.Errorf("json marshal failed: %v\n%v\n", err, spew.Sdump(v)) } return j, nil @@ -1424,7 +1956,7 @@ func toJSON(v interface{}) ([]byte, error) { func jsonToYAML(j []byte) ([]byte, error) { y, err := yaml.JSONToYAML(j) if err != nil { - return nil, fmt.Errorf("json to yaml failed:%v\n%v\n", err, j) + return nil, fmt.Errorf("json to yaml failed: %v\n%v\n", err, j) } return y, nil