From 31c6f49846866f4a20acc52a7d41c39f666bacb1 Mon Sep 17 00:00:00 2001 From: Anastasis Andronidis Date: Mon, 17 Oct 2016 21:28:16 -0700 Subject: [PATCH] CreateThreeWayMergePatch should propagate explicit null fields --- .../pkg/util/strategicpatch/patch.go | 28 +++++++----- .../pkg/util/strategicpatch/patch_test.go | 45 ++++++++++++++----- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index b9124b322e6..1694aea3376 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -548,7 +548,7 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS if err != nil { return nil, err } - return mergeMap(original, patch, t, true) + return mergeMap(original, patch, t, true, true) } func getTagStructType(dataStruct interface{}) (reflect.Type, error) { @@ -574,7 +574,10 @@ var errBadPatchTypeFmt = "unknown patch type: %s in map: %v" // both the original map and the patch because getting a deep copy of a map in // golang is highly non-trivial. // flag mergeDeleteList controls if using the parallel list to delete or keeping the list. -func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDeleteList bool) (map[string]interface{}, error) { +// If patch contains any null field (e.g. field_1: null) that is not +// present in original, then to propagate it to the end result use +// ignoreUnmatchedNulls == false. +func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDeleteList, ignoreUnmatchedNulls bool) (map[string]interface{}, error) { if v, ok := patch[directiveMarker]; ok { if v == replaceDirective { // If the patch contains "$patch: replace", don't merge it, just use the @@ -619,13 +622,17 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet } // If the value of this key is null, delete the key if it exists in the - // original. Otherwise, skip it. + // original. Otherwise, check if we want to preserve it or skip it. + // Preserving the null value is useful when we want to send an explicit + // delete to the API server. if patchV == nil { if _, ok := original[k]; ok { delete(original, k) } - continue + if ignoreUnmatchedNulls { + continue + } } _, ok := original[k] @@ -654,7 +661,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet typedOriginal := original[k].(map[string]interface{}) typedPatch := patchV.(map[string]interface{}) var err error - original[k], err = mergeMap(typedOriginal, typedPatch, fieldType, mergeDeleteList) + original[k], err = mergeMap(typedOriginal, typedPatch, fieldType, mergeDeleteList, ignoreUnmatchedNulls) if err != nil { return nil, err } @@ -667,7 +674,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet typedOriginal := original[k].([]interface{}) typedPatch := patchV.([]interface{}) var err error - original[k], err = mergeSlice(typedOriginal, typedPatch, elemType, fieldPatchMergeKey, mergeDeleteList, isDeleteList) + original[k], err = mergeSlice(typedOriginal, typedPatch, elemType, fieldPatchMergeKey, mergeDeleteList, isDeleteList, ignoreUnmatchedNulls) if err != nil { return nil, err } @@ -688,7 +695,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet // Merge two slices together. Note: This may modify both the original slice and // the patch because getting a deep copy of a slice in golang is highly // non-trivial. -func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey string, mergeDeleteList, isDeleteList bool) ([]interface{}, error) { +func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey string, mergeDeleteList, isDeleteList, ignoreUnmatchedNulls bool) ([]interface{}, error) { if len(original) == 0 && len(patch) == 0 { return original, nil } @@ -779,7 +786,7 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s var mergedMaps interface{} var err error // Merge into original. - mergedMaps, err = mergeMap(originalMap, typedV, elemType, mergeDeleteList) + mergedMaps, err = mergeMap(originalMap, typedV, elemType, mergeDeleteList, ignoreUnmatchedNulls) if err != nil { return nil, err } @@ -1282,7 +1289,8 @@ func mapsOfMapsHaveConflicts(typedLeft, typedRight map[string]interface{}, struc // 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). +// value). We also propagate values fields that do not exist in original but are explicitly +// defined in modified. func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, overwrite bool, fns ...PreconditionFunc) ([]byte, error) { originalMap := map[string]interface{}{} if len(original) > 0 { @@ -1324,7 +1332,7 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int return nil, err } - patchMap, err := mergeMap(deletionsMap, deltaMap, t, false) + patchMap, err := mergeMap(deletionsMap, deltaMap, t, false, false) if err != nil { return nil, err } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go index 5af6d6ba6b4..0f07342b498 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go @@ -532,7 +532,7 @@ testCases: threeWay: name: null value: null - result: + result: other: a - description: delete all fields from map with conflict original: @@ -549,7 +549,7 @@ testCases: threeWay: name: null value: null - result: + result: other: a - description: add field and delete all fields from map original: @@ -677,12 +677,12 @@ testCases: mergingList: - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 modified: mergingList: - - name: 4 - value: 4 + - name: 4 + value: 4 - name: 1 - name: 2 value: 2 @@ -699,8 +699,8 @@ testCases: mergingList: - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 result: mergingList: - name: 1 @@ -710,8 +710,8 @@ testCases: other: b - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 - description: merge lists of maps with conflict original: mergingList: @@ -1817,6 +1817,27 @@ testCases: other: b - name: 2 other: b + - description: defined null values should propagate overwrite current fields (with conflict) (ignore two-way application) + original: + name: 2 + twoWay: + name: 1 + value: 1 + other: null + modified: + name: 1 + value: 1 + other: null + current: + name: a + other: a + threeWay: + name: 1 + value: 1 + other: null + result: + name: 1 + value: 1 `) var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{ @@ -1992,7 +2013,9 @@ func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) { } testPatchCreation(t, expected, actual, c.Description) - testPatchApplication(t, original, actual, modified, c.Description) + if !strings.Contains(c.Description, "ignore two-way application") { + testPatchApplication(t, original, actual, modified, c.Description) + } } func testTwoWayPatchForRawTestCase(t *testing.T, c StrategicMergePatchRawTestCase) {