From 67d4d0dd8061227ef21a3ed7a0ad57c18d51861b Mon Sep 17 00:00:00 2001 From: Thijs van den Anker Date: Tue, 16 Apr 2024 20:51:25 +0200 Subject: [PATCH] [10.x] Binding order is incorrect when using cursor paginate with multiple 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 * Add docblock * Use FQN in the docblock Co-authored-by: Mior Muhammad Zaki * Use FQN in the docblock Co-authored-by: Mior Muhammad Zaki * formatting * formatting --------- Signed-off-by: Mior Muhammad Zaki Co-authored-by: Mior Muhammad Zaki Co-authored-by: Taylor Otwell --- .../Database/Concerns/BuildsQueries.php | 22 +++-- src/Illuminate/Database/Eloquent/Builder.php | 13 +++ src/Illuminate/Database/Query/Builder.php | 12 +++ tests/Database/DatabaseQueryBuilderTest.php | 99 +++++++++++++++++++ .../Database/EloquentCursorPaginateTest.php | 28 ++++++ 5 files changed, 166 insertions(+), 8 deletions(-) diff --git a/src/Illuminate/Database/Concerns/BuildsQueries.php b/src/Illuminate/Database/Concerns/BuildsQueries.php index 0d45b0a6f6fc..b2f973afb18e 100644 --- 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,25 +405,27 @@ 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' ? '>' : '<', @@ -428,11 +433,12 @@ protected function paginateUsingCursor($perPage, $columns = ['*'], $cursorName = ); 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 index 185781580684..8ed9229e0894 100755 --- 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 index e47a8942993a..210d8cbb25a1 100755 --- 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 index 1743b8966d05..90125356bdd8 100755 --- 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 index 01fd4b49e8b8..02034c485632 100644 --- 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++) {