Fix #73: Add default order by primary key when no order is specified #76
Conversation
When using fastPaginate without an explicit ORDER BY clause, MySQL may return rows in an unpredictable order, which can cause inconsistent pagination results (e.g., missing the first row). This fix adds a default ORDER BY on the primary key when no explicit order is specified, ensuring deterministic results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe changes implement automatic primary key ordering for pagination queries to ensure deterministic result ordering. When a query lacks an explicit ORDER BY clause, the paginator now automatically applies ordering by the primary key. The implementation is added to the core FastPaginate class, test expectations are updated to reflect the new behavior, and a changelog entry documents the enhancement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Update the expected query to include the default ORDER BY clause. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai review |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)src/FastPaginate.php(1 hunks)tests/Integration/BuilderTest.php(11 hunks)tests/Integration/RelationTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Integration/BuilderTest.php (3)
src/FastPaginate.php (1)
fastPaginate(14-25)src/ScoutMixin.php (1)
fastPaginate(11-17)src/RelationMixin.php (1)
fastPaginate(14-28)
🪛 PHPMD (2.15.0)
tests/Integration/BuilderTest.php
157-157: Avoid unused local variables such as '$results'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (4)
CHANGELOG.md (1)
2-3: LGTM!The changelog entry clearly describes the behavioral change and properly references the related issue.
tests/Integration/RelationTest.php (1)
22-25: LGTM!The test expectation correctly reflects the new default primary key ordering behavior for relation queries without explicit ordering.
tests/Integration/BuilderTest.php (2)
153-171: Well-structured test for the new behavior.This test effectively validates both the inner query (used for ID selection) and the outer query include the default primary key ordering. The test clearly documents the purpose with inline comments.
234-244: LGTM!Updated comments and expectations correctly reflect the new default ordering behavior for queries without explicit ORDER BY.
| // If no order is specified, we need to add a default order by | ||
| // primary key to ensure deterministic results. Without this, | ||
| // MySQL may return rows in an unpredictable order. | ||
| // https://github.com/aarondfrancis/fast-paginate/issues/73 | ||
| if (empty($base->orders)) { | ||
| $this->orderBy("$table.$key"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the FastPaginate.php file to understand $key assignment and usage
find . -name "FastPaginate.php" -type f -exec wc -l {} \;Repository: aarondfrancis/fast-paginate
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for composite key handling and getKeyName usage
rg -n "getKeyName|composite" --type php -B2 -A2Repository: aarondfrancis/fast-paginate
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Search for test files
find . -name "*test*" -o -name "*Test*" | grep -E "\.(php|json)$"Repository: aarondfrancis/fast-paginate
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for composite key tests
rg -n "composite|KeyName|key.*array|multiple.*key" tests/ --type php -B1 -A1Repository: aarondfrancis/fast-paginate
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for any documentation or README mentioning keys
find . -name "README*" -o -name "*.md" | xargs grep -l "key\|composite" 2>/dev/null | head -3Repository: aarondfrancis/fast-paginate
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check CLAUDE.md for composite key discussion
cat ./CLAUDE.mdRepository: aarondfrancis/fast-paginate
Length of output: 2371
🏁 Script executed:
#!/bin/bash
# Also read the second usage around line 120-130
sed -n '115,135p' ./src/FastPaginate.phpRepository: aarondfrancis/fast-paginate
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Also get the context where $key is first used (around line 55)
sed -n '50,65p' ./src/FastPaginate.phpRepository: aarondfrancis/fast-paginate
Length of output: 869
🏁 Script executed:
#!/bin/bash
# Search for any existing handling or comments about composite keys or arrays
rg -n "array|KeyName" ./src/ --type phpRepository: aarondfrancis/fast-paginate
Length of output: 565
🏁 Script executed:
#!/bin/bash
# Get the entire file in readable format
wc -l ./src/FastPaginate.php && echo "---" && cat ./src/FastPaginate.phpRepository: aarondfrancis/fast-paginate
Length of output: 7076
🏁 Script executed:
#!/bin/bash
# Check if there are any issues or discussions about composite keys in the repo
git log --all --oneline --grep="composite\|key" 2>/dev/null | head -20Repository: aarondfrancis/fast-paginate
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Search for any documentation in the repo about key limitations
find . -name "*.md" -type f -exec grep -l "key\|composite\|primary" {} \;Repository: aarondfrancis/fast-paginate
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search Laravel documentation concept: check if any test models use composite keys
find ./tests -name "*.php" -type f -exec grep -l "primary\|key" {} \;Repository: aarondfrancis/fast-paginate
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Check Base.php to see if there are any composite key model definitions
grep -n -A5 -B5 "primary\|keyName" ./tests/Integration/Base.phpRepository: aarondfrancis/fast-paginate
Length of output: 456
Based on my verification of the codebase, here is the rewritten review comment:
Verify composite primary key support or add documentation about limitation.
The placement and use of fully qualified column names ($table.$key) are correct for simple primary keys. However, the implementation assumes getKeyName() returns a string and will fail silently or produce incorrect SQL if used with Eloquent models that define composite primary keys.
Testing reveals only single-key scenarios (including UUID keys) are tested. The code uses $key as a string throughout (lines 78, 103, 108, 169). If a composite key model is used, string interpolation will convert the array to the literal string "Array", breaking pagination.
Recommended fix: Either:
- Add a guard clause checking
is_string($key)with an appropriate fallback, or - Document that fast pagination requires single-column primary keys
This aligns with Laravel's native design which assumes single primary keys, but makes the constraint explicit rather than implicit.
🤖 Prompt for AI Agents
In src/FastPaginate.php around lines 72–78, the code assumes getKeyName()
returns a string and uses "$table.$key" which will break if a model has a
composite primary key (array); add a guard to detect non-string keys and handle
explicitly: if getKeyName() is not a string, either throw a clear exception
documenting that fast-paginate requires single-column primary keys or select a
deterministic fallback (e.g., use the first key element or build a safe
qualified list) and update the orderBy call accordingly, and also add/update
README/docs to state the single-key requirement if you choose the
exception/documentation route.
When using fastPaginate without an explicit ORDER BY clause, MySQL may return rows in an unpredictable order, which can cause inconsistent pagination results (e.g., missing the first row).
This fix adds a default ORDER BY on the primary key when no explicit order is specified, ensuring deterministic results.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.