fix(database): fix DatabasePagingSource pagination when using orderByChild#2336
fix(database): fix DatabasePagingSource pagination when using orderByChild#2336demolaf wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new DatabasePagingKey class to replace String as the paging key in DatabasePagingSource and DatabasePagingOptions, allowing the paging source to correctly handle queries ordered by child values without producing duplicate items across pages. It also adds integration tests to verify this behavior. The review feedback suggests making DatabasePagingKey public and implementing equals() and hashCode() to ensure compatibility with the Paging library, as well as simplifying numeric type handling in startAtChildValue by checking for Number instead of Double and Long separately.
mikehardy
left a comment
mikehardy
left a comment
There was a problem hiding this comment.
looks like a perfect fix for the existing issue but may have a not-too-hard extension possibility to be even more general and fix the general case that 2000 was just a specific instance of
| public void setUp() throws InterruptedException { | ||
| FirebaseApp app; | ||
| try { | ||
| app = FirebaseApp.getInstance(APP_NAME); |
There was a problem hiding this comment.
curious if using s separate app is a deliberate choice, if not TestUtils has app init code in TestUtils.getAppInstance()
| if (pathIndex != null) { | ||
| task = startAtChildValue(key.getChildValue(), key.getNodeKey()) | ||
| .limitToFirst(params.getLoadSize() + 1).get(); | ||
| } else { |
There was a problem hiding this comment.
For non-PathIndex queries this falls back to startAt(null, nodeKey), which works for orderByKey() but likely not for orderByValue() — same cursor issue as #2000. Worth handling here (and in the lastKey construction ~L89) or updating the setQuery() docs to drop orderByValue() until supported.
| } else if (childValue instanceof Number) { | ||
| return mQuery.startAt(((Number) childValue).doubleValue(), nodeKey); | ||
| } | ||
| return mQuery.startAt(null, nodeKey); |
There was a problem hiding this comment.
hmm - looks like this fallback to startAt(null, nodeKey) is the same broken pattern as #2000 for orderByChild() — probably fine for missing child fields, but worth a one-line comment so it's not mistaken as the general case
2 participants
Fixes #2000 .
DatabasePagingSourcewas callingstartAt(null, key)for all queries, but Firebase Database ignores the key parameter when the query usesorderByChild(), causing every subsequent page load to return the same first page of results and throwingIllegalStateException: The same value was passed as the nextKey in two sequential Pages.The fix detects
orderByChild()queries via the internalPathIndexand callsstartAt(childValue, key)instead, storing both the child value and node key as the page cursor in a new package-privateDatabasePagingKeytype.