From 70be0c86e1a59f36f06c2b9f04d28dfa86db0c9a Mon Sep 17 00:00:00 2001 From: Eoghan Lawless Date: Wed, 23 Apr 2025 03:47:05 -0700 Subject: [PATCH 1/5] chore: add logs to pagination Signed-off-by: Eoghan Lawless --- internal/pagination/pagination.go | 72 ++++++++++++++++--------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/internal/pagination/pagination.go b/internal/pagination/pagination.go index 3a45a9b8..e0f70f73 100644 --- a/internal/pagination/pagination.go +++ b/internal/pagination/pagination.go @@ -15,6 +15,24 @@ import ( "github.com/open-edge-platform/cluster-manager/v2/pkg/api" ) +var ( + // validOrderByFields is a map of valid order by fields + validOrderByFields = map[string]bool{ + "name": true, + "kubernetesVersion": true, + "providerStatus": true, + "lifecyclePhase": true, + } + + // validFilterFields is a map of valid filter fields + validFilterFields = map[string]bool{ + "name": true, + "kubernetesVersion": true, + "providerStatus": true, + "lifecyclePhase": true, + } +) + type Filter struct { Name string Value string @@ -245,44 +263,30 @@ func ValidateParams(params any) (pageSize, offset *int, orderBy, filter *string, return nil, nil, nil, nil, fmt.Errorf("invalid offset: must be non-negative") } - if orderBy != nil { - validOrderByFields := map[string]bool{ - "name": true, - "kubernetesVersion": true, - "providerStatus": true, - "lifecyclePhase": true, - } - orderByParts := strings.Split(*orderBy, " ") - if len(orderByParts) == 1 { - orderBy = convert.Ptr(orderByParts[0] + " asc") - } else if len(orderByParts) == 2 { - if !validOrderByFields[orderByParts[0]] || (orderByParts[1] != "asc" && orderByParts[1] != "desc") { - return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field") - } - } else if *orderBy == "" { + if orderBy == nil || *orderBy == "" { + return nil, nil, nil, nil, fmt.Errorf("invalid orderBy: cannot be empty") + } + + if filter == nil || *filter == "" { + return nil, nil, nil, nil, fmt.Errorf("invalid filter: cannot be empty") + } + + orderByParts := strings.Split(*orderBy, " ") + if len(orderByParts) == 1 { + orderBy = convert.Ptr(orderByParts[0] + " asc") + } else if len(orderByParts) == 2 { + if !validOrderByFields[orderByParts[0]] || (orderByParts[1] != "asc" && orderByParts[1] != "desc") { return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field") } } - if filter != nil { - if *filter == "" { - return nil, nil, nil, nil, fmt.Errorf("invalid filter: cannot be empty") - } - validFilterFields := map[string]bool{ - "name": true, - "kubernetesVersion": true, - "providerStatus": true, - "lifecyclePhase": true, - "version": true, - } - filterParts := strings.FieldsFunc(*filter, func(r rune) bool { - return r == ' ' || r == 'O' || r == 'R' || r == 'A' || r == 'N' || r == 'D' - }) - for _, part := range filterParts { - subParts := strings.Split(part, "=") - if len(subParts) != 2 || !validFilterFields[subParts[0]] { - return nil, nil, nil, nil, fmt.Errorf("invalid filter field") - } + filterParts := strings.FieldsFunc(*filter, func(r rune) bool { + return r == ' ' || r == 'O' || r == 'R' || r == 'A' || r == 'N' || r == 'D' + }) + for _, part := range filterParts { + subParts := strings.Split(part, "=") + if len(subParts) != 2 || !validFilterFields[subParts[0]] { + return nil, nil, nil, nil, fmt.Errorf("invalid filter field") } } From b7e7658407c01f0d2e1888e76044d4557e201ff7 Mon Sep 17 00:00:00 2001 From: Eoghan Lawless Date: Wed, 23 Apr 2025 04:15:11 -0700 Subject: [PATCH 2/5] chore: add the logging Signed-off-by: Eoghan Lawless --- internal/pagination/pagination.go | 45 ++++++++++++++++--------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/internal/pagination/pagination.go b/internal/pagination/pagination.go index e0f70f73..c1674dd8 100644 --- a/internal/pagination/pagination.go +++ b/internal/pagination/pagination.go @@ -53,6 +53,7 @@ func parseFilter(filterParameter string) ([]*Filter, bool, error) { if filterParameter == "" { return nil, false, nil } + // Replace the matched pattern in regexp 'normalizeEqualsRe' with just '=' (basically the spaces and tabs are removed) normalizedFilterParameter := normalizeEqualsRe.ReplaceAllString(filterParameter, "=") @@ -73,7 +74,7 @@ func parseFilter(filterParameter string) ([]*Filter, bool, error) { selectors := strings.Split(element, "=") if currentFilter != nil || len(selectors) != 2 || selectors[0] == "" || selectors[1] == "" { // Error condition - too many equals - return nil, false, fmt.Errorf("filter: invalid filter request: %s", elements) + return nil, false, fmt.Errorf("filter: invalid filter request (=): %s", elements) } currentFilter = &Filter{ Name: selectors[0], @@ -81,13 +82,13 @@ func parseFilter(filterParameter string) ([]*Filter, bool, error) { } case element == "OR": if currentFilter == nil || index == len(elements)-1 { - return nil, false, fmt.Errorf("filter: invalid filter request: %s", elements) + return nil, false, fmt.Errorf("filter: invalid filter request (OR): %s", elements) } filters = append(filters, currentFilter) currentFilter = nil case element == "AND": if currentFilter == nil || index == len(elements)-1 { - return nil, false, fmt.Errorf("filter: invalid filter request: %s", elements) + return nil, false, fmt.Errorf("filter: invalid filter request (AND): %s", elements) } filters = append(filters, currentFilter) currentFilter = nil @@ -116,6 +117,7 @@ func parseOrderBy(orderByParameter string) ([]*OrderBy, error) { if orderByParameter == "" { return nil, nil } + // orderBy commands should be separated by ',' if there are more than one. // Split them by ',' delimiter. elements := strings.Split(orderByParameter, ",") @@ -128,6 +130,7 @@ func parseOrderBy(orderByParameter string) ([]*OrderBy, error) { if len(direction) == 0 || len(direction) > 2 { return nil, errors.New("invalid order by: " + element) } + if len(direction) == 2 { switch direction[1] { case "asc": @@ -135,7 +138,7 @@ func parseOrderBy(orderByParameter string) ([]*OrderBy, error) { case "desc": descending = true default: - return nil, errors.New("invalid order by: " + element) + return nil, errors.New("invalid order by direction: " + element) } } orderBys = append(orderBys, &OrderBy{ @@ -167,6 +170,7 @@ func computePageRange(pageSize int32, offset int32, totalCount int) (int, int) { func PaginateItems[T any](items []T, pageSize, offset int) (*[]T, error) { paginatedItems, err := applyPagination(items, pageSize, offset) if err != nil { + slog.Error("failed to apply pagination", "pageSize", pageSize, "offset", offset, "error", err) return nil, err } return &paginatedItems, nil @@ -183,6 +187,7 @@ func applyPagination[T any](items []T, pageSize, offset int) ([]T, error) { func FilterItems[T any](items []T, filter string, filterFunc func(T, *Filter) bool) ([]T, error) { filters, useAnd, err := parseFilter(filter) if err != nil { + slog.Error("failed to parse filter", "filter", filter, "error", err) return nil, err } @@ -211,17 +216,20 @@ func FilterItems[T any](items []T, filter string, filterFunc func(T, *Filter) bo } } + slog.Debug("applied filter", "filter", filter, "items", filteredItems) + return filteredItems, nil } func OrderItems[T any](items []T, orderBy string, orderFunc func(T, T, *OrderBy) bool) ([]T, error) { orderBys, err := parseOrderBy(orderBy) if err != nil { + slog.Error("failed to parse order by", "orderBy", orderBy, "error", err) return nil, err } orderedItems := applyOrderBy(items, orderBys, orderFunc) - slog.Debug("applied order by", "orderBy", orderBy, "orderedItems", orderedItems) + slog.Debug("applied order by", "orderBy", orderBy, "items", orderedItems) return orderedItems, nil } @@ -244,30 +252,23 @@ func extractParamsFields(params any) (pageSize, offset *int, orderBy, filter *st case api.GetV2TemplatesParams: return p.PageSize, p.Offset, p.OrderBy, p.Filter, nil default: - return nil, nil, nil, nil, fmt.Errorf("unsupported params type") + return nil, nil, nil, nil, fmt.Errorf("unsupported params type: %v (%v)", p, params) } } // ValidateParams validates the incoming parameters for pagination func ValidateParams(params any) (pageSize, offset *int, orderBy, filter *string, err error) { pageSize, offset, orderBy, filter, err = extractParamsFields(params) - if err != nil { - return nil, nil, nil, nil, err - } - - if pageSize == nil || *pageSize <= 0 { + switch { + case err != nil: + return nil, nil, nil, nil, fmt.Errorf("failed to extract params fields: %w", err) + case pageSize == nil, *pageSize <= 0: return nil, nil, nil, nil, fmt.Errorf("invalid pageSize: must be greater than 0") - } - - if offset == nil || *offset < 0 { + case offset == nil, *offset < 0: return nil, nil, nil, nil, fmt.Errorf("invalid offset: must be non-negative") - } - - if orderBy == nil || *orderBy == "" { + case orderBy == nil, *orderBy == "": return nil, nil, nil, nil, fmt.Errorf("invalid orderBy: cannot be empty") - } - - if filter == nil || *filter == "" { + case filter == nil, *filter == "": return nil, nil, nil, nil, fmt.Errorf("invalid filter: cannot be empty") } @@ -276,7 +277,7 @@ func ValidateParams(params any) (pageSize, offset *int, orderBy, filter *string, orderBy = convert.Ptr(orderByParts[0] + " asc") } else if len(orderByParts) == 2 { if !validOrderByFields[orderByParts[0]] || (orderByParts[1] != "asc" && orderByParts[1] != "desc") { - return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field") + return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field: %s", *orderBy) } } @@ -286,7 +287,7 @@ func ValidateParams(params any) (pageSize, offset *int, orderBy, filter *string, for _, part := range filterParts { subParts := strings.Split(part, "=") if len(subParts) != 2 || !validFilterFields[subParts[0]] { - return nil, nil, nil, nil, fmt.Errorf("invalid filter field") + return nil, nil, nil, nil, fmt.Errorf("invalid filter field: %s", *filter) } } From 27b1dd50fe4d2e6c6cbe485d2586558ffa6d9c3b Mon Sep 17 00:00:00 2001 From: Eoghan Lawless Date: Fri, 25 Apr 2025 02:28:59 -0700 Subject: [PATCH 3/5] revert: version field removal Signed-off-by: Eoghan Lawless --- internal/pagination/pagination.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/pagination/pagination.go b/internal/pagination/pagination.go index c1674dd8..b6dafa24 100644 --- a/internal/pagination/pagination.go +++ b/internal/pagination/pagination.go @@ -22,6 +22,7 @@ var ( "kubernetesVersion": true, "providerStatus": true, "lifecyclePhase": true, + "version": true, } // validFilterFields is a map of valid filter fields @@ -30,6 +31,7 @@ var ( "kubernetesVersion": true, "providerStatus": true, "lifecyclePhase": true, + "version": true, } ) From 5061ff622bfaaf3afebe4409051caaa971bac06d Mon Sep 17 00:00:00 2001 From: Eoghan Lawless Date: Mon, 28 Apr 2025 02:01:55 -0700 Subject: [PATCH 4/5] revert: refactor Signed-off-by: Eoghan Lawless --- internal/pagination/pagination.go | 53 ++++++++++++++++++------------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/internal/pagination/pagination.go b/internal/pagination/pagination.go index b6dafa24..72cf8402 100644 --- a/internal/pagination/pagination.go +++ b/internal/pagination/pagination.go @@ -261,35 +261,44 @@ func extractParamsFields(params any) (pageSize, offset *int, orderBy, filter *st // ValidateParams validates the incoming parameters for pagination func ValidateParams(params any) (pageSize, offset *int, orderBy, filter *string, err error) { pageSize, offset, orderBy, filter, err = extractParamsFields(params) - switch { - case err != nil: - return nil, nil, nil, nil, fmt.Errorf("failed to extract params fields: %w", err) - case pageSize == nil, *pageSize <= 0: + if err != nil { + return nil, nil, nil, nil, err + } + if pageSize == nil || *pageSize <= 0 { return nil, nil, nil, nil, fmt.Errorf("invalid pageSize: must be greater than 0") - case offset == nil, *offset < 0: + } + if offset == nil || *offset < 0 { return nil, nil, nil, nil, fmt.Errorf("invalid offset: must be non-negative") - case orderBy == nil, *orderBy == "": - return nil, nil, nil, nil, fmt.Errorf("invalid orderBy: cannot be empty") - case filter == nil, *filter == "": - return nil, nil, nil, nil, fmt.Errorf("invalid filter: cannot be empty") } - orderByParts := strings.Split(*orderBy, " ") - if len(orderByParts) == 1 { - orderBy = convert.Ptr(orderByParts[0] + " asc") - } else if len(orderByParts) == 2 { - if !validOrderByFields[orderByParts[0]] || (orderByParts[1] != "asc" && orderByParts[1] != "desc") { - return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field: %s", *orderBy) + if orderBy != nil { + if *orderBy == "" { + return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field") + } + + orderByParts := strings.Split(*orderBy, " ") + if len(orderByParts) == 1 { + orderBy = convert.Ptr(orderByParts[0] + " asc") + } else if len(orderByParts) == 2 { + if !validOrderByFields[orderByParts[0]] || (orderByParts[1] != "asc" && orderByParts[1] != "desc") { + return nil, nil, nil, nil, fmt.Errorf("invalid orderBy field") + } } } - filterParts := strings.FieldsFunc(*filter, func(r rune) bool { - return r == ' ' || r == 'O' || r == 'R' || r == 'A' || r == 'N' || r == 'D' - }) - for _, part := range filterParts { - subParts := strings.Split(part, "=") - if len(subParts) != 2 || !validFilterFields[subParts[0]] { - return nil, nil, nil, nil, fmt.Errorf("invalid filter field: %s", *filter) + if filter != nil { + if *filter == "" { + return nil, nil, nil, nil, fmt.Errorf("invalid filter: cannot be empty") + } + + filterParts := strings.FieldsFunc(*filter, func(r rune) bool { + return r == ' ' || r == 'O' || r == 'R' || r == 'A' || r == 'N' || r == 'D' + }) + for _, part := range filterParts { + subParts := strings.Split(part, "=") + if len(subParts) != 2 || !validFilterFields[subParts[0]] { + return nil, nil, nil, nil, fmt.Errorf("invalid filter field") + } } } From ca4ff8822a0d6e5ffcfefaab72e88154ae7655a7 Mon Sep 17 00:00:00 2001 From: Eoghan Lawless Date: Mon, 28 Apr 2025 03:05:36 -0700 Subject: [PATCH 5/5] chore: address comments Signed-off-by: Eoghan Lawless --- internal/pagination/pagination.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/internal/pagination/pagination.go b/internal/pagination/pagination.go index 72cf8402..d4e5f174 100644 --- a/internal/pagination/pagination.go +++ b/internal/pagination/pagination.go @@ -5,7 +5,6 @@ package pagination import ( "errors" "fmt" - "log/slog" "math" "regexp" "sort" @@ -172,9 +171,9 @@ func computePageRange(pageSize int32, offset int32, totalCount int) (int, int) { func PaginateItems[T any](items []T, pageSize, offset int) (*[]T, error) { paginatedItems, err := applyPagination(items, pageSize, offset) if err != nil { - slog.Error("failed to apply pagination", "pageSize", pageSize, "offset", offset, "error", err) - return nil, err + return nil, fmt.Errorf("failed to apply pagination: %w", err) } + return &paginatedItems, nil } @@ -183,14 +182,14 @@ func applyPagination[T any](items []T, pageSize, offset int) ([]T, error) { if end == -1 { return nil, fmt.Errorf("no items to paginate") } + return items[start:end], nil } func FilterItems[T any](items []T, filter string, filterFunc func(T, *Filter) bool) ([]T, error) { filters, useAnd, err := parseFilter(filter) if err != nil { - slog.Error("failed to parse filter", "filter", filter, "error", err) - return nil, err + return nil, fmt.Errorf("failed to parse filter: %w", err) } var filteredItems []T @@ -218,21 +217,16 @@ func FilterItems[T any](items []T, filter string, filterFunc func(T, *Filter) bo } } - slog.Debug("applied filter", "filter", filter, "items", filteredItems) - return filteredItems, nil } func OrderItems[T any](items []T, orderBy string, orderFunc func(T, T, *OrderBy) bool) ([]T, error) { orderBys, err := parseOrderBy(orderBy) if err != nil { - slog.Error("failed to parse order by", "orderBy", orderBy, "error", err) - return nil, err + return nil, fmt.Errorf("failed to parse order by: %w", err) } - orderedItems := applyOrderBy(items, orderBys, orderFunc) - slog.Debug("applied order by", "orderBy", orderBy, "items", orderedItems) - return orderedItems, nil + return applyOrderBy(items, orderBys, orderFunc), nil } func applyOrderBy[T any](items []T, orderBys []*OrderBy, orderFunc orderFunc[T]) []T { @@ -244,6 +238,7 @@ func applyOrderBy[T any](items []T, orderBys []*OrderBy, orderFunc orderFunc[T]) } return false }) + return items }