Skip to content

Two pagination surfaces with divergent resume-token and loop-guard contracts share one error field #61

@OmarAlJarrah

Description

@OmarAlJarrah

Current design

We ship two unrelated pagination systems.

  1. http.common.paginationPager / ItemPaged and their async twins (packages/dexpace-sdk-core/src/dexpace/sdk/core/http/common/pagination.py). These are callable-driven: the caller supplies get_next(continuation_token) and extract_data(page) -> (next_token, items). They are transport-agnostic with no pipeline involvement. All four are re-exported from http/common/__init__.py (__all__ lines 21–29), but nothing in the tree references them except tests/http/test_pagination.py.

  2. pagination/Paginator / AsyncPaginator (packages/dexpace-sdk-core/src/dexpace/sdk/core/pagination/paginator.py), driven by the PaginationStrategy SPI and integrated with the pipeline via the shared dispatch seam. This is the system the README advertises (README.md lines 184, 221–223) and the only one mentioned in the changelog.

The two are wired to the same error field, SdkError.continuation_token (errors/base.py). Its docstring and docs/errors.md (lines 65–68) describe that field exclusively in terms of the first system:

SdkError.continuation_token is auto-populated by the paging iterators (Pager / AsyncPager / ItemPaged / AsyncItemPaged) when a get_next call raises. Callers can resume by passing the token to by_page(continuation_token=…).

But the system that the docs actually point users at — Paginator — is the one that populates that field in practice, and it does so with different semantics:

  • It stamps the token only inside _decode_for (paginator.py:76–104), only on a JSON DeserializationError, and the value it stamps is str(request.url) — a URL string.
  • Paginator.by_page() (paginator.py:158) takes no continuation_token argument, so the resume path the docstring names does not exist on this surface. A URL string is also not what Pager.get_next(token) consumes, so the value is not re-feedable to the surface the docstring actually describes either.

The loop guards also differ in shape: Pager terminates when continuation_token is None after the first call (pagination.py:71), while Paginator terminates when page.next_request is None (paginator.py:168, paginator.py:175). And the error-stamping scope differs: Pager stamps the token onto any SdkError raised by get_next/extract_data (pagination.py:79–82), whereas Paginator stamps only on a decode failure.

Trade-off / concern

One concern (paging) is served by two systems with two mental models, two import paths, and divergent termination and resume semantics. A reader who starts from the README sees Paginator; a reader who starts from http.common.__init__ or the SdkError docstring sees Pager. The shared continuation_token field anchors the error vocabulary to the system that is not canonical, and the value the canonical system writes into it (a URL) is not consumable by the surface the docstring names — nor by any documented Paginator resume entry point, because there isn't one.

This also sits in tension with the project's narrow-public-API convention (the public surface is what __init__ re-exports): we export a second, undocumented pagination primitive that nothing in the SDK uses, and the only place a consumer would learn its resume contract describes a method signature that doesn't match the system actually emitting the token.

Proposed direction

Pick one canonical surface and make the error field describe a single, re-feedable token.

The strategy-driven, pipeline-integrated pagination/ package is the one the docs already advertise, so make it canonical. Then either:

  • Demote Pager/ItemPaged: drop them from http.common (or move them under pagination/ as an explicitly-layered raw-callable primitive), and rewrite the SdkError.continuation_token docstring and docs/errors.md around Paginator's resume contract. If we keep them, document the layering plainly (Pager = raw callable loop; Paginator = pipeline + strategy over the same notion of a page sequence) and unify both on one loop-guard predicate and one resume-token contract, so max_pages, termination, and continuation stamping behave identically across both.

  • Regardless of which way we go, redefine SdkError.continuation_token so its value is a token that is actually re-feedable into whichever surface produced it, and give the canonical surface a real resume entry point that accepts it (e.g. Paginator(..., resume_from=token) or by_page(continuation_token=…)).

Trade-offs: deletion is the cleanest and removes the dual mental model, but it removes a callable-driven primitive some users may want for non-pipeline paging. Unification keeps both layers but requires designing a shared continuation/resume contract that fits both the strategy form and the opaque-token-callable form — more design work, but it makes the error vocabulary coherent and lets a resume token round-trip through either surface.

A note on the current shape

The _decode_for docstring (paginator.py:80–84) explicitly says it "mirrors Pager's resume contract" — so the intent to keep the two consistent is already in the code. The gap is that the mirror is partial: only the decode-failure path stamps a token, the value is a URL rather than a strategy-consumable token, and Paginator exposes no way to feed it back. Closing that gap (one token, one resume path, one loop guard) is what would make the stated intent actually hold, and is worth doing before more strategies or a third caller accrete on top of the current split.

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentationenhancementNew feature or request

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions