fix(#73): add reverse range pagination regression coverage - #75
Merged
Conversation
Reverse iteration already yields each key exactly once: the reverse branch advances the end selector with firstGreaterOrEqual(lastKey), which correctly excludes the boundary key. The issue's proposed firstGreaterThan change was verified against a live FDB cluster to cause an infinite loop and was not applied. - Extract RangeResult::paginate() so pagination can be unit-tested without a cluster - Add tests/Unit/RangeResultTest.php (FDB-emulating mock, forward + reverse) - Add tests/Integration/ReversePaginationTest.php (1000 keys across server batches)
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #73.
Investigation against a live FDB 7.3.75 cluster showed that the current implementation already yields each key exactly once during reverse iteration. The reverse branch advances the end selector with
KeySelector::firstGreaterOrEqual($lastKey), which correctly excludes the already-yielded boundary key.The fix proposed in the issue (
firstGreaterThan($lastKey)) was empirically verified to be incorrect — it causes an infinite loop that returns the boundary key forever (see reproduction below). It was therefore deliberately not applied; applying it would breakgetRangeAll/getRangewithreverse: true.Changes
RangeResult::paginate()(a static method taking an injectable fetcher) so pagination logic can be unit-tested without a running cluster. Behavior is unchanged.tests/Unit/RangeResultTest.php— FDB-emulating mock; asserts forward and reverse iteration each key exactly once, descending/ascending order, boundary keys, and zero-limit short-circuit. This test fails if the wrongfirstGreaterThanchange is applied (it reproduces the duplicate/loop symptom).tests/Integration/ReversePaginationTest.php— 1000 keys iterated across real server batches in reverse; asserts count == 1000 and no duplicates.CHANGELOG.mdentry under Fixed.Verification
composer lint— passes (PHPCS + Rector + PHPStan).composer test:unit— passes (pre-existing GMP-relatedTupleTesterrors are unrelated to this change).composer test:integration— 203/203 pass against FDB 7.3.75.Reproduction of why the proposed fix is wrong
With the issue's
firstGreaterThan($lastKey)on the reverse branch, iterating 1000 keys yields: