pagination: redundant Paginator iterator state, query-param parsing, and dead Link-header guards

[OmarAlJarrah]

Related: #28 will eventually replace RequestRebuilder’s query handling with a QueryParams multimap, and #30 will unify the pagination stacks; the items below are independent interim cleanups to the current code.

While reading through the pagination package I spotted a few small simplifications worth folding
in together, since they all sit in the same subsystem. Each one removes redundant state or a
provably-dead branch and is strictly behavior-preserving: no public signatures move, and the
existing tests should stay green. Grouping them in one ticket so they can land as a single small PR.

---

sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/Paginator.kt:148-232 — drop the started flag and collapse advance()'s request selection

The iterator carries a started boolean purely to pick initialRequest on the first fetch. If we
instead seed nextRequest with initialRequest, the first fetch reads nextRequest like every
later one, and the nextRequest ?: finishedPage.nextPageRequest() fallback becomes dead (by the time
advance() runs again, nextRequest already holds exactly that page's nextPageRequest()). That
lets the two-stage request selection collapse into a single guard.

Ordering must be preserved: keep pagesFetched++ after httpClient.execute(request), and keep
the nextRequest = … reassignment after a successful strategy.parse(…). Do not hoist either
ahead of the fetch. The sync iterator deliberately supports retry — if execute() throws, a caller
can catch it and call hasNext() again, which re-enters advance() and re-attempts the same
request. Mutating pagesFetched or nextRequest before the fetch would burn the page budget or
advance the cursor on a failed exchange, silently breaking that retry path.

Old code:

            // null = first iteration (use initialRequest); else the next-page request scheduled by
            // the previous page's strategy.parse() call.
            private var nextRequest: Request? = null

            // true once we have fetched at least one page. Used to distinguish "never fetched" from
            // "exhausted after fetching" — both have currentPage == null right after iteration ends.
            private var started: Boolean = false

            // true after iteration is definitively over; prevents further fetches.
            private var done: Boolean = false

            // Number of pages (HTTP exchanges) fetched so far. Iteration stops once this reaches
            // maxPages, guarding against servers that never advance their paging cursor.
            private var pagesFetched: Long = 0L

            override fun hasNext(): Boolean {
                while (true) {
                    val page = currentPage
                    if (page != null && currentItemIndex < page.items.size) {
                        return true
                    }
                    if (done) {
                        return false
                    }
                    if (!advance()) {
                        return false
                    }
                }
            }

            override fun next(): T {
                if (!hasNext()) {
                    throw NoSuchElementException("Paginator iterator exhausted.")
                }
                val page = currentPage!!
                val item = page.items[currentItemIndex]
                currentItemIndex++
                return item
            }

            /**
             * Fetches the next page. Returns `true` if a new page was loaded into
             * [currentPage]; `false` if iteration is now done.
             */
            private fun advance(): Boolean {
                if (pagesFetched >= maxPages) {
                    // Safety cap reached: stop before fetching a page we would otherwise yield,
                    // even if the previous page still reports hasNext.
                    done = true
                    currentPage = null
                    return false
                }
                val request: Request? =
                    if (!started) {
                        initialRequest
                    } else {
                        // We have a current page that's exhausted — see if it can produce a next.
                        val finishedPage = currentPage
                        if (finishedPage == null || !finishedPage.hasNext) {
                            null
                        } else {
                            nextRequest ?: finishedPage.nextPageRequest()
                        }
                    }
                if (request == null) {
                    done = true
                    currentPage = null
                    return false
                }
                val response: Response = httpClient.execute(request)
                pagesFetched++
                val page: Page<T> =
                    try {
                        strategy.parse(response, initialRequest)
                    } finally {
                        response.close()
                    }
                started = true
                currentPage = page
                currentItemIndex = 0
                // Compute the next request now so we don't have to retain the (closed) response.
                // If this page has no next, nextRequest stays null; advance() will treat that as done.
                nextRequest = if (page.hasNext) page.nextPageRequest() else null
                return true
            }

New code:

            // The next request to fetch: seeded with initialRequest, then replaced after each page
            // with that page's nextPageRequest(), or null once a page reports no successor (which
            // ends the stream on the following advance()).
            private var nextRequest: Request? = initialRequest

            // true after iteration is definitively over; prevents further fetches.
            private var done: Boolean = false

            // Number of pages (HTTP exchanges) fetched so far. Iteration stops once this reaches
            // maxPages, guarding against servers that never advance their paging cursor.
            private var pagesFetched: Long = 0L

            override fun hasNext(): Boolean {
                while (true) {
                    val page = currentPage
                    if (page != null && currentItemIndex < page.items.size) {
                        return true
                    }
                    if (done) {
                        return false
                    }
                    if (!advance()) {
                        return false
                    }
                }
            }

            override fun next(): T {
                if (!hasNext()) {
                    throw NoSuchElementException("Paginator iterator exhausted.")
                }
                val page = currentPage!!
                val item = page.items[currentItemIndex]
                currentItemIndex++
                return item
            }

            /**
             * Fetches the next page. Returns `true` if a new page was loaded into
             * [currentPage]; `false` if iteration is now done.
             */
            private fun advance(): Boolean {
                val request = nextRequest
                if (request == null || pagesFetched >= maxPages) {
                    // Either no next request is scheduled (the previous page had no successor) or
                    // the safety cap is reached — stop before fetching a page we would otherwise
                    // yield, even if the previous page still reports hasNext.
                    done = true
                    currentPage = null
                    return false
                }
                val response: Response = httpClient.execute(request)
                pagesFetched++
                val page: Page<T> =
                    try {
                        strategy.parse(response, initialRequest)
                    } finally {
                        response.close()
                    }
                currentPage = page
                currentItemIndex = 0
                // Compute the next request now so we don't have to retain the (closed) response.
                // If this page has no next, nextRequest goes null; the next advance() ends the stream.
                nextRequest = if (page.hasNext) page.nextPageRequest() else null
                return true
            }

Usage — before → after:

val paginator = Paginator(httpClient, initialRequest, strategy, maxPages = 100)
for (item in paginator.iterateAll()) {
    handle(item)
}
paginator.streamAll().forEach(::handle)

val paginator = Paginator(httpClient, initialRequest, strategy, maxPages = 100)
for (item in paginator.iterateAll()) {
    handle(item)
}
paginator.streamAll().forEach(::handle)

call sites unchanged — behavior-preserving (the edits are entirely inside the private
PaginatorIterator).

Why: started only encodes "has the first page been fetched", which is redundant once
nextRequest is seeded with initialRequest; the nextRequest ?: finishedPage.nextPageRequest()
fallback then re-derives a value advance() already computed at the end of the previous page. Seeding
the field collapses the request selection to one guard and removes both the flag and the dead
re-derivation.

API / Build: No public-API change — the edits live entirely inside the private
PaginatorIterator, so apiCheck is unaffected (no apiDump needed). Java-8 bytecode and
explicit-API strict mode are likewise unaffected.

---

sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt:109-125 — parse query params with substringBefore / substringAfter, matching setQueryParam

getQueryParam hand-rolls indexOf('=') plus two conditional substring calls, while its sibling
setQueryParam already extracts the key with segment.substringBefore('=', segment). Aligning the
two reads better and keeps the edge-case handling in one recognizable idiom.

Old code:

    fun getQueryParam(
        url: URL,
        name: String,
    ): String? {
        val existing = url.query ?: return null
        if (existing.isEmpty()) return null
        for (segment in existing.split('&')) {
            if (segment.isEmpty()) continue
            val eq = segment.indexOf('=')
            val rawKey = if (eq >= 0) segment.substring(0, eq) else segment
            if (decodeOrRaw(rawKey) == name) {
                val rawValue = if (eq >= 0) segment.substring(eq + 1) else ""
                return decodeOrRaw(rawValue)
            }
        }
        return null
    }

New code:

    fun getQueryParam(
        url: URL,
        name: String,
    ): String? {
        val existing = url.query ?: return null
        if (existing.isEmpty()) return null
        for (segment in existing.split('&')) {
            if (segment.isEmpty()) continue
            val key = segment.substringBefore('=', segment)
            if (decodeOrRaw(key) == name) {
                return decodeOrRaw(segment.substringAfter('=', ""))
            }
        }
        return null
    }

Edge cases are identical to the index-juggling version (substringBefore/substringAfter split on
the first = and fall back to the supplied default when there is none):

segment	old (rawKey, rawValue)	new (key, value)
flag (no =)	("flag", "")	("flag", "")
key= (empty value)	("key", "")	("key", "")
k=a=b (= in value)	("k", "a=b")	("k", "a=b")

Usage — before → after:

// PageNumberPaginationStrategy.kt
val executedPageParam: String? = RequestRebuilder.getQueryParam(executedUrl, pageParam)

// PageNumberPaginationStrategy.kt
val executedPageParam: String? = RequestRebuilder.getQueryParam(executedUrl, pageParam)

call sites unchanged — behavior-preserving (sole caller is PageNumberPaginationStrategy).

Why: Reusing the same substringBefore('=', segment) key extraction as setQueryParam removes
the manual offset bookkeeping and reads more clearly, with identical first-= split and empty-value
semantics.

API / Build: RequestRebuilder is an internal object, so this touches no public-API surface;
apiCheck is unaffected.

---

sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/LinkHeaderPaginationStrategy.kt:57-62 — drop the empty-string guards around the Link-header join

The join already produces the right input for extractNextUrl in every case, so the two empty-string
guards are dead: an empty values(...) list joins to "", and extractNextUrl("") already returns
null because splitLinkValues("") yields an empty list. The whole thing folds into one expression.

Old code:

            // Some servers emit multiple Link headers (one per link-value) instead of a single
            // comma-separated header. Join with ',' so a single parser handles both shapes.
            val linkValues: List<String> = response.headers.values(linkHeader)
            val combined: String =
                if (linkValues.isEmpty()) "" else linkValues.joinToString(separator = ",")
            val nextUrlString: String? = if (combined.isEmpty()) null else extractNextUrl(combined)

New code:

            // Some servers emit multiple Link headers (one per link-value) instead of a single
            // comma-separated header. Join with ',' so a single parser handles both shapes; an
            // empty values list joins to "", and extractNextUrl("") already returns null.
            val nextUrlString: String? =
                extractNextUrl(response.headers.values(linkHeader).joinToString(separator = ","))

Usage — before → after:

val strategy = LinkHeaderPaginationStrategy(itemsExtractor = { resp -> parseItems(resp) })
val paginator = Paginator(httpClient, firstPageRequest, strategy)
paginator.iterateAll().forEach(::handle)

val strategy = LinkHeaderPaginationStrategy(itemsExtractor = { resp -> parseItems(resp) })
val paginator = Paginator(httpClient, firstPageRequest, strategy)
paginator.iterateAll().forEach(::handle)

call sites unchanged — behavior-preserving (the change is internal to parse()).

Why: Both guards are provably dead — an empty list joins to "" and extractNextUrl("") is
already null — so the two intermediate vals and the unreachable branches collapse into a single
extractNextUrl(...) call. The separator = "," argument is retained so the join still uses a bare
comma rather than joinToString's default ", ".

API / Build: parse(...) keeps its signature (it overrides PaginationStrategy), so there is no
public-API change.