From c0f7ad052e1acd97d35a146833cb2db1b290da66 Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Tue, 21 Jul 2026 15:49:41 -0500 Subject: [PATCH 1/2] HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer Extract query parameter parsing from pkg/services/types.go into pkg/handlers/list_params.go. Use go-playground/validator for struct-level validation and return structured error details via ValidationWithDetails. Remove defensive size checks from generic.go service layer (now validated at the handler boundary) while keeping a guard for Size <= 0. Update stale mockgen example in docs/development.md. --- docs/development.md | 2 +- pkg/handlers/framework.go | 3 +- pkg/handlers/list_params.go | 174 +++++++++++ pkg/handlers/list_params_test.go | 321 +++++++++++++++++++++ pkg/handlers/resource_handler.go | 2 +- pkg/handlers/resource_status_handler.go | 2 +- pkg/handlers/root_resource_handler.go | 4 +- pkg/services/generic.go | 18 +- pkg/services/resource.go | 8 +- pkg/services/types.go | 135 +-------- pkg/services/types_test.go | 369 ------------------------ 11 files changed, 510 insertions(+), 528 deletions(-) create mode 100644 pkg/handlers/list_params.go create mode 100644 pkg/handlers/list_params_test.go mode change 100755 => 100644 pkg/services/types.go delete mode 100644 pkg/services/types_test.go diff --git a/docs/development.md b/docs/development.md index 7592e509..33ff4a51 100644 --- a/docs/development.md +++ b/docs/development.md @@ -306,7 +306,7 @@ Mock implementations of service interfaces are used for unit testing. Mocks are Service files contain `//go:generate` directives that specify how to generate mocks: ```go -//go:generate mockgen-v0.6.0 -source=resource.go -package=services -destination=resource_mock.go +//go:generate go tool -modfile=../../tools/go.mod mockgen -source=resource.go -package=services -destination=resource_mock.go ``` **Commands**: diff --git a/pkg/handlers/framework.go b/pkg/handlers/framework.go index e178ff57..c8539a4c 100755 --- a/pkg/handlers/framework.go +++ b/pkg/handlers/framework.go @@ -10,7 +10,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/response" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" ) // handlerConfig defines the common things each REST controller must do. @@ -59,7 +58,7 @@ func handleError(r *http.Request, w http.ResponseWriter, err *errors.ServiceErro // Note: This function only validates the fields parameter, not pagination parameters, // to avoid rejecting irrelevant query params on single-resource GET endpoints. func applyFieldFilter(r *http.Request, presented interface{}) (interface{}, *errors.ServiceError) { - fields := services.ParseFieldsParameter(r.URL.Query()) + fields := ensureIDField(normalizeList(r.URL.Query()["fields"])) if fields != nil { filtered, filterErr := presenters.FilterSingle(fields, presented) if filterErr != nil { diff --git a/pkg/handlers/list_params.go b/pkg/handlers/list_params.go new file mode 100644 index 00000000..4343b495 --- /dev/null +++ b/pkg/handlers/list_params.go @@ -0,0 +1,174 @@ +package handlers + +import ( + "net/url" + "slices" + "strconv" + "strings" + + "github.com/go-playground/validator/v10" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" +) + +const defaultOrder = "created_time desc" + +type listParams struct { + RefType string `validate:"required_with=RefTargetID"` + RefTargetID string `validate:"required_with=RefType"` + Size int64 `validate:"min=1,max=100"` + Page int `validate:"min=1,max=10000000"` +} + +var listParamsValidator = validator.New() + +func parseListParams(query url.Values) (*services.ListArguments, *errors.ServiceError) { + p, formatErr := bindListParams(query) + if formatErr != nil { + return nil, formatErr + } + + if err := validateListParams(p); err != nil { + return nil, err + } + + args := &services.ListArguments{ + Page: p.Page, + Size: p.Size, + Search: strings.TrimSpace(query.Get("search")), + RefType: p.RefType, + RefTargetID: p.RefTargetID, + Fields: ensureIDField(normalizeList(query["fields"])), + Order: normalizeList(query["order"]), + } + + if len(args.Order) == 0 { + args.Order = []string{defaultOrder} + } + + return args, nil +} + +func bindListParams(query url.Values) (*listParams, *errors.ServiceError) { + defaults := services.NewListArguments() + p := &listParams{ + Page: defaults.Page, + Size: defaults.Size, + RefType: strings.TrimSpace(query.Get("ref_type")), + RefTargetID: strings.TrimSpace(query.Get("ref_target_id")), + } + + var formatErrors []errors.ValidationDetail + + if v := strings.TrimSpace(query.Get("page")); v != "" { + page, err := strconv.Atoi(v) + if err != nil { + formatErrors = append(formatErrors, errors.ValidationDetail{ + Field: "page", + Value: v, + Constraint: "format", + Message: "must be a valid integer", + }) + } else { + p.Page = page + } + } + + if v := strings.TrimSpace(query.Get("size")); v != "" { + size, err := strconv.ParseInt(v, 10, 64) + if err != nil { + formatErrors = append(formatErrors, errors.ValidationDetail{ + Field: "size", + Value: v, + Constraint: "format", + Message: "must be a valid integer", + }) + } else { + p.Size = size + } + } + + if len(formatErrors) > 0 { + return nil, errors.ValidationWithDetails("Invalid query parameters", formatErrors) + } + + return p, nil +} + +func validateListParams(p *listParams) *errors.ServiceError { + err := listParamsValidator.Struct(p) + if err == nil { + return nil + } + + validationErrors, ok := err.(validator.ValidationErrors) + if !ok { + return errors.GeneralError("unexpected validation error: %s", err) + } + + var details []errors.ValidationDetail + for _, fe := range validationErrors { + field := strings.ToLower(fe.Field()) + details = append(details, errors.ValidationDetail{ + Field: field, + Value: fe.Value(), + Constraint: mapConstraint(fe.Tag()), + Message: validationMessage(field, fe), + }) + } + + return errors.ValidationWithDetails("Invalid query parameters", details) +} + +func validationMessage(field string, fe validator.FieldError) string { + switch fe.Tag() { + case "min": + return field + " must be at least " + fe.Param() + case "max": + return field + " must be at most " + fe.Param() + case "required_with": + return "ref_type and ref_target_id must be provided together" + default: + return fe.Error() + } +} + +func mapConstraint(tag string) string { + switch tag { + case "required_with": + return "required" + default: + return tag + } +} + +func ensureIDField(fields []string) []string { + if len(fields) == 0 { + return nil + } + if slices.Contains(fields, "id") { + return fields + } + return append(fields, "id") +} + +// normalizeList splits comma-separated values, trims whitespace, and drops empties. +// Supports both ?key=a,b and ?key=a&key=b. +func normalizeList(values []string) []string { + var result []string + + for _, value := range values { + for _, item := range strings.Split(value, ",") { + if item = strings.TrimSpace(item); item != "" { + result = append(result, item) + } + } + } + + if len(result) == 0 { + return nil + } + + return result +} diff --git a/pkg/handlers/list_params_test.go b/pkg/handlers/list_params_test.go new file mode 100644 index 00000000..3acd5da4 --- /dev/null +++ b/pkg/handlers/list_params_test.go @@ -0,0 +1,321 @@ +package handlers + +import ( + "net/url" + "testing" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" +) + +func Test_parseListParams(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + name string + query string + expected *services.ListArguments + errors []expectedDetail + }{ + // Defaults + { + name: "defaults", + query: "", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + }, + }, + // Custom values + { + name: "custom page and size", + query: "?page=2&size=50", + expected: &services.ListArguments{ + Page: 2, + Size: 50, + Order: []string{"created_time desc"}, + }, + }, + { + name: "search", + query: "?search=status%20%3D%20active", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Search: "status = active", + Order: []string{"created_time desc"}, + }, + }, + // Order + { + name: "custom order", + query: "?order=name%20asc,created_time%20desc", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"name asc", "created_time desc"}, + }, + }, + { + name: "order preserves raw value for downstream validation", + query: "?order=name%20%20%20asc", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"name asc"}, + }, + }, + // Fields + { + name: "fields auto-includes id", + query: "?fields=name,kind", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + Fields: []string{"name", "kind", "id"}, + }, + }, + { + name: "fields preserves existing id", + query: "?fields=id,name", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + Fields: []string{"id", "name"}, + }, + }, + // Ref pairing + { + name: "paired ref parameters", + query: "?ref_type=dep&ref_target_id=target-1", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + RefType: "dep", + RefTargetID: "target-1", + }, + }, + { + name: "ref_type without target_id", + query: "?ref_type=dep", + errors: []expectedDetail{ + {message: "ref_type and ref_target_id must be provided together"}, + }, + }, + { + name: "ref_target_id without ref_type", + query: "?ref_target_id=some-id", + errors: []expectedDetail{ + {message: "ref_type and ref_target_id must be provided together"}, + }, + }, + // Format errors + { + name: "non-numeric page", + query: "?page=abc", + errors: []expectedDetail{ + {field: "page", message: "must be a valid integer"}, + }, + }, + { + name: "non-numeric size", + query: "?size=xyz", + errors: []expectedDetail{ + {field: "size", message: "must be a valid integer"}, + }, + }, + { + name: "both page and size non-numeric", + query: "?page=abc&size=xyz", + errors: []expectedDetail{ + {field: "page", message: "must be a valid integer"}, + {field: "size", message: "must be a valid integer"}, + }, + }, + // Range errors + { + name: "negative page", + query: "?page=-1", + errors: []expectedDetail{ + {field: "page", message: "page must be at least 1"}, + }, + }, + { + name: "zero page", + query: "?page=0", + errors: []expectedDetail{ + {field: "page", message: "page must be at least 1"}, + }, + }, + { + name: "negative size", + query: "?size=-1", + errors: []expectedDetail{ + {field: "size", message: "size must be at least 1"}, + }, + }, + { + name: "zero size", + query: "?size=0", + errors: []expectedDetail{ + {field: "size", message: "size must be at least 1"}, + }, + }, + { + name: "size above maximum", + query: "?size=101", + errors: []expectedDetail{ + {field: "size", message: "size must be at most 100"}, + }, + }, + { + name: "very large size", + query: "?size=999999", + errors: []expectedDetail{ + {field: "size", message: "size must be at most 100"}, + }, + }, + { + name: "multiple range errors at once", + query: "?page=0&size=999", + errors: []expectedDetail{ + {field: "page", message: "page must be at least 1"}, + {field: "size", message: "size must be at most 100"}, + }, + }, + // Edge cases + { + name: "search with leading and trailing spaces", + query: "?search=%20%20status%20%3D%20active%20%20", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Search: "status = active", + Order: []string{"created_time desc"}, + }, + }, + { + name: "empty order falls back to default", + query: "?order=", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + }, + }, + { + name: "whitespace-only order falls back to default", + query: "?order=%20%20%20", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + }, + }, + { + name: "pageSize is ignored", + query: "?pageSize=50", + expected: &services.ListArguments{ + Page: 1, + Size: 20, + Order: []string{"created_time desc"}, + }, + }, + { + name: "valid boundary page=1 size=1", + query: "?page=1&size=1", + expected: &services.ListArguments{ + Page: 1, + Size: 1, + Order: []string{"created_time desc"}, + }, + }, + { + name: "valid boundary page=1 size=100", + query: "?page=1&size=100", + expected: &services.ListArguments{ + Page: 1, + Size: 100, + Order: []string{"created_time desc"}, + }, + }, + { + name: "valid large page", + query: "?page=999&size=50", + expected: &services.ListArguments{ + Page: 999, + Size: 50, + Order: []string{"created_time desc"}, + }, + }, + { + name: "page above maximum", + query: "?page=10000001", + errors: []expectedDetail{ + {field: "page", message: "page must be at most 10000000"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + u, _ := url.Parse("/resources" + tt.query) + result, err := parseListParams(u.Query()) + + if len(tt.errors) > 0 { + Expect(err).ToNot(BeNil()) + Expect(err.HTTPCode).To(Equal(400)) + Expect(result).To(BeNil()) + for _, exp := range tt.errors { + matcher := HaveField("Message", exp.message) + if exp.field != "" { + matcher = And(HaveField("Field", exp.field), matcher) + } + Expect(err.Details).To(ContainElement(matcher)) + } + } else { + Expect(err).To(BeNil()) + Expect(result).To(Equal(tt.expected)) + } + }) + } +} + +type expectedDetail struct { + field string + message string +} + +func TestNormalizeList(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + name string + input []string + expected []string + }{ + {"nil input", nil, nil}, + {"empty input", []string{}, nil}, + {"single value", []string{"name"}, []string{"name"}}, + {"comma-separated", []string{"name,kind,id"}, []string{"name", "kind", "id"}}, + {"repeated params", []string{"name", "kind", "id"}, []string{"name", "kind", "id"}}, + {"mixed", []string{"name,kind", "id"}, []string{"name", "kind", "id"}}, + {"with whitespace", []string{" name , kind , id "}, []string{"name", "kind", "id"}}, + {"trailing comma", []string{"name,"}, []string{"name"}}, + {"only commas", []string{",,,"}, nil}, + {"empty strings", []string{"", ""}, nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + Expect(normalizeList(tt.input)).To(Equal(tt.expected)) + }) + } +} diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index 8d5f2b96..42d3c16b 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -109,7 +109,7 @@ func (h *ResourceHandler) List(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() - listArgs, err := services.NewListArguments(r.URL.Query()) + listArgs, err := parseListParams(r.URL.Query()) if err != nil { return nil, err } diff --git a/pkg/handlers/resource_status_handler.go b/pkg/handlers/resource_status_handler.go index 156e36d8..5ef3ce2d 100644 --- a/pkg/handlers/resource_status_handler.go +++ b/pkg/handlers/resource_status_handler.go @@ -43,7 +43,7 @@ func (h *ResourceStatusHandler) List(w http.ResponseWriter, r *http.Request) { Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() id := mux.Vars(r)["id"] - listArgs, err := services.NewListArguments(r.URL.Query()) + listArgs, err := parseListParams(r.URL.Query()) if err != nil { return nil, err } diff --git a/pkg/handlers/root_resource_handler.go b/pkg/handlers/root_resource_handler.go index 7ec7c07b..26ce6afd 100644 --- a/pkg/handlers/root_resource_handler.go +++ b/pkg/handlers/root_resource_handler.go @@ -36,7 +36,7 @@ func NewRootResourceHandler( func (h *RootResourceHandler) List(w http.ResponseWriter, r *http.Request) { cfg := &handlerConfig{ Action: func() (interface{}, *errors.ServiceError) { - listArgs, err := services.NewListArguments(r.URL.Query()) + listArgs, err := parseListParams(r.URL.Query()) if err != nil { return nil, err } @@ -206,7 +206,7 @@ func (h *RootResourceHandler) ListStatuses(w http.ResponseWriter, r *http.Reques Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() id := mux.Vars(r)["id"] - listArgs, err := services.NewListArguments(r.URL.Query()) + listArgs, err := parseListParams(r.URL.Query()) if err != nil { return nil, err } diff --git a/pkg/services/generic.go b/pkg/services/generic.go index 9b009db0..91a32a76 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -17,7 +17,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) //go:generate go tool -modfile=../../tools/go.mod mockgen -source=generic.go -package=services -destination=generic_mock.go @@ -61,7 +60,6 @@ func (s *sqlGenericService) newListContext( if resourceTypeStr == "" { return nil, nil, errors.GeneralError("Could not determine resource type") } - args.Search = strings.Trim(args.Search, " ") return &listContext{ ctx: ctx, args: args, @@ -269,23 +267,11 @@ func (s *sqlGenericService) loadList(listCtx *listContext, d *dao.GenericDao) *e return err } - switch { - case args.Size > MaxListSize: - // Note: Currently unreachable via HTTP requests (capped at MaxPageSize=100), - // but kept as defensive check for direct service layer usage. - logger.Warn(listCtx.ctx, "A query with a size greater than the maximum was requested.") - case args.Size < 0: - logger.Warn(listCtx.ctx, "A query with an unbound size was requested.") - case args.Size == 0: - // This early return is not only performant, but also necessary. - // gorm does not support Limit(0) any longer. - logger.Info(listCtx.ctx, - "A query with 0 size requested, returning early without collecting any resources from database") + // gorm does not support Limit(0); also reject negative sizes defensively. + if args.Size <= 0 { return nil } - // NOTE: Limit no longer supports '0' size and will cause issues. There is an early return, do not remove it. - // https://github.com/go-gorm/gorm/blob/master/clause/limit.go#L18-L21 if err := (*d).Fetch((args.Page-1)*int(args.Size), int(args.Size), listCtx.resourceList); err != nil { switch { case e.Is(err, gorm.ErrRecordNotFound): diff --git a/pkg/services/resource.go b/pkg/services/resource.go index b4987f30..e0a93ad2 100644 --- a/pkg/services/resource.go +++ b/pkg/services/resource.go @@ -17,8 +17,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/util" ) -const defaultPageSize = 20 - //go:generate go tool -modfile=../../tools/go.mod mockgen -source=resource.go -package=services -destination=resource_mock.go type ResourceService interface { @@ -402,7 +400,7 @@ func (s *sqlResourceService) List( return nil, nil, svcErr } if args == nil { - args = &ListArguments{Page: 1, Size: defaultPageSize} + args = NewListArguments() } scopedArgs := *args scopedArgs.Preloads = append(append([]string(nil), scopedArgs.Preloads...), "Labels", "Conditions", "References") @@ -433,7 +431,7 @@ func (s *sqlResourceService) ListByOwner( return nil, nil, svcErr } if args == nil { - args = &ListArguments{Page: 1, Size: defaultPageSize} + args = NewListArguments() } scopedArgs := *args scopedArgs.Preloads = append(append([]string(nil), scopedArgs.Preloads...), "Labels", "Conditions", "References") @@ -473,7 +471,7 @@ func (s *sqlResourceService) ListAll( ctx context.Context, args *ListArguments, ) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) { if args == nil { - args = &ListArguments{Page: 1, Size: defaultPageSize} + args = NewListArguments() } scopedArgs := *args scopedArgs.Preloads = append(append([]string(nil), scopedArgs.Preloads...), "Labels", "Conditions", "References") diff --git a/pkg/services/types.go b/pkg/services/types.go old mode 100755 new mode 100644 index 2591f703..27c785b0 --- a/pkg/services/types.go +++ b/pkg/services/types.go @@ -1,13 +1,5 @@ package services -import ( - "net/url" - "strconv" - "strings" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" -) - // ListArguments are arguments relevant for listing objects. // This struct is common to all service List funcs in this package type ListArguments struct { @@ -21,128 +13,9 @@ type ListArguments struct { Page int } -// MaxListSize defines the PostgreSQL WHERE IN clause parameter limit (~65500). -// Note: This is currently unreachable via HTTP requests since MaxPageSize caps at 100, -// but is kept as a defensive check for direct service layer usage and to document the -// technical database constraint. -const MaxListSize = 65500 - -// MaxPageSize is the maximum allowed page size for pagination via HTTP requests. -// Set to 100 to prevent excessive resource usage and ensure reasonable response times. -const MaxPageSize = 100 - -// ParseFieldsParameter extracts and parses the ?fields query parameter. -// Returns a slice of field names with "id" always included when valid fields are provided. -// Returns nil if no valid fields are specified (empty or whitespace-only parameter). -func ParseFieldsParameter(params url.Values) []string { - if v := strings.TrimSpace(params.Get("fields")); v != "" { - fields := strings.Split(v, ",") - result := make([]string, 0, len(fields)+1) - idPresent := false - for _, field := range fields { - trimmed := strings.TrimSpace(field) - if trimmed == "" { - continue - } - if trimmed == "id" { - idPresent = true - } - result = append(result, trimmed) - } - // If no valid fields were provided (e.g., "fields=" or "fields= "), return nil - if len(result) == 0 { - return nil - } - // Always include id field when user provided valid fields - if !idPresent { - result = append(result, "id") - } - return result - } - return nil -} - -// NewListArguments Create ListArguments from url query parameters with sane defaults -// Returns an error if page or size parameters are invalid (negative, non-numeric, or out of range) -func NewListArguments(params url.Values) (*ListArguments, *errors.ServiceError) { - listArgs := &ListArguments{ - Page: 1, - Size: 20, - Search: "", - } - - // Validate page parameter - if v := strings.Trim(params.Get("page"), " "); v != "" { - page, err := strconv.Atoi(v) - if err != nil { - return nil, errors.New( - errors.CodeValidationFormat, - "Invalid page parameter: must be a positive integer", - ) - } - if page < 1 { - return nil, errors.New( - errors.CodeValidationRange, - "Invalid page parameter: %d is less than 1", - page, - ) - } - listArgs.Page = page - } - - // Validate size parameter - if v := strings.Trim(params.Get("size"), " "); v != "" { - size, err := strconv.ParseInt(v, 10, 64) - if err != nil { - return nil, errors.New( - errors.CodeValidationFormat, - "Invalid size parameter: must be a positive integer", - ) - } - if size < 1 { - return nil, errors.New( - errors.CodeValidationRange, - "Invalid size parameter: %d is less than 1", - size, - ) - } - if size > MaxPageSize { - return nil, errors.New( - errors.CodeValidationRange, - "Invalid size parameter: %d exceeds maximum allowed value of %d", - size, MaxPageSize, - ) - } - listArgs.Size = size - } - - if v := strings.Trim(params.Get("search"), " "); v != "" { - listArgs.Search = v - } - if v := strings.Trim(params.Get("order"), " "); v != "" { - rawFields := strings.Split(v, ",") - for _, field := range rawFields { - if trimmed := strings.TrimSpace(field); trimmed != "" { - listArgs.Order = append(listArgs.Order, strings.Join(strings.Fields(trimmed), " ")) - } - } - } - - if len(listArgs.Order) == 0 { - listArgs.Order = []string{"created_time desc"} +func NewListArguments() *ListArguments { + return &ListArguments{ + Page: 1, + Size: 20, } - - // Parse fields parameter using shared logic - listArgs.Fields = ParseFieldsParameter(params) - - listArgs.RefType = strings.TrimSpace(params.Get("ref_type")) - listArgs.RefTargetID = strings.TrimSpace(params.Get("ref_target_id")) - if (listArgs.RefType == "") != (listArgs.RefTargetID == "") { - return nil, errors.New( - errors.CodeValidationFormat, - "ref_type and ref_target_id must be provided together", - ) - } - - return listArgs, nil } diff --git a/pkg/services/types_test.go b/pkg/services/types_test.go deleted file mode 100644 index db20150a..00000000 --- a/pkg/services/types_test.go +++ /dev/null @@ -1,369 +0,0 @@ -package services - -import ( - "net/url" - "reflect" - "testing" - - . "github.com/onsi/gomega" -) - -func TestNewListArguments_Order(t *testing.T) { - RegisterTestingT(t) - - tests := []struct { - name string - queryParams url.Values - expectedOrder []string - }{ - { - name: "no order - should use default created_time desc", - queryParams: url.Values{}, - expectedOrder: []string{"created_time desc"}, - }, - { - name: "order with asc direction", - queryParams: url.Values{"order": []string{"name asc"}}, - expectedOrder: []string{"name asc"}, - }, - { - name: "order with desc direction", - queryParams: url.Values{"order": []string{"name desc"}}, - expectedOrder: []string{"name desc"}, - }, - { - name: "order without direction", - queryParams: url.Values{"order": []string{"name"}}, - expectedOrder: []string{"name"}, - }, - { - name: "multiple order fields", - queryParams: url.Values{"order": []string{"name asc,created_time desc"}}, - expectedOrder: []string{"name asc", "created_time desc"}, - }, - { - name: "order with spaces should be trimmed", - queryParams: url.Values{"order": []string{" name asc "}}, - expectedOrder: []string{"name asc"}, - }, - { - name: "empty order string - should use default", - queryParams: url.Values{"order": []string{""}}, - expectedOrder: []string{"created_time desc"}, - }, - { - name: "order with whitespace only - should use default", - queryParams: url.Values{"order": []string{" "}}, - expectedOrder: []string{"created_time desc"}, - }, - { - name: "order with empty tokens - should filter out", - queryParams: url.Values{"order": []string{"name,,created_time"}}, - expectedOrder: []string{"name", "created_time"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - listArgs, err := NewListArguments(tt.queryParams) - Expect(err).To(BeNil(), "Should not return error for valid order parameters") - Expect(listArgs.Order).To(Equal(tt.expectedOrder), - "Order mismatch for test case: %s", tt.name) - }) - } -} - -func TestNewListArguments_DefaultValues(t *testing.T) { - RegisterTestingT(t) - - listArgs, err := NewListArguments(url.Values{}) - - Expect(err).To(BeNil(), "Should not return error for default values") - Expect(listArgs.Page).To(Equal(1), "Default page should be 1") - Expect(listArgs.Size).To(Equal(int64(20)), "Default size should be 20") - Expect(listArgs.Search).To(Equal(""), "Default search should be empty") - Expect(listArgs.Order).To(Equal([]string{"created_time desc"}), "Default order should be created_time desc") -} - -func TestNewListArguments_Size(t *testing.T) { - RegisterTestingT(t) - - tests := []struct { - queryParams url.Values - name string - expectedPage int - expectedSize int64 - }{ - { - name: "custom page and size", - queryParams: url.Values{"page": []string{"2"}, "size": []string{"50"}}, - expectedPage: 2, - expectedSize: 50, - }, - { - name: "custom page and size (different values)", - queryParams: url.Values{"page": []string{"3"}, "size": []string{"25"}}, - expectedPage: 3, - expectedSize: 25, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - listArgs, err := NewListArguments(tt.queryParams) - Expect(err).To(BeNil(), "Should not return error for valid parameters") - Expect(listArgs.Page).To(Equal(tt.expectedPage), "Page mismatch") - Expect(listArgs.Size).To(Equal(tt.expectedSize), "Size mismatch") - }) - } -} - -func TestNewListArguments_Search(t *testing.T) { - RegisterTestingT(t) - - tests := []struct { - name string - queryParams url.Values - expectedSearch string - }{ - { - name: "no search parameter", - queryParams: url.Values{}, - expectedSearch: "", - }, - { - name: "search with value", - queryParams: url.Values{"search": []string{"name='test'"}}, - expectedSearch: "name='test'", - }, - { - name: "search with spaces should be trimmed", - queryParams: url.Values{"search": []string{" name='test' "}}, - expectedSearch: "name='test'", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - listArgs, err := NewListArguments(tt.queryParams) - Expect(err).To(BeNil(), "Should not return error for valid search parameters") - Expect(listArgs.Search).To(Equal(tt.expectedSearch), "Search mismatch") - }) - } -} - -func TestNewListArguments_Fields(t *testing.T) { - RegisterTestingT(t) - - tests := []struct { - name string - queryParams url.Values - expectedFields []string - }{ - { - name: "no fields parameter", - queryParams: url.Values{}, - expectedFields: nil, - }, - { - name: "fields without id - should add id automatically", - queryParams: url.Values{"fields": []string{"name,status"}}, - expectedFields: []string{"name", "status", "id"}, - }, - { - name: "fields with id - should not duplicate", - queryParams: url.Values{"fields": []string{"id,name,status"}}, - expectedFields: []string{"id", "name", "status"}, - }, - { - name: "fields with spaces and commas", - queryParams: url.Values{"fields": []string{" name , status , id "}}, - expectedFields: []string{"name", "status", "id"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - listArgs, err := NewListArguments(tt.queryParams) - Expect(err).To(BeNil(), "Should not return error for valid fields parameters") - if !reflect.DeepEqual(listArgs.Fields, tt.expectedFields) { - t.Errorf("Fields = %v, want %v", listArgs.Fields, tt.expectedFields) - } - }) - } -} - -func TestNewListArguments_RefTypeWithoutTargetID_Returns400(t *testing.T) { - RegisterTestingT(t) - - params := url.Values{"ref_type": []string{"dep"}} - listArgs, err := NewListArguments(params) - Expect(listArgs).To(BeNil()) - Expect(err).ToNot(BeNil()) - Expect(err.HTTPCode).To(Equal(400)) - Expect(err.Reason).To(ContainSubstring("ref_type and ref_target_id must be provided together")) -} - -func TestNewListArguments_RefTargetIDWithoutRefType_Returns400(t *testing.T) { - RegisterTestingT(t) - - params := url.Values{"ref_target_id": []string{"some-id"}} - listArgs, err := NewListArguments(params) - Expect(listArgs).To(BeNil()) - Expect(err).ToNot(BeNil()) - Expect(err.HTTPCode).To(Equal(400)) - Expect(err.Reason).To(ContainSubstring("ref_type and ref_target_id must be provided together")) -} - -func TestNewListArguments_RefTypePairValid(t *testing.T) { - RegisterTestingT(t) - - params := url.Values{ - "ref_type": []string{"dep"}, - "ref_target_id": []string{"target-1"}, - } - listArgs, err := NewListArguments(params) - Expect(err).To(BeNil()) - Expect(listArgs.RefType).To(Equal("dep")) - Expect(listArgs.RefTargetID).To(Equal("target-1")) -} - -// TestNewListArguments_Validation tests pagination parameter validation (HYPERFLEET-1241) -func TestNewListArguments_Validation(t *testing.T) { - RegisterTestingT(t) - - tests := []struct { - name string - queryParams url.Values - errorContains string - errorCode string - expectError bool - }{ - // Page validation tests - // Page boundary checks — VAL-004 (CodeValidationRange) for values outside valid range - { - name: "negative page returns error", - queryParams: url.Values{"page": []string{"-1"}}, - expectError: true, - errorContains: "Invalid page parameter", - errorCode: "HYPERFLEET-VAL-004", - }, - { - name: "zero page returns error", - queryParams: url.Values{"page": []string{"0"}}, - expectError: true, - errorContains: "Invalid page parameter", - errorCode: "HYPERFLEET-VAL-004", - }, - // Page parse errors — VAL-003 (CodeValidationFormat) for non-numeric input - { - name: "non-numeric page returns error", - queryParams: url.Values{"page": []string{"abc"}}, - expectError: true, - errorContains: "Invalid page parameter", - errorCode: "HYPERFLEET-VAL-003", - }, - { - name: "page with special characters returns error", - queryParams: url.Values{"page": []string{"