feat: handled review comments

NetApp · Oct 3, 2024 · bed7d26 · bed7d26
1 parent 3becf39
commit bed7d26
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 111 deletions.
diff --git a/cmd/collectors/rest/rest.go b/cmd/collectors/rest/rest.go
@@ -116,20 +116,6 @@ func (r *Rest) Fields(prop *prop) []string {
 			fields = []string{"*"}
 		}
 	}
-	if len(prop.HiddenFields) > 0 {
-		fieldsMap := make(map[string]bool)
-		for _, field := range fields {
-			fieldsMap[field] = true
-		}
-
-		// append hidden fields
-		for _, hiddenField := range prop.HiddenFields {
-			if _, exists := fieldsMap[hiddenField]; !exists {
-				fields = append(fields, hiddenField)
-				fieldsMap[hiddenField] = true
-			}
-		}
-	}
 	return fields
 }
 
@@ -368,6 +354,7 @@ func (r *Rest) updateHref() {
 	r.Prop.Href = rest.NewHrefBuilder().
 		APIPath(r.Prop.Query).
 		Fields(r.Fields(r.Prop)).
+		HiddenFields(r.Prop.HiddenFields).
 		Filter(r.Prop.Filter).
 		ReturnTimeout(r.Prop.ReturnTimeOut).
 		IsIgnoreUnknownFieldsEnabled(r.isIgnoreUnknownFieldsEnabled).
@@ -377,6 +364,7 @@ func (r *Rest) updateHref() {
 		e.prop.Href = rest.NewHrefBuilder().
 			APIPath(r.query(e)).
 			Fields(r.Fields(e.prop)).
+			HiddenFields(e.prop.HiddenFields).
 			Filter(r.filter(e)).
 			ReturnTimeout(r.Prop.ReturnTimeOut).
 			IsIgnoreUnknownFieldsEnabled(r.isIgnoreUnknownFieldsEnabled).

diff --git a/cmd/collectors/rest/rest_test.go b/cmd/collectors/rest/rest_test.go
@@ -268,7 +268,7 @@ func TestFields(t *testing.T) {
 		expectedResult []string
 	}{
 		{
-			name: "Test with valid fields and no hidden fields",
+			name: "Test with valid fields",
 			r: &Rest{
 				isIgnoreUnknownFieldsEnabled: true,
 			},
@@ -287,7 +287,7 @@ func TestFields(t *testing.T) {
 			},
 		},
 		{
-			name: "Test with invalid fields and no hidden fields",
+			name: "Test with invalid fields",
 			r: &Rest{
 				isIgnoreUnknownFieldsEnabled: true,
 			},
@@ -302,32 +302,7 @@ func TestFields(t *testing.T) {
 			expectedResult: []string{"*"},
 		},
 		{
-			name: "Test with valid fields and hidden fields",
-			r: &Rest{
-				isIgnoreUnknownFieldsEnabled: true,
-			},
-			p: &prop{
-				Fields: []string{
-					"uuid",
-					"block_storage.primary.disk_type",
-					"block_storage.primary.raid_type",
-				},
-				HiddenFields: []string{
-					"hidden_field1",
-					"hidden_field2",
-				},
-				IsPublic: true,
-			},
-			expectedResult: []string{
-				"uuid",
-				"block_storage.primary.disk_type",
-				"block_storage.primary.raid_type",
-				"hidden_field1",
-				"hidden_field2",
-			},
-		},
-		{
-			name: "Test with valid fields and hidden fields and prior versions to 9.11.1",
+			name: "Test with valid fields and prior versions to 9.11.1",
 			r: &Rest{
 				isIgnoreUnknownFieldsEnabled: false,
 			},
@@ -337,20 +312,14 @@ func TestFields(t *testing.T) {
 					"block_storage.primary.disk_type",
 					"block_storage.primary.raid_type",
 				},
-				HiddenFields: []string{
-					"hidden_field1",
-					"hidden_field2",
-				},
 				IsPublic: true,
 			},
 			expectedResult: []string{
 				"*",
-				"hidden_field1",
-				"hidden_field2",
 			},
 		},
 		{
-			name: "Test with valid fields and no hidden fields for private API",
+			name: "Test with valid fields for private API",
 			r: &Rest{
 				isIgnoreUnknownFieldsEnabled: false,
 			},
@@ -368,65 +337,6 @@ func TestFields(t *testing.T) {
 				"block_storage.primary.raid_type",
 			},
 		},
-		{
-			name: "Test with invalid fields and no hidden fields, ignore unknown fields disabled",
-			r: &Rest{
-				isIgnoreUnknownFieldsEnabled: false,
-			},
-			p: &prop{
-				Fields: []string{
-					"uuid",
-					"cloud_storage.stores.0.cloud_store.name",
-					"block_storage.primary.raid_type",
-				},
-				IsPublic: true,
-			},
-			expectedResult: []string{"*"},
-		},
-		{
-			name: "Test with invalid fields and no hidden fields for private API",
-			r: &Rest{
-				isIgnoreUnknownFieldsEnabled: true,
-			},
-			p: &prop{
-				Fields: []string{
-					"uuid",
-					"cloud_storage.stores.0.cloud_store.name",
-					"block_storage.primary.raid_type",
-				},
-				IsPublic: false,
-			},
-			expectedResult: []string{
-				"uuid",
-				"cloud_storage.stores.0.cloud_store.name",
-				"block_storage.primary.raid_type",
-			},
-		},
-		{
-			name: "Test with valid fields and hidden fields for private API",
-			r: &Rest{
-				isIgnoreUnknownFieldsEnabled: true,
-			},
-			p: &prop{
-				Fields: []string{
-					"uuid",
-					"block_storage.primary.disk_type",
-					"block_storage.primary.raid_type",
-				},
-				HiddenFields: []string{
-					"hidden_field1",
-					"hidden_field2",
-				},
-				IsPublic: true,
-			},
-			expectedResult: []string{
-				"uuid",
-				"block_storage.primary.disk_type",
-				"block_storage.primary.raid_type",
-				"hidden_field1",
-				"hidden_field2",
-			},
-		},
 	}
 
 	for _, tt := range tests {

diff --git a/cmd/tools/rest/href_builder.go b/cmd/tools/rest/href_builder.go
@@ -12,6 +12,7 @@ const URLMaxLimit = 8 * 1024
 type HrefBuilder struct {
 	apiPath                      string
 	fields                       []string
+	hiddenFields                 []string
 	counterSchema                string
 	filter                       []string
 	queryFields                  string
@@ -35,6 +36,11 @@ func (b *HrefBuilder) Fields(fields []string) *HrefBuilder {
 	return b
 }
 
+func (b *HrefBuilder) HiddenFields(hiddenFields []string) *HrefBuilder {
+	b.hiddenFields = hiddenFields
+	return b
+}
+
 func (b *HrefBuilder) CounterSchema(counterSchema []string) *HrefBuilder {
 	b.counterSchema = strings.Join(counterSchema, ",")
 	return b
@@ -83,6 +89,22 @@ func (b *HrefBuilder) Build() string {
 		fmt.Printf("converting to * due to URL max limit")
 		b.fields = []string{"*"}
 	}
+
+	if len(b.hiddenFields) > 0 {
+		fieldsMap := make(map[string]bool)
+		for _, field := range b.fields {
+			fieldsMap[field] = true
+		}
+
+		// append hidden fields
+		for _, hiddenField := range b.hiddenFields {
+			if _, exists := fieldsMap[hiddenField]; !exists {
+				b.fields = append(b.fields, hiddenField)
+				fieldsMap[hiddenField] = true
+			}
+		}
+	}
+
 	// Sort fields so that the href is deterministic
 	slices.Sort(b.fields)
 

diff --git a/cmd/tools/rest/href_builder_test.go b/cmd/tools/rest/href_builder_test.go
@@ -1,21 +1,23 @@
 package rest
 
 import (
+	"github.com/google/go-cmp/cmp"
 	"strconv"
 	"testing"
 )
 
 func TestBuild(t *testing.T) {
-
 	testQuery := "storage/volumes"
 	testFields := []string{"name", "svm"}
+	testHiddenFields := []string{"statistics"}
 	testFilter := []string{""}
 	testReturnTimeout := 10
 	isIgnoreUnknownFieldsEnabled := true
-	expectedHrefTest1 := "api/storage/volumes?return_records=true&fields=name,svm&return_timeout=10&ignore_unknown_fields=true"
+	expectedHrefTest1 := "api/storage/volumes?return_records=true&fields=name,statistics,svm&return_timeout=10&ignore_unknown_fields=true"
 	hrefTest1 := NewHrefBuilder().
 		APIPath(testQuery).
 		Fields(testFields).
+		HiddenFields(testHiddenFields).
 		Filter(testFilter).
 		ReturnTimeout(&testReturnTimeout).
 		IsIgnoreUnknownFieldsEnabled(isIgnoreUnknownFieldsEnabled).
@@ -30,10 +32,11 @@ func TestBuild(t *testing.T) {
 		testFields = append(testFields, "Test"+strconv.Itoa(i))
 	}
 
-	expectedHrefTest2 := "api/storage/volumes?return_records=true&fields=*&return_timeout=10&ignore_unknown_fields=true"
+	expectedHrefTest2 := "api/storage/volumes?return_records=true&fields=*,statistics&return_timeout=10&ignore_unknown_fields=true"
 	hrefTest2 := NewHrefBuilder().
 		APIPath(testQuery).
 		Fields(testFields).
+		HiddenFields(testHiddenFields).
 		Filter(testFilter).
 		ReturnTimeout(&testReturnTimeout).
 		IsIgnoreUnknownFieldsEnabled(isIgnoreUnknownFieldsEnabled).
@@ -43,3 +46,56 @@ func TestBuild(t *testing.T) {
 		t.Errorf("hrefTest2 should be %s but got %s", expectedHrefTest2, hrefTest2)
 	}
 }
+
+func TestFields(t *testing.T) {
+	tests := []struct {
+		name           string
+		fields         []string
+		hiddenFields   []string
+		expectedResult []string
+	}{
+		{
+			name: "Test with fields and no hidden fields",
+			fields: []string{
+				"uuid",
+				"block_storage.primary.disk_type",
+				"block_storage.primary.raid_type",
+			},
+			expectedResult: []string{
+				"block_storage.primary.disk_type",
+				"block_storage.primary.raid_type",
+				"uuid",
+			},
+		},
+		{
+			name: "Test with fields and hidden fields",
+			fields: []string{
+				"uuid",
+				"block_storage.primary.disk_type",
+				"block_storage.primary.raid_type",
+			},
+			hiddenFields: []string{
+				"hidden_field1",
+				"hidden_field2",
+			},
+			expectedResult: []string{
+				"block_storage.primary.disk_type",
+				"block_storage.primary.raid_type",
+				"hidden_field1",
+				"hidden_field2",
+				"uuid",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hBuilder := NewHrefBuilder()
+			hBuilder.Fields(tt.fields).HiddenFields(tt.hiddenFields).Build()
+			diff := cmp.Diff(hBuilder.fields, tt.expectedResult)
+			if diff != "" {
+				t.Errorf("Mismatch (-got +want):\n%s", diff)
+			}
+		})
+	}
+}