Skip to content

pagination: dedupe close-on-parse-failure, the iterator→Stream adapter, and maxPages validation #204

Description

@OmarAlJarrah

While reading the unified pagination package from #202, a few small, behavior-preserving
simplifications stand out. Grouping them so they can land as a single small PR (in the spirit
of #177). None move a public signature; existing tests should stay green.


close-on-parse-failure is copied into the sync and async paginators

Paginator.walker() (Paginator.kt:117-125) and AsyncPaginator.fetchPage()
(AsyncPaginator.kt:478-485) implement the same sequence — parse under a try, close the
response and rethrow on failure, otherwise build Page(response, info.items) — down to a
byte-identical comment:

response.close() // parse failed → this page never becomes a Page; release it now

This is exactly the close-on-parse-failure invariant the unification most needs to keep
consistent, yet it lives in two classes and can silently diverge. Both call sites share
strategy and initialRequest, so it collapses to one internal helper:

private fun parseOrClose(response: Response): PageInfo<T> =
    try {
        strategy.parse(response, initialRequest)
    } catch (t: Throwable) {
        response.close()
        throw t
    }

CloseablePages.stream() re-implements the iterator→Stream adapter PageWalker claims to own

PageWalker's KDoc states it is "the single home for the iterator→Stream adaptation, so the
public wrappers do not each re-implement it," backed by a private streamOf(...) and
itemStream(). But PageWalker exposes no page-level stream accessor, so
CloseablePages.stream() (CloseablePages.kt:76-80) inlines the same
StreamSupport.stream(Spliterators.spliteratorUnknownSize(...), false) construction and
re-imports the same Spliterator(s) / StreamSupport symbols. The "single home" claim is
false as written. Either add a page-level pageStream() accessor to PageWalker and route
CloseablePages through it (folding in the onClose fix tracked separately), or soften the
PageWalker KDoc to acknowledge the page-level wrapper adapts its own auto-closing iterator.

maxPages validation + message is duplicated across four init blocks

require(maxPages > 0L) { "maxPages must be positive, was $maxPages" }

appears verbatim in PageWalker.kt:35, Paginator.kt:94, PagedIterable.kt:50, and
AsyncPaginator.kt:134. The eager construction-time checks in the public wrappers are worth
keeping (they fail fast when the object is built, not lazily on first walk), so the fix is one
shared helper rather than deleting copies:

internal fun requirePositiveMaxPages(maxPages: Long) {
    require(maxPages > 0L) { "maxPages must be positive, was $maxPages" }
}

called from all four. (PageWalker's copy is currently unreachable from the shipped API —
both sync callers pre-validate before constructing the walker; the async path is its own
validator.)

References

Follows the #202 unification (#30).

Metadata

Metadata

Assignees

No one assigned

    Labels

    sdk-coresdk-core toolkittech-debtsimplification / cleanup

    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