diff --git a/cmd/collectors/rest/rest.go b/cmd/collectors/rest/rest.go index 04f7a4a60..616affc2f 100644 --- 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 index 2c41759ff..b4b4373dc 100644 --- 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 index be04eadaf..50b3b797b 100644 --- a/cmd/tools/rest/href_builder.go +++ b/cmd/tools/rest/href_builder.go @@ -1,14 +1,18 @@ package rest import ( + "log/slog" "slices" "strconv" "strings" ) +const URLMaxLimit = 8 * 1024 + type HrefBuilder struct { apiPath string fields []string + hiddenFields []string counterSchema string filter []string queryFields string @@ -32,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 @@ -76,6 +85,31 @@ func (b *HrefBuilder) Build() string { href.WriteString("?return_records=true") + 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 + } + } + } + + if len(strings.Join(b.fields, ",")) > URLMaxLimit { + b.fields = append([]string{"*"}, b.hiddenFields...) + if len(strings.Join(b.fields, ",")) > URLMaxLimit { + slog.Info("fields converting to * due to URL max limit") + b.fields = []string{"*"} + } else { + slog.Info("fields converting to *,hiddenFields due to URL max limit") + } + } + // 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 new file mode 100644 index 000000000..26e32b0ea --- /dev/null +++ b/cmd/tools/rest/href_builder_test.go @@ -0,0 +1,100 @@ +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 + 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(true). + Build() + + if hrefTest1 != expectedHrefTest1 { + t.Errorf("hrefTest1 should be %s but got %s", expectedHrefTest1, hrefTest1) + } + + testFields = make([]string, 0) + for i := range URLMaxLimit / len("Test") { + testFields = append(testFields, "Test"+strconv.Itoa(i)) + } + + 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(true). + Build() + + if hrefTest2 != expectedHrefTest2 { + 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) + } + }) + } +}