Merge pull request #43469 from enisoc/has-conflicts

Automatic merge from submit-queue

Fix mergepatch.HasConflicts().

**What this PR does / why we need it**:

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.

**Which issue this PR fixes**:

**Special notes for your reviewer**:

**Release note**:
```release-note
NONE
```

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

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 {
+	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("%d: unexpected error: %v", i, err)
+				t.Errorf("%v: unexpected error: %v", testStr, err)
+				break
 			}
 			if out != testCase.Ret {
-			t.Errorf("%d: expected %t got %t", i, testCase.Ret, out)
+				t.Errorf("%v: expected %t got %t", testStr, testCase.Ret, out)
-			continue
+				break
 			}
 			out, err = HasConflicts(testCase.B, testCase.A)
 			if err != nil {
-			t.Errorf("%d: unexpected error: %v", i, err)
+				t.Errorf("%v: unexpected error: %v", testStr, err)
+				break
 			}
 			if out != testCase.Ret {
-			t.Errorf("%d: expected reversed %t got %t", i, testCase.Ret, out)
+				t.Errorf("%v: expected reversed %t got %t", testStr, testCase.Ret, out)
+				break
+			}
 		}
 	}
 }