Skip to content

fix: detect duplicate page tokens in REST catalog listing methods - #17767

Open
waterWang wants to merge 2 commits into
apache:mainfrom
waterWang:fix/17755-repeated-page-token
Open

fix: detect duplicate page tokens in REST catalog listing methods#17767
waterWang wants to merge 2 commits into
apache:mainfrom
waterWang:fix/17755-repeated-page-token

Conversation

@waterWang

Copy link
Copy Markdown

Detect duplicate page tokens in RESTSessionCatalog pagination to prevent infinite loops when a REST catalog server returns a repeated next-page-token.

Problem

RESTSessionCatalog.listTables(), listNamespaces(), and listViews() all use a do-while loop that continues as long as nextPageToken != null. If a REST catalog server returns a token that the client has already seen, the loop never terminates — each repeated response is also added to the result builder, so memory use and request volume grow unbounded.

Fix

Track every page token used by each listing call with a HashSet<String>. If the set already contains the token, throw IllegalStateException with a clear error message before the next request.

Changes

  • RESTSessionCatalog.java: Added seenPageTokens set and duplicate detection guard to listTables(), listNamespaces(), and listViews()
  • TestDuplicatePageToken.java: New test class verifying all three methods throw on a repeated page token

@github-actions github-actions Bot added the core label Aug 22, 2026
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("duplicate page token");
}
} No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, one note below regarding testing coverage

assertThatThrownBy(() -> catalog.listViews(Namespace.of("demo")))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("duplicate page token");
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage gap on the new guard code path. All three tests assert only the throw, driven by an always-"stuck" server that repeats one token immediately. There is no positive test proving the guard leaves a valid multi-page listing intact (distinct tokens across N pages, null terminator, all results returned), and none covering the alternating-token loop (A to B to A to B) that the full-HashSet design specifically exists to catch. A no-throw happy-path test plus an alternating-token test should be added so a future regression in the guard (for example a stray double-add of the "" sentinel) that silently truncates real listings would be caught. Live regression risk is bounded because the existing CatalogTests/RESTCatalog pagination suites exercise normal multi-page listing, which is why this is minor rather than blocking, but the strengthening belongs in this PR since it introduces the new code path.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants