From 0e41760cea1d1ce16f4cc311c2d6bd3525bb3da7 Mon Sep 17 00:00:00 2001
From: Matt Moore <mattmoor@google.com>
Date: Wed, 5 Dec 2018 15:04:26 -0800
Subject: [PATCH] Undo the patch sorting. (#192)

My prior change added sorting to the duck.CreatePatch method to try and stabilize the result of jsonpatch.CreatePatch, which is otherwise non-deterministic (probably walking a map?).

My bad assumption was that the patch operations this generated wouldn't conflict, e.g. it should use `replace` vs. `remove` and `add`.

Clearly this was bad because we start getting really strange errors trying to import this into knative/serving, e.g.
https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_serving/2646/pull-knative-serving-integration-tests/1070435951391543298/
---
 apis/duck/patch.go      | 16 +---------------
 apis/duck/patch_test.go |  6 +++---
 webhook/webhook_test.go | 19 +++++++++++++++++++
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/apis/duck/patch.go b/apis/duck/patch.go
index 762479e24..386aa1f32 100644
--- a/apis/duck/patch.go
+++ b/apis/duck/patch.go
@@ -18,7 +18,6 @@ package duck
 
 import (
 	"encoding/json"
-	"sort"
 
 	jsonmergepatch "github.com/evanphx/json-patch"
 	"github.com/mattbaird/jsonpatch"
@@ -51,20 +50,7 @@ func CreatePatch(before, after interface{}) (JSONPatch, error) {
 	if err != nil {
 		return nil, err
 	}
-	patch, err := jsonpatch.CreatePatch(rawBefore, rawAfter)
-	if err != nil {
-		return nil, err
-	}
-
-	// Give the patch a deterministic ordering.
-	sort.Slice(patch, func(i, j int) bool {
-		lhs, rhs := patch[i], patch[j]
-		if lhs.Operation != rhs.Operation {
-			return lhs.Operation < rhs.Operation
-		}
-		return lhs.Path < rhs.Path
-	})
-	return patch, nil
+	return jsonpatch.CreatePatch(rawBefore, rawAfter)
 }
 
 type JSONPatch []jsonpatch.JsonPatchOperation
diff --git a/apis/duck/patch_test.go b/apis/duck/patch_test.go
index 546136bf7..d63cbd5fe 100644
--- a/apis/duck/patch_test.go
+++ b/apis/duck/patch_test.go
@@ -163,12 +163,12 @@ func TestCreatePatch(t *testing.T) {
 			},
 		},
 		want: JSONPatch{{
-			Operation: "remove",
-			Path:      "/status/patchable/field2",
-		}, {
 			Operation: "replace",
 			Path:      "/status/patchable/field1",
 			Value:     42.0,
+		}, {
+			Operation: "remove",
+			Path:      "/status/patchable/field2",
 		}},
 	}, {
 		name: "patch array",
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index 3ff6af050..29850ae43 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -22,6 +22,7 @@ import (
 	"encoding/pem"
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -486,6 +487,24 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) {
 		return
 	}
 
+	// Give the patch a deterministic ordering.
+	// Technically this can change the meaning, but the ordering is otherwise unstable
+	// and difficult to test.
+	sort.Slice(e, func(i, j int) bool {
+		lhs, rhs := e[i], e[j]
+		if lhs.Operation != rhs.Operation {
+			return lhs.Operation < rhs.Operation
+		}
+		return lhs.Path < rhs.Path
+	})
+	sort.Slice(got, func(i, j int) bool {
+		lhs, rhs := got[i], got[j]
+		if lhs.Operation != rhs.Operation {
+			return lhs.Operation < rhs.Operation
+		}
+		return lhs.Path < rhs.Path
+	})
+
 	if diff := cmp.Diff(e, got, cmpopts.EquateEmpty()); diff != "" {
 		t.Errorf("expectPatches (-want, +got) = %v", diff)
 	}