From f67560a3e311faac6018265c1fdba158a97b73ed Mon Sep 17 00:00:00 2001 From: Xie Zheng Date: Thu, 31 Oct 2024 15:43:58 +0800 Subject: [PATCH] Avoid potential nil pointers in compare.go Signed-off-by: Xie Zheng --- pkg/nsx/services/common/compare.go | 20 +++++ pkg/nsx/services/common/compare_test.go | 102 ++++++++++++++---------- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/pkg/nsx/services/common/compare.go b/pkg/nsx/services/common/compare.go index e428dd0aa..9375fd71f 100644 --- a/pkg/nsx/services/common/compare.go +++ b/pkg/nsx/services/common/compare.go @@ -1,6 +1,8 @@ package common import ( + "reflect" + "github.com/vmware/vsphere-automation-sdk-go/runtime/data" "github.com/vmware/vsphere-automation-sdk-go/runtime/data/serializers/cleanjson" @@ -15,6 +17,10 @@ type Comparable interface { } func CompareResource(existing Comparable, expected Comparable) (isChanged bool) { + // avoid nil pointer + if reflect.ValueOf(existing).IsNil() || reflect.ValueOf(expected).IsNil() { + return true + } dataValueToJSONEncoder := cleanjson.NewDataValueToJsonEncoder() s1, _ := dataValueToJSONEncoder.Encode(existing.Value()) s2, _ := dataValueToJSONEncoder.Encode(expected.Value()) @@ -25,6 +31,20 @@ func CompareResource(existing Comparable, expected Comparable) (isChanged bool) } func CompareResources(existing []Comparable, expected []Comparable) (changed []Comparable, stale []Comparable) { + // remove nil item in existing and expected + for i := 0; i < len(existing); i++ { + if reflect.ValueOf(existing[i]).IsNil() { + existing = append(existing[:i], existing[i+1:]...) + i-- + } + } + for i := 0; i < len(expected); i++ { + if reflect.ValueOf(expected[i]).IsNil() { + expected = append(expected[:i], expected[i+1:]...) + i-- + } + } + stale = make([]Comparable, 0) changed = make([]Comparable, 0) diff --git a/pkg/nsx/services/common/compare_test.go b/pkg/nsx/services/common/compare_test.go index c18e95d83..19971e770 100644 --- a/pkg/nsx/services/common/compare_test.go +++ b/pkg/nsx/services/common/compare_test.go @@ -12,87 +12,103 @@ type mockComparable struct { value data.DataValue } -func (m mockComparable) Key() string { +func (m *mockComparable) Key() string { return m.key } -func (m mockComparable) Value() data.DataValue { +func (m *mockComparable) Value() data.DataValue { return m.value } +func mockComparableToComparable(mc []*mockComparable) []Comparable { + res := make([]Comparable, 0, len(mc)) + for i := range mc { + res = append(res, (*mockComparable)(mc[i])) + } + return res +} + +func comparableToMockComparable(c []Comparable) []*mockComparable { + res := make([]*mockComparable, 0, len(c)) + for i := range c { + res = append(res, c[i].(*mockComparable)) + } + return res +} + func TestCompareResources(t *testing.T) { tests := []struct { name string - existing []Comparable - expected []Comparable - wantChanged []Comparable - wantStale []Comparable + existing []*mockComparable + expected []*mockComparable + wantChanged []*mockComparable + wantStale []*mockComparable }{ { name: "No changes", - existing: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + existing: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2")}, }, - expected: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + expected: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2")}, }, - wantChanged: []Comparable{}, - wantStale: []Comparable{}, + wantChanged: []*mockComparable{}, + wantStale: []*mockComparable{}, }, { name: "Changed resources", - existing: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + existing: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2")}, }, - expected: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2_changed")}, + expected: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2_changed")}, }, - wantChanged: []Comparable{ - mockComparable{key: "key2", value: data.NewStringValue("value2_changed")}, + wantChanged: []*mockComparable{ + {key: "key2", value: data.NewStringValue("value2_changed")}, }, - wantStale: []Comparable{}, + wantStale: []*mockComparable{}, }, { name: "Stale resources", - existing: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + existing: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2")}, }, - expected: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, + expected: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, }, - wantChanged: []Comparable{}, - wantStale: []Comparable{ - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + wantChanged: []*mockComparable{}, + wantStale: []*mockComparable{ + {key: "key2", value: data.NewStringValue("value2")}, }, }, { name: "Changed and stale resources", - existing: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1")}, - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + existing: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1")}, + {key: "key2", value: data.NewStringValue("value2")}, }, - expected: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1_changed")}, + expected: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1_changed")}, }, - wantChanged: []Comparable{ - mockComparable{key: "key1", value: data.NewStringValue("value1_changed")}, + wantChanged: []*mockComparable{ + {key: "key1", value: data.NewStringValue("value1_changed")}, }, - wantStale: []Comparable{ - mockComparable{key: "key2", value: data.NewStringValue("value2")}, + wantStale: []*mockComparable{ + {key: "key2", value: data.NewStringValue("value2")}, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotChanged, gotStale := CompareResources(tt.existing, tt.expected) - assert.Equal(t, tt.wantChanged, gotChanged) - assert.Equal(t, tt.wantStale, gotStale) + gotChanged, gotStale := CompareResources(mockComparableToComparable(tt.existing), mockComparableToComparable(tt.expected)) + assert.Equal(t, tt.wantChanged, comparableToMockComparable(gotChanged)) + assert.Equal(t, tt.wantStale, comparableToMockComparable(gotStale)) }) } }