fix(go): offset pagination with offsetSemantics: item-index generates compiling SDKs - #17364
fix(go): offset pagination with offsetSemantics: item-index generates compiling SDKs#17364devin-ai-integration[bot] wants to merge 5 commits into
offsetSemantics: item-index generates compiling SDKs#17364Conversation
…d required offsets Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
AI Review Summary
Reorders the item-index offset increment after the results declaration, types the increment to the page type, fixes the required-offset initializer, and starts item-index offsets at 0. Logic looks right for the int/int64/float64 paths; two gaps remain around non-numeric page types and item-index without a step.
- 🟡 1 warning(s)
- 🔵 1 suggestion(s)
| const underlying = pageType.underlying(); | ||
| switch (underlying.internalType.type) { | ||
| case "int64": | ||
| return "next += int64(len(results))"; | ||
| case "float64": | ||
| return "next += float64(len(results))"; | ||
| default: | ||
| return "next += int(len(results))"; | ||
| } |
There was a problem hiding this comment.
🟡 warning
The default branch still emits next += int(len(results)), which won't compile when the page property is a string (var next string = "0") or uuid. Since getOffsetInitializer explicitly handles those cases, this function should too — either emit a string-safe increment or fall back to page-index behavior rather than generating broken Go. Same class of bug the PR is fixing, just one type away.
Also note int(len(results)) is a redundant conversion (len already returns int); next += len(results) reads better for the int case.
| context: SdkGeneratorContext; | ||
| offset: FernIr.OffsetPagination; | ||
| }): boolean { | ||
| return offset.step != null && context.customConfig.offsetSemantics === "item-index"; |
There was a problem hiding this comment.
🔵 suggestion
usesItemIndexOffset gates on offset.step != null, so an API configured with offsetSemantics: item-index but no step still generates page-index behavior (next := 1 / next += 1). This preserves prior behavior, but given the PR's framing (item-index offsets address records), it's worth confirming that's intentional rather than a second off-by-one waiting to be reported.
| function getOffsetIncrementByResultCount({ pageType }: { pageType: go.Type }): string { | ||
| const underlying = pageType.underlying(); | ||
| switch (underlying.internalType.type) { | ||
| case "int64": | ||
| return "next += int64(len(results))"; | ||
| case "float64": | ||
| return "next += float64(len(results))"; | ||
| default: | ||
| return "next += int(len(results))"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Generated pagination code still fails to compile when the page marker is text-based
The per-page advance amount is always produced as a whole number (getOffsetIncrementByResultCount at generators/go-v2/sdk/src/endpoint/utils/getPaginationInfo.ts:410-420) even when the page marker is text- or id-shaped, so the generated SDK does not build for those APIs.
Impact: Customers whose pagination marker is a string or UUID still get an SDK that fails to compile under item-index semantics.
Type switch omits the string/uuid page types that the initializer explicitly supports
getOffsetInitializer (generators/go-v2/sdk/src/endpoint/utils/getPaginationInfo.ts:615-632) explicitly supports string (var next string = "0") and uuid (var next uuid.UUID) page types. For those same types, getOffsetIncrementByResultCount falls through to the default branch and emits next += int(len(results)), which is invalid Go (invalid operation: next += int(...) (mismatched types string and int)).
This is pre-existing for the page-index branch as well (next += 1 on a string), but the PR's stated goal is that offsetSemantics: item-index generates compiling SDKs, and the newly added type switch covers only int/int64/float64.
A reasonable fix is to only apply item-index increment logic for numeric page types (and fall back to page-index behavior or raise a clear generator error otherwise).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed the behavior, but it's pre-existing and orthogonal to this PR, so I've left it alone.
A string/uuid offset never compiled under either semantics: page-index emits next += 1 against var next string = "1", which is just as invalid as next += int(len(results)). getOffsetInitializer handling those types doesn't mean the pager ever worked for them — nothing downstream can increment a string offset. So this PR neither introduces nor widens the breakage; it fixes the numeric cases that are reachable in practice.
The suggested remedies are behavior decisions rather than bug fixes: silently falling back to page-index would send the wrong offsets, and failing generation would break any spec that currently generates (broken) output. Both deserve their own change — happy to open a follow-up if you'd like, ideally erroring out at generation time with a clear message for non-numeric offset page types.
Co-Authored-By: bot_apk <apk@cognition.ai>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Co-Authored-By: bot_apk <apk@cognition.ai>
Behavioral verification against a mock HTTP server — all scenarios passedRather than asserting on generated source, the generated pagers were run against item-index, no caller offset (corpus of 5, Starts at item-index, caller passes The caller's starting offset is honored, not overwritten. Seed fixture Seed fixture Every loop was bounded at 200 iterations; all terminated naturally on the first empty page. Collected results matched the expected corpora exactly — no gaps, duplicates or reordering. Regression:
|
Description
offsetSemantics: item-index+ a paginationstepproduced Go SDKs that do not compile — reproduced against releasedfernapi/fern-go-sdk:1.55.0, so this is not a regression from any open PR:Three distinct defects, each in its own commit:
undefined: results) —getReadPageResponseBodyForOffsetwrote the increment beforegetNextResultsSetteremittedresults := response.GetX(). Only the item-index branch is reordered, sopage-indexoutput (next += 1) is untouched. The increment is also now typed to the page property (int64(len(results))/float64(len(results))), which previously could not compile for non-intoffsets.getPagePropertyInitializer's non-optional branch emittedrequest.Offset := fmt.Sprintf("%v", pageRequest.Cursor): invalid Go (non-name request.Offset on left side of :=), pluspageRequestout of scope. It now emitsnext := request.Offset. Independent ofoffsetSemantics— verified on released 1.55.0 withpage-indexand a requiredintquery offset, which fails the same way. This does not apply to request-body offsets under feat(go): opt-in auto-pagination for request body cursors and offsets #17352, which take that PR's owngetRequestBodyOffsetInitializerpath and already emitnext := request.Offset.getOffsetInitializerhardcodednext := 1for every numeric page type, so item-index pagination skipped the first record of every collection. Item-index now starts at0; page-index still starts at1.Changes Made
generators/go-v2/sdk/src/endpoint/utils/getPaginationInfo.ts: declareresultsbefore the item-index increment, type the increment to the page type, seed required offsets from the request, and start item-index offsets at0.go-pagination-offset-item-index(offsetSemantics: item-index) covering an optional query-param offset, a required query-param offset (the customer-facing shape), and a request-body offset. No fixture previously combinedstepwith item-index semantics, which is why this shipped.generators/go/sdk/changes/unreleased/(one per concern).Generated output for the new fixture:
Note on the request-body endpoint: on
main, body-property offsets still generate a delegating (non-paginated) endpoint, so that endpoint's snapshot does not exercise the pager yet. It starts exercising this path — with no further generator changes needed — once #17352 (enableRequestBodyPagination) lands; this PR does not touch or depend on that branch.Testing
fern-go-sdk:1.55.0with a minimal GET + query-param offset spec (undefined: results), and separately with a required query-param offset underpage-index(non-name request.Offset on left side of :=).0, 2, 4, honors a caller-supplied starting offset, terminates on the empty page, and concatenates pages with no gaps or duplicates;page-indexstill sends1, 2, 3. Full observed sequences in the PR comment below.seed test --generator go-sdk --fixture go-pagination-offset-item-indexpasses with build (go build,golangci-lint) and test scripts — the fixture failed to build before the fix with the sameundefined: resultserror.pagination,pagination-custom,pagination-uri-pathfor go-sdk: zero diff, so page-index behavior is provably unchanged.Link to Devin session: https://app.devin.ai/sessions/45f5319518da45f9bb66779546d36e9f