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.