From e5512a448e4ff67e090a8cc2838acc713a35baf5 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 6 Jan 2025 09:50:49 +0100 Subject: [PATCH 1/9] Fix order by subqueries --- packages/support/src/Services/RelationshipJoiner.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/support/src/Services/RelationshipJoiner.php b/packages/support/src/Services/RelationshipJoiner.php index 6f6fade8b0..5828c0bbee 100644 --- a/packages/support/src/Services/RelationshipJoiner.php +++ b/packages/support/src/Services/RelationshipJoiner.php @@ -95,7 +95,7 @@ class RelationshipJoiner continue; } - if (str($order['column'])->startsWith("{$relationshipQuery->getModel()->getTable()}.")) { + if (is_string($order['column']) && str($order['column'])->startsWith("{$relationshipQuery->getModel()->getTable()}.")) { continue; } From f15369ea3f0dc03764e88d34aed1769efb01d25c Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:15:26 +0100 Subject: [PATCH 2/9] Improve RelationshipJoiner orderBy support --- .../src/Services/RelationshipJoiner.php | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/support/src/Services/RelationshipJoiner.php b/packages/support/src/Services/RelationshipJoiner.php index 5828c0bbee..cef705f15f 100644 --- a/packages/support/src/Services/RelationshipJoiner.php +++ b/packages/support/src/Services/RelationshipJoiner.php @@ -5,6 +5,7 @@ namespace Filament\Support\Services; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\Relations\Relation; +use Illuminate\Database\Query\Expression; use Illuminate\Database\Query\JoinClause; use Illuminate\Support\Arr; use Illuminate\Support\Str; @@ -91,15 +92,26 @@ class RelationshipJoiner /** @phpstan-ignore-next-line */ foreach (($relationshipQuery->getQuery()->orders ?? []) as $order) { - if (! array_key_exists('column', $order)) { + // Regular orders: { column: string, direction: 'asc' | 'desc' } + // Raw orders: { type: 'Raw', sql: string } + // Sub-query orders look like: { column: Illuminate\Database\Query\Expression, direction: 'asc' | 'desc' } + if (! array_key_exists('column', $order) && ! array_key_exists('sql', $order)) { continue; } - if (is_string($order['column']) && str($order['column'])->startsWith("{$relationshipQuery->getModel()->getTable()}.")) { + $columnValue = $order['column'] ?? new Expression($order['sql']); + + if ($columnValue instanceof Expression && str($columnValue->getValue($relationship->getGrammar()))->contains('?')) { + // Heuristic to determine if the expression contains (a) binding(s), if so, as of + // yet we cannot reliably determine (which) bindings are used in the expression. continue; } - $relationshipQuery->addSelect($order['column']); + if (str($columnValue instanceof Expression ? $columnValue->getValue($relationship->getGrammar()) : $columnValue)->startsWith("{$relationshipQuery->getModel()->getTable()}.")) { + continue; + } + + $relationshipQuery->addSelect($columnValue); } } From f63d15544ff0f8deb4cf805f6a78dceef4df7e50 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:55:43 +0100 Subject: [PATCH 3/9] Update RelationshipJoiner.php --- packages/support/src/Services/RelationshipJoiner.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/support/src/Services/RelationshipJoiner.php b/packages/support/src/Services/RelationshipJoiner.php index cef705f15f..1b38318e49 100644 --- a/packages/support/src/Services/RelationshipJoiner.php +++ b/packages/support/src/Services/RelationshipJoiner.php @@ -93,8 +93,8 @@ class RelationshipJoiner /** @phpstan-ignore-next-line */ foreach (($relationshipQuery->getQuery()->orders ?? []) as $order) { // Regular orders: { column: string, direction: 'asc' | 'desc' } - // Raw orders: { type: 'Raw', sql: string } // Sub-query orders look like: { column: Illuminate\Database\Query\Expression, direction: 'asc' | 'desc' } + // Raw orders: { type: 'Raw', sql: string } if (! array_key_exists('column', $order) && ! array_key_exists('sql', $order)) { continue; } From 5b9b970440c8ab124e8709ebae26cf95f9ded725 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:55:59 +0100 Subject: [PATCH 4/9] Update RelationshipJoiner.php --- packages/support/src/Services/RelationshipJoiner.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/support/src/Services/RelationshipJoiner.php b/packages/support/src/Services/RelationshipJoiner.php index 1b38318e49..57ea8157a9 100644 --- a/packages/support/src/Services/RelationshipJoiner.php +++ b/packages/support/src/Services/RelationshipJoiner.php @@ -93,7 +93,7 @@ class RelationshipJoiner /** @phpstan-ignore-next-line */ foreach (($relationshipQuery->getQuery()->orders ?? []) as $order) { // Regular orders: { column: string, direction: 'asc' | 'desc' } - // Sub-query orders look like: { column: Illuminate\Database\Query\Expression, direction: 'asc' | 'desc' } + // Sub-query orders: { column: Illuminate\Database\Query\Expression, direction: 'asc' | 'desc' } // Raw orders: { type: 'Raw', sql: string } if (! array_key_exists('column', $order) && ! array_key_exists('sql', $order)) { continue; From 693a0e3c09511aafe16ef006312167691da01ab8 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Mon, 6 Jan 2025 11:02:48 +0100 Subject: [PATCH 5/9] Style --- packages/support/src/Services/RelationshipJoiner.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/support/src/Services/RelationshipJoiner.php b/packages/support/src/Services/RelationshipJoiner.php index 57ea8157a9..441d53562b 100644 --- a/packages/support/src/Services/RelationshipJoiner.php +++ b/packages/support/src/Services/RelationshipJoiner.php @@ -101,13 +101,19 @@ class RelationshipJoiner $columnValue = $order['column'] ?? new Expression($order['sql']); - if ($columnValue instanceof Expression && str($columnValue->getValue($relationship->getGrammar()))->contains('?')) { + if ( + $columnValue instanceof Expression + && str($columnValue->getValue($relationship->getGrammar()))->contains('?') + ) { // Heuristic to determine if the expression contains (a) binding(s), if so, as of // yet we cannot reliably determine (which) bindings are used in the expression. continue; } - if (str($columnValue instanceof Expression ? $columnValue->getValue($relationship->getGrammar()) : $columnValue)->startsWith("{$relationshipQuery->getModel()->getTable()}.")) { + if ( + str($columnValue instanceof Expression ? $columnValue->getValue($relationship->getGrammar()) : $columnValue) + ->startsWith("{$relationshipQuery->getModel()->getTable()}.") + ) { continue; } From d13c357e5dc0dffa38712815c538a13b032e4709 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Wed, 15 Jan 2025 21:31:24 +0100 Subject: [PATCH 6/9] WIP --- .../migrations/create_team_user_table.php | 24 +++++ tests/src/Models/User.php | 6 ++ .../Services/RelationshipJoinerTest.php | 88 +++++++++++++++++++ 3 files changed, 118 insertions(+) create mode 100644 tests/database/migrations/create_team_user_table.php create mode 100644 tests/src/Support/Services/RelationshipJoinerTest.php diff --git a/tests/database/migrations/create_team_user_table.php b/tests/database/migrations/create_team_user_table.php new file mode 100644 index 0000000000..40347a989b --- /dev/null +++ b/tests/database/migrations/create_team_user_table.php @@ -0,0 +1,24 @@ +id(); + $table->foreignId('team_id')->constrained(); + $table->foreignId('user_id')->constrained(); + $table->string('role')->nullable(); + $table->timestamps(); + }); + } + + public function down(): void + { + Schema::dropIfExists('team_user'); + } +}; diff --git a/tests/src/Models/User.php b/tests/src/Models/User.php index 82e98a060b..184f85e375 100644 --- a/tests/src/Models/User.php +++ b/tests/src/Models/User.php @@ -9,6 +9,7 @@ use Filament\Tests\Database\Factories\UserFactory; use Illuminate\Contracts\Auth\MustVerifyEmail; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsToMany; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Foundation\Auth\User as Authenticatable; use Illuminate\Notifications\Notifiable; @@ -36,6 +37,11 @@ class User extends Authenticatable implements FilamentUser, HasTenants, MustVeri return $this->hasMany(Post::class, 'author_id'); } + public function teams(): BelongsToMany + { + return $this->belongsToMany(Team::class); + } + protected static function newFactory() { return UserFactory::new(); diff --git a/tests/src/Support/Services/RelationshipJoinerTest.php b/tests/src/Support/Services/RelationshipJoinerTest.php new file mode 100644 index 0000000000..88d9321b93 --- /dev/null +++ b/tests/src/Support/Services/RelationshipJoinerTest.php @@ -0,0 +1,88 @@ +create(); + + expect($user->teams()->toBase()) + ->distinct->toBeFalse() + ->getColumns()->toBe([]) + ->orders->toBeNull(); + + $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints($user->teams()); + + expect($query->toBase()) + ->distinct->toBeTrue() + ->getColumns()->toBe(['teams.*']) + ->orders->toBeNull(); + + $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $user + ->teams() + ->orderBy('id') + ->orderBy((new Team)->qualifyColumn('name')) + ->orderBy('team_user.role') + ); + + expect($query->toBase()) + ->distinct->toBeTrue() + ->getColumns()->toBe([ + (new Team)->qualifyColumn('*'), // Default select... + 'id', // Select without a qualified table also included just to be sure... + // Select for `team.name` not included as that is already included in the `team.*`... + 'team_user.role', // Select for a qualitified other table included... + ]) + ->orders->toBe([ + [ + 'column' => 'id', + 'direction' => 'asc', + ], + [ + 'column' => 'teams.name', + 'direction' => 'asc', + ], + [ + 'column' => 'team_user.role', + 'direction' => 'asc', + ], + ]); + + $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $user->teams()->orderByRaw("CASE WHEN role = 'admin' THEN 1 ELSE 2 END") + ); + + expect($query->toBase()) + ->distinct->toBeTrue() + ->getColumns()->toBe([ + (new Team)->qualifyColumn('*'), + "CASE WHEN role = 'admin' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... + ]) + ->orders->toBe([ + [ + 'type' => 'Raw', + 'sql' => "CASE WHEN role = 'admin' THEN 1 ELSE 2 END", + ], + ]); + + $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $user->teams()->orderBy(new Expression("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END")) + ); + + expect($query->toBase()) + ->distinct->toBeTrue() + ->getColumns()->toBe([ + (new Team)->qualifyColumn('*'), + "CASE WHEN role = 'user' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... + ]) + ->orders->toHaveCount(1) + ->and($query->toBase()->orders[0]) + ->column->getValue($user->teams()->getGrammar())->toBe("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END") + ->direction->toBe('asc'); +}); From 557e90519614ba3c0255b155a4b14f1af85c2d00 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Wed, 15 Jan 2025 21:32:28 +0100 Subject: [PATCH 7/9] Style --- tests/database/migrations/create_team_user_table.php | 6 +++--- .../src/Support/Services/RelationshipJoinerTest.php | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/database/migrations/create_team_user_table.php b/tests/database/migrations/create_team_user_table.php index 40347a989b..9565ec19e0 100644 --- a/tests/database/migrations/create_team_user_table.php +++ b/tests/database/migrations/create_team_user_table.php @@ -10,9 +10,9 @@ return new class extends Migration { Schema::create('team_user', function (Blueprint $table): void { $table->id(); - $table->foreignId('team_id')->constrained(); - $table->foreignId('user_id')->constrained(); - $table->string('role')->nullable(); + $table->foreignId('team_id')->constrained(); + $table->foreignId('user_id')->constrained(); + $table->string('role')->nullable(); $table->timestamps(); }); } diff --git a/tests/src/Support/Services/RelationshipJoinerTest.php b/tests/src/Support/Services/RelationshipJoinerTest.php index 88d9321b93..94c2b8c45d 100644 --- a/tests/src/Support/Services/RelationshipJoinerTest.php +++ b/tests/src/Support/Services/RelationshipJoinerTest.php @@ -8,7 +8,7 @@ use Illuminate\Database\Query\Expression; uses(TestCase::class); -it('can prepare query for no constraints', function () { +it('can prepare query for no constraints for a BelongsToMany relationship', function () { $user = User::factory()->create(); expect($user->teams()->toBase()) @@ -72,7 +72,7 @@ it('can prepare query for no constraints', function () { ]); $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( - $user->teams()->orderBy(new Expression("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END")) + $user->teams()->orderBy(new Expression("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END")) ); expect($query->toBase()) @@ -81,8 +81,8 @@ it('can prepare query for no constraints', function () { (new Team)->qualifyColumn('*'), "CASE WHEN role = 'user' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... ]) - ->orders->toHaveCount(1) - ->and($query->toBase()->orders[0]) - ->column->getValue($user->teams()->getGrammar())->toBe("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END") - ->direction->toBe('asc'); + ->orders->toHaveCount(1) + ->and($query->toBase()->orders[0]) + ->column->getValue($user->teams()->getGrammar())->toBe("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END") + ->direction->toBe('asc'); }); From b8bba48cca3e2859a8ac3728204ddd11f39d32a3 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Wed, 15 Jan 2025 21:33:20 +0100 Subject: [PATCH 8/9] Update name --- .../Services/RelationshipJoinerTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/src/Support/Services/RelationshipJoinerTest.php b/tests/src/Support/Services/RelationshipJoinerTest.php index 94c2b8c45d..a7b1455b8a 100644 --- a/tests/src/Support/Services/RelationshipJoinerTest.php +++ b/tests/src/Support/Services/RelationshipJoinerTest.php @@ -16,14 +16,14 @@ it('can prepare query for no constraints for a BelongsToMany relationship', func ->getColumns()->toBe([]) ->orders->toBeNull(); - $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints($user->teams()); + $preparedQuery = app(RelationshipJoiner::class)->prepareQueryForNoConstraints($user->teams()); - expect($query->toBase()) + expect($preparedQuery->toBase()) ->distinct->toBeTrue() ->getColumns()->toBe(['teams.*']) ->orders->toBeNull(); - $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $preparedQuery = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( $user ->teams() ->orderBy('id') @@ -31,7 +31,7 @@ it('can prepare query for no constraints for a BelongsToMany relationship', func ->orderBy('team_user.role') ); - expect($query->toBase()) + expect($preparedQuery->toBase()) ->distinct->toBeTrue() ->getColumns()->toBe([ (new Team)->qualifyColumn('*'), // Default select... @@ -54,11 +54,11 @@ it('can prepare query for no constraints for a BelongsToMany relationship', func ], ]); - $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $preparedQuery = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( $user->teams()->orderByRaw("CASE WHEN role = 'admin' THEN 1 ELSE 2 END") ); - expect($query->toBase()) + expect($preparedQuery->toBase()) ->distinct->toBeTrue() ->getColumns()->toBe([ (new Team)->qualifyColumn('*'), @@ -71,18 +71,18 @@ it('can prepare query for no constraints for a BelongsToMany relationship', func ], ]); - $query = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( + $preparedQuery = app(RelationshipJoiner::class)->prepareQueryForNoConstraints( $user->teams()->orderBy(new Expression("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END")) ); - expect($query->toBase()) + expect($preparedQuery->toBase()) ->distinct->toBeTrue() ->getColumns()->toBe([ (new Team)->qualifyColumn('*'), "CASE WHEN role = 'user' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... ]) ->orders->toHaveCount(1) - ->and($query->toBase()->orders[0]) + ->and($preparedQuery->toBase()->orders[0]) ->column->getValue($user->teams()->getGrammar())->toBe("CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END") ->direction->toBe('asc'); }); From c7155f1429e3af6f752614db892dd1e574d459c7 Mon Sep 17 00:00:00 2001 From: "Ralph J. Smit" <59207045+ralphjsmit@users.noreply.github.com> Date: Thu, 16 Jan 2025 09:29:25 +0100 Subject: [PATCH 9/9] Fix failing test --- tests/src/Support/Services/RelationshipJoinerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Support/Services/RelationshipJoinerTest.php b/tests/src/Support/Services/RelationshipJoinerTest.php index a7b1455b8a..bae862a727 100644 --- a/tests/src/Support/Services/RelationshipJoinerTest.php +++ b/tests/src/Support/Services/RelationshipJoinerTest.php @@ -79,7 +79,7 @@ it('can prepare query for no constraints for a BelongsToMany relationship', func ->distinct->toBeTrue() ->getColumns()->toBe([ (new Team)->qualifyColumn('*'), - "CASE WHEN role = 'user' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... + "CASE WHEN role = 'some_other_role' THEN 1 ELSE 2 END", // Select added from `orderByRaw`... ]) ->orders->toHaveCount(1) ->and($preparedQuery->toBase()->orders[0])