HYPERFLEET-1280 - refactor: rename pagination and ordering query params#253
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cluster list API docs now describe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| @@ -141,8 +141,8 @@ func (s *sqlGenericService) buildPreload(listCtx *listContext, d *dao.GenericDao | |||
| } | |||
|
|
|||
| func (s *sqlGenericService) buildOrderBy(listCtx *listContext, d *dao.GenericDao) (bool, *errors.ServiceError) { | |||
There was a problem hiding this comment.
Should we change the name to buildOrder?
Then you would also subsequently need to change https://github.com/openshift-hyperfleet/hyperfleet-api/blob/main/pkg/services/generic.go#L100C5-L100C17
There was a problem hiding this comment.
As this is an non-exported function, I don't think matters that much, let's move on with the PR
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 245 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/types.go (1)
91-115: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not drop
pageSizeuntil linked clients move tosize.Lines 91-115 stop reading
pageSize, and the updated tests now assert that legacy param is ignored. Sentinel, Broker, and Adapter still construct list requests withPageSize, so those callers will silently fall back to the default page size of 20 instead of the requested value. Keep a temporarypageSizealias here, or land the downstream client updates first.Suggested compatibility patch
- // Validate size parameter - if v := strings.Trim(params.Get("size"), " "); v != "" { - size, err := strconv.ParseInt(v, 10, 64) + // Validate size parameter + sizeParam := strings.Trim(params.Get("size"), " ") + if sizeParam == "" { + sizeParam = strings.Trim(params.Get("pageSize"), " ") + } + if sizeParam != "" { + size, err := strconv.ParseInt(sizeParam, 10, 64) if err != nil { return nil, errors.New( errors.CodeValidationFormat, "Invalid size parameter: must be a positive integer", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/types.go` around lines 91 - 115, The size parsing in the list-args validation now only reads "size", which breaks existing callers still sending "pageSize" from Sentinel, Broker, and Adapter. Update the parameter handling in the validation logic to temporarily accept "pageSize" as an alias (or preserve the legacy fallback) in the same place where size is parsed, so ListArgs.Size is populated correctly for both names until downstream clients are migrated.Source: Linked repositories
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/services/types_test.go`:
- Around line 290-293: The `pageSize` test case in `types_test.go` is locking in
a breaking contract change while `PageSize` is still used by linked clients.
Update the `queryParams`/list parameter handling so the `PageSize` field remains
temporarily compatible (or otherwise coordinated with downstream client updates)
in the relevant request parsing path, and keep the behavior covered by the
existing test around the list request types.
In `@test/integration/clusters_test.go`:
- Around line 205-208: Coordinate the `pageSize` query-contract change across
all downstream consumers before finalizing this test update: `GetClustersParams`
in the cluster integration tests should match the new API shape, but the
Sentinel, Broker, and Adapter client request builders and assertions still rely
on `PageSize` and `pageSize` expectations. Update the affected client helpers
and related tests to use the current pagination fields consistently, and only
keep references that align with the renamed contract so `GetClustersParams` and
the other `Get*Params` usages stay in sync.
---
Outside diff comments:
In `@pkg/services/types.go`:
- Around line 91-115: The size parsing in the list-args validation now only
reads "size", which breaks existing callers still sending "pageSize" from
Sentinel, Broker, and Adapter. Update the parameter handling in the validation
logic to temporarily accept "pageSize" as an alias (or preserve the legacy
fallback) in the same place where size is parsed, so ListArgs.Size is populated
correctly for both names until downstream clients are migrated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d2db453f-28af-491c-9a7f-cdebc4fe2c95
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (10)
docs/api-resources.mdgo.modpkg/services/CLAUDE.mdpkg/services/generic.gopkg/services/types.gopkg/services/types_test.gotest/integration/adapter_status_test.gotest/integration/channels_test.gotest/integration/clusters_test.gotest/integration/node_pools_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| { | ||
| name: "non-numeric pageSize returns error", | ||
| queryParams: url.Values{"pageSize": []string{"xyz"}}, | ||
| expectError: true, | ||
| errorContains: "Invalid pageSize parameter", | ||
| errorCode: "HYPERFLEET-VAL-003", | ||
| name: "pageSize is ignored (no longer a valid parameter)", | ||
| queryParams: url.Values{"pageSize": []string{"50"}}, | ||
| expectError: false, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Blocking contract break: pageSize is now ignored while linked clients still send it.
This test locks in silent fallback to the default page size for callers that still use the old parameter. The linked hyperfleet-sentinel, hyperfleet-broker, and hyperfleet-adapter clients still populate PageSize, so their list calls will stop honoring requested pagination and degrade to size=20 semantics until those repos ship coordinated updates. Either preserve temporary compatibility here or merge this only with the downstream client changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/services/types_test.go` around lines 290 - 293, The `pageSize` test case
in `types_test.go` is locking in a breaking contract change while `PageSize` is
still used by linked clients. Update the `queryParams`/list parameter handling
so the `PageSize` field remains temporarily compatible (or otherwise coordinated
with downstream client updates) in the relevant request parsing path, and keep
the behavior covered by the existing test around the list request types.
Source: Linked repositories
| size := openapi.QueryParamsSize(5) | ||
| params := &openapi.GetClustersParams{ | ||
| Page: &page, | ||
| PageSize: &pageSize, | ||
| Page: &page, | ||
| Size: &size, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Coordinate downstream client updates before removing pageSize.
pkg/services/types_test.go:260-320 now treats pageSize as invalid/ignored, but the linked Sentinel, Broker, and Adapter clients still build cluster and node pool requests with Get*Params{PageSize: ...} and still assert pageSize in their tests. Shipping this rename by itself leaves those consumers on the stale query contract from v1.0.23.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/clusters_test.go` around lines 205 - 208, Coordinate the
`pageSize` query-contract change across all downstream consumers before
finalizing this test update: `GetClustersParams` in the cluster integration
tests should match the new API shape, but the Sentinel, Broker, and Adapter
client request builders and assertions still rely on `PageSize` and `pageSize`
expectations. Update the affected client helpers and related tests to use the
current pagination fields consistently, and only keep references that align with
the renamed contract so `GetClustersParams` and the other `Get*Params` usages
stay in sync.
Source: Linked repositories
openshift-merge-bot
Bot
merged commit 43c0474
into
openshift-hyperfleet:main
3 participants
Summary
This is a follow-up for openshift-hyperfleet/hyperfleet-api-spec#63
Aligns query parameter names with the updated OpenAPI spec (v1.0.24).
Rename
pageSizequery parameter tosizeRename
orderByquery parameter toorderRemove backward-compatible
pageSizefallback logicRemove
OrderDirectionenum from API docsRename
ListArguments.OrderByfield toOrderUpdate integration tests and documentation to match
Bump hyperfleet-api-spec from v1.0.23 to v1.0.24
HYPERFLEET-1280
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)