Avoid potential nil pointers in compare.go

Signed-off-by: Xie Zheng <[email protected]>
vmware-tanzu · Nov 1, 2024 · d87ac10 · d87ac10
1 parent 20379c1
commit d87ac10
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 43 deletions.
diff --git 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
+	}
 	var 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
@@ -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))
 		})
 	}
 }