[CFX-7945] refactor(api): unify next-link pagination behind internal/paginate - #861
Draft
ajalon1 wants to merge 5 commits into
Draft
[CFX-7945] refactor(api): unify next-link pagination behind internal/paginate#861ajalon1 wants to merge 5 commits into
ajalon1 wants to merge 5 commits into
Conversation
…tflags Adds internal/countflags, a pflag.Value pair for count-like int flags: PositiveInt rejects zero and negative values (a page size of 0 would fetch nothing and read as an empty result), and NonNegativeInt allows zero while rejecting negatives (offset 0 means "from the start"). Set keeps pflag's base-0 ParseInt semantics and reports type "int", so Flags().GetInt reads in telemetry closures are unchanged; only validation is new. Migrates --limit (and --offset where present) on dr workload list, dr workload logs, dr artifact list, dr artifact build list, and dr artifact code versions to the new values and deletes the runtime checks from their RunE bodies. Invalid input now fails at parse time as `invalid argument "0" for "--limit" flag: must be a positive integer` instead of `invalid --limit 0: must be positive` after the command starts. Test assertions updated to the parse-time messages; the artifact code versions invalid-limit case now asserts the Flags().Set error directly since rejection happens before Execute. Documented the pattern in docs/development/flags.md alongside the pollflags precedent. Excluded on purpose: workload config --port/--replicas (0 means "use default") and selector flags like --node-id. Library-level limit checks in internal/workload stay as defense in depth for non-CLI callers. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
🎫 Jira: |
CodeQL (incorrect-conversion-between-integer-types) flags the int64 -> int conversion of the strconv.ParseInt result: int is 32-bit on some architectures, so a value in [2^31, 2^63) would silently truncate there. Set now also rejects values above math.MaxInt, satisfying the upper-bound check; on 64-bit builds the bound is unreachable and the check only documents intent. Adds a parse-level test pinning that out-of-range numerics error instead of clamping. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Migrates the pipeline family's count flags onto internal/countflags from the base stack: - --limit -> countflags.PositiveInt on pipeline list (default 50), version/run/image/input/schedule list (default 100) - --offset -> countflags.NonNegativeInt on all six list commands - --tail on pipeline run task logs -> countflags.NonNegativeInt, keeping 0 as "no limit" These commands previously did no validation at all, so --limit 0/-1 and negative offsets went straight to the API; they are now rejected during flag parsing with cobra's standard "invalid argument" message, before any request is made. Adds invalid-limit/invalid-offset reject tests per list command and an invalid-tail test for task logs, in the existing runCmd style. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Migrates dr task run --concurrency onto internal/countflags.PositiveInt (default 2 unchanged). Zero or negative values previously reached the task runner and ran nothing or misbehaved; they are now rejected during flag parsing with cobra's standard "invalid argument" message. Adds a reject test in the existing command-test style. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…paginate
Introduces internal/paginate.Walk, the one generic loop for list endpoints
that answer one page per request plus a next link: accumulate rows, stop
once limit rows are collected or the pages run out, and return at most
limit rows.
internal/workload: ListWorkloads, ListArtifacts, and ListArtifactBuilds
now share one listWalk helper over their identical {data,next} envelope,
replacing the three hand-rolled loop copies (and the per-endpoint
WorkloadList/ArtifactList/BuildList envelopes they decoded into, whose
count fields were never read). The builds endpoint additionally clamps
its page size to the shared maxPageSize ceiling instead of sending the
full limit per request; next-links satisfy larger totals either way.
internal/pipeline: the six List* functions previously decoded a single
DataPage and returned it, so --limit values beyond one page were silently
truncated. They now clamp the per-request page size to 200 (the documented
1-200 ceiling) and walk next-links until the limit is met or the pages run
out. ListPipelines keeps its *DataPage return, restating Count over the
returned rows while preserving the server's TotalCount from the last page
so "Showing N of <total>" rendering stays truthful. Argument checks
(limit positive, offset non-negative) now mirror the workload family;
TestsListInputs/ListImages that asserted the old "zero limit omits the
param" contract are updated, and multi-page walk tests cover both the
DataPage and rows paths.
llm-gateway list already walks pages internally and ListTaskExecutions
fetches all rows by design, so neither adopts the walker.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
ajalon1
force-pushed
the
aj/CFX-7834-list-pagination
branch
from
August 27, 2026 20:49
bbe797a to
596b00f
Compare
Contributor
Author
|
Leaving this in draft because I'm not 100% sure on the changes here, unlike for the others in the stack. Still looking for reviews from workload and pipeline, though. |
ajalon1
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of CFX-7834 and PR stack 858/859/860. While working on that ticket, found two problems with list-API pagination:
DataPageand returns it (ListPipelines,internal/pipeline/pipeline.go:144), so--limit 500silently returns one page — the server enforces a 1..100 page-size ceiling (maxPageSize = 100,internal/workload/workload.go:706) and larger totals are only reachable via next-links, which the pipeline family never follows.paginateWorkloads,paginateArtifacts, inline loops inbuild.go,credential.go,execenv.go, and ininternal/drapifor templates and llms) instead of sharing one helper.Scope
internal/paginate.Walk: the one generic next-link loop (accumulate rows, stop at limit or end of pages, return at most limit rows), guarded by the existingdrapi.AssertNextOnSameHost.ListWorkloads,ListArtifacts, andListArtifactBuildsto share that loop, replacing the per-endpointpaginate*copies.offset/limitthrough unclamped as today, and either adopt the walk or leave it single-page by design — current behavior kept, flagged for the API team re: the 422-on-limit>100 contract.ListTaskExecutions(fetch-all by design).Verification
task lint(all GOOS legs) andtask test(race + coverage) pass.DataPageand rows paths; tests pin that out-of-range numerics error instead of clamping.