[10.x] Binding order is incorrect when using cursor paginate with mul…

…tiple unions with a where (#50884) * Add failing test for binding order * Add failing test for bindings with multiple order clauses * Fix binding order for cursor paginate with multiple unions * Use setBindings to clear the union bindings * Add bindings to nested union builders * Only reset union bindings if unions is set * Fix getting union builders for Eloquent queries * Remove duplicate bindings * Passthrough getRawBindings to support cursor pagination logic * Give each builder its own name to make the code easier to trace * Add test for multi union, multi where binding order problem * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Add docblock * Use FQN in the docblock Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com> * Use FQN in the docblock Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com> * formatting * formatting --------- Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com> Co-authored-by: Taylor Otwell <taylor@laravel.com>
laravel · Apr 16, 2024 · 67d4d0d · 67d4d0d
1 parent 8e9ab6d
commit 67d4d0d
Show file tree

Hide file tree

Showing 5 changed files with 166 additions and 8 deletions.
diff --git a/src/Illuminate/Database/Concerns/BuildsQueries.php b/src/Illuminate/Database/Concerns/BuildsQueries.php
@@ -379,8 +379,11 @@ protected function paginateUsingCursor($perPage, $columns = ['*'], $cursorName =
  $orders = $this->ensureOrderForCursorPagination(! is_null($cursor) && $cursor->pointsToPreviousItems());
 
  if (! is_null($cursor)) {
+ // Reset the union bindings so we can add the cursor where in the correct position...
+ $this->setBindings([], 'union');
+
  $addCursorConditions = function (self $builder, $previousColumn, $i) use (&$addCursorConditions, $cursor, $orders) {
- $unionBuilders = isset($builder->unions) ? collect($builder->unions)->pluck('query') : collect();
+ $unionBuilders = $builder->getUnionBuilders();
 
  if (! is_null($previousColumn)) {
  $originalColumn = $this->getOriginalColumnNameForCursorPagination($this, $previousColumn);
@@ -402,37 +405,40 @@ protected function paginateUsingCursor($perPage, $columns = ['*'], $cursorName =
  });
  }
 
- $builder->where(function (self $builder) use ($addCursorConditions, $cursor, $orders, $i, $unionBuilders) {
+ $builder->where(function (self $secondBuilder) use ($addCursorConditions, $cursor, $orders, $i, $unionBuilders) {
  ['column' => $column, 'direction' => $direction] = $orders[$i];
 
  $originalColumn = $this->getOriginalColumnNameForCursorPagination($this, $column);
 
- $builder->where(
+ $secondBuilder->where(
  Str::contains($originalColumn, ['(', ')']) ? new Expression($originalColumn) : $originalColumn,
  $direction === 'asc' ? '>' : '<',
  $cursor->parameter($column)
  );
 
  if ($i < $orders->count() - 1) {
- $builder->orWhere(function (self $builder) use ($addCursorConditions, $column, $i) {
- $addCursorConditions($builder, $column, $i + 1);
+ $secondBuilder->orWhere(function (self $thirdBuilder) use ($addCursorConditions, $column, $i) {
+ $addCursorConditions($thirdBuilder, $column, $i + 1);
  });
  }
 
  $unionBuilders->each(function ($unionBuilder) use ($column, $direction, $cursor, $i, $orders, $addCursorConditions) {
- $unionBuilder->where(function ($unionBuilder) use ($column, $direction, $cursor, $i, $orders, $addCursorConditions) {
+ $unionWheres = $unionBuilder->getRawBindings()['where'];
+
+ $unionBuilder->where(function ($unionBuilder) use ($column, $direction, $cursor, $i, $orders, $addCursorConditions, $unionWheres) {
  $unionBuilder->where(
  $this->getOriginalColumnNameForCursorPagination($this, $column),
  $direction === 'asc' ? '>' : '<',
  $cursor->parameter($column)
  );
 
  if ($i < $orders->count() - 1) {
- $unionBuilder->orWhere(function (self $builder) use ($addCursorConditions, $column, $i) {
- $addCursorConditions($builder, $column, $i + 1);
+ $unionBuilder->orWhere(function (self $fourthBuilder) use ($addCursorConditions, $column, $i) {
+ $addCursorConditions($fourthBuilder, $column, $i + 1);
  });
  }
 
+ $this->addBinding($unionWheres, 'union');
  $this->addBinding($unionBuilder->getRawBindings()['where'], 'union');
  });
  });

diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php
@@ -108,6 +108,7 @@ class Builder implements BuilderContract
  'getbindings',
  'getconnection',
  'getgrammar',
+ 'getrawbindings',
  'implode',
  'insert',
  'insertgetid',
@@ -1727,6 +1728,18 @@ public function withSavepointIfNeeded(Closure $scope): mixed
  : $scope();
  }
 
+ /**
+ * Get the Eloquent builder instances that are used in the union of the query.
+ *
+ * @return \Illuminate\Support\Collection
+ */
+ protected function getUnionBuilders()
+ {
+ return isset($this->query->unions)
+ ? collect($this->query->unions)->pluck('query')
+ : collect();
+ }
+
  /**
  * Get the underlying query builder instance.
  *

diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php
@@ -3815,6 +3815,18 @@ public function raw($value)
  return $this->connection->raw($value);
  }
 
+ /**
+ * Get the query builder instances that are used in the union of the query.
+ *
+ * @return \Illuminate\Support\Collection
+ */
+ protected function getUnionBuilders()
+ {
+ return isset($this->unions)
+ ? collect($this->unions)->pluck('query')
+ : collect();
+ }
+
  /**
  * Get the current query value bindings in a flattened array.
  *

diff --git a/tests/Database/DatabaseQueryBuilderTest.php b/tests/Database/DatabaseQueryBuilderTest.php
@@ -5297,6 +5297,105 @@ public function testCursorPaginateWithUnionWheres()
  ]), $result);
  }
 
+ public function testCursorPaginateWithMultipleUnionsAndMultipleWheres()
+ {
+ $ts = now()->toDateTimeString();
+
+ $perPage = 16;
+ $columns = ['test'];
+ $cursorName = 'cursor-name';
+ $cursor = new Cursor(['created_at' => $ts]);
+ $builder = $this->getMockQueryBuilder();
+ $builder->select('id', 'start_time as created_at')->selectRaw("'video' as type")->from('videos');
+ $builder->union($this->getBuilder()->select('id', 'created_at')->selectRaw("'news' as type")->from('news')->where('extra', 'first'));
+ $builder->union($this->getBuilder()->select('id', 'created_at')->selectRaw("'podcast' as type")->from('podcasts')->where('extra', 'second'));
+ $builder->orderBy('created_at');
+
+ $builder->shouldReceive('newQuery')->andReturnUsing(function () use ($builder) {
+ return new Builder($builder->connection, $builder->grammar, $builder->processor);
+ });
+
+ $path = 'http://foo.bar?cursor='.$cursor->encode();
+
+ $results = collect([
+ ['id' => 1, 'created_at' => now(), 'type' => 'video'],
+ ['id' => 2, 'created_at' => now(), 'type' => 'news'],
+ ['id' => 3, 'created_at' => now(), 'type' => 'podcasts'],
+ ]);
+
+ $builder->shouldReceive('get')->once()->andReturnUsing(function () use ($builder, $results, $ts) {
+ $this->assertEquals(
+ '(select "id", "start_time" as "created_at", \'video\' as type from "videos" where ("start_time" > ?)) union (select "id", "created_at", \'news\' as type from "news" where "extra" = ? and ("start_time" > ?)) union (select "id", "created_at", \'podcast\' as type from "podcasts" where "extra" = ? and ("start_time" > ?)) order by "created_at" asc limit 17',
+ $builder->toSql());
+ $this->assertEquals([$ts], $builder->bindings['where']);
+ $this->assertEquals(['first', $ts, 'second', $ts], $builder->bindings['union']);
+
+ return $results;
+ });
+
+ Paginator::currentPathResolver(function () use ($path) {
+ return $path;
+ });
+
+ $result = $builder->cursorPaginate($perPage, $columns, $cursorName, $cursor);
+
+ $this->assertEquals(new CursorPaginator($results, $perPage, $cursor, [
+ 'path' => $path,
+ 'cursorName' => $cursorName,
+ 'parameters' => ['created_at'],
+ ]), $result);
+ }
+
+ public function testCursorPaginateWithUnionMultipleWheresMultipleOrders()
+ {
+ $ts = now()->toDateTimeString();
+
+ $perPage = 16;
+ $columns = ['id', 'created_at', 'type'];
+ $cursorName = 'cursor-name';
+ $cursor = new Cursor(['id' => 1, 'created_at' => $ts, 'type' => 'news']);
+ $builder = $this->getMockQueryBuilder();
+ $builder->select('id', 'start_time as created_at', 'type')->from('videos')->where('extra', 'first');
+ $builder->union($this->getBuilder()->select('id', 'created_at', 'type')->from('news')->where('extra', 'second'));
+ $builder->union($this->getBuilder()->select('id', 'created_at', 'type')->from('podcasts')->where('extra', 'third'));
+ $builder->orderBy('id')->orderByDesc('created_at')->orderBy('type');
+
+ $builder->shouldReceive('newQuery')->andReturnUsing(function () use ($builder) {
+ return new Builder($builder->connection, $builder->grammar, $builder->processor);
+ });
+
+ $path = 'http://foo.bar?cursor='.$cursor->encode();
+
+ $results = collect([
+ ['id' => 1, 'created_at' => now()->addDay(), 'type' => 'video'],
+ ['id' => 1, 'created_at' => now(), 'type' => 'news'],
+ ['id' => 1, 'created_at' => now(), 'type' => 'podcast'],
+ ['id' => 2, 'created_at' => now(), 'type' => 'podcast'],
+ ]);
+
+ $builder->shouldReceive('get')->once()->andReturnUsing(function () use ($builder, $results, $ts) {
+ $this->assertEquals(
+ '(select "id", "start_time" as "created_at", "type" from "videos" where "extra" = ? and ("id" > ? or ("id" = ? and ("start_time" < ? or ("start_time" = ? and ("type" > ?)))))) union (select "id", "created_at", "type" from "news" where "extra" = ? and ("id" > ? or ("id" = ? and ("start_time" < ? or ("start_time" = ? and ("type" > ?)))))) union (select "id", "created_at", "type" from "podcasts" where "extra" = ? and ("id" > ? or ("id" = ? and ("start_time" < ? or ("start_time" = ? and ("type" > ?)))))) order by "id" asc, "created_at" desc, "type" asc limit 17',
+ $builder->toSql());
+ $this->assertEquals(['first', 1, 1, $ts, $ts, 'news'], $builder->bindings['where']);
+ $this->assertEquals(['second', 1, 1, $ts, $ts, 'news', 'third', 1, 1, $ts, $ts, 'news'], $builder->bindings ['union']);
+
+ return $results;
+ });
+
+ Paginator::currentPathResolver(function () use ($path) {
+ return $path;
+ });
+
+ $result = $builder->cursorPaginate($perPage, $columns, $cursorName, $cursor);
+
+ $this->assertEquals(new CursorPaginator($results, $perPage, $cursor, [
+ 'path' => $path,
+ 'cursorName' => $cursorName,
+ 'parameters' => ['id', 'created_at', 'type'],
+ ]), $result);
+ }
+
  public function testCursorPaginateWithUnionWheresWithRawOrderExpression()
  {
  $ts = now()->toDateTimeString();

diff --git a/tests/Integration/Database/EloquentCursorPaginateTest.php b/tests/Integration/Database/EloquentCursorPaginateTest.php
@@ -167,6 +167,34 @@ public function testPaginationWithMultipleWhereClauses()
  );
  }
 
+ public function testPaginationWithMultipleUnionAndMultipleWhereClauses()
+ {
+ TestPost::create(['title' => 'Post A', 'user_id' => 100]);
+ TestPost::create(['title' => 'Post B', 'user_id' => 101]);
+
+ $table1 = TestPost::select(['id', 'title', 'user_id'])->where('user_id', 100);
+ $table2 = TestPost::select(['id', 'title', 'user_id'])->where('user_id', 101);
+ $table3 = TestPost::select(['id', 'title', 'user_id'])->where('user_id', 101);
+
+ $columns = ['id'];
+ $cursorName = 'cursor-name';
+ $cursor = new Cursor(['id' => 1]);
+
+ $result = $table1->toBase()
+ ->union($table2->toBase())
+ ->union($table3->toBase())
+ ->orderBy('id', 'asc')
+ ->cursorPaginate(1, $columns, $cursorName, $cursor);
+
+ $this->assertSame(['id'], $result->getOptions()['parameters']);
+
+ $postB = $table2->where('id', '>', 1)->first();
+ $this->assertEquals('Post B', $postB->title, 'Expect `Post B` is the result of the second query');
+
+ $this->assertCount(1, $result->items(), 'Expect cursor paginated query should have 1 result');
+ $this->assertEquals('Post B', current($result->items())->title, 'Expect the paginated query would return `Post B`');
+ }
+
  public function testPaginationWithAliasedOrderBy()
  {
  for ($i = 1; $i <= 6; $i++) {