Skip to content

Commit e682a67

Browse files
anderseknertanderseknert
authored
Don't use reflect.DeepEqual for errors (#7311)
I pushed most of these fixed previously but these ones required more work. Probably more work that it was worth, lol, but now the work is done... and at least we can add this to the list of enabled checks. Fixes #7238 Signed-off-by: Anders Eknert <[email protected]>
1 parent d20dd18 commit e682a67

File tree

10 files changed

+102
-31
lines changed

10 files changed

+102
-31
lines changed

‎.golangci.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ issues:
140140
linters-settings:
141141
lll:
142142
line-length: 200
143+
govet:
144+
enable:
145+
- deepequalerrors
143146

144147
linters:
145148
disable-all: true

‎v1/ast/parser_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1932,7 +1932,7 @@ func TestIsValidImportPath(t *testing.T) {
19321932
result := IsValidImportPath(path)
19331933
if tc.expected == nil && result != nil {
19341934
t.Errorf("Unexpected error for %v: %v", path, result)
1935-
} else if !reflect.DeepEqual(tc.expected, result) {
1935+
} else if tc.expected.Error() != result.Error() {
19361936
t.Errorf("For %v expected %v but got: %v", path, tc.expected, result)
19371937
}
19381938
}

‎v1/dependencies/deps_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -511,8 +511,7 @@ func assertRefSliceEq(t *testing.T, exp, result []ast.Ref) {
511511
}
512512

513513
for i, e := range exp {
514-
r := result[i]
515-
if e.Compare(r) != 0 {
514+
if !e.Equal(result[i]) {
516515
t.Fatalf("Expected refs %v, got %v", exp, result)
517516
}
518517
}

‎v1/plugins/bundle/plugin_test.go‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func TestPluginOneShot(t *testing.T) {
107107

108108
data, err := manager.Store.Read(ctx, txn, storage.Path{})
109109
expData := util.MustUnmarshalJSON([]byte(`{
110-
"foo": {"bar": 1, "baz": "qux"},
110+
"foo": {"bar": 1, "baz": "qux"},
111111
"system": {
112112
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}},
113113
"modules": {"test-bundle/foo/bar": {"rego_version": 1}}
@@ -968,7 +968,7 @@ func TestPluginStartLazyLoadInMem(t *testing.T) {
968968
}
969969

970970
expected := `{
971-
"p": "x1", "q": "x2",
971+
"p": "x1", "q": "x2",
972972
"system": {
973973
"bundles": {"test-1": {"etag": "", "manifest": {"revision": "", "roots": ["p", "authz"]}}, "test-2": {"etag": "", "manifest": {"revision": "", "roots": ["q"]}}},
974974
"modules": {"test-1/bar/policy.rego": {"rego_version": 1}}
@@ -1087,7 +1087,7 @@ func TestPluginOneShotDiskStorageMetrics(t *testing.T) {
10871087

10881088
data, err := manager.Store.Read(ctx, txn, storage.Path{})
10891089
expData := util.MustUnmarshalJSON([]byte(`{
1090-
"foo": {"bar": 1, "baz": "qux"},
1090+
"foo": {"bar": 1, "baz": "qux"},
10911091
"system": {
10921092
"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}},
10931093
"modules": {"test-bundle/foo/bar": {"rego_version": 1}}
@@ -1194,7 +1194,7 @@ func TestPluginOneShotDeltaBundle(t *testing.T) {
11941194
t.Fatal(err)
11951195
}
11961196
expData := util.MustUnmarshalJSON([]byte(`{
1197-
"a": {"baz": "bux", "foo": ["hello", "world"]},
1197+
"a": {"baz": "bux", "foo": ["hello", "world"]},
11981198
"system": {
11991199
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "delta", "roots": ["a"]}}},
12001200
"modules": {"test-bundle/a/policy.rego": {"rego_version": 1}}
@@ -1299,7 +1299,7 @@ func TestPluginOneShotDeltaBundleWithAstStore(t *testing.T) {
12991299
t.Fatal(err)
13001300
}
13011301
expData := ast.MustParseTerm(`{
1302-
"a": {"baz": "bux", "foo": ["hello", "world"]},
1302+
"a": {"baz": "bux", "foo": ["hello", "world"]},
13031303
"system": {
13041304
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "delta", "roots": ["a"]}}},
13051305
"modules": {"test-bundle/a/policy.rego": {"rego_version": 1}}
@@ -1486,7 +1486,7 @@ func TestPluginOneShotBundlePersistence(t *testing.T) {
14861486

14871487
data, err := manager.Store.Read(ctx, txn, storage.Path{})
14881488
expData := util.MustUnmarshalJSON([]byte(`{
1489-
"foo": {"bar": 1, "baz": "qux"},
1489+
"foo": {"bar": 1, "baz": "qux"},
14901490
"system": {
14911491
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}},
14921492
"modules": {"test-bundle/foo/bar.rego": {"rego_version": 1}}
@@ -1675,7 +1675,7 @@ corge contains 1 if {
16751675
}
16761676

16771677
expData := util.MustUnmarshalJSON([]byte(`{
1678-
"foo": {"bar": 1, "baz": "qux"},
1678+
"foo": {"bar": 1, "baz": "qux"},
16791679
"system": {
16801680
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}
16811681
}
@@ -1980,7 +1980,7 @@ corge contains 1 if {
19801980
}
19811981

19821982
expData := util.MustUnmarshalJSON([]byte(fmt.Sprintf(`{
1983-
"foo": {"bar": 1, "baz": "qux"},
1983+
"foo": {"bar": 1, "baz": "qux"},
19841984
"system": {
19851985
"bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux"%s, "roots": [""]}}}%s
19861986
}
@@ -2169,7 +2169,7 @@ func TestLoadAndActivateBundlesFromDisk(t *testing.T) {
21692169

21702170
data, err := manager.Store.Read(ctx, txn, storage.Path{})
21712171
expData := util.MustUnmarshalJSON([]byte(`{
2172-
"foo": {"bar": 1, "baz": "qux"},
2172+
"foo": {"bar": 1, "baz": "qux"},
21732173
"system": {
21742174
"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}},
21752175
"modules": {"test-bundle/foo/bar.rego": {"rego_version": 1}}
@@ -2255,7 +2255,7 @@ func TestLoadAndActivateBundlesFromDiskReservedChars(t *testing.T) {
22552255

22562256
data, err := manager.Store.Read(ctx, txn, storage.Path{})
22572257
expData := util.MustUnmarshalJSON([]byte(`{
2258-
"foo": {"bar": 1, "baz": "qux"},
2258+
"foo": {"bar": 1, "baz": "qux"},
22592259
"system": {
22602260
"bundles": {"test?bundle=opa": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}},
22612261
"modules": {"test/foo/bar.rego": {"rego_version": 1}}
@@ -2504,7 +2504,7 @@ corge contains 2 if {
25042504
}
25052505

25062506
expData := util.MustUnmarshalJSON([]byte(`{
2507-
"foo": {"bar": 1, "baz": "qux"},
2507+
"foo": {"bar": 1, "baz": "qux"},
25082508
"system": {
25092509
"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}
25102510
}
@@ -2739,7 +2739,7 @@ corge contains 1 if {
27392739
var expData any
27402740
if moduleRegoVersion != runtimeRegoVersion {
27412741
expData = util.MustUnmarshalJSON([]byte(fmt.Sprintf(`{
2742-
"foo": {"bar": 1, "baz": "qux"},
2742+
"foo": {"bar": 1, "baz": "qux"},
27432743
"system": {
27442744
"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux"%s, "roots": [""]}}},
27452745
"modules": {"test-bundle/foo/bar.rego": {"rego_version": %d}}
@@ -2748,7 +2748,7 @@ corge contains 1 if {
27482748
manifestRegoVersionStr, moduleRegoVersion)))
27492749
} else {
27502750
expData = util.MustUnmarshalJSON([]byte(fmt.Sprintf(`{
2751-
"foo": {"bar": 1, "baz": "qux"},
2751+
"foo": {"bar": 1, "baz": "qux"},
27522752
"system": {
27532753
"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux"%s, "roots": [""]}}}
27542754
}
@@ -3534,7 +3534,7 @@ p contains x if { x = 1 }`
35343534
t.Errorf("Expected to have bundle status for %q included in update, got: %+v", name, s1)
35353535
}
35363536
// they should be defaults at this point
3537-
if !reflect.DeepEqual(s, &Status{Name: name}) {
3537+
if !s.Equal(&Status{Name: name}) {
35383538
t.Errorf("Expected bundle %q to have an empty status, got: %+v", name, s1)
35393539
}
35403540
}
@@ -3567,7 +3567,7 @@ p contains x`
35673567
t.Errorf("Expected to have bundle status for %q included in update, got: %+v", name, s2)
35683568
}
35693569
// they should be still defaults
3570-
if !reflect.DeepEqual(s, &Status{Name: name}) {
3570+
if !s.Equal(&Status{Name: name}) {
35713571
t.Errorf("Expected bundle %q to have an empty status, got: %+v", name, s2)
35723572
}
35733573
}
@@ -3599,7 +3599,7 @@ p contains 1`
35993599
t.Errorf("Expected to have bundle status for %q included in update, got: %+v", name, s3)
36003600
}
36013601
// they should still be defaults
3602-
if !reflect.DeepEqual(s, &Status{Name: name}) {
3602+
if !s.Equal(&Status{Name: name}) {
36033603
t.Errorf("Expected bundle %q to have an empty status, got: %+v", name, s3)
36043604
}
36053605
}
@@ -3645,7 +3645,7 @@ p contains x if { x = 1 }`
36453645
t.Fatal("Unexpected status update, got:", s5)
36463646
}
36473647

3648-
if !reflect.DeepEqual(s5[bundleNames[0]], s4[bundleNames[0]]) {
3648+
if !s5[bundleNames[0]].Equal(s4[bundleNames[0]]) {
36493649
t.Fatalf("Expected bundle %q to have the same status as before updating bundle %q, got: %+v", bundleNames[0], bundleNames[1], s5)
36503650
}
36513651

@@ -3656,7 +3656,7 @@ p contains x if { x = 1 }`
36563656
t.Errorf("Expected to have bundle status for %q included in update, got: %+v", name, s5)
36573657
}
36583658
// they should still be defaults
3659-
if !reflect.DeepEqual(s, &Status{Name: name}) {
3659+
if !s.Equal(&Status{Name: name}) {
36603660
t.Errorf("Expected bundle %q to have an empty status, got: %+v", name, s5)
36613661
}
36623662
}
@@ -6858,7 +6858,7 @@ func TestPluginManualTriggerMultipleDiskStorage(t *testing.T) {
68586858

68596859
data, err := manager.Store.Read(ctx, txn, storage.Path{})
68606860
expData := util.MustUnmarshalJSON([]byte(`{
6861-
"p": "x1", "q": "x2",
6861+
"p": "x1", "q": "x2",
68626862
"system": {
68636863
"bundles": {"test-1": {"etag": "", "manifest": {"revision": "", "roots": ["p", "authz"]}}, "test-2": {"etag": "", "manifest": {"revision": "", "roots": ["q"]}}},
68646864
"modules": {"test-1/bar/policy.rego": {"rego_version": 1}}

‎v1/plugins/bundle/status.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package bundle
77
import (
88
"encoding/json"
99
"errors"
10+
"reflect"
1011
"strconv"
1112
"time"
1213

@@ -95,3 +96,40 @@ func (s *Status) SetError(err error) {
9596
s.Errors = nil
9697
}
9798
}
99+
100+
func (s *Status) Equal(other *Status) bool {
101+
if s == nil || other == nil {
102+
return s == nil && other == nil
103+
}
104+
105+
equal := s.Name == other.Name &&
106+
s.Type == other.Type &&
107+
s.Size == other.Size &&
108+
s.Code == other.Code &&
109+
s.Message == other.Message &&
110+
s.HTTPCode == other.HTTPCode &&
111+
s.ActiveRevision == other.ActiveRevision &&
112+
s.LastSuccessfulActivation.Equal(other.LastSuccessfulActivation) &&
113+
s.LastSuccessfulDownload.Equal(other.LastSuccessfulDownload) &&
114+
s.LastSuccessfulRequest.Equal(other.LastSuccessfulRequest) &&
115+
s.LastRequest.Equal(other.LastRequest)
116+
117+
if !equal {
118+
return false
119+
}
120+
121+
if len(s.Errors) != len(other.Errors) {
122+
return false
123+
}
124+
for i := range s.Errors {
125+
if s.Errors[i].Error() != other.Errors[i].Error() {
126+
return false
127+
}
128+
}
129+
130+
if s.Metrics != nil && other.Metrics != nil && s.Metrics.All() != nil && other.Metrics.All() != nil {
131+
return reflect.DeepEqual(s.Metrics.All(), other.Metrics.All())
132+
}
133+
134+
return s.Metrics == nil && other.Metrics == nil
135+
}

‎v1/plugins/logs/status/status.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"errors"
1010
"fmt"
1111
"net/http"
12+
"reflect"
1213
"strconv"
1314

1415
"github.com/open-policy-agent/opa/v1/metrics"
@@ -49,6 +50,10 @@ func (s *Status) SetError(err error) {
4950
}
5051
}
5152

53+
func (s *Status) Equal(other *Status) bool {
54+
return reflect.DeepEqual(s, other)
55+
}
56+
5257
type HTTPError struct {
5358
StatusCode int
5459
}

‎v1/plugins/plugins.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,14 @@ func (s *Status) String() string {
163163
return fmt.Sprintf("{%v %q}", s.State, s.Message)
164164
}
165165

166+
func (s *Status) Equal(other *Status) bool {
167+
if s == nil || other == nil {
168+
return s == nil && other == nil
169+
}
170+
171+
return s.State == other.State && s.Message == other.Message
172+
}
173+
166174
// StatusListener defines a handler to register for status updates.
167175
type StatusListener func(status map[string]*Status)
168176

‎v1/plugins/status/plugin.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"context"
1010
"encoding/json"
1111
"fmt"
12+
"maps"
1213
"net/http"
1314
"reflect"
1415

@@ -529,3 +530,20 @@ func (p *Plugin) updatePrometheusMetrics(u *UpdateRequestV1) {
529530
}
530531
}
531532
}
533+
534+
func (u UpdateRequestV1) Equal(other UpdateRequestV1) bool {
535+
return maps.Equal(u.Labels, other.Labels) &&
536+
maps.EqualFunc(u.Bundles, other.Bundles, (*bundle.Status).Equal) &&
537+
maps.EqualFunc(u.Plugins, other.Plugins, (*plugins.Status).Equal) &&
538+
u.Bundle.Equal(other.Bundle) &&
539+
u.Discovery.Equal(other.Discovery) &&
540+
u.DecisionLogs.Equal(other.DecisionLogs) &&
541+
nullSafeDeepEqual(u.Metrics, other.Metrics)
542+
}
543+
544+
func nullSafeDeepEqual(a, b interface{}) bool {
545+
if a == nil && b == nil {
546+
return true
547+
}
548+
return a != nil && b != nil && reflect.DeepEqual(a, b)
549+
}

‎v1/plugins/status/plugin_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ func TestPluginStart(t *testing.T) {
334334
},
335335
}
336336

337-
if !reflect.DeepEqual(result, exp) {
337+
if !result.Equal(exp) {
338338
t.Fatalf("Expected: %v but got: %v", exp, result)
339339
}
340340

@@ -345,7 +345,7 @@ func TestPluginStart(t *testing.T) {
345345

346346
exp.Bundles = map[string]*bundle.Status{"test": status}
347347

348-
if !reflect.DeepEqual(result, exp) {
348+
if !result.Equal(exp) {
349349
t.Fatalf("Expected: %v but got: %v", exp, result)
350350
}
351351
}
@@ -434,7 +434,7 @@ func TestPluginStartTriggerManual(t *testing.T) {
434434

435435
exp.Bundles = map[string]*bundle.Status{"test": status}
436436

437-
if !reflect.DeepEqual(result.Bundles, exp.Bundles) {
437+
if !maps.EqualFunc(result.Bundles, exp.Bundles, (*bundle.Status).Equal) {
438438
t.Fatalf("Expected: %v but got: %v", exp, result)
439439
}
440440
}
@@ -480,7 +480,7 @@ func TestPluginStartTriggerManualMultiple(t *testing.T) {
480480
exp.Bundles = map[string]*bundle.Status{"test": status}
481481
exp.Discovery = status
482482

483-
if !reflect.DeepEqual(result, exp) {
483+
if !result.Equal(exp) {
484484
t.Fatalf("Expected: %v but got: %v", exp, result)
485485
}
486486
}
@@ -632,7 +632,7 @@ func TestPluginStartBulkUpdate(t *testing.T) {
632632

633633
exp.Bundles = map[string]*bundle.Status{status.Name: status}
634634

635-
if !reflect.DeepEqual(result, exp) {
635+
if !result.Equal(exp) {
636636
t.Fatalf("Expected: %v but got: %v", exp, result)
637637
}
638638
}
@@ -729,7 +729,7 @@ func TestPluginStartDiscovery(t *testing.T) {
729729
},
730730
}
731731

732-
if !reflect.DeepEqual(result, exp) {
732+
if !result.Equal(exp) {
733733
t.Fatalf("Expected: %+v but got: %+v", exp, result)
734734
}
735735
}
@@ -772,7 +772,7 @@ func TestPluginStartDecisionLogs(t *testing.T) {
772772
},
773773
}
774774

775-
if !reflect.DeepEqual(result, exp) {
775+
if !result.Equal(exp) {
776776
t.Fatalf("Expected: %+v but got: %+v", exp, result)
777777
}
778778
}

‎v1/storage/inmem/inmem_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ func TestInMemoryWrite(t *testing.T) {
220220
t.Errorf("Test case %d (%v): expected patch error, but got nil instead", i+1, tc.note)
221221
continue
222222
}
223-
if !reflect.DeepEqual(err, tc.expected) {
223+
if err.Error() != tc.expected.Error() {
224224
t.Errorf("Test case %d (%v): expected patch error %v but got: %v", i+1, tc.note, tc.expected, err)
225225
continue
226226
}
@@ -238,7 +238,7 @@ func TestInMemoryWrite(t *testing.T) {
238238
t.Errorf("Test case %d (%v): expected get error but got: %v", i+1, tc.note, result)
239239
continue
240240
}
241-
if !reflect.DeepEqual(err, expected) {
241+
if err.Error() != expected.Error() {
242242
t.Errorf("Test case %d (%v): expected get error %v but got: %v", i+1, tc.note, expected, err)
243243
continue
244244
}

0 commit comments

Comments
 (0)