Skip to content

refactor: unify pagination onto a single Page-based surface#145

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
refactor/unify-pagination-stacks
Open

refactor: unify pagination onto a single Page-based surface#145
OmarAlJarrah wants to merge 1 commit into
mainfrom
refactor/unify-pagination-stacks

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Problem

sdk-core shipped two parallel pagination implementations:

  • pagination/*Paginator + PaginationStrategy + Page, which drives requests through an HttpClient, closes each response right after the strategy parses it, applies a maxPages safety cap, and ships cursor, page-number, and link-header strategies.
  • http/paging/*PagedIterable + PagedResponse + PagingOptions, a thinner first-/next-page fetcher abstraction. Its byPage() sequence leaked the underlying response — and the pooled connection behind it — on partial consumption unless the caller manually closed every yielded page.

Two surfaces for the same job meant duplicated concepts, divergent close semantics, and a genuine connection leak in the second one.

Change

  • Collapse onto the Paginator/Page stack — the more complete one, with correct per-page response lifecycle — and delete the PagedIterable stack (PagedIterable, PagedResponse, PagingOptions, FirstPageFetcher, NextPageFetcher) along with its test.
  • Fold the page-level signals PagedResponse exposed into the unified Page<T> contract: optional statusCode and headers, plus the HATEOAS links / continuation token (nextLink, previousLink, firstLink, lastLink, continuationToken). The accessors are interface defaults returning null, so existing third-party Page implementations keep compiling unchanged.
  • Populate that metadata from the bundled strategies. LinkHeaderPaginationStrategy now parses all four navigation relations (next / prev / first / last) instead of only rel="next"; CursorPaginationStrategy surfaces the next cursor as continuationToken; all three record statusCode and headers.
  • Add PageMetadataTest covering the new accessors (defaults, link extraction across all relations, cursor token, status/headers on populated and empty pages).
  • Update README.md, docs/architecture.md, and docs/refs-comparison.md to describe the single surface.

The max-pages cap, per-page response close, and interrupt-aware blocking already lived in Paginator; the byPage leak is gone because that path no longer exists. Item-level iteration (iterateAll() / streamAll()) was already leak-free — the paginator closes each response after parsing.

Rationale

One pagination surface removes a maintenance hazard and a real resource leak, and it settles the page model before the first code-generator run (typed page classes and async pagination build on this).

Public API

Removes org.dexpace.sdk.core.http.paging.*. Adds the default metadata accessors to Page. api/sdk-core.api regenerated and committed.

Coordination

This overlaps the QueryParams work in #28 — both touch RequestRebuilder. Please sequence the merges so the RequestRebuilder changes land cleanly.

Gated build (scoped to sdk-core)

./gradlew :sdk-core:apiDump --no-daemon      # BUILD SUCCESSFUL (regenerated api snapshot)
./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck --no-daemon
                                             # BUILD SUCCESSFUL

All four gates pass. One unrelated, pre-existing flaky concurrency test (ContextStoreTest.concurrent untraced calls keep independent entries) intermittently NPEs; it passes in isolation and on re-run, and is untouched by this change.

Closes #30

The SDK carried two parallel pagination implementations:

  - pagination/* — Paginator + PaginationStrategy + Page, which drives
    HTTP requests through an HttpClient, closes each response after the
    strategy parses it, applies a maxPages safety cap, and ships cursor,
    page-number, and link-header strategies.
  - http/paging/* — PagedIterable + PagedResponse + PagingOptions, a
    thinner first-/next-page fetcher abstraction whose byPage() sequence
    leaked the underlying response (and pooled connection) on partial
    consumption unless the caller closed every page by hand.

Two surfaces for the same job means duplicated concepts, divergent
close semantics, and a real connection leak in the second one. This
collapses everything onto the Paginator/Page stack — the more complete
one, with correct per-page response lifecycle — and removes the
PagedIterable stack entirely.

To preserve the page-level signals PagedResponse exposed, the Page
interface now carries optional response metadata (statusCode, headers)
and the HATEOAS links / continuation token (nextLink, previousLink,
firstLink, lastLink, continuationToken). The accessors default to null,
so existing third-party Page implementations keep compiling unchanged;
the bundled strategies populate the envelope and links they already
parse. LinkHeaderPaginationStrategy now extracts all four navigation
relations instead of only rel="next".

Public API: removes org.dexpace.sdk.core.http.paging.*; adds the default
metadata accessors to Page. api/sdk-core.api regenerated accordingly.

Closes #30
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

Nice consolidation — folding the http.paging metadata into pagination.Page and reworking the Link-header parser into an extractLinks map (so prev/first/last surface alongside next) removes a redundant surface without losing capability. The .api snapshot, README, and docs all move in step. Good to merge once the small doc gap below is closed.

Issues

  • Stale package reference in CLAUDE.mdCLAUDE.md:52. This PR deletes the whole http.paging package and updates README, docs/architecture.md, and docs/refs-comparison.md to match, but the project CLAUDE.md key-packages map still lists http.paging/PagedIterable. It won't break the build, but it leaves contributor guidance pointing at types that no longer exist; worth fixing here alongside the other doc edits so they stay in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify the two pagination stacks into one

1 participant