From 72bcdda3f0a5b80432d9f72e5b30827a530ac349 Mon Sep 17 00:00:00 2001
From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Date: Tue, 17 Sep 2024 13:19:20 -0400
Subject: [PATCH] chore: avoid unnecessary json marshal (#626)

* chore: avoid unnecessary json marshal

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* more tests

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* refactor test to satisfy sonarcloud

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
---
 pkg/diff/diff.go      | 27 ++++++++++-----------------
 pkg/diff/diff_test.go | 37 +++++++++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go
index 080240a4b..a45618a14 100644
--- a/pkg/diff/diff.go
+++ b/pkg/diff/diff.go
@@ -667,22 +667,6 @@ func ThreeWayDiff(orig, config, live *unstructured.Unstructured) (*DiffResult, e
 	return buildDiffResult(predictedLiveBytes, liveBytes), nil
 }
 
-// stripTypeInformation strips any type information (e.g. float64 vs. int) from the unstructured
-// object by remarshalling the object. This is important for diffing since it will cause godiff
-// to report a false difference.
-func stripTypeInformation(un *unstructured.Unstructured) *unstructured.Unstructured {
-	unBytes, err := json.Marshal(un)
-	if err != nil {
-		panic(err)
-	}
-	var newUn unstructured.Unstructured
-	err = json.Unmarshal(unBytes, &newUn)
-	if err != nil {
-		panic(err)
-	}
-	return &newUn
-}
-
 // removeNamespaceAnnotation remove the namespace and an empty annotation map from the metadata.
 // The namespace field is present in live (namespaced) objects, but not necessarily present in
 // config or last-applied. This results in a diff which we don't care about. We delete the two so
@@ -1081,11 +1065,20 @@ func toString(val interface{}) string {
 // Remarshalling also strips any type information (e.g. float64 vs. int) from the unstructured
 // object. This is important for diffing since it will cause godiff to report a false difference.
 func remarshal(obj *unstructured.Unstructured, o options) *unstructured.Unstructured {
-	obj = stripTypeInformation(obj)
 	data, err := json.Marshal(obj)
 	if err != nil {
 		panic(err)
 	}
+
+	// Unmarshal again to strip type information (e.g. float64 vs. int) from the unstructured
+	// object. This is important for diffing since it will cause godiff to report a false difference.
+	var newUn unstructured.Unstructured
+	err = json.Unmarshal(data, &newUn)
+	if err != nil {
+		panic(err)
+	}
+	obj = &newUn
+
 	gvk := obj.GroupVersionKind()
 	item, err := scheme.Scheme.New(obj.GroupVersionKind())
 	if err != nil {
diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go
index 922db590e..fd2fd754e 100644
--- a/pkg/diff/diff_test.go
+++ b/pkg/diff/diff_test.go
@@ -1127,6 +1127,14 @@ metadata:
 }
 
 func TestRemarshalResources(t *testing.T) {
+	getRequests := func(un *unstructured.Unstructured) map[string]interface{} {
+		return un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})
+	}
+
+	setRequests := func(un *unstructured.Unstructured, requests map[string]interface{}) {
+		un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"] = requests
+	}
+
 	manifest := []byte(`
 apiVersion: v1
 kind: Pod
@@ -1142,14 +1150,27 @@ spec:
 `)
 	un := unstructured.Unstructured{}
 	err := yaml.Unmarshal(manifest, &un)
-	assert.NoError(t, err)
-	requestsBefore := un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})
-	t.Log(requestsBefore)
-	newUn := remarshal(&un, applyOptions(diffOptionsForTest()))
-	requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})
-	t.Log(requestsAfter)
-	assert.Equal(t, float64(0.2), requestsBefore["cpu"])
-	assert.Equal(t, "200m", requestsAfter["cpu"])
+	require.NoError(t, err)
+
+	testCases := []struct {
+		name        string
+		cpu         any
+		expectedCPU any
+	}{
+		{"from float", 0.2, "200m"},
+		{"from float64", float64(0.2), "200m"},
+		{"from string", "0.2", "200m"},
+		{"from invalid", "invalid", "invalid"},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			setRequests(&un, map[string]interface{}{"cpu": tc.cpu})
+			newUn := remarshal(&un, applyOptions(diffOptionsForTest()))
+			requestsAfter := getRequests(newUn)
+			assert.Equal(t, tc.expectedCPU, requestsAfter["cpu"])
+		})
+	}
 }
 
 func ExampleDiff() {