Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: avoid unnecessary json marshal #626

Merged
merged 3 commits into from
Sep 17, 2024
+39 −25
Merged

chore: avoid unnecessary json marshal #626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f416878
chore: avoid unnecessary json marshal
crenshaw-dev Sep 10, 2024
e9af447
more tests
crenshaw-dev Sep 17, 2024
06e9545
refactor test to satisfy sonarcloud
crenshaw-dev Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 10 additions & 17 deletions pkg/diff/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,22 +673,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
Expand Down Expand Up @@ -1087,11 +1071,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to provide the same results if compared with the original code. Previously we were stripping the types from obj and using that to marshal into data. In the new code, data will just hold the Marshal(obj) result which could lead to a different behaviour. To validate this, it would be better to write unit tests that validate the type stripping logic. Checking on Pod resources values will probably be an easy way to validate this:

    resources:
      requests:
        memory: "64Mi"
        cpu: "250m"
      limits:
        memory: "128Mi"
        cpu: "500m"

Copy link
Member Author

@crenshaw-dev crenshaw-dev Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data is only used only for another Unmarshal (or, more specifically, unmarshaling via json.NewDecoder, but the behavior should be the same).

So before we had:

unstructured -> marshal -> unmarshal to unstructured -> marshal -> unmarshal to schema type

Now it will be:

unstructured -> marshal -> unmarshal to schema type

i.e. we're skipping unmarshal to unstructured (which is just map[string]interface{}) and the second marshal.

My belief is that those to steps don't remove any type information that unstructured -> marshal doesn't already remove.

But I'll write some tests to give us a bit more confidence that we're not losing anything important.

if err != nil {
panic(err)
}
obj = &newUn

gvk := obj.GroupVersionKind()
item, err := scheme.Scheme.New(obj.GroupVersionKind())
if err != nil {
Expand Down
37 changes: 29 additions & 8 deletions pkg/diff/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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() {
Expand Down
Loading