From b81ebf76a87b4a1c4c4bbe67ec35bb2fbf250dd0 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Sun, 16 Sep 2018 11:46:32 +0200 Subject: [PATCH] Revert #320 and use fetch() instead of load() again (#323) **Related Issue(s)** Adds test for #319 Reverts misguided WIP #320 https://github.com/laravel/framework/issues/16217 **PR Type** Test/Fix **Changes** - Add test to ensure the global scope is called when using the @hasmany directive - Add TODO for removing custom fetch logic once Laravel resolves their issue with eager loading **Breaking changes** Nope --- src/Providers/LighthouseServiceProvider.php | 2 ++ .../DataLoader/Loaders/BelongsToLoader.php | 25 +++++++------ .../DataLoader/Loaders/HasManyLoader.php | 26 ++++++++------ .../DataLoader/Loaders/HasOneLoader.php | 36 +++++++++---------- .../Fields/HasManyDirectiveTest.php | 17 ++++++--- .../Support/DataLoader/QueryBuilderTest.php | 1 + tests/Utils/Models/Task.php | 11 ++++++ 7 files changed, 74 insertions(+), 44 deletions(-) diff --git a/src/Providers/LighthouseServiceProvider.php b/src/Providers/LighthouseServiceProvider.php index 4ec499e0dc..7abed80fcd 100644 --- a/src/Providers/LighthouseServiceProvider.php +++ b/src/Providers/LighthouseServiceProvider.php @@ -96,6 +96,8 @@ function () { */ protected function registerCollectionMacros() { + // TODO remove and just use load() as soon as Laravel fixes https://github.com/laravel/framework/issues/16217 + // This fixes the behaviour of how eager loading queries are built Collection::macro('fetch', function ($eagerLoadRelations = null) { if (count($this->items) > 0) { if (is_string($eagerLoadRelations)) { diff --git a/src/Support/DataLoader/Loaders/BelongsToLoader.php b/src/Support/DataLoader/Loaders/BelongsToLoader.php index 95075f4f9c..ae9247f45a 100644 --- a/src/Support/DataLoader/Loaders/BelongsToLoader.php +++ b/src/Support/DataLoader/Loaders/BelongsToLoader.php @@ -28,16 +28,19 @@ public function __construct(string $relation, array $resolveArgs) */ public function resolve(): array { - $parents = \Illuminate\Database\Eloquent\Collection::make( - collect($this->keys)->pluck('parent') - ); - - return $parents->load([$this->relation => function ($query) { - $query->when(isset($args['query.filter']), function ($query) { - return QueryFilter::build($query, $this->resolveArgs); - }); - }])->mapWithKeys(function (Model $model) { - return [$model->getKey() => $model->getRelation($this->relation)]; - })->all(); + return collect($this->keys) + ->pluck('parent') + // Using our own Collection macro + ->fetch([$this->relation => + function ($query) { + $query->when(isset($args['query.filter']), function ($query) { + return QueryFilter::build($query, $this->resolveArgs); + }); + } + ]) + ->mapWithKeys(function (Model $model) { + return [$model->getKey() => $model->getRelation($this->relation)]; + }) + ->all(); } } diff --git a/src/Support/DataLoader/Loaders/HasManyLoader.php b/src/Support/DataLoader/Loaders/HasManyLoader.php index f9cd79eae1..6c8ac35057 100644 --- a/src/Support/DataLoader/Loaders/HasManyLoader.php +++ b/src/Support/DataLoader/Loaders/HasManyLoader.php @@ -48,18 +48,22 @@ public function __construct(string $relation, array $resolveArgs, array $scopes, */ public function resolve(): array { - $eagerLoadRelationWithConstraints = [$this->relation => function ($query) { - foreach ($this->scopes as $scope) { - call_user_func_array([$query, $scope], [$this->resolveArgs]); - } + $eagerLoadRelationWithConstraints = [$this->relation => + function ($query) { + foreach ($this->scopes as $scope) { + call_user_func_array([$query, $scope], [$this->resolveArgs]); + } - $query->when(isset($args['query.filter']), function ($q) { - return QueryFilter::build($q, $this->resolveArgs); - }); - }]; + $query->when(isset($args['query.filter']), function ($q) { + return QueryFilter::build($q, $this->resolveArgs); + }); + } + ]; /** @var Collection $parents */ - $parents = collect($this->keys)->pluck('parent'); + $parents = collect($this->keys) + ->pluck('parent'); + switch ($this->paginationType) { case PaginationManipulator::PAGINATION_TYPE_CONNECTION: case PaginationManipulator::PAGINATION_ALIAS_RELAY: @@ -78,8 +82,8 @@ public function resolve(): array $parents->fetchForPage($count, $page, $eagerLoadRelationWithConstraints); break; default: - $parents = new \Illuminate\Database\Eloquent\Collection($parents); - $parents->load($eagerLoadRelationWithConstraints); + // Using our own Collection macro + $parents->fetch($eagerLoadRelationWithConstraints); break; } diff --git a/src/Support/DataLoader/Loaders/HasOneLoader.php b/src/Support/DataLoader/Loaders/HasOneLoader.php index f4b873bf46..02377befc9 100644 --- a/src/Support/DataLoader/Loaders/HasOneLoader.php +++ b/src/Support/DataLoader/Loaders/HasOneLoader.php @@ -39,23 +39,23 @@ public function __construct(string $relation, array $resolveArgs, array $scopes) */ public function resolve(): array { - $eagerLoadRelationWithConstraints = [$this->relation => function ($query) { - foreach ($this->scopes as $scope) { - call_user_func_array([$query, $scope], [$this->resolveArgs]); - } - - $query->when(isset($args['query.filter']), function ($q) { - return QueryFilter::build($q, $this->resolveArgs); - }); - }]; - - /** @var Collection $parents */ - $parents = collect($this->keys)->pluck('parent'); - $parents = new \Illuminate\Database\Eloquent\Collection($parents); - $parents->load($eagerLoadRelationWithConstraints); - - return $parents->mapWithKeys(function (Model $model) { - return [$model->getKey() => $model->getRelation($this->relation)]; - })->all(); + return collect($this->keys) + ->pluck('parent') + // Using our own Collection macro + ->fetch([$this->relation => + function ($query) { + foreach ($this->scopes as $scope) { + call_user_func_array([$query, $scope], [$this->resolveArgs]); + } + + $query->when(isset($args['query.filter']), function ($q) { + return QueryFilter::build($q, $this->resolveArgs); + }); + } + ]) + ->mapWithKeys(function (Model $model) { + return [$model->getKey() => $model->getRelation($this->relation)]; + }) + ->all(); } } diff --git a/tests/Integration/Schema/Directives/Fields/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/Fields/HasManyDirectiveTest.php index dc677e9893..9a86a87ecb 100644 --- a/tests/Integration/Schema/Directives/Fields/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/Fields/HasManyDirectiveTest.php @@ -38,6 +38,11 @@ protected function setUp() $this->tasks = factory(Task::class, 3)->create([ 'user_id' => $this->user->getKey(), ]); + factory(Task::class)->create([ + 'user_id' => $this->user->getKey(), + // This task should be ignored via global scope on the Task model + 'name' => 'cleaning' + ]); $this->be($this->user); } @@ -72,6 +77,10 @@ public function itCanQueryHasManyRelationship() } '); + $tasksWithoutGlobalScope = auth()->user()->tasks()->withoutGlobalScope('no_cleaning')->count(); + $this->assertSame(4, $tasksWithoutGlobalScope); + + // Ensure global scopes are respected here $this->assertCount(3, array_get($result->data, 'user.tasks')); } @@ -82,8 +91,8 @@ public function itCanQueryHasManyPaginator() { $schema = ' type User { - tasks: [Task!]! @hasMany(type:"paginator") - posts: [Post!]! @hasMany(type:"paginator") + tasks: [Task!]! @hasMany(type: "paginator") + posts: [Post!]! @hasMany(type: "paginator") } type Task { @@ -129,7 +138,7 @@ public function itCanQueryHasManyRelayConnection() { $schema = ' type User { - tasks: [Task!]! @hasMany(type:"relay") + tasks: [Task!]! @hasMany(type: "relay") } type Task { @@ -169,7 +178,7 @@ public function itCanQueryHasManyNestedRelationships() { $schema = ' type User { - tasks: [Task!]! @hasMany(type:"relay") + tasks: [Task!]! @hasMany(type: "relay") } type Task { diff --git a/tests/Integration/Support/DataLoader/QueryBuilderTest.php b/tests/Integration/Support/DataLoader/QueryBuilderTest.php index 63e89610e4..a967859ed0 100644 --- a/tests/Integration/Support/DataLoader/QueryBuilderTest.php +++ b/tests/Integration/Support/DataLoader/QueryBuilderTest.php @@ -34,6 +34,7 @@ public function setUp() public function itCanLoadRelationshipsWithLimitsOnCollection() { $users = User::all(); + // TODO remove this as soon as Laravel fixes https://github.com/laravel/framework/issues/16217 $users->fetch(['tasks' => function ($query) { $query->take(3); }]); diff --git a/tests/Utils/Models/Task.php b/tests/Utils/Models/Task.php index 2a711ba1d6..bad94317d1 100644 --- a/tests/Utils/Models/Task.php +++ b/tests/Utils/Models/Task.php @@ -3,6 +3,7 @@ namespace Tests\Utils\Models; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Database\Eloquent\Relations\HasOne; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -14,6 +15,16 @@ class Task extends Model protected $guarded = []; + protected static function boot() + { + parent::boot(); + + // This is used to test that this scope works in all kinds of queries + static::addGlobalScope('no_cleaning', function (Builder $builder) { + $builder->where('name', '!=', 'cleaning'); + }); + } + public function user(): BelongsTo { return $this->belongsTo(User::class);