Skip to content

Commit 8561b0b

Browse files
kuudorikuudori
committed
HYPERFLEET-995 - fix: use semantic JSON comparison instead of bytes.Equal
1 parent 950e126 commit 8561b0b

7 files changed

Lines changed: 200 additions & 7 deletions

File tree

‎pkg/services/cluster.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package services
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76
"fmt"
@@ -110,7 +109,7 @@ func (s *sqlClusterService) Patch(
110109
return nil, errors.Validation("Invalid patch data: %v", applyErr)
111110
}
112111

113-
if bytes.Equal(oldSpec, cluster.Spec) && bytes.Equal(oldLabels, cluster.Labels) {
112+
if jsonEqual(oldSpec, cluster.Spec) && jsonEqual(oldLabels, cluster.Labels) {
114113
return cluster, nil
115114
}
116115

@@ -287,7 +286,7 @@ func (s *sqlClusterService) recomputeAndSaveClusterStatus(
287286
return nil, errors.GeneralError("Failed to marshal conditions: %s", err)
288287
}
289288

290-
if bytes.Equal(cluster.StatusConditions, conditionsJSON) {
289+
if jsonEqual(cluster.StatusConditions, conditionsJSON) {
291290
return cluster, nil
292291
}
293292

‎pkg/services/cluster_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1796,6 +1796,56 @@ func TestClusterPatch(t *testing.T) {
17961796
g.Expect(result.Generation).To(Equal(int32(2)))
17971797
})
17981798

1799+
t.Run("spec unchanged with different key order keeps generation", func(t *testing.T) {
1800+
t.Parallel()
1801+
g := NewWithT(t)
1802+
clusterDao := newMockClusterDao()
1803+
nodePoolDao := newMockNodePoolDao()
1804+
adapterStatusDao := newMockAdapterStatusDao()
1805+
adapterConfig := testAdapterConfig()
1806+
adapterConfig.Required.Cluster = []string{}
1807+
service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig)
1808+
ctx := context.Background()
1809+
1810+
clusterDao.clusters["c1"] = &api.Cluster{
1811+
Meta: api.Meta{ID: "c1"},
1812+
Spec: []byte(`{"z":"last","a":"first","m":"middle"}`),
1813+
Labels: []byte(`{}`),
1814+
Generation: 5,
1815+
}
1816+
1817+
sameSpec := map[string]interface{}{"z": "last", "a": "first", "m": "middle"}
1818+
result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Spec: &sameSpec})
1819+
1820+
g.Expect(svcErr).To(BeNil())
1821+
g.Expect(result.Generation).To(Equal(int32(5)))
1822+
})
1823+
1824+
t.Run("labels unchanged with different key order keeps generation", func(t *testing.T) {
1825+
t.Parallel()
1826+
g := NewWithT(t)
1827+
clusterDao := newMockClusterDao()
1828+
nodePoolDao := newMockNodePoolDao()
1829+
adapterStatusDao := newMockAdapterStatusDao()
1830+
adapterConfig := testAdapterConfig()
1831+
adapterConfig.Required.Cluster = []string{}
1832+
service := NewClusterService(clusterDao, nodePoolDao, newMockNodePoolService(), adapterStatusDao, adapterConfig)
1833+
ctx := context.Background()
1834+
1835+
clusterDao.clusters["c1"] = &api.Cluster{
1836+
Meta: api.Meta{ID: "c1"},
1837+
Spec: []byte(`{}`),
1838+
Labels: []byte(`{"z":"zulu","a":"alpha"}`),
1839+
Generation: 4,
1840+
}
1841+
1842+
sameLabels := map[string]string{"z": "zulu", "a": "alpha"}
1843+
result, svcErr := service.Patch(ctx, "c1", &api.ClusterPatchRequest{Labels: &sameLabels})
1844+
1845+
g.Expect(svcErr).To(BeNil())
1846+
g.Expect(result.Generation).To(Equal(int32(4)))
1847+
})
1848+
17991849
t.Run("not found returns 404", func(t *testing.T) {
18001850
t.Parallel()
18011851
g := NewWithT(t)

‎pkg/services/node_pool.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package services
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76
"fmt"
@@ -120,7 +119,7 @@ func (s *sqlNodePoolService) Patch(
120119
return nil, errors.Validation("Invalid patch data: %v", applyErr)
121120
}
122121

123-
if bytes.Equal(oldSpec, nodePool.Spec) && bytes.Equal(oldLabels, nodePool.Labels) {
122+
if jsonEqual(oldSpec, nodePool.Spec) && jsonEqual(oldLabels, nodePool.Labels) {
124123
return nodePool, nil
125124
}
126125

‎pkg/services/node_pool_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,6 +1102,54 @@ func TestNodePoolPatch(t *testing.T) {
11021102
g.Expect(result.Generation).To(Equal(int32(2)))
11031103
})
11041104

1105+
t.Run("spec unchanged with different key order keeps generation", func(t *testing.T) {
1106+
t.Parallel()
1107+
g := NewWithT(t)
1108+
nodePoolDao := newMockNodePoolDao()
1109+
adapterStatusDao := newMockAdapterStatusDao()
1110+
adapterConfig := testNodePoolAdapterConfig()
1111+
adapterConfig.Required.Nodepool = []string{}
1112+
service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil)
1113+
ctx := context.Background()
1114+
1115+
nodePoolDao.nodePools["np1"] = &api.NodePool{
1116+
Meta: api.Meta{ID: "np1"},
1117+
Spec: []byte(`{"z":"last","a":"first","m":"middle"}`),
1118+
Labels: []byte(`{}`),
1119+
Generation: 5,
1120+
}
1121+
1122+
sameSpec := map[string]interface{}{"z": "last", "a": "first", "m": "middle"}
1123+
result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Spec: &sameSpec})
1124+
1125+
g.Expect(svcErr).To(BeNil())
1126+
g.Expect(result.Generation).To(Equal(int32(5)))
1127+
})
1128+
1129+
t.Run("labels unchanged with different key order keeps generation", func(t *testing.T) {
1130+
t.Parallel()
1131+
g := NewWithT(t)
1132+
nodePoolDao := newMockNodePoolDao()
1133+
adapterStatusDao := newMockAdapterStatusDao()
1134+
adapterConfig := testNodePoolAdapterConfig()
1135+
adapterConfig.Required.Nodepool = []string{}
1136+
service := NewNodePoolService(nodePoolDao, nil, adapterStatusDao, adapterConfig, nil)
1137+
ctx := context.Background()
1138+
1139+
nodePoolDao.nodePools["np1"] = &api.NodePool{
1140+
Meta: api.Meta{ID: "np1"},
1141+
Spec: []byte(`{}`),
1142+
Labels: []byte(`{"z":"zulu","a":"alpha"}`),
1143+
Generation: 4,
1144+
}
1145+
1146+
sameLabels := map[string]string{"z": "zulu", "a": "alpha"}
1147+
result, svcErr := service.Patch(ctx, "np1", &api.NodePoolPatchRequest{Labels: &sameLabels})
1148+
1149+
g.Expect(svcErr).To(BeNil())
1150+
g.Expect(result.Generation).To(Equal(int32(4)))
1151+
})
1152+
11051153
t.Run("not found returns 404", func(t *testing.T) {
11061154
t.Parallel()
11071155
g := NewWithT(t)

‎pkg/services/status_helpers.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package services
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76

@@ -41,7 +40,7 @@ func computeNodePoolConditionsJSON(
4140
return nil, errors.GeneralError("Failed to marshal conditions: %s", err)
4241
}
4342

44-
if bytes.Equal(np.StatusConditions, conditionsJSON) {
43+
if jsonEqual(np.StatusConditions, conditionsJSON) {
4544
return nil, nil
4645
}
4746

‎pkg/services/util.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package services
22

33
import (
4+
"encoding/json"
45
e "errors"
6+
"reflect"
57
"strings"
68

79
"gorm.io/gorm"
@@ -10,6 +12,17 @@ import (
1012
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors"
1113
)
1214

15+
func jsonEqual(a, b []byte) bool {
16+
var va, vb any
17+
if err := json.Unmarshal(a, &va); err != nil {
18+
return false
19+
}
20+
if err := json.Unmarshal(b, &vb); err != nil {
21+
return false
22+
}
23+
return reflect.DeepEqual(va, vb)
24+
}
25+
1326
// Field names suspected to contain personally identifiable information
1427
var piiFields = []string{
1528
"username",

‎pkg/services/util_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
package services
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/gomega"
7+
)
8+
9+
func TestJSONEqual(t *testing.T) {
10+
RegisterTestingT(t)
11+
12+
tests := []struct {
13+
name string
14+
a, b []byte
15+
expected bool
16+
}{
17+
{
18+
name: "identical bytes",
19+
a: []byte(`{"a":1,"b":2}`),
20+
b: []byte(`{"a":1,"b":2}`),
21+
expected: true,
22+
},
23+
{
24+
name: "different key order same values",
25+
a: []byte(`{"b":2,"a":1}`),
26+
b: []byte(`{"a":1,"b":2}`),
27+
expected: true,
28+
},
29+
{
30+
name: "nested objects different key order",
31+
a: []byte(`{"x":{"b":2,"a":1},"y":3}`),
32+
b: []byte(`{"y":3,"x":{"a":1,"b":2}}`),
33+
expected: true,
34+
},
35+
{
36+
name: "different values",
37+
a: []byte(`{"a":1}`),
38+
b: []byte(`{"a":2}`),
39+
expected: false,
40+
},
41+
{
42+
name: "extra key",
43+
a: []byte(`{"a":1}`),
44+
b: []byte(`{"a":1,"b":2}`),
45+
expected: false,
46+
},
47+
{
48+
name: "arrays preserve order",
49+
a: []byte(`[1,2,3]`),
50+
b: []byte(`[1,2,3]`),
51+
expected: true,
52+
},
53+
{
54+
name: "arrays different order not equal",
55+
a: []byte(`[1,2,3]`),
56+
b: []byte(`[3,2,1]`),
57+
expected: false,
58+
},
59+
{
60+
name: "invalid json a",
61+
a: []byte(`not json`),
62+
b: []byte(`{"a":1}`),
63+
expected: false,
64+
},
65+
{
66+
name: "invalid json b",
67+
a: []byte(`{"a":1}`),
68+
b: []byte(`not json`),
69+
expected: false,
70+
},
71+
{
72+
name: "whitespace differences",
73+
a: []byte(`{ "a" : 1 }`),
74+
b: []byte(`{"a":1}`),
75+
expected: true,
76+
},
77+
}
78+
79+
for _, tt := range tests {
80+
t.Run(tt.name, func(t *testing.T) {
81+
RegisterTestingT(t)
82+
Expect(jsonEqual(tt.a, tt.b)).To(Equal(tt.expected))
83+
})
84+
}
85+
}

0 commit comments

Comments
 (0)