Merge pull request #76 from aarondfrancis/fix-73

Fix #73: Add default order by primary key when no order is specified

Author: aarondfrancis

CHANGELOG.md

@@ -1,2 +1,3 @@
 # Changelog
-## Unreleased 
+## Unreleased
+- Add default `ORDER BY` primary key when no order is specified to ensure deterministic pagination results ([#73](https://github.com/aarondfrancis/fast-paginate/issues/73))

src/FastPaginate.php

@@ -69,6 +69,14 @@ protected function paginate(string $paginationMethod, Closure $paginatorOutput)
                 return $this->{$paginationMethod}($perPage, $columns, $pageName, $page);
             }
 
+            // 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");
+            }
+
             // This is the copy of the query that becomes
             // the inner query that selects keys only.
             $paginator = $this->clone()

tests/Integration/BuilderTest.php

@@ -38,7 +38,7 @@ public function basic_test()
         $this->assertCount(3, $queries);
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) limit 16 offset 0',
+            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) order by `users`.`id` asc limit 16 offset 0',
             $queries[2]['query']
         );
 
@@ -58,7 +58,7 @@ public function different_page_size()
         $this->assertEquals(5, $results->count());
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5) limit 6 offset 0',
+            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5) order by `users`.`id` asc limit 6 offset 0',
             $queries[2]['query']
         );
 
@@ -78,7 +78,7 @@ public function page_2()
         $this->assertEquals(5, $results->count());
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (6, 7, 8, 9, 10) limit 6 offset 0',
+            'select * from `users` where `users`.`id` in (6, 7, 8, 9, 10) order by `users`.`id` asc limit 6 offset 0',
             $queries[2]['query']
         );
 
@@ -98,7 +98,7 @@ public function pk_attribute_mutations_are_skipped()
         $this->assertEquals(5, $results->count());
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5) limit 6 offset 0',
+            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5) order by `users`.`id` asc limit 6 offset 0',
             $queries[2]['query']
         );
     }
@@ -114,7 +114,7 @@ public function custom_page_is_preserved()
         $this->assertEquals(2, $results->count());
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (1, 2) limit 3 offset 0',
+            'select * from `users` where `users`.`id` in (1, 2) order by `users`.`id` asc limit 3 offset 0',
             $queries[2]['query']
         );
 
@@ -150,6 +150,26 @@ public function custom_table_is_preserved()
         UserCustomTable::query()->fastPaginate();
     }
 
+    #[Test]
+    public function default_order_by_primary_key_when_no_order_specified()
+    {
+        $queries = $this->withQueriesLogged(function () use (&$results) {
+            $results = User::query()->fastPaginate(5);
+        });
+
+        // Inner query should have order by primary key to ensure deterministic results
+        $this->assertEquals(
+            'select `users`.`id` from `users` order by `users`.`id` asc limit 5 offset 0',
+            $queries[1]['query']
+        );
+
+        // Outer query should also preserve the order
+        $this->assertEquals(
+            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5) order by `users`.`id` asc limit 6 offset 0',
+            $queries[2]['query']
+        );
+    }
+
     #[Test]
     public function order_is_propagated()
     {
@@ -211,15 +231,15 @@ public function selects_are_overwritten()
             $results = User::query()->selectRaw('(select 1 as complicated_subquery)')->fastPaginate();
         });
 
-        // Dropped for our inner query
+        // Dropped for our inner query (default order by primary key is added)
         $this->assertEquals(
-            'select `users`.`id` from `users` limit 15 offset 0',
+            'select `users`.`id` from `users` order by `users`.`id` asc limit 15 offset 0',
             $queries[1]['query']
         );
 
-        // Restored for the user's query
+        // Restored for the user's query (with default order)
         $this->assertEquals(
-            'select (select 1 as complicated_subquery) from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) limit 16 offset 0',
+            'select (select 1 as complicated_subquery) from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) order by `users`.`id` asc limit 16 offset 0',
             $queries[2]['query']
         );
     }
@@ -327,7 +347,7 @@ public function uuids_are_bound_correctly()
 
         $this->assertCount(3, $queries);
         $this->assertEquals(
-            'select * from `notifications` where `notifications`.`id` in (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) limit 16 offset 0',
+            'select * from `notifications` where `notifications`.`id` in (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) order by `notifications`.`id` asc limit 16 offset 0',
             $queries[2]['query']
         );
 
@@ -363,7 +383,7 @@ public function basic_simple_test()
         $this->assertCount(2, $queries);
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) limit 16 offset 0',
+            'select * from `users` where `users`.`id` in (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) order by `users`.`id` asc limit 16 offset 0',
             $queries[1]['query']
         );
 
@@ -385,7 +405,7 @@ public function basic_simple_test_page_two()
         $this->assertCount(2, $queries);
 
         $this->assertEquals(
-            'select * from `users` where `users`.`id` in (6, 7, 8, 9, 10) limit 6 offset 0',
+            'select * from `users` where `users`.`id` in (6, 7, 8, 9, 10) order by `users`.`id` asc limit 6 offset 0',
             $queries[1]['query']
         );
 
@@ -407,7 +427,7 @@ public function basic_simple_test_from_relation()
         $this->assertCount(3, $queries);
 
         $this->assertEquals(
-            'select * from `posts` where `posts`.`user_id` = ? and `posts`.`user_id` is not null and `posts`.`id` in (1) limit 16 offset 0',
+            'select * from `posts` where `posts`.`user_id` = ? and `posts`.`user_id` is not null and `posts`.`id` in (1) order by `posts`.`id` asc limit 16 offset 0',
             $queries[2]['query']
         );
 

tests/Integration/RelationTest.php

@@ -20,7 +20,7 @@ public function basic_test()
         });
 
         $this->assertEquals(
-            'select * from `posts` where `posts`.`user_id` = ? and `posts`.`user_id` is not null and `posts`.`id` in (1) limit 16 offset 0',
+            'select * from `posts` where `posts`.`user_id` = ? and `posts`.`user_id` is not null and `posts`.`id` in (1) order by `posts`.`id` asc limit 16 offset 0',
             $queries[2]['query']
         );
     }