Fix Cosmos DB pagination for backwards compatibility (3.x)#1595
Fix Cosmos DB pagination for backwards compatibility (3.x)#1595MarcAstr0 wants to merge 5 commits intoboostercloud:release/3.xfrom
Conversation
PR Summary
|
|
/integration sha=7ba540a |
|
⌛ Integration tests are running... Check their status here 👈 |
There was a problem hiding this comment.
Pull request overview
Backports the Cosmos DB pagination compatibility fix to the release/3.x line, aiming to restore a numeric cursor id for frontends while keeping continuation-token pagination for Azure Cosmos DB.
Changes:
- Update Azure provider Cosmos query pagination logic (continuation token vs legacy OFFSET/LIMIT) and introduce a default page size.
- Update Rush/pnpm lockfile with workspace specifier bumps and an updated resolved TypeScript nightly used by tooling.
- Add a Rush changefile for a patch release.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/framework-provider-azure/src/helpers/query-helper.ts | Adjusts Cosmos DB paginated search behavior, including continuation-token feed options and cursor shaping. |
| common/config/rush/pnpm-lock.yaml | Updates workspace dependency specifiers and refreshes some resolved toolchain dependencies in the lockfile. |
| common/changes/@boostercloud/framework-core/fix-cosmos-pagination_3.x_2026-02-27-21-16.json | Declares a patch change entry for publishing/versioning. |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| [email protected].20250822: | ||
| resolution: {integrity: sha512-omHezTVn6vg+B/eFHkIzUGFvlbbkJdsdmdBohcsw8NMLyKOhKRMinE9aLu8f0EALT4R2YS41xak2KinK74/6Xg==} | ||
| [email protected].20260302: |
There was a problem hiding this comment.
This lockfile update bumps the resolved [email protected].* nightly used by downlevel-dts (from 20250822 to 20260302) without any corresponding direct dependency change. If this wasn’t intentional, consider reverting the Typescript nightly resolution to avoid unrelated toolchain drift in a pagination-focused backport PR.
| [email protected].20260302: | |
| [email protected].20250822: |
| { | ||
| "packageName": "@boostercloud/framework-core", | ||
| "comment": "Fix pagination issues with Cosmos DB", | ||
| "type": "patch" | ||
| } |
There was a problem hiding this comment.
This changeset is recorded under @boostercloud/framework-core, but the functional change in this PR is in @boostercloud/framework-provider-azure (Cosmos DB query helper). If the goal is to publish the Azure provider pagination fix, the changeset should target the provider package (so changelogs/version bumps line up with the shipped fix).
| // Always set maxItemCount when limit is provided or when using continuation token | ||
| if (limit || afterCursor?.continuationToken) { | ||
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE | ||
| } |
There was a problem hiding this comment.
In the continuation-token path, maxItemCount is only set when limit is provided or when afterCursor.continuationToken is present. For the first page when limit is omitted (but continuation-token pagination is still used), this can yield an implementation-defined page size from the SDK/service, which makes cursor.id calculations non-deterministic and may break clients relying on stable offsets. Consider always setting feedOptions.maxItemCount when using the continuation-token pagination branch (e.g., to limit ?? DEFAULT_PAGE_SIZE), not only when a token is already present.
| // Always set maxItemCount when limit is provided or when using continuation token | |
| if (limit || afterCursor?.continuationToken) { | |
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE | |
| } | |
| // Always set maxItemCount when using continuation-token pagination | |
| feedOptions.maxItemCount = limit ?? DEFAULT_PAGE_SIZE |
| count: processedResources.length, | ||
| cursor: { | ||
| id: (offset + processedResources.length).toString(), | ||
| id: (offset + effectiveLimit).toString(), |
There was a problem hiding this comment.
The legacy OFFSET/LIMIT branch advances cursor.id by effectiveLimit rather than by the number of items actually returned (processedResources.length). If Cosmos returns fewer than effectiveLimit for any reason, an old client that paginates using only cursor.id will skip items on the next request. Advancing by the actual returned count keeps the numeric cursor aligned with the true offset.
| id: (offset + effectiveLimit).toString(), | |
| id: (offset + processedResources.length).toString(), |
| // cursor.id advances by the page size (limit) to maintain consistent page-based offsets | ||
| // that frontends rely on (e.g., limit=5 produces cursors 5, 10 ,15, ...) | ||
| const previousOffset = afterCursor?.id ? parseInt(afterCursor.id) : 0 | ||
| const effectiveLimit = limit ?? DEFAULT_PAGE_SIZE | ||
|
|
||
| let cursor: Record<string, string> | undefined | ||
| if (continuationToken) { | ||
| cursor = { continuationToken } | ||
| cursor = { continuationToken, id: (previousOffset + effectiveLimit).toString() } | ||
| } else if (finalResources.length > 0) { | ||
| const currentOffset = afterCursor?.id && !isNaN(parseInt(afterCursor.id)) ? parseInt(afterCursor.id) : 0 | ||
| cursor = { id: (currentOffset + finalResources.length).toString() } // Use the length of the results to calculate the next id | ||
| cursor = { id: (previousOffset + effectiveLimit).toString() } |
There was a problem hiding this comment.
previousOffset is computed via parseInt(afterCursor.id) without validating that afterCursor.id is numeric. If a client sends a non-numeric id alongside a continuationToken (or any other non-legacy cursor shape), this becomes NaN and the response cursor will include id: 'NaN'. Also, advancing cursor.id by effectiveLimit (instead of finalResources.length) can desynchronize offsets when fetchNext() returns fewer than maxItemCount, causing legacy clients (that ignore continuationToken) to skip results if they later fall back to OFFSET pagination. Consider (1) defaulting previousOffset to 0 unless afterCursor.id matches a numeric string, and (2) advancing by the actual number of returned items.
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
/integration sha=832ebbe |
|
⌛ Integration tests are running... Check their status here 👈 |
|
❌ Oh no! Integration tests have failed |
|
⌛ Integration tests are running... Check their status here 👈 |
|
✅ Integration tests have finished successfully! |
alvaroloes
left a comment
alvaroloes
left a comment
There was a problem hiding this comment.
Codex: I found a backward-compatibility regression in the new numeric cursor semantics. When limit is omitted, the implementation now treats the default page size as the cursor position, so a one-item page can emit { id: "100" }. A legacy client that feeds that numeric cursor back into the next request will skip rows instead of continuing from the next consumed item.
| } | ||
| } | ||
| if (process.env.TESTED_PROVIDER === 'AZURE' || process.env.TESTED_PROVIDER === 'LOCAL') { | ||
| expect(cartShippingAddress.cursor.id).to.equal('100') |
There was a problem hiding this comment.
Codex: This expectation is locking in the regression instead of the backward-compatibility guarantee from the PR description. For a paginated query that returns a single item with no explicit limit, the numeric cursor should still represent the next consumed position ("1"), not the default page size ("100"). Otherwise a legacy client that reuses { id: "100" } on the next request will skip 99 unseen rows.
Claude: Code Review — Fix Cosmos DB pagination for backwards compatibility (3.x backport)Este PR es un backport de #1594 a la rama ✅ Lo que está bien
🔴 Problemas que bloquean el merge (heredados del PR #1594)Los bugs del PR padre aplican directamente a este backport: 1. Mismo problema que en #1594. Si const parsedId = afterCursor?.id ? parseInt(afterCursor.id, 10) : NaN
const previousOffset = Number.isFinite(parsedId) ? parsedId : 02. En el path sin 3. La condición 🔴 Problemas específicos del backport4. Rush change file declara { "packageName": "@boostercloud/framework-core", "comment": "Fix pagination issues with Cosmos DB" }Mismo error que en #1594. El código afectado es 5. TypeScript nightly bump no relacionado con el fix El lockfile actualiza la versión de Copilot también señala esto. Recomendación: revertir este cambio en el lockfile antes de mergear, dejando solo los cambios de dependencias estrictamente necesarios para el fix. 6. Integration tests también fallaron múltiples veces en este PR Al igual que #1594, este PR tuvo múltiples rondas de integration tests fallidos antes de pasar con 🟡 Observaciones específicas del backport7. Divergencia entre #1594 y #1595 Si #1594 (main) recibe correcciones para los bugs señalados (puntos 1-3), ese fix deberá cherry-pickearse también a este PR antes de mergearlo. Si se mergea #1595 antes que #1594 esté corregido, la rama 3.x heredará los bugs del fix incompleto y habrá que hacer otro backport. Recomendación de proceso: Resolver primero todos los issues en #1594, luego actualizar #1595 con cherry-pick de los commits corregidos para mantener ambas ramas sincronizadas. 8. No hay indicación del número de versión 3.x que recibirá el fix En {
"packageName": "@boostercloud/framework-provider-azure",
"comment": "Restore numeric cursor id in CosmosDB pagination for backwards compatibility with frontends built on v3.4.1 and earlier",
"type": "patch"
}VeredictoRequest Changes. Los bugs lógicos de #1594 (puntos 1-3) bloquean este PR también. Adicionalmente, el TypeScript nightly bump no relacionado (punto 5) debería revertirse antes de mergear en una rama de release estable. La recomendación es corregir #1594 primero y cherry-pickear las correcciones aquí. |
|
/integration sha=f8cf991 |
|
⌛ Integration tests are running... Check their status here 👈 |
|
✅ Integration tests have finished successfully! |
|
@alvaroloes to not repeat myself, look at this comment from #1594. |
Description
Same fix as #1594, targeting the
release/3.xbranch (Node 20-compatible versions).Checks