From 103cb17bad7673e6934920ae20df8c67fd6b2bcd Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Tue, 21 Mar 2017 12:04:34 -0700 Subject: [PATCH] Fix mergepatch.HasConflicts(). This fixes some false negatives: * If a map had multiple entries, only the first was checked. * If a list had multiple entries, only the first was checked. --- .../apimachinery/pkg/util/mergepatch/util.go | 13 ++- .../pkg/util/mergepatch/util_test.go | 92 +++++++++++++++---- 2 files changed, 83 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go b/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go index 591884e5c8c..9261290a76c 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go @@ -86,6 +86,9 @@ func toYAML(v interface{}) (string, error) { // 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 JSON merge patch semantics. +// +// NOTE: Numbers with different types (e.g. int(0) vs int64(0)) will be detected as conflicts. +// Make sure the unmarshaling of left and right are consistent (e.g. use the same library). func HasConflicts(left, right interface{}) (bool, error) { switch typedLeft := left.(type) { case map[string]interface{}: @@ -94,9 +97,11 @@ func HasConflicts(left, right interface{}) (bool, error) { for key, leftValue := range typedLeft { rightValue, ok := typedRight[key] if !ok { - return false, nil + continue + } + if conflict, err := HasConflicts(leftValue, rightValue); err != nil || conflict { + return conflict, err } - return HasConflicts(leftValue, rightValue) } return false, nil @@ -111,7 +116,9 @@ func HasConflicts(left, right interface{}) (bool, error) { } for i := range typedLeft { - return HasConflicts(typedLeft[i], typedRight[i]) + if conflict, err := HasConflicts(typedLeft[i], typedRight[i]); err != nil || conflict { + return conflict, err + } } return false, nil diff --git a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util_test.go b/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util_test.go index 9e761c005ba..1b37e3ef5e0 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util_test.go @@ -17,6 +17,7 @@ limitations under the License. package mergepatch import ( + "fmt" "testing" ) @@ -26,28 +27,29 @@ func TestHasConflicts(t *testing.T) { B interface{} Ret bool }{ - {A: "hello", B: "hello", Ret: false}, // 0 + {A: "hello", B: "hello", Ret: false}, {A: "hello", B: "hell", Ret: true}, {A: "hello", B: nil, Ret: true}, {A: "hello", B: 1, Ret: true}, {A: "hello", B: float64(1.0), Ret: true}, {A: "hello", B: false, Ret: true}, {A: 1, B: 1, Ret: false}, + {A: nil, B: nil, Ret: false}, {A: false, B: false, Ret: false}, {A: float64(3), B: float64(3), Ret: false}, - {A: "hello", B: []interface{}{}, Ret: true}, // 6 + {A: "hello", B: []interface{}{}, Ret: true}, {A: []interface{}{1}, B: []interface{}{}, Ret: true}, {A: []interface{}{}, B: []interface{}{}, Ret: false}, {A: []interface{}{1}, B: []interface{}{1}, Ret: false}, {A: map[string]interface{}{}, B: []interface{}{1}, Ret: true}, - {A: map[string]interface{}{}, B: map[string]interface{}{"a": 1}, Ret: false}, // 11 + {A: map[string]interface{}{}, B: map[string]interface{}{"a": 1}, Ret: false}, {A: map[string]interface{}{"a": 1}, B: map[string]interface{}{"a": 1}, Ret: false}, {A: map[string]interface{}{"a": 1}, B: map[string]interface{}{"a": 2}, Ret: true}, {A: map[string]interface{}{"a": 1}, B: map[string]interface{}{"b": 2}, Ret: false}, - { // 15 + { A: map[string]interface{}{"a": []interface{}{1}}, B: map[string]interface{}{"a": []interface{}{1}}, Ret: false, @@ -62,23 +64,75 @@ func TestHasConflicts(t *testing.T) { B: map[string]interface{}{"a": 1}, Ret: true, }, + + // Maps and lists with multiple entries. + { + A: map[string]interface{}{"a": 1, "b": 2}, + B: map[string]interface{}{"a": 1, "b": 0}, + Ret: true, + }, + { + A: map[string]interface{}{"a": 1, "b": 2}, + B: map[string]interface{}{"a": 1, "b": 2}, + Ret: false, + }, + { + A: map[string]interface{}{"a": 1, "b": 2}, + B: map[string]interface{}{"a": 1, "b": 0, "c": 3}, + Ret: true, + }, + { + A: map[string]interface{}{"a": 1, "b": 2}, + B: map[string]interface{}{"a": 1, "b": 2, "c": 3}, + Ret: false, + }, + { + A: map[string]interface{}{"a": []interface{}{1, 2}}, + B: map[string]interface{}{"a": []interface{}{1, 0}}, + Ret: true, + }, + { + A: map[string]interface{}{"a": []interface{}{1, 2}}, + B: map[string]interface{}{"a": []interface{}{1, 2}}, + Ret: false, + }, + + // Numeric types are not interchangeable. + // Callers are expected to ensure numeric types are consistent in 'left' and 'right'. + {A: int(0), B: int64(0), Ret: true}, + {A: int(0), B: float64(0), Ret: true}, + {A: int64(0), B: float64(0), Ret: true}, + // Other types are not interchangeable. + {A: int(0), B: "0", Ret: true}, + {A: int(0), B: nil, Ret: true}, + {A: int(0), B: false, Ret: true}, + {A: "true", B: true, Ret: true}, + {A: "null", B: nil, Ret: true}, } - for i, testCase := range testCases { - out, err := HasConflicts(testCase.A, testCase.B) - if err != nil { - t.Errorf("%d: unexpected error: %v", i, err) - } - if out != testCase.Ret { - t.Errorf("%d: expected %t got %t", i, testCase.Ret, out) - continue - } - out, err = HasConflicts(testCase.B, testCase.A) - if err != nil { - t.Errorf("%d: unexpected error: %v", i, err) - } - if out != testCase.Ret { - t.Errorf("%d: expected reversed %t got %t", i, testCase.Ret, out) + for _, testCase := range testCases { + testStr := fmt.Sprintf("A = %#v, B = %#v", testCase.A, testCase.B) + // Run each test case multiple times if it passes because HasConflicts() + // uses map iteration, which returns keys in nondeterministic order. + for try := 0; try < 10; try++ { + out, err := HasConflicts(testCase.A, testCase.B) + if err != nil { + t.Errorf("%v: unexpected error: %v", testStr, err) + break + } + if out != testCase.Ret { + t.Errorf("%v: expected %t got %t", testStr, testCase.Ret, out) + break + } + out, err = HasConflicts(testCase.B, testCase.A) + if err != nil { + t.Errorf("%v: unexpected error: %v", testStr, err) + break + } + if out != testCase.Ret { + t.Errorf("%v: expected reversed %t got %t", testStr, testCase.Ret, out) + break + } } } }