From 65206d24e02f0acc27ae167311e87ed8cc0e5c5b Mon Sep 17 00:00:00 2001 From: Louis Charette Date: Sat, 24 Feb 2024 17:16:55 -0500 Subject: [PATCH] Fix bug in sprunje when using BelongsToManyThrough --- app/src/Sprunje/Sprunje.php | 69 ++--- .../SprunjeBelongsToManyThroughTest.php | 283 ++++++++++++++++++ 2 files changed, 316 insertions(+), 36 deletions(-) create mode 100644 app/tests/Integration/Sprunje/SprunjeBelongsToManyThroughTest.php diff --git a/app/src/Sprunje/Sprunje.php b/app/src/Sprunje/Sprunje.php index 37526fb9..716eda26 100644 --- a/app/src/Sprunje/Sprunje.php +++ b/app/src/Sprunje/Sprunje.php @@ -12,10 +12,9 @@ namespace UserFrosting\Sprinkle\Core\Sprunje; -use Illuminate\Database\Eloquent\Builder as EloquentBuilder; +use Illuminate\Contracts\Database\Eloquent\Builder as EloquentBuilderContract; +use Illuminate\Contracts\Database\Query\Builder as QueryBuilderContract; use Illuminate\Database\Eloquent\Model; -use Illuminate\Database\Eloquent\Relations\Relation; -use Illuminate\Database\Query\Builder as QueryBuilder; use Illuminate\Support\Arr; use Illuminate\Support\Collection; use Illuminate\Support\Str; @@ -37,9 +36,9 @@ abstract class Sprunje protected string $name = 'export'; /** - * @var EloquentBuilder|QueryBuilder The base (unfiltered) query. + * @var EloquentBuilderContract|QueryBuilderContract The base (unfiltered) query. */ - protected EloquentBuilder|QueryBuilder $query; + protected EloquentBuilderContract|QueryBuilderContract $query; /** * @var array{ @@ -130,12 +129,10 @@ public function __construct(array $options = []) // Start a new query on any Model instances $query = $this->baseQuery(); if (is_a($query, Model::class)) { - $this->query = $query->newQuery(); - } elseif (is_a($query, Relation::class)) { - $this->query = $query->getQuery(); - } else { - $this->query = $query; + $query = $query->newQuery(); } + + $this->query = $query; } /** @@ -376,9 +373,9 @@ public function getListable(): array /** * Get the underlying queryable object in its current state. * - * @return EloquentBuilder|QueryBuilder + * @return EloquentBuilderContract|QueryBuilderContract */ - public function getQuery(): EloquentBuilder|QueryBuilder + public function getQuery(): EloquentBuilderContract|QueryBuilderContract { return $this->query; } @@ -386,11 +383,11 @@ public function getQuery(): EloquentBuilder|QueryBuilder /** * Set the underlying QueryBuilder object. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return static */ - public function setQuery(EloquentBuilder|QueryBuilder $query): static + public function setQuery(EloquentBuilderContract|QueryBuilderContract $query): static { $this->query = $query; @@ -400,11 +397,11 @@ public function setQuery(EloquentBuilder|QueryBuilder $query): static /** * Apply any filters from the options, calling a custom filter callback when appropriate. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return static */ - public function applyFilters(EloquentBuilder|QueryBuilder $query): static + public function applyFilters(EloquentBuilderContract|QueryBuilderContract $query): static { foreach ($this->options['filters'] as $name => $value) { // Check that this filter is allowed @@ -427,11 +424,11 @@ public function applyFilters(EloquentBuilder|QueryBuilder $query): static /** * Apply any sorts from the options, calling a custom sorter callback when appropriate. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return static */ - public function applySorts(EloquentBuilder|QueryBuilder $query): static + public function applySorts(EloquentBuilderContract|QueryBuilderContract $query): static { foreach ($this->options['sorts'] as $name => $direction) { // Check that this sort is allowed @@ -460,11 +457,11 @@ public function applySorts(EloquentBuilder|QueryBuilder $query): static /** * Apply pagination based on the `page` and `size` options. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return static */ - public function applyPagination(EloquentBuilder|QueryBuilder $query): static + public function applyPagination(EloquentBuilderContract|QueryBuilderContract $query): static { $page = $this->options['page']; $size = $this->options['size']; @@ -550,12 +547,12 @@ public function setCsvChunk(int $csvChunk): static /** * Match any filter in `filterable`. * - * @param EloquentBuilder|QueryBuilder $query - * @param mixed $value + * @param EloquentBuilderContract|QueryBuilderContract $query + * @param mixed $value * * @return static */ - protected function filterAll(EloquentBuilder|QueryBuilder $query, mixed $value): static + protected function filterAll(EloquentBuilderContract|QueryBuilderContract $query, mixed $value): static { foreach ($this->filterable as $name) { if (Str::studly($name) != 'all' && !in_array($name, $this->excludeForAll, true)) { @@ -572,13 +569,13 @@ protected function filterAll(EloquentBuilder|QueryBuilder $query, mixed $value): /** * Build the filter query for a single field. * - * @param EloquentBuilder|QueryBuilder $query - * @param string $name - * @param mixed $value + * @param EloquentBuilderContract|QueryBuilderContract $query + * @param string $name + * @param mixed $value * * @return static */ - protected function buildFilterQuery(EloquentBuilder|QueryBuilder $query, string $name, mixed $value): static + protected function buildFilterQuery(EloquentBuilderContract|QueryBuilderContract $query, string $name, mixed $value): static { $methodName = 'filter' . Str::studly($name); @@ -597,13 +594,13 @@ protected function buildFilterQuery(EloquentBuilder|QueryBuilder $query, string * Perform a 'like' query on a single field, separating the value string on the or separator and * matching any of the supplied values. * - * @param EloquentBuilder|QueryBuilder $query - * @param string $name - * @param mixed $value + * @param EloquentBuilderContract|QueryBuilderContract $query + * @param string $name + * @param mixed $value * * @return static */ - protected function buildFilterDefaultFieldQuery(EloquentBuilder|QueryBuilder $query, string $name, mixed $value): static + protected function buildFilterDefaultFieldQuery(EloquentBuilderContract|QueryBuilderContract $query, string $name, mixed $value): static { if (is_bool($value)) { // Bool doesn't behave correctly when cast to string (false is empty instead of 0). @@ -636,7 +633,7 @@ protected function applyTransformations(Collection $collection): Collection /** * Set the initial query used by your Sprunje. * - * @return EloquentBuilder|QueryBuilder|Model|Relation + * @return EloquentBuilderContract|QueryBuilderContract|Model */ abstract protected function baseQuery(); @@ -667,11 +664,11 @@ protected function getColumnValues(string $column): array /** * Get the unpaginated count of items (before filtering) in this query. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return int */ - protected function count(EloquentBuilder|QueryBuilder $query): int + protected function count(EloquentBuilderContract|QueryBuilderContract $query): int { return $query->count(); } @@ -679,11 +676,11 @@ protected function count(EloquentBuilder|QueryBuilder $query): int /** * Get the unpaginated count of items (after filtering) in this query. * - * @param EloquentBuilder|QueryBuilder $query + * @param EloquentBuilderContract|QueryBuilderContract $query * * @return int */ - protected function countFiltered(EloquentBuilder|QueryBuilder $query): int + protected function countFiltered(EloquentBuilderContract|QueryBuilderContract $query): int { return $query->count(); } diff --git a/app/tests/Integration/Sprunje/SprunjeBelongsToManyThroughTest.php b/app/tests/Integration/Sprunje/SprunjeBelongsToManyThroughTest.php new file mode 100644 index 00000000..d5c5a78c --- /dev/null +++ b/app/tests/Integration/Sprunje/SprunjeBelongsToManyThroughTest.php @@ -0,0 +1,283 @@ +ci->get(Builder::class); + (new TestTableUsersMigration($builder))->up(); + (new TestRolesTableMigration($builder))->up(); + (new TestPermissionsTableMigration($builder))->up(); + (new TestRoleUsersTableMigration($builder))->up(); + (new TestRolePermissionsTableMigration($builder))->up(); + + // Insert some data + $this->createData(); + } + + public function tearDown(): void + { + // Run custom migration down + /** @var Builder */ + $builder = $this->ci->get(Builder::class); + (new TestTableUsersMigration($builder))->down(); + (new TestRolesTableMigration($builder))->down(); + (new TestPermissionsTableMigration($builder))->down(); + (new TestRoleUsersTableMigration($builder))->down(); + (new TestRolePermissionsTableMigration($builder))->down(); + + parent::tearDown(); + } + + protected function createData(): void + { + (new TestSprunjeUserModel([ + 'id' => 1, + 'name' => 'foo', + ]))->save(); + + (new TestSprunjeRoleModel([ + 'id' => 1, + 'name' => 'Role of Foo', + ]))->save(); + + (new TestSprunjePermissionModel([ + 'id' => 1, + 'name' => 'Permission for Role of Foo', + ]))->save(); + + (new TestSprunjeRoleUserModel([ + 'user_id' => 1, + 'role_id' => 1, + ]))->save(); + + (new TestSprunjeRolePermissionsModel([ + 'permission_id' => 1, + 'role_id' => 1, + ]))->save(); + } + + public function testBaseSprunje(): void + { + /** @var TestSprunjeUserModel */ + $user = TestSprunjeUserModel::find(1); + $sprunje = new TestBelongsToManyThroughSprunje($user); + $data = $sprunje->getArray(); + + $this->assertEquals([ + 'count' => 1, + 'count_filtered' => 1, + 'rows' => [ + [ + 'id' => 1, + 'name' => 'Permission for Role of Foo', + 'roles_via' => [ + [ + 'id' => 1, + 'name' => 'Role of Foo', + 'pivot' => [ + 'permission_id' => 1, + 'role_id' => 1, + ], + ], + ], + 'pivot' => [ + 'user_id' => 1, + 'permission_id' => 1, + ] + ], + ], + 'listable' => [], + ], $data); + } +} + +/** + * Custom sprunje for testing. Will return a list of permissions the user has. + */ +class TestBelongsToManyThroughSprunje extends Sprunje +{ + public function __construct(protected TestSprunjeUserModel $user) + { + parent::__construct(); + } + + protected function baseQuery() + { + return $this->user->permissions()->withVia('roles_via'); + } +} + +/** + * Custom models for testing, and adding test data to database. + */ +class TestSprunjeUserModel extends UfModel +{ + protected $table = 'test_sprunje_users'; + protected $fillable = ['name']; + + /** @var bool */ + public $timestamps = false; + + public function permissions(): BelongsToManyThrough + { + return $this->belongsToManyThrough( + TestSprunjePermissionModel::class, + TestSprunjeRoleModel::class, + firstJoiningTable: 'test_sprunje_role_users', + firstForeignPivotKey: 'user_id', + firstRelatedKey: 'role_id', + secondJoiningTable: 'test_sprunje_role_permissions', + secondForeignPivotKey: 'role_id', + secondRelatedKey: 'permission_id', + ); + } +} + +class TestSprunjeRoleModel extends UfModel +{ + protected $table = 'test_sprunje_roles'; + protected $fillable = ['name']; + + /** @var bool */ + public $timestamps = false; +} + +class TestSprunjePermissionModel extends UfModel +{ + protected $table = 'test_sprunje_permissions'; + protected $fillable = ['name']; + + /** @var bool */ + public $timestamps = false; +} + +class TestSprunjeRoleUserModel extends UfModel +{ + protected $table = 'test_sprunje_role_users'; + protected $fillable = ['user_id', 'role_id']; + + /** @var bool */ + public $timestamps = false; +} + +class TestSprunjeRolePermissionsModel extends UfModel +{ + protected $table = 'test_sprunje_role_permissions'; + protected $fillable = ['permission_id', 'role_id']; + + /** @var bool */ + public $timestamps = false; +} + +/** + * Custom migration for testing. + */ +class TestTableUsersMigration extends Migration +{ + public function up(): void + { + $this->schema->create('test_sprunje_users', function (Blueprint $table) { + $table->increments('id'); + $table->string('name'); + }); + } + + public function down(): void + { + $this->schema->drop('test_sprunje_users'); + } +} + +class TestRolesTableMigration extends Migration +{ + public function up(): void + { + $this->schema->create('test_sprunje_roles', function (Blueprint $table) { + $table->increments('id'); + $table->string('name'); + }); + } + + public function down(): void + { + $this->schema->drop('test_sprunje_roles'); + } +} + +class TestPermissionsTableMigration extends Migration +{ + public function up(): void + { + $this->schema->create('test_sprunje_permissions', function (Blueprint $table) { + $table->increments('id'); + $table->string('name'); + }); + } + + public function down(): void + { + $this->schema->drop('test_sprunje_permissions'); + } +} + +class TestRoleUsersTableMigration extends Migration +{ + public function up(): void + { + $this->schema->create('test_sprunje_role_users', function (Blueprint $table) { + $table->integer('user_id')->unsigned(); + $table->integer('role_id')->unsigned(); + }); + } + + public function down(): void + { + $this->schema->drop('test_sprunje_role_users'); + } +} + +class TestRolePermissionsTableMigration extends Migration +{ + public function up(): void + { + $this->schema->create('test_sprunje_role_permissions', function (Blueprint $table) { + $table->integer('permission_id')->unsigned(); + $table->integer('role_id')->unsigned(); + }); + } + + public function down(): void + { + $this->schema->drop('test_sprunje_role_permissions'); + } +}