From 2f13537ad867b161dd0fcda5f798cdecec9a658f Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 11 Nov 2022 14:34:23 +0800 Subject: [PATCH 01/45] optimize select --- src/Pagination/PaginateDirective.php | 20 ++++ src/Schema/Directives/AllDirective.php | 24 ++++- src/Select/SelectHelper.php | 132 +++++++++++++++++++++++++ 3 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 src/Select/SelectHelper.php diff --git a/src/Pagination/PaginateDirective.php b/src/Pagination/PaginateDirective.php index efcb3470a1..6682be103c 100644 --- a/src/Pagination/PaginateDirective.php +++ b/src/Pagination/PaginateDirective.php @@ -9,10 +9,12 @@ use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Database\Query\Builder as QueryBuilder; +use Illuminate\Support\Arr; use Laravel\Scout\Builder as ScoutBuilder; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Schema\Values\FieldValue; +use Nuwave\Lighthouse\Select\SelectHelper; use Nuwave\Lighthouse\Support\Contracts\FieldManipulator; use Nuwave\Lighthouse\Support\Contracts\FieldResolver; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; @@ -129,6 +131,24 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->directiveArgValue('scopes', []) ); + if (($query instanceof QueryBuilder || $query instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { + $fieldSelection = $resolveInfo->getFieldSelection(4); + + if (Arr::has($fieldSelection, 'data') || Arr::has($fieldSelection, 'edges')) { + $fieldSelection = array_keys(Arr::has($fieldSelection, 'data') ? $fieldSelection['data'] : $fieldSelection['edges']['node']); + + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); + + if (! empty($selectColumns)) { + $query = $query->select($selectColumns); + } + } + } + $paginationArgs = PaginationArgs::extractArgs($args, $this->paginationType(), $this->paginateMaxCount()); $paginationArgs->type = $this->optimalPaginationType($resolveInfo); diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index 4fe3b1a005..a9bba4bf78 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -9,6 +9,7 @@ use Illuminate\Support\Collection; use Laravel\Scout\Builder as ScoutBuilder; use Nuwave\Lighthouse\Schema\Values\FieldValue; +use Nuwave\Lighthouse\Select\SelectHelper; use Nuwave\Lighthouse\Support\Contracts\FieldResolver; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; @@ -56,13 +57,30 @@ public function resolveField(FieldValue $fieldValue): FieldValue $query = $this->getModelClass()::query(); } - return $resolveInfo + $builder = $resolveInfo ->argumentSet ->enhanceBuilder( $query, $this->directiveArgValue('scopes', []) - ) - ->get(); + ); + + if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { + $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); + + if (empty($selectColumns)) { + return $builder->get(); + } + + $builder = $builder->select($selectColumns); + } + + return $builder->get(); }); return $fieldValue; diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php new file mode 100644 index 0000000000..d7a7dd37ac --- /dev/null +++ b/src/Select/SelectHelper.php @@ -0,0 +1,132 @@ +documentAST(); + + if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'], true)) { + $returnTypeName = Str::replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); + } + + // ignore relation closure, e.g. RelationOrderByClause + foreach (array_keys($documentAST->types) as $type) { + if (Str::contains($type, ['RelationOrderByClause'], true)) { + return []; + } + } + + $type = $documentAST->types[$returnTypeName]; + + if ($type instanceof UnionTypeDefinitionNode) { + $type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])]; + } + + /** @var iterable<\GraphQL\Language\AST\FieldDefinitionNode> $fieldDefinitions */ + $fieldDefinitions = $type->fields; + + $model = new $modelName(); + + $selectColumns = []; + + foreach ($fieldSelection as $field) { + $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); + + if ($fieldDefinition) { + $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey); + + foreach ($directivesRequiringKeys as $directiveType) { + if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { + $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); + + if (in_array($directiveType, self::DirectivesRequiringLocalKey)) { + $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); + + if (method_exists($model, $relationName)) { + array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); + } + } + + if (in_array($directiveType, self::DirectivesRequiringForeignKey)) { + $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); + + if (method_exists($model, $relationName)) { + if ($directiveType === 'belongsToMany') { + array_push($selectColumns, $model->{$relationName}()->getForeignPivotKeyName()); + } else { + array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); + } + } + } + + continue 2; + } + } + + if (ASTHelper::hasDirective($fieldDefinition, 'select')) { + // append selected columns in select directive to seletion + $directive = ASTHelper::directiveDefinition($fieldDefinition, 'select'); + + if ($directive) { + $selectFields = ASTHelper::directiveArgValue($directive, 'columns') ?? []; + $selectColumns = array_merge($selectColumns, $selectFields); + } + } elseif (ASTHelper::hasDirective($fieldDefinition, 'rename')) { + // append renamed attribute to selection + $directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename'); + + if ($directive) { + $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); + array_push($selectColumns, $renamedAttribute); + } + } else { + // fallback to selecting the field name + array_push($selectColumns, $field); + } + } + } + + // for unit test query log check + + try { + $allColumns = Schema::getColumnListing($model->getTable()); + } catch (\Exception $e) { + // connection refuse + $allColumns = []; + } + + DB::enableQueryLog(); + + return !empty($allColumns) + ? array_intersect($allColumns, array_unique($selectColumns)) + : array_unique($selectColumns); + } +} From 290e464ccd205381eb9bcd6abdb501cb33de77dd Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 11 Nov 2022 15:35:01 +0800 Subject: [PATCH 02/45] add reference --- src/Select/SelectHelper.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index d7a7dd37ac..bd50832f95 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -23,6 +23,7 @@ class SelectHelper * @param mixed[] $fieldSelection * * @return string[] + * @reference https://github.com/nuwave/lighthouse/pull/1626 */ public static function getSelectColumns(Node $definitionNode, array $fieldSelection, string $modelName): array { From 03d41244d95100478358739ed3f855184bb3fe11 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 11 Nov 2022 18:42:31 +0800 Subject: [PATCH 03/45] optimize find directive and fix tests --- src/Schema/Directives/FindDirective.php | 24 +++- src/Select/SelectHelper.php | 9 +- tests/AssertsQueryCounts.php | 8 +- .../Directives/CountDirectiveDBTest.php | 29 ++-- .../Directives/HasManyDirectiveTest.php | 82 ++++------- .../Schema/Directives/LimitDirectiveTest.php | 39 ++---- .../WhereConditionsDirectiveTest.php | 128 ++++++++---------- 7 files changed, 151 insertions(+), 168 deletions(-) diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 7ccdda45f3..26fad00ada 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -4,8 +4,11 @@ use GraphQL\Error\Error; use GraphQL\Type\Definition\ResolveInfo; +use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Query\Builder as QueryBuilder; use Nuwave\Lighthouse\Schema\Values\FieldValue; +use Nuwave\Lighthouse\Select\SelectHelper; use Nuwave\Lighthouse\Support\Contracts\FieldResolver; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; @@ -35,13 +38,28 @@ public static function definition(): string public function resolveField(FieldValue $fieldValue): FieldValue { $fieldValue->setResolver(function ($root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo): ?Model { - $results = $resolveInfo + $builder = $resolveInfo ->argumentSet ->enhanceBuilder( $this->getModelClass()::query(), $this->directiveArgValue('scopes', []) - ) - ->get(); + ); + + if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('model')) { + $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); + + if (!empty($selectColumns)) { + $builder = $builder->select($selectColumns); + } + } + + $results = $builder->get(); if ($results->count() > 1) { throw new Error('The query returned more than one result.'); diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index bd50832f95..f38f02df05 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -16,6 +16,8 @@ class SelectHelper public const DirectivesRequiringForeignKey = ['belongsTo', 'belongsToMany', 'morphTo']; + public const DirectivesIgnore = ['morphTo']; + /** * Given a field definition node, resolve info, and a model name, return the SQL columns that should be selected. * Accounts for relationships and the rename and select directives. @@ -62,7 +64,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); if ($fieldDefinition) { - $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey); + $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey, self::DirectivesIgnore); foreach ($directivesRequiringKeys as $directiveType) { if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { @@ -88,6 +90,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } + if (in_array($directiveType, self::DirectivesIgnore)) { + return []; + } + continue 2; } } @@ -116,7 +122,6 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } // for unit test query log check - try { $allColumns = Schema::getColumnListing($model->getTable()); } catch (\Exception $e) { diff --git a/tests/AssertsQueryCounts.php b/tests/AssertsQueryCounts.php index 8a44b5abee..12a7a3cb80 100644 --- a/tests/AssertsQueryCounts.php +++ b/tests/AssertsQueryCounts.php @@ -3,6 +3,7 @@ namespace Tests; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Str; /** * This trait was taken from a package that supports fewer Laravel versions than us. @@ -15,7 +16,12 @@ trait AssertsQueryCounts { protected function countQueries(?int &$count): void { - DB::listen(function () use (&$count): void { + DB::listen(function ($query) use (&$count): void { + // ignore fetch column list. + if (Str::contains($query->sql, ['select column_name'])) { + return; + } + ++$count; }); } diff --git a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php index 396d16a2b7..c3f400d94b 100644 --- a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php +++ b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php @@ -94,27 +94,24 @@ public function testCountRelationAndEagerLoadsTheCount(): void }); $this->assertQueryCountMatches(2, function (): void { - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users { tasks_count } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'tasks_count' => 3, - ], - [ - 'tasks_count' => 2, - ], - [ - 'tasks_count' => 1, - ], - ], - ], - ]); + '); + + + $response->assertJsonCount(3, 'data.users'); + + $users = $response->json('data.users'); + + $counts = collect($users)->pluck('tasks_count')->all(); + + sort($counts); + + $this->assertEquals([1, 2, 3], $counts); }); } diff --git a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php index c5b87621f3..24b36a5787 100644 --- a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php @@ -168,7 +168,7 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void $lastTask = $tasks2->last(); assert($lastTask instanceof Task); - $this + $response = $this ->graphQL(/** @lang GraphQL */ ' query ($firstId: ID!, $lastId: ID!) { users { @@ -183,29 +183,20 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void ', [ 'firstId' => $firstTask->id, 'lastId' => $lastTask->id, - ]) - ->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'firstTasks' => [ - [ - 'id' => $firstTask->id, - ], - ], - 'lastTasks' => [], - ], - [ - 'firstTasks' => [], - 'lastTasks' => [ - [ - 'id' => $lastTask->id, - ], - ], - ], - ], - ], ]); + + $response->assertJsonCount(2, 'data.users'); + + $firstTaskData = empty($response->json('data.users.0.firstTasks')) + ? $response->json('data.users.1.firstTasks.0') + : $response->json('data.users.0.firstTasks.0'); + + $lastTaskData = empty($response->json('data.users.0.lastTasks')) + ? $response->json('data.users.1.lastTasks.0') + : $response->json('data.users.0.lastTasks.0'); + + $this->assertEquals($firstTask->id, $firstTaskData['id']); + $this->assertEquals($lastTask->id, $lastTaskData['id']); } public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void @@ -244,7 +235,7 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void $lastTask = $tasks2->last(); assert($lastTask instanceof Task); - $this + $response = $this ->graphQL(/** @lang GraphQL */ ' query ($firstId: ID!, $lastId: ID!) { users { @@ -263,37 +254,20 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void ', [ 'firstId' => $firstTask->id, 'lastId' => $lastTask->id, - ]) - ->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'firstTasks' => [ - 'data' => [ - [ - 'id' => $firstTask->id, - ], - ], - ], - 'lastTasks' => [ - 'data' => [], - ], - ], - [ - 'firstTasks' => [ - 'data' => [], - ], - 'lastTasks' => [ - 'data' => [ - [ - 'id' => $lastTask->id, - ], - ], - ], - ], - ], - ], ]); + + $response->assertJsonCount(2, 'data.users'); + + $firstTaskData = empty($response->json('data.users.0.firstTasks.data')) + ? $response->json('data.users.1.firstTasks.data.0') + : $response->json('data.users.0.firstTasks.data.0'); + + $lastTaskData = empty($response->json('data.users.0.lastTasks.data')) + ? $response->json('data.users.1.lastTasks.data.0') + : $response->json('data.users.0.lastTasks.data.0'); + + $this->assertEquals($firstTask->id, $firstTaskData['id']); + $this->assertEquals($lastTask->id, $lastTaskData['id']); } public function testCallsScopeWithResolverArgs(): void diff --git a/tests/Integration/Schema/Directives/LimitDirectiveTest.php b/tests/Integration/Schema/Directives/LimitDirectiveTest.php index 0c8e676a30..4d34d12a88 100644 --- a/tests/Integration/Schema/Directives/LimitDirectiveTest.php +++ b/tests/Integration/Schema/Directives/LimitDirectiveTest.php @@ -173,7 +173,7 @@ public function testLimitsWithCache(): void $this->assertInstanceOf(Task::class, $task); $this->assertSame(3, $task->id); - $this + $response = $this ->graphQL(/** @lang GraphQL */ ' { user { @@ -183,28 +183,19 @@ public function testLimitsWithCache(): void } } } - ') - ->assertJson([ - 'data' => [ - 'user' => [ - [ - 'id' => 1, - 'tasks' => [ - [ - 'id' => 1, - ], - ], - ], - [ - 'id' => 2, - 'tasks' => [ - [ - 'id' => 3, - ], - ], - ], - ], - ], - ]); + '); + + $response->assertJsonCount(2, 'data.user'); + + $user1Tasks = intval($response->json('data.user.0.id')) === 1 + ? $response->json('data.user.0.tasks.0') + : $response->json('data.user.1.tasks.0'); + + $user2Tasks = intval($response->json('data.user.0.id')) === 2 + ? $response->json('data.user.0.tasks.0') + : $response->json('data.user.1.tasks.0'); + + $this->assertEquals(1, $user1Tasks['id']); + $this->assertEquals(3, $user2Tasks['id']); } } diff --git a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php index acdea07f08..1073bf5633 100644 --- a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php +++ b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php @@ -95,7 +95,7 @@ public function testOperatorIn(): void { factory(User::class, 5)->create(); - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -107,18 +107,17 @@ public function testOperatorIn(): void id } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'id' => '2', - ], - [ - 'id' => '5', - ], - ], - ], - ]); + '); + + $response->assertJsonCount(2, 'data.users'); + + $users = $response->json('data.users'); + + $ids = collect($users)->pluck('id')->all(); + + sort($ids); + + $this->assertEquals(['2', '5'], $ids); } public function testOperatorIsNull(): void @@ -187,7 +186,7 @@ public function testOperatorNotBetween(): void { factory(User::class, 5)->create(); - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -199,18 +198,17 @@ public function testOperatorNotBetween(): void id } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'id' => '1', - ], - [ - 'id' => '5', - ], - ], - ], - ]); + '); + + $response->assertJsonCount(2, 'data.users'); + + $users = $response->json('data.users'); + + $ids = collect($users)->pluck('id')->all(); + + sort($ids); + + $this->assertEquals(['1', '5'], $ids); } public function testAddsNestedAnd(): void @@ -245,7 +243,7 @@ public function testAddsNestedOr(): void { factory(User::class, 5)->create(); - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -273,28 +271,24 @@ public function testAddsNestedOr(): void id } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'id' => '1', - ], - [ - 'id' => '3', - ], - [ - 'id' => '5', - ], - ], - ], - ]); + '); + + $response->assertJsonCount(3, 'data.users'); + + $users = $response->json('data.users'); + + $ids = collect($users)->pluck('id')->all(); + + sort($ids); + + $this->assertEquals(['1', '3', '5'], $ids); } public function testAddsNestedAndOr(): void { factory(User::class, 5)->create(); - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -323,18 +317,17 @@ public function testAddsNestedAndOr(): void id } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'id' => '2', - ], - [ - 'id' => '3', - ], - ], - ], - ]); + '); + + $response->assertJsonCount(2, 'data.users'); + + $users = $response->json('data.users'); + + $ids = collect($users)->pluck('id')->all(); + + sort($ids); + + $this->assertEquals(['2', '3'], $ids); } public function testHasMixed(): void @@ -439,7 +432,7 @@ public function testHasRelation(): void $commentTwo->comment = 'none'; $commentTwo->save(); - $this->graphQL(/** @lang GraphQL */ ' + $response = $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -451,18 +444,17 @@ public function testHasRelation(): void id } } - ')->assertExactJson([ - 'data' => [ - 'users' => [ - [ - 'id' => '2', - ], - [ - 'id' => '4', - ], - ], - ], - ]); + '); + + $response->assertJsonCount(2, 'data.users'); + + $users = $response->json('data.users'); + + $ids = collect($users)->pluck('id')->all(); + + sort($ids); + + $this->assertEquals(['2', '4'], $ids); } public function testHasAmount(): void From cacc576b54e7c9de61e67200cb07928d659a7720 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Sun, 13 Nov 2022 15:36:59 +0800 Subject: [PATCH 04/45] add select directvie --- src/Select/SelectDirective.php | 23 +++++++++++++++++++++++ src/Select/SelectHelper.php | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 src/Select/SelectDirective.php diff --git a/src/Select/SelectDirective.php b/src/Select/SelectDirective.php new file mode 100644 index 0000000000..7bdbcc9f78 --- /dev/null +++ b/src/Select/SelectDirective.php @@ -0,0 +1,23 @@ + Date: Sun, 13 Nov 2022 15:58:16 +0800 Subject: [PATCH 05/45] move select directive --- src/{Select => Schema/Directives}/SelectDirective.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/{Select => Schema/Directives}/SelectDirective.php (89%) diff --git a/src/Select/SelectDirective.php b/src/Schema/Directives/SelectDirective.php similarity index 89% rename from src/Select/SelectDirective.php rename to src/Schema/Directives/SelectDirective.php index 7bdbcc9f78..d5c3b45d47 100644 --- a/src/Select/SelectDirective.php +++ b/src/Schema/Directives/SelectDirective.php @@ -1,6 +1,6 @@ Date: Sun, 13 Nov 2022 16:03:30 +0800 Subject: [PATCH 06/45] add optimize select config --- src/Pagination/PaginateDirective.php | 30 +++++++++++++------------ src/Schema/Directives/AllDirective.php | 24 +++++++++++--------- src/Schema/Directives/FindDirective.php | 20 +++++++++-------- src/lighthouse.php | 13 +++++++++++ 4 files changed, 53 insertions(+), 34 deletions(-) diff --git a/src/Pagination/PaginateDirective.php b/src/Pagination/PaginateDirective.php index 6682be103c..17b64b4d6a 100644 --- a/src/Pagination/PaginateDirective.php +++ b/src/Pagination/PaginateDirective.php @@ -131,20 +131,22 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->directiveArgValue('scopes', []) ); - if (($query instanceof QueryBuilder || $query instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { - $fieldSelection = $resolveInfo->getFieldSelection(4); - - if (Arr::has($fieldSelection, 'data') || Arr::has($fieldSelection, 'edges')) { - $fieldSelection = array_keys(Arr::has($fieldSelection, 'data') ? $fieldSelection['data'] : $fieldSelection['edges']['node']); - - $selectColumns = SelectHelper::getSelectColumns( - $this->definitionNode, - $fieldSelection, - $this->getModelClass() - ); - - if (! empty($selectColumns)) { - $query = $query->select($selectColumns); + if (config('lighthouse.optimized_selects')) { + if (($query instanceof QueryBuilder || $query instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { + $fieldSelection = $resolveInfo->getFieldSelection(4); + + if (Arr::has($fieldSelection, 'data') || Arr::has($fieldSelection, 'edges')) { + $fieldSelection = array_keys(Arr::has($fieldSelection, 'data') ? $fieldSelection['data'] : $fieldSelection['edges']['node']); + + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); + + if (! empty($selectColumns)) { + $query = $query->select($selectColumns); + } } } } diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index a9bba4bf78..e0dd52cc0b 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -64,20 +64,22 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->directiveArgValue('scopes', []) ); - if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { - $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + if (config('lighthouse.optimized_selects')) { + if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && !$this->directiveHasArgument('builder')) { + $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); - $selectColumns = SelectHelper::getSelectColumns( - $this->definitionNode, - $fieldSelection, - $this->getModelClass() - ); + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); - if (empty($selectColumns)) { - return $builder->get(); - } + if (empty($selectColumns)) { + return $builder->get(); + } - $builder = $builder->select($selectColumns); + $builder = $builder->select($selectColumns); + } } return $builder->get(); diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 26fad00ada..958de3c8e1 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -45,17 +45,19 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->directiveArgValue('scopes', []) ); - if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('model')) { - $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + if (config('lighthouse.optimized_selects')) { + if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('model')) { + $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); - $selectColumns = SelectHelper::getSelectColumns( - $this->definitionNode, - $fieldSelection, - $this->getModelClass() - ); + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); - if (!empty($selectColumns)) { - $builder = $builder->select($selectColumns); + if (!empty($selectColumns)) { + $builder = $builder->select($selectColumns); + } } } diff --git a/src/lighthouse.php b/src/lighthouse.php index ab19880f67..0b04e172b2 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -355,6 +355,19 @@ 'batchload_relations' => true, + /* + |-------------------------------------------------------------------------- + | Optimized Selects + |-------------------------------------------------------------------------- + | + | If set to true, Eloquent will only select the columns necessary to resolve + | a query. You must use the select directive to resolve advanced field dependencies + | on other columns. + | + */ + + 'optimized_selects' => true, + /* |-------------------------------------------------------------------------- | Shortcut Foreign Key Selection From ed0bee975b7ea9b3c35ef3c02910362b9c69790e Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 15 Nov 2022 09:00:45 +0800 Subject: [PATCH 07/45] filter non-array document --- src/Select/SelectHelper.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index b18aae374c..29f7ae642f 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -41,9 +41,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } // ignore relation closure, e.g. RelationOrderByClause - foreach (array_keys($documentAST->types) as $type) { - if (Str::contains($type, ['RelationOrderByClause'], true)) { - return []; + if (is_array($documentAST->types)) { + foreach (array_keys($documentAST->types) as $type) { + if (Str::contains($type, ['RelationOrderByClause'], true)) { + return []; + } } } From 3ef48bddf5a17e461b0de27586907d80bc4aa410 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 15 Nov 2022 12:28:49 +0800 Subject: [PATCH 08/45] remove Schema request --- src/Schema/Directives/FindDirective.php | 2 +- src/Select/SelectHelper.php | 43 +++++++------------ tests/AssertsQueryCounts.php | 5 --- .../Schema/Directives/FindDirectiveTest.php | 2 +- 4 files changed, 17 insertions(+), 35 deletions(-) diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 958de3c8e1..76c8285149 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -46,7 +46,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue ); if (config('lighthouse.optimized_selects')) { - if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('model')) { + if ($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) { $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); $selectColumns = SelectHelper::getSelectColumns( diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 29f7ae642f..5330141b85 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -5,18 +5,19 @@ use GraphQL\Language\AST\Node; use GraphQL\Language\AST\UnionTypeDefinitionNode; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; class SelectHelper { - public const DirectivesRequiringLocalKey = ['hasOne', 'hasMany', 'count']; + public const DirectivesRequiringLocalKey = ['hasOne', 'hasMany', 'count', 'morphOne', 'morphMany']; - public const DirectivesRequiringForeignKey = ['belongsTo', 'belongsToMany', 'morphTo']; + public const DirectivesRequiringForeignKey = ['belongsTo']; - public const DirectivesIgnore = ['morphTo', 'morphMany']; + public const DirectivesReturn = ['morphTo', 'morphToMany']; + + public const DirectivesIgnore = ['aggregate', 'withCount', 'belongsToMany']; /** * Given a field definition node, resolve info, and a model name, return the SQL columns that should be selected. @@ -29,8 +30,6 @@ class SelectHelper */ public static function getSelectColumns(Node $definitionNode, array $fieldSelection, string $modelName): array { - DB::disableQueryLog(); - $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); /** @var \Nuwave\Lighthouse\Schema\AST\DocumentAST $documentAST */ @@ -66,12 +65,16 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); if ($fieldDefinition) { - $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey, self::DirectivesIgnore); + $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey, self::DirectivesReturn, self::DirectivesIgnore); foreach ($directivesRequiringKeys as $directiveType) { if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); + if (in_array($directiveType, self::DirectivesReturn)) { + return []; + } + if (in_array($directiveType, self::DirectivesRequiringLocalKey)) { $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); @@ -84,18 +87,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - if ($directiveType === 'belongsToMany') { - array_push($selectColumns, $model->{$relationName}()->getForeignPivotKeyName()); - } else { - array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); - } + array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); } } - if (in_array($directiveType, self::DirectivesIgnore)) { - return []; - } - continue 2; } } @@ -123,18 +118,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } - // for unit test query log check - try { - $allColumns = Schema::getColumnListing($model->getTable()); - } catch (\Exception $e) { - // connection refuse - $allColumns = []; - } - - DB::enableQueryLog(); + $selectColumns = array_filter($selectColumns, function($column) use ($model) { + return !$model->hasGetMutator($column) && !method_exists($model, $column); + }); - return !empty($allColumns) - ? array_intersect($allColumns, array_unique($selectColumns)) - : array_unique($selectColumns); + return array_unique($selectColumns); } } diff --git a/tests/AssertsQueryCounts.php b/tests/AssertsQueryCounts.php index 12a7a3cb80..e5734dc7b0 100644 --- a/tests/AssertsQueryCounts.php +++ b/tests/AssertsQueryCounts.php @@ -17,11 +17,6 @@ trait AssertsQueryCounts protected function countQueries(?int &$count): void { DB::listen(function ($query) use (&$count): void { - // ignore fetch column list. - if (Str::contains($query->sql, ['select column_name'])) { - return; - } - ++$count; }); } diff --git a/tests/Integration/Schema/Directives/FindDirectiveTest.php b/tests/Integration/Schema/Directives/FindDirectiveTest.php index 34dc436986..2a11895613 100644 --- a/tests/Integration/Schema/Directives/FindDirectiveTest.php +++ b/tests/Integration/Schema/Directives/FindDirectiveTest.php @@ -170,7 +170,7 @@ public function testReturnsCustomAttributes(): void type User { id: ID! name: String! - companyName: String! + companyName: String! @select(columns: ["company_id"]) } type Query { From 23237a1e9fbcfe9539a9b61aca7600feab7f82ea Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Wed, 16 Nov 2022 08:50:55 +0800 Subject: [PATCH 09/45] handle RelationOrderBy problems --- src/Schema/Directives/AllDirective.php | 19 ++++++++++++++++++- src/Select/SelectHelper.php | 10 ---------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index e0dd52cc0b..64dce25490 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -6,6 +6,7 @@ use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Database\Query\Builder as QueryBuilder; +use Illuminate\Database\Query\Expression; use Illuminate\Support\Collection; use Laravel\Scout\Builder as ScoutBuilder; use Nuwave\Lighthouse\Schema\Values\FieldValue; @@ -78,7 +79,23 @@ public function resolveField(FieldValue $fieldValue): FieldValue return $builder->get(); } - $builder = $builder->select($selectColumns); + $query = $builder->getQuery(); + + if ($query->columns !== null) { + $bindings = $query->getRawBindings(); + + $expressions = array_filter($query->columns, function ($column) { + return $column instanceof Expression; + }); + + $builder = $builder->select(array_unique(array_merge($selectColumns, $expressions))); + + foreach ($bindings as $type => $binding) { + $builder = $builder->addBinding($binding, $type); + } + } else { + $builder = $builder->select($selectColumns); + } } } diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 5330141b85..230f7d3476 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -4,7 +4,6 @@ use GraphQL\Language\AST\Node; use GraphQL\Language\AST\UnionTypeDefinitionNode; -use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; @@ -39,15 +38,6 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $returnTypeName = Str::replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); } - // ignore relation closure, e.g. RelationOrderByClause - if (is_array($documentAST->types)) { - foreach (array_keys($documentAST->types) as $type) { - if (Str::contains($type, ['RelationOrderByClause'], true)) { - return []; - } - } - } - $type = $documentAST->types[$returnTypeName]; if ($type instanceof UnionTypeDefinitionNode) { From 1b73b78b3e11bfb5789111439f8eed627ce7cd64 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Wed, 16 Nov 2022 08:54:11 +0800 Subject: [PATCH 10/45] lint --- src/Schema/Directives/AllDirective.php | 4 ++-- src/Schema/Directives/FindDirective.php | 2 +- src/Schema/Directives/SelectDirective.php | 2 -- src/Select/SelectHelper.php | 17 ++++++++++++----- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index 64dce25490..ec9bb1b29e 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -66,7 +66,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue ); if (config('lighthouse.optimized_selects')) { - if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && !$this->directiveHasArgument('builder')) { + if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); $selectColumns = SelectHelper::getSelectColumns( @@ -81,7 +81,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue $query = $builder->getQuery(); - if ($query->columns !== null) { + if (null !== $query->columns) { $bindings = $query->getRawBindings(); $expressions = array_filter($query->columns, function ($column) { diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 76c8285149..7b5eb7414d 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -55,7 +55,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->getModelClass() ); - if (!empty($selectColumns)) { + if (! empty($selectColumns)) { $builder = $builder->select($selectColumns); } } diff --git a/src/Schema/Directives/SelectDirective.php b/src/Schema/Directives/SelectDirective.php index d5c3b45d47..b0ab7cceb9 100644 --- a/src/Schema/Directives/SelectDirective.php +++ b/src/Schema/Directives/SelectDirective.php @@ -2,8 +2,6 @@ namespace Nuwave\Lighthouse\Schema\Directives; -use Nuwave\Lighthouse\Schema\Directives\BaseDirective; - class SelectDirective extends BaseDirective { public static function definition(): string diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 230f7d3476..995afa14ce 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -2,11 +2,15 @@ namespace Nuwave\Lighthouse\Select; +use GraphQL\Language\AST\DirectiveNode; +use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\Node; use GraphQL\Language\AST\UnionTypeDefinitionNode; +use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Str; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; +use Nuwave\Lighthouse\Schema\AST\DocumentAST; class SelectHelper { @@ -20,18 +24,19 @@ class SelectHelper /** * Given a field definition node, resolve info, and a model name, return the SQL columns that should be selected. - * Accounts for relationships and the rename and select directives. + * Accounts for relationships and to rename and select directives. * * @param mixed[] $fieldSelection * * @return string[] + * * @reference https://github.com/nuwave/lighthouse/pull/1626 */ public static function getSelectColumns(Node $definitionNode, array $fieldSelection, string $modelName): array { $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); - /** @var \Nuwave\Lighthouse\Schema\AST\DocumentAST $documentAST */ + /** @var DocumentAST $documentAST */ $documentAST = app(ASTBuilder::class)->documentAST(); if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'], true)) { @@ -44,9 +49,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])]; } - /** @var iterable<\GraphQL\Language\AST\FieldDefinitionNode> $fieldDefinitions */ + /** @var iterable $fieldDefinitions */ $fieldDefinitions = $type->fields; + /** @var Model $model */ $model = new $modelName(); $selectColumns = []; @@ -59,6 +65,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect foreach ($directivesRequiringKeys as $directiveType) { if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { + /** @var DirectiveNode $directive */ $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); if (in_array($directiveType, self::DirectivesReturn)) { @@ -108,8 +115,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } - $selectColumns = array_filter($selectColumns, function($column) use ($model) { - return !$model->hasGetMutator($column) && !method_exists($model, $column); + $selectColumns = array_filter($selectColumns, function ($column) use ($model) { + return ! $model->hasGetMutator($column) && ! method_exists($model, $column); }); return array_unique($selectColumns); From bcf8f8c0f00d6e4b50f3ed085d63cf79d8c5edf5 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 21 Nov 2022 15:12:30 +0800 Subject: [PATCH 11/45] add tests --- .../Pagination/PaginateDirectiveDBTest.php | 39 ++++++++++++++++ .../Schema/Directives/AllDirectiveTest.php | 46 +++++++++++++++++++ .../Schema/Directives/FindDirectiveTest.php | 44 ++++++++++++++++++ 3 files changed, 129 insertions(+) diff --git a/tests/Integration/Pagination/PaginateDirectiveDBTest.php b/tests/Integration/Pagination/PaginateDirectiveDBTest.php index d09a48a4ab..85a9a12fc9 100644 --- a/tests/Integration/Pagination/PaginateDirectiveDBTest.php +++ b/tests/Integration/Pagination/PaginateDirectiveDBTest.php @@ -265,6 +265,45 @@ public function testPaginateWithScopes(): void ]); } + public function testPaginateOptimizedSelected(): void + { + factory(User::class, 2)->create(); + + $this->schema = /** @lang GraphQL */ ' + type User { + id: ID! + } + + type Query { + users: [User!]! @paginate + } + '; + + self::trackQueries(); + + $this->graphQL(/** @lang GraphQL */ ' + { + users(first: 1) { + paginatorInfo { + count + total + currentPage + } + data { + id + } + } + } + ')->assertJsonCount(1, 'data.users.data'); + + $queries = self::getQueriesExecuted(); + + $this->assertStringContainsString( + 'select `id` from `users`', + $queries[1]['query'] + ); + } + public function builder(): EloquentBuilder { return User::orderBy('id', 'DESC'); diff --git a/tests/Integration/Schema/Directives/AllDirectiveTest.php b/tests/Integration/Schema/Directives/AllDirectiveTest.php index 5a63173d4a..63de13be37 100644 --- a/tests/Integration/Schema/Directives/AllDirectiveTest.php +++ b/tests/Integration/Schema/Directives/AllDirectiveTest.php @@ -302,6 +302,52 @@ public function testSpecifyCustomBuilderForScoutBuilder(): void ]); } + public function testGetAllOptimizeSelected(): void + { + factory(Post::class, 2)->create([ + // Do not create those, as they would create more users + 'task_id' => 1, + ]); + + $this->schema = /** @lang GraphQL */ ' + type User { + posts: [Post!]! @all + } + + type Post { + id: ID! + } + + type Query { + users: [User!]! @all + } + '; + + self::trackQueries(); + + $this->graphQL(/** @lang GraphQL */ ' + { + users { + posts { + id + } + } + } + '); + + $queries = self::getQueriesExecuted(); + + $this->assertStringContainsString( + 'select `id` from `posts`', + $queries[1]['query'] + ); + + $this->assertStringContainsString( + 'select `id` from `posts`', + $queries[2]['query'] + ); + } + public function builder(): EloquentBuilder { return User::orderBy('id', 'DESC'); diff --git a/tests/Integration/Schema/Directives/FindDirectiveTest.php b/tests/Integration/Schema/Directives/FindDirectiveTest.php index 2a11895613..cb20ee3958 100644 --- a/tests/Integration/Schema/Directives/FindDirectiveTest.php +++ b/tests/Integration/Schema/Directives/FindDirectiveTest.php @@ -196,4 +196,48 @@ public function testReturnsCustomAttributes(): void ], ]); } + + public function testReturnsOptimizeSelected(): void + { + $company = factory(Company::class)->create(); + $user = factory(User::class)->create([ + 'company_id' => $company->id, + ]); + + $this->schema = ' + type User { + id: ID! + companyName: String! @select(columns: ["company_id"]) + } + + type Query { + user(id: ID @eq): User @find(model: "User") + } + '; + + self::trackQueries(); + + $this->graphQL(" + { + user(id: {$user->id}) { + id + companyName + } + } + ")->assertJson([ + 'data' => [ + 'user' => [ + 'id' => (string) $user->id, + 'companyName' => $company->name, + ], + ], + ]); + + $queries = self::getQueriesExecuted(); + + $this->assertStringContainsString( + 'select `id`, `company_id` from `users`', + $queries[0]['query'] + ); + } } From 6ca50aef0bc91cbe76909e8f5ba19f6ad4d4aaa3 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 21 Nov 2022 16:21:12 +0800 Subject: [PATCH 12/45] rename tests --- tests/Integration/Pagination/PaginateDirectiveDBTest.php | 2 +- tests/Integration/Schema/Directives/AllDirectiveTest.php | 2 +- tests/Integration/Schema/Directives/FindDirectiveTest.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Integration/Pagination/PaginateDirectiveDBTest.php b/tests/Integration/Pagination/PaginateDirectiveDBTest.php index 85a9a12fc9..7d4c998442 100644 --- a/tests/Integration/Pagination/PaginateDirectiveDBTest.php +++ b/tests/Integration/Pagination/PaginateDirectiveDBTest.php @@ -265,7 +265,7 @@ public function testPaginateWithScopes(): void ]); } - public function testPaginateOptimizedSelected(): void + public function testPaginateOptimizedSelect(): void { factory(User::class, 2)->create(); diff --git a/tests/Integration/Schema/Directives/AllDirectiveTest.php b/tests/Integration/Schema/Directives/AllDirectiveTest.php index 63de13be37..9322c50b90 100644 --- a/tests/Integration/Schema/Directives/AllDirectiveTest.php +++ b/tests/Integration/Schema/Directives/AllDirectiveTest.php @@ -302,7 +302,7 @@ public function testSpecifyCustomBuilderForScoutBuilder(): void ]); } - public function testGetAllOptimizeSelected(): void + public function testGetAllOptimizedSelect(): void { factory(Post::class, 2)->create([ // Do not create those, as they would create more users diff --git a/tests/Integration/Schema/Directives/FindDirectiveTest.php b/tests/Integration/Schema/Directives/FindDirectiveTest.php index cb20ee3958..2721ee37e5 100644 --- a/tests/Integration/Schema/Directives/FindDirectiveTest.php +++ b/tests/Integration/Schema/Directives/FindDirectiveTest.php @@ -197,7 +197,7 @@ public function testReturnsCustomAttributes(): void ]); } - public function testReturnsOptimizeSelected(): void + public function testReturnsOptimizedSelect(): void { $company = factory(Company::class)->create(); $user = factory(User::class)->create([ From c9a73439fa6574e65c87818f6ddef266c32b512a Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 21 Nov 2022 17:06:56 +0800 Subject: [PATCH 13/45] use str_replace --- src/Select/SelectHelper.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 995afa14ce..ff1c4b07c7 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -39,8 +39,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect /** @var DocumentAST $documentAST */ $documentAST = app(ASTBuilder::class)->documentAST(); - if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'], true)) { - $returnTypeName = Str::replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); + if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { + $returnTypeName = str_replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); } $type = $documentAST->types[$returnTypeName]; From 7e75f24e4cfcebace73a90a27f9d297e45e89362 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 21 Nov 2022 09:08:44 +0000 Subject: [PATCH 14/45] Apply php-cs-fixer changes --- tests/AssertsQueryCounts.php | 1 - tests/Integration/Schema/Directives/CountDirectiveDBTest.php | 1 - tests/Integration/Schema/Directives/LimitDirectiveTest.php | 4 ++-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/AssertsQueryCounts.php b/tests/AssertsQueryCounts.php index e5734dc7b0..ba26bea6a1 100644 --- a/tests/AssertsQueryCounts.php +++ b/tests/AssertsQueryCounts.php @@ -3,7 +3,6 @@ namespace Tests; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Str; /** * This trait was taken from a package that supports fewer Laravel versions than us. diff --git a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php index c3f400d94b..c9fa32dbd9 100644 --- a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php +++ b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php @@ -102,7 +102,6 @@ public function testCountRelationAndEagerLoadsTheCount(): void } '); - $response->assertJsonCount(3, 'data.users'); $users = $response->json('data.users'); diff --git a/tests/Integration/Schema/Directives/LimitDirectiveTest.php b/tests/Integration/Schema/Directives/LimitDirectiveTest.php index 4d34d12a88..eef7a4e324 100644 --- a/tests/Integration/Schema/Directives/LimitDirectiveTest.php +++ b/tests/Integration/Schema/Directives/LimitDirectiveTest.php @@ -187,11 +187,11 @@ public function testLimitsWithCache(): void $response->assertJsonCount(2, 'data.user'); - $user1Tasks = intval($response->json('data.user.0.id')) === 1 + $user1Tasks = 1 === intval($response->json('data.user.0.id')) ? $response->json('data.user.0.tasks.0') : $response->json('data.user.1.tasks.0'); - $user2Tasks = intval($response->json('data.user.0.id')) === 2 + $user2Tasks = 2 === intval($response->json('data.user.0.id')) ? $response->json('data.user.0.tasks.0') : $response->json('data.user.1.tasks.0'); From 45d12f103798e462f4c42b989d4306c95a1c21b6 Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 17:20:05 +0800 Subject: [PATCH 15/45] lint --- src/Schema/Directives/AllDirective.php | 2 +- src/Schema/Directives/FindDirective.php | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index ec9bb1b29e..afcddaeb27 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -79,7 +79,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue return $builder->get(); } - $query = $builder->getQuery(); + $query = $builder instanceof EloquentBuilder ? $builder->getQuery() : $builder; if (null !== $query->columns) { $bindings = $query->getRawBindings(); diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 7b5eb7414d..7581c5c57b 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -46,18 +46,16 @@ public function resolveField(FieldValue $fieldValue): FieldValue ); if (config('lighthouse.optimized_selects')) { - if ($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) { - $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); - $selectColumns = SelectHelper::getSelectColumns( - $this->definitionNode, - $fieldSelection, - $this->getModelClass() - ); + $selectColumns = SelectHelper::getSelectColumns( + $this->definitionNode, + $fieldSelection, + $this->getModelClass() + ); - if (! empty($selectColumns)) { - $builder = $builder->select($selectColumns); - } + if (! empty($selectColumns)) { + $builder = $builder->select($selectColumns); } } From cd30eea3161905e5566d422ac39c07d262d8581b Mon Sep 17 00:00:00 2001 From: bepsvpt Date: Mon, 21 Nov 2022 09:21:16 +0000 Subject: [PATCH 16/45] Apply php-cs-fixer changes --- src/Schema/Directives/FindDirective.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index 7581c5c57b..f110388704 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -4,9 +4,7 @@ use GraphQL\Error\Error; use GraphQL\Type\Definition\ResolveInfo; -use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Model; -use Illuminate\Database\Query\Builder as QueryBuilder; use Nuwave\Lighthouse\Schema\Values\FieldValue; use Nuwave\Lighthouse\Select\SelectHelper; use Nuwave\Lighthouse\Support\Contracts\FieldResolver; From fa6f99a6af5fab34650c5e08de55fbb286cc87ff Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 21 Nov 2022 17:32:37 +0800 Subject: [PATCH 17/45] add old version compatibility --- src/Select/SelectHelper.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index ff1c4b07c7..0f7e2b333b 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -11,6 +11,7 @@ use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; use Nuwave\Lighthouse\Schema\AST\DocumentAST; +use Nuwave\Lighthouse\Support\AppVersion; class SelectHelper { @@ -76,7 +77,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); + if (AppVersion::below(5.8)) { + array_push($selectColumns, $model->{$relationName}()->getLocalKey()); + } else { + array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); + } } } @@ -84,7 +89,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); + if (AppVersion::below(5.8)) { + array_push($selectColumns, $model->{$relationName}()->getForeignKey()); + } else { + array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); + } } } From 7bc7c7695a16266bc58c8903d7c34747287c77ab Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 17:56:05 +0800 Subject: [PATCH 18/45] fix localKeyName for laravel version below 5.7 --- src/Select/SelectHelper.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 0f7e2b333b..bb7093b3ce 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -12,6 +12,7 @@ use Nuwave\Lighthouse\Schema\AST\ASTHelper; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Support\AppVersion; +use ReflectionClass; class SelectHelper { @@ -77,8 +78,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - if (AppVersion::below(5.8)) { - array_push($selectColumns, $model->{$relationName}()->getLocalKey()); + if (AppVersion::below(5.7)) { + $relation = new ReflectionClass($model->{$relationName}()); + $localKey = $relation->getProperty('localKey'); + $localKey->setAccessible(true); + array_push($selectColumns, $localKey->getValue($relation)); } else { array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); } From f983dec16574598c1727ac648f01a5aba934e869 Mon Sep 17 00:00:00 2001 From: bepsvpt Date: Mon, 21 Nov 2022 09:56:55 +0000 Subject: [PATCH 19/45] Apply php-cs-fixer changes --- src/Select/SelectHelper.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index bb7093b3ce..9baa3540aa 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -12,7 +12,6 @@ use Nuwave\Lighthouse\Schema\AST\ASTHelper; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Support\AppVersion; -use ReflectionClass; class SelectHelper { @@ -79,7 +78,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if (method_exists($model, $relationName)) { if (AppVersion::below(5.7)) { - $relation = new ReflectionClass($model->{$relationName}()); + $relation = new \ReflectionClass($model->{$relationName}()); $localKey = $relation->getProperty('localKey'); $localKey->setAccessible(true); array_push($selectColumns, $localKey->getValue($relation)); From 25e0ecc18587f2cc265945edad56d71bd027aa27 Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:09:07 +0800 Subject: [PATCH 20/45] fix reflection issue --- src/Select/SelectHelper.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 9baa3540aa..9a6d8836d5 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -79,6 +79,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if (method_exists($model, $relationName)) { if (AppVersion::below(5.7)) { $relation = new \ReflectionClass($model->{$relationName}()); + + while ($relation->getShortName() !== 'HasOneOrMany') { + $relation = $relation->getParentClass(); + } + $localKey = $relation->getProperty('localKey'); $localKey->setAccessible(true); array_push($selectColumns, $localKey->getValue($relation)); From abf875fd675f01596da957ec20baea8918234c89 Mon Sep 17 00:00:00 2001 From: bepsvpt Date: Mon, 21 Nov 2022 10:11:16 +0000 Subject: [PATCH 21/45] Apply php-cs-fixer changes --- src/Select/SelectHelper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 9a6d8836d5..2e66aafd40 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -80,7 +80,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if (AppVersion::below(5.7)) { $relation = new \ReflectionClass($model->{$relationName}()); - while ($relation->getShortName() !== 'HasOneOrMany') { + while ('HasOneOrMany' !== $relation->getShortName()) { $relation = $relation->getParentClass(); } From 61ed5cd7c0ba982e7648bd4ee20d266dce0c09dc Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:26:11 +0800 Subject: [PATCH 22/45] fix ReflectionClass getValue --- src/Select/SelectHelper.php | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 2e66aafd40..2eeda255f7 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -78,13 +78,9 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if (method_exists($model, $relationName)) { if (AppVersion::below(5.7)) { - $relation = new \ReflectionClass($model->{$relationName}()); - - while ('HasOneOrMany' !== $relation->getShortName()) { - $relation = $relation->getParentClass(); - } - - $localKey = $relation->getProperty('localKey'); + $relation = $model->{$relationName}(); + $rc = new \ReflectionClass($model->{$relationName}()); + $localKey = $rc->getProperty('localKey'); $localKey->setAccessible(true); array_push($selectColumns, $localKey->getValue($relation)); } else { From 768b28bf1467cd0c9792e7f641c1363fc135ecdf Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 21:21:02 +0800 Subject: [PATCH 23/45] wip, fix tests --- .../Directives/CountDirectiveDBTest.php | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php index c9fa32dbd9..96c7c70f62 100644 --- a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php +++ b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php @@ -78,7 +78,7 @@ public function testCountRelationAndEagerLoadsTheCount(): void { $this->schema = /** @lang GraphQL */ ' type Query { - users: [User!] @all + users: [User!] @all @orderBy(column: "id") } type User { @@ -94,23 +94,27 @@ public function testCountRelationAndEagerLoadsTheCount(): void }); $this->assertQueryCountMatches(2, function (): void { - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users { tasks_count } } - '); - - $response->assertJsonCount(3, 'data.users'); - - $users = $response->json('data.users'); - - $counts = collect($users)->pluck('tasks_count')->all(); - - sort($counts); - - $this->assertEquals([1, 2, 3], $counts); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'tasks_count' => 3, + ], + [ + 'tasks_count' => 2, + ], + [ + 'tasks_count' => 1, + ], + ], + ], + ]); }); } From 6e21e28b5e842557b070996b3febac14e423fc7f Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 21:47:26 +0800 Subject: [PATCH 24/45] wip, fix tests --- .../Directives/CountDirectiveDBTest.php | 2 +- .../Directives/HasManyDirectiveTest.php | 86 +++++++---- .../Schema/Directives/LimitDirectiveTest.php | 43 +++--- .../WhereConditionsDirectiveTest.php | 136 +++++++++--------- 4 files changed, 155 insertions(+), 112 deletions(-) diff --git a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php index 96c7c70f62..a44cec93eb 100644 --- a/tests/Integration/Schema/Directives/CountDirectiveDBTest.php +++ b/tests/Integration/Schema/Directives/CountDirectiveDBTest.php @@ -78,7 +78,7 @@ public function testCountRelationAndEagerLoadsTheCount(): void { $this->schema = /** @lang GraphQL */ ' type Query { - users: [User!] @all @orderBy(column: "id") + users: [User!] @all @orderBy(column: "id") } type User { diff --git a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php index 24b36a5787..565089b68b 100644 --- a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php @@ -146,7 +146,7 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void } type Query { - users: [User!]! @all + users: [User!]! @all @orderBy(column: "id") } '; @@ -168,7 +168,7 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void $lastTask = $tasks2->last(); assert($lastTask instanceof Task); - $response = $this + $this ->graphQL(/** @lang GraphQL */ ' query ($firstId: ID!, $lastId: ID!) { users { @@ -183,20 +183,29 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void ', [ 'firstId' => $firstTask->id, 'lastId' => $lastTask->id, + ]) + ->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'firstTasks' => [ + [ + 'id' => $firstTask->id, + ], + ], + 'lastTasks' => [], + ], + [ + 'firstTasks' => [], + 'lastTasks' => [ + [ + 'id' => $lastTask->id, + ], + ], + ], + ], + ], ]); - - $response->assertJsonCount(2, 'data.users'); - - $firstTaskData = empty($response->json('data.users.0.firstTasks')) - ? $response->json('data.users.1.firstTasks.0') - : $response->json('data.users.0.firstTasks.0'); - - $lastTaskData = empty($response->json('data.users.0.lastTasks')) - ? $response->json('data.users.1.lastTasks.0') - : $response->json('data.users.0.lastTasks.0'); - - $this->assertEquals($firstTask->id, $firstTaskData['id']); - $this->assertEquals($lastTask->id, $lastTaskData['id']); } public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void @@ -213,7 +222,7 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void } type Query { - users: [User!]! @all + users: [User!]! @all @orderBy(column: "id") } '; @@ -235,7 +244,7 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void $lastTask = $tasks2->last(); assert($lastTask instanceof Task); - $response = $this + $this ->graphQL(/** @lang GraphQL */ ' query ($firstId: ID!, $lastId: ID!) { users { @@ -254,20 +263,37 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void ', [ 'firstId' => $firstTask->id, 'lastId' => $lastTask->id, + ]) + ->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'firstTasks' => [ + 'data' => [ + [ + 'id' => $firstTask->id, + ], + ], + ], + 'lastTasks' => [ + 'data' => [], + ], + ], + [ + 'firstTasks' => [ + 'data' => [], + ], + 'lastTasks' => [ + 'data' => [ + [ + 'id' => $lastTask->id, + ], + ], + ], + ], + ], + ], ]); - - $response->assertJsonCount(2, 'data.users'); - - $firstTaskData = empty($response->json('data.users.0.firstTasks.data')) - ? $response->json('data.users.1.firstTasks.data.0') - : $response->json('data.users.0.firstTasks.data.0'); - - $lastTaskData = empty($response->json('data.users.0.lastTasks.data')) - ? $response->json('data.users.1.lastTasks.data.0') - : $response->json('data.users.0.lastTasks.data.0'); - - $this->assertEquals($firstTask->id, $firstTaskData['id']); - $this->assertEquals($lastTask->id, $lastTaskData['id']); } public function testCallsScopeWithResolverArgs(): void diff --git a/tests/Integration/Schema/Directives/LimitDirectiveTest.php b/tests/Integration/Schema/Directives/LimitDirectiveTest.php index eef7a4e324..86693d3701 100644 --- a/tests/Integration/Schema/Directives/LimitDirectiveTest.php +++ b/tests/Integration/Schema/Directives/LimitDirectiveTest.php @@ -104,7 +104,7 @@ public function testLimitsRelations(): void } type Query { - users: [User!]! @all + users: [User!]! @all @orderBy(column: "id") } '; @@ -148,7 +148,7 @@ public function testLimitsWithCache(): void } type Query { - user: [User!]! @all + user: [User!]! @all @orderBy(column: "id") } '; @@ -173,7 +173,7 @@ public function testLimitsWithCache(): void $this->assertInstanceOf(Task::class, $task); $this->assertSame(3, $task->id); - $response = $this + $this ->graphQL(/** @lang GraphQL */ ' { user { @@ -183,19 +183,28 @@ public function testLimitsWithCache(): void } } } - '); - - $response->assertJsonCount(2, 'data.user'); - - $user1Tasks = 1 === intval($response->json('data.user.0.id')) - ? $response->json('data.user.0.tasks.0') - : $response->json('data.user.1.tasks.0'); - - $user2Tasks = 2 === intval($response->json('data.user.0.id')) - ? $response->json('data.user.0.tasks.0') - : $response->json('data.user.1.tasks.0'); - - $this->assertEquals(1, $user1Tasks['id']); - $this->assertEquals(3, $user2Tasks['id']); + ') + ->assertJson([ + 'data' => [ + 'user' => [ + [ + 'id' => 1, + 'tasks' => [ + [ + 'id' => 1, + ], + ], + ], + [ + 'id' => 2, + 'tasks' => [ + [ + 'id' => 3, + ], + ], + ], + ], + ], + ]); } } diff --git a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php index 1073bf5633..7f76910908 100644 --- a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php +++ b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php @@ -30,14 +30,14 @@ final class WhereConditionsDirectiveTest extends DBTestCase } type Query { - posts(where: _ @whereConditions): [Post!]! @all - users(where: _ @whereConditions): [User!]! @all + posts(where: _ @whereConditions): [Post!]! @all @orderBy(column: "id") + users(where: _ @whereConditions): [User!]! @all @orderBy(column: "id") whitelistedColumns( where: _ @whereConditions(columns: ["id", "camelCase"]) - ): [User!]! @all + ): [User!]! @all @orderBy(column: "id") enumColumns( where: _ @whereConditions(columnsEnum: "UserColumn") - ): [User!]! @all + ): [User!]! @all @orderBy(column: "id") } enum UserColumn { @@ -95,7 +95,7 @@ public function testOperatorIn(): void { factory(User::class, 5)->create(); - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -107,17 +107,18 @@ public function testOperatorIn(): void id } } - '); - - $response->assertJsonCount(2, 'data.users'); - - $users = $response->json('data.users'); - - $ids = collect($users)->pluck('id')->all(); - - sort($ids); - - $this->assertEquals(['2', '5'], $ids); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'id' => '2', + ], + [ + 'id' => '5', + ], + ], + ], + ]); } public function testOperatorIsNull(): void @@ -186,7 +187,7 @@ public function testOperatorNotBetween(): void { factory(User::class, 5)->create(); - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -198,17 +199,18 @@ public function testOperatorNotBetween(): void id } } - '); - - $response->assertJsonCount(2, 'data.users'); - - $users = $response->json('data.users'); - - $ids = collect($users)->pluck('id')->all(); - - sort($ids); - - $this->assertEquals(['1', '5'], $ids); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'id' => '1', + ], + [ + 'id' => '5', + ], + ], + ], + ]); } public function testAddsNestedAnd(): void @@ -243,7 +245,7 @@ public function testAddsNestedOr(): void { factory(User::class, 5)->create(); - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -271,24 +273,28 @@ public function testAddsNestedOr(): void id } } - '); - - $response->assertJsonCount(3, 'data.users'); - - $users = $response->json('data.users'); - - $ids = collect($users)->pluck('id')->all(); - - sort($ids); - - $this->assertEquals(['1', '3', '5'], $ids); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'id' => '1', + ], + [ + 'id' => '3', + ], + [ + 'id' => '5', + ], + ], + ], + ]); } public function testAddsNestedAndOr(): void { factory(User::class, 5)->create(); - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -317,17 +323,18 @@ public function testAddsNestedAndOr(): void id } } - '); - - $response->assertJsonCount(2, 'data.users'); - - $users = $response->json('data.users'); - - $ids = collect($users)->pluck('id')->all(); - - sort($ids); - - $this->assertEquals(['2', '3'], $ids); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'id' => '2', + ], + [ + 'id' => '3', + ], + ], + ], + ]); } public function testHasMixed(): void @@ -432,7 +439,7 @@ public function testHasRelation(): void $commentTwo->comment = 'none'; $commentTwo->save(); - $response = $this->graphQL(/** @lang GraphQL */ ' + $this->graphQL(/** @lang GraphQL */ ' { users( where: { @@ -444,17 +451,18 @@ public function testHasRelation(): void id } } - '); - - $response->assertJsonCount(2, 'data.users'); - - $users = $response->json('data.users'); - - $ids = collect($users)->pluck('id')->all(); - - sort($ids); - - $this->assertEquals(['2', '4'], $ids); + ')->assertExactJson([ + 'data' => [ + 'users' => [ + [ + 'id' => '2', + ], + [ + 'id' => '4', + ], + ], + ], + ]); } public function testHasAmount(): void From 099091942a2adc78c0c8ddd9f5726d476a89ef51 Mon Sep 17 00:00:00 2001 From: bepsvpt <8221099+bepsvpt@users.noreply.github.com> Date: Mon, 21 Nov 2022 21:56:27 +0800 Subject: [PATCH 25/45] cleanup debug code --- tests/AssertsQueryCounts.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/AssertsQueryCounts.php b/tests/AssertsQueryCounts.php index ba26bea6a1..8a44b5abee 100644 --- a/tests/AssertsQueryCounts.php +++ b/tests/AssertsQueryCounts.php @@ -15,7 +15,7 @@ trait AssertsQueryCounts { protected function countQueries(?int &$count): void { - DB::listen(function ($query) use (&$count): void { + DB::listen(function () use (&$count): void { ++$count; }); } From deabb67ed87f0d7050de6fd369182261c5c94756 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 16:23:06 +0800 Subject: [PATCH 26/45] lint --- src/Select/SelectHelper.php | 100 +++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 2eeda255f7..ed8d9a4b3d 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -3,8 +3,8 @@ namespace Nuwave\Lighthouse\Select; use GraphQL\Language\AST\DirectiveNode; -use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\Node; +use GraphQL\Language\AST\NodeList; use GraphQL\Language\AST\UnionTypeDefinitionNode; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Str; @@ -12,24 +12,39 @@ use Nuwave\Lighthouse\Schema\AST\ASTHelper; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Support\AppVersion; +use Nuwave\Lighthouse\Support\Utils; class SelectHelper { - public const DirectivesRequiringLocalKey = ['hasOne', 'hasMany', 'count', 'morphOne', 'morphMany']; + public const DIRECTIVES_REQUIRING_LOCAL_KEY = ['hasOne', 'hasMany', 'count', 'morphOne', 'morphMany']; - public const DirectivesRequiringForeignKey = ['belongsTo']; + public const DIRECTIVES_REQUIRING_FOREIGN_KEY = ['belongsTo']; - public const DirectivesReturn = ['morphTo', 'morphToMany']; + public const DIRECTIVES_RETURN = ['morphTo', 'morphToMany']; - public const DirectivesIgnore = ['aggregate', 'withCount', 'belongsToMany']; + public const DIRECTIVES_IGNORE = ['aggregate', 'withCount', 'belongsToMany']; + + public const DIRECTIVES = [ + 'aggregate', + 'belongsTo', + 'belongsToMany', + 'count', + 'hasOne', + 'hasMany', + 'morphOne', + 'morphMany', + 'morphTo', + 'morphToMany', + 'withCount', + ]; /** * Given a field definition node, resolve info, and a model name, return the SQL columns that should be selected. * Accounts for relationships and to rename and select directives. * - * @param mixed[] $fieldSelection + * @param array $fieldSelection * - * @return string[] + * @return array * * @reference https://github.com/nuwave/lighthouse/pull/1626 */ @@ -37,9 +52,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect { $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); - /** @var DocumentAST $documentAST */ $documentAST = app(ASTBuilder::class)->documentAST(); + assert($documentAST instanceof DocumentAST); + if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { $returnTypeName = str_replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); } @@ -50,54 +66,53 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])]; } - /** @var iterable $fieldDefinitions */ $fieldDefinitions = $type->fields; + assert($fieldDefinitions instanceof NodeList); + /** @var Model $model */ $model = new $modelName(); + assert($model instanceof Model); + $selectColumns = []; foreach ($fieldSelection as $field) { $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); if ($fieldDefinition) { - $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey, self::DirectivesReturn, self::DirectivesIgnore); - - foreach ($directivesRequiringKeys as $directiveType) { + foreach (self::DIRECTIVES as $directiveType) { if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { /** @var DirectiveNode $directive */ $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); - if (in_array($directiveType, self::DirectivesReturn)) { + if (in_array($directiveType, self::DIRECTIVES_RETURN)) { return []; } - if (in_array($directiveType, self::DirectivesRequiringLocalKey)) { + if (in_array($directiveType, self::DIRECTIVES_REQUIRING_LOCAL_KEY)) { $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - if (AppVersion::below(5.7)) { - $relation = $model->{$relationName}(); - $rc = new \ReflectionClass($model->{$relationName}()); - $localKey = $rc->getProperty('localKey'); - $localKey->setAccessible(true); - array_push($selectColumns, $localKey->getValue($relation)); - } else { - array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); - } + $relation = $model->{$relationName}(); + + $localKey = AppVersion::below(5.7) + ? Utils::accessProtected($relation, 'localKey') + : $relation->getLocalKeyName(); + + $selectColumns[] = $localKey; } } - if (in_array($directiveType, self::DirectivesRequiringForeignKey)) { + if (in_array($directiveType, self::DIRECTIVES_REQUIRING_FOREIGN_KEY)) { $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); if (method_exists($model, $relationName)) { - if (AppVersion::below(5.8)) { - array_push($selectColumns, $model->{$relationName}()->getForeignKey()); - } else { - array_push($selectColumns, $model->{$relationName}()->getForeignKeyName()); - } + $foreignKey = AppVersion::below(5.8) + ? $model->{$relationName}()->getForeignKey() + : $model->{$relationName}()->getForeignKeyName(); + + $selectColumns[] = $foreignKey; } } @@ -105,30 +120,23 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } - if (ASTHelper::hasDirective($fieldDefinition, 'select')) { - // append selected columns in select directive to seletion - $directive = ASTHelper::directiveDefinition($fieldDefinition, 'select'); - - if ($directive) { - $selectFields = ASTHelper::directiveArgValue($directive, 'columns') ?? []; - $selectColumns = array_merge($selectColumns, $selectFields); - } - } elseif (ASTHelper::hasDirective($fieldDefinition, 'rename')) { + if ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'select')) { + // append selected columns in select directive to selection + $selectFields = ASTHelper::directiveArgValue($directive, 'columns', []); + $selectColumns = array_merge($selectColumns, $selectFields); + } elseif ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename')) { // append renamed attribute to selection - $directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename'); - - if ($directive) { - $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); - array_push($selectColumns, $renamedAttribute); - } + $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); + $selectColumns[] = $renamedAttribute; } else { // fallback to selecting the field name - array_push($selectColumns, $field); + $selectColumns[] = $field; } } } - $selectColumns = array_filter($selectColumns, function ($column) use ($model) { + /** @var array $selectColumns */ + $selectColumns = array_filter($selectColumns, function ($column) use ($model): bool { return ! $model->hasGetMutator($column) && ! method_exists($model, $column); }); From 667f222c50c1501fb8d917e5c1697b85102a1066 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 17:18:40 +0800 Subject: [PATCH 27/45] update select directive doc --- src/Schema/Directives/SelectDirective.php | 4 ++-- src/lighthouse.php | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Schema/Directives/SelectDirective.php b/src/Schema/Directives/SelectDirective.php index b0ab7cceb9..79259095dc 100644 --- a/src/Schema/Directives/SelectDirective.php +++ b/src/Schema/Directives/SelectDirective.php @@ -12,9 +12,9 @@ public static function definition(): string """ directive @select( """ - SQL columns names to pass to the Eloquent query builder + SQL column names to include in the `SELECT` part of the query. """ - columns: [String!] + columns: [String!]! ) on FIELD_DEFINITION GRAPHQL; } diff --git a/src/lighthouse.php b/src/lighthouse.php index 0b04e172b2..ace2574899 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -360,9 +360,8 @@ | Optimized Selects |-------------------------------------------------------------------------- | - | If set to true, Eloquent will only select the columns necessary to resolve - | a query. You must use the select directive to resolve advanced field dependencies - | on other columns. + | If set to true, Eloquent will only select the columns necessary to resolve a query. + | Use the @select directive to specify column dependencies of compound fields. | */ From 6b456f68f3ca2f6a6619b93f6504f03eb5afa5f8 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 17:19:09 +0800 Subject: [PATCH 28/45] only `EloquentBuilder` can be optimized and lint --- src/Pagination/PaginateDirective.php | 15 ++++++++++----- src/Schema/Directives/AllDirective.php | 6 +++--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Pagination/PaginateDirective.php b/src/Pagination/PaginateDirective.php index 17b64b4d6a..0f49564279 100644 --- a/src/Pagination/PaginateDirective.php +++ b/src/Pagination/PaginateDirective.php @@ -132,16 +132,21 @@ public function resolveField(FieldValue $fieldValue): FieldValue ); if (config('lighthouse.optimized_selects')) { - if (($query instanceof QueryBuilder || $query instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { - $fieldSelection = $resolveInfo->getFieldSelection(4); + if ($query instanceof EloquentBuilder) { + $fieldSelection = $resolveInfo->getFieldSelection(2); - if (Arr::has($fieldSelection, 'data') || Arr::has($fieldSelection, 'edges')) { - $fieldSelection = array_keys(Arr::has($fieldSelection, 'data') ? $fieldSelection['data'] : $fieldSelection['edges']['node']); + if (($hasData = Arr::has($fieldSelection, 'data')) || Arr::has($fieldSelection, 'edges')) { + $data = $hasData + ? $fieldSelection['data'] + : $fieldSelection['edges']['node']; + + /** @var array $fieldSelection */ + $fieldSelection = array_keys($data); $selectColumns = SelectHelper::getSelectColumns( $this->definitionNode, $fieldSelection, - $this->getModelClass() + get_class($query->getModel()) ); if (! empty($selectColumns)) { diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index afcddaeb27..045926be23 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -66,20 +66,20 @@ public function resolveField(FieldValue $fieldValue): FieldValue ); if (config('lighthouse.optimized_selects')) { - if (($builder instanceof QueryBuilder || $builder instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { + if ($builder instanceof EloquentBuilder) { $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); $selectColumns = SelectHelper::getSelectColumns( $this->definitionNode, $fieldSelection, - $this->getModelClass() + get_class($builder->getModel()) ); if (empty($selectColumns)) { return $builder->get(); } - $query = $builder instanceof EloquentBuilder ? $builder->getQuery() : $builder; + $query = $builder->getQuery(); if (null !== $query->columns) { $bindings = $query->getRawBindings(); From c3ff536c52c57bba83169286f1cbe458aaa9aeb9 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 17:29:36 +0800 Subject: [PATCH 29/45] use container --- src/Select/SelectHelper.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index ed8d9a4b3d..fc2537a60e 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -6,6 +6,7 @@ use GraphQL\Language\AST\Node; use GraphQL\Language\AST\NodeList; use GraphQL\Language\AST\UnionTypeDefinitionNode; +use Illuminate\Container\Container; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Str; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; @@ -52,7 +53,11 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect { $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); - $documentAST = app(ASTBuilder::class)->documentAST(); + $astBuilder = Container::getInstance()->make(ASTBuilder::class); + + assert($astBuilder instanceof ASTBuilder); + + $documentAST = $astBuilder->documentAST(); assert($documentAST instanceof DocumentAST); From 9ec7c71ef5c0ea4961532898e7411d8958afac12 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 18:46:21 +0800 Subject: [PATCH 30/45] update tests --- .../Schema/Directives/FindDirectiveTest.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/Integration/Schema/Directives/FindDirectiveTest.php b/tests/Integration/Schema/Directives/FindDirectiveTest.php index 2721ee37e5..869e241aed 100644 --- a/tests/Integration/Schema/Directives/FindDirectiveTest.php +++ b/tests/Integration/Schema/Directives/FindDirectiveTest.php @@ -200,9 +200,14 @@ public function testReturnsCustomAttributes(): void public function testReturnsOptimizedSelect(): void { $company = factory(Company::class)->create(); - $user = factory(User::class)->create([ - 'company_id' => $company->id, - ]); + + $user = factory(User::class)->make(); + + assert($user instanceof User); + + $user->company()->associate($company); + + $user->save(); $this->schema = ' type User { From ee2442f8e02cc8220512db7db461e9c411e7e0f4 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Tue, 22 Nov 2022 18:46:39 +0800 Subject: [PATCH 31/45] remove dead code --- src/Select/SelectHelper.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index fc2537a60e..4ba150fbd9 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -5,7 +5,6 @@ use GraphQL\Language\AST\DirectiveNode; use GraphQL\Language\AST\Node; use GraphQL\Language\AST\NodeList; -use GraphQL\Language\AST\UnionTypeDefinitionNode; use Illuminate\Container\Container; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Str; @@ -67,10 +66,6 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $type = $documentAST->types[$returnTypeName]; - if ($type instanceof UnionTypeDefinitionNode) { - $type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])]; - } - $fieldDefinitions = $type->fields; assert($fieldDefinitions instanceof NodeList); From 3221dd2a7657d6d7c309ace502b46244a15e4fad Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 2 Dec 2022 10:21:19 +0800 Subject: [PATCH 32/45] lint --- src/Select/SelectHelper.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 4ba150fbd9..09bdc8ad2c 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -22,8 +22,6 @@ class SelectHelper public const DIRECTIVES_RETURN = ['morphTo', 'morphToMany']; - public const DIRECTIVES_IGNORE = ['aggregate', 'withCount', 'belongsToMany']; - public const DIRECTIVES = [ 'aggregate', 'belongsTo', @@ -70,7 +68,6 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect assert($fieldDefinitions instanceof NodeList); - /** @var Model $model */ $model = new $modelName(); assert($model instanceof Model); @@ -82,9 +79,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if ($fieldDefinition) { foreach (self::DIRECTIVES as $directiveType) { - if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { - /** @var DirectiveNode $directive */ - $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); + if ($directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType)) { + assert($directive instanceof DirectiveNode); if (in_array($directiveType, self::DIRECTIVES_RETURN)) { return []; From 982f4c155ad2ea88fb84f085700ec6494dd39bfe Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Wed, 14 Dec 2022 14:16:56 +0800 Subject: [PATCH 33/45] Apply suggestions Co-authored-by: Benedikt Franke --- src/Select/SelectHelper.php | 4 ---- tests/Integration/Schema/Directives/FindDirectiveTest.php | 3 --- 2 files changed, 7 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 09bdc8ad2c..f6c867925c 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -51,11 +51,9 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); $astBuilder = Container::getInstance()->make(ASTBuilder::class); - assert($astBuilder instanceof ASTBuilder); $documentAST = $astBuilder->documentAST(); - assert($documentAST instanceof DocumentAST); if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { @@ -65,11 +63,9 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $type = $documentAST->types[$returnTypeName]; $fieldDefinitions = $type->fields; - assert($fieldDefinitions instanceof NodeList); $model = new $modelName(); - assert($model instanceof Model); $selectColumns = []; diff --git a/tests/Integration/Schema/Directives/FindDirectiveTest.php b/tests/Integration/Schema/Directives/FindDirectiveTest.php index 869e241aed..7f6d0ca668 100644 --- a/tests/Integration/Schema/Directives/FindDirectiveTest.php +++ b/tests/Integration/Schema/Directives/FindDirectiveTest.php @@ -202,11 +202,8 @@ public function testReturnsOptimizedSelect(): void $company = factory(Company::class)->create(); $user = factory(User::class)->make(); - assert($user instanceof User); - $user->company()->associate($company); - $user->save(); $this->schema = ' From 5cd23042b1a111ff59bd7fcb01dc9090acf37a53 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Thu, 15 Dec 2022 14:42:45 +0800 Subject: [PATCH 34/45] select without relation directives and fix morphTo --- src/Select/SelectHelper.php | 77 +++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index f6c867925c..eefbc854bf 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -7,6 +7,9 @@ use GraphQL\Language\AST\NodeList; use Illuminate\Container\Container; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\HasOneOrMany; +use Illuminate\Database\Eloquent\Relations\MorphTo; use Illuminate\Support\Str; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; @@ -20,7 +23,7 @@ class SelectHelper public const DIRECTIVES_REQUIRING_FOREIGN_KEY = ['belongsTo']; - public const DIRECTIVES_RETURN = ['morphTo', 'morphToMany']; + public const DIRECTIVES_REQUIRING_MORPH_KEY = ['morphTo']; public const DIRECTIVES = [ 'aggregate', @@ -78,33 +81,16 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if ($directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType)) { assert($directive instanceof DirectiveNode); - if (in_array($directiveType, self::DIRECTIVES_RETURN)) { - return []; - } - - if (in_array($directiveType, self::DIRECTIVES_REQUIRING_LOCAL_KEY)) { - $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); - - if (method_exists($model, $relationName)) { - $relation = $model->{$relationName}(); - - $localKey = AppVersion::below(5.7) - ? Utils::accessProtected($relation, 'localKey') - : $relation->getLocalKeyName(); - - $selectColumns[] = $localKey; - } - } - - if (in_array($directiveType, self::DIRECTIVES_REQUIRING_FOREIGN_KEY)) { - $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); - - if (method_exists($model, $relationName)) { - $foreignKey = AppVersion::below(5.8) - ? $model->{$relationName}()->getForeignKey() - : $model->{$relationName}()->getForeignKeyName(); - - $selectColumns[] = $foreignKey; + $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); + if (method_exists($model, $relationName)) { + $relation = $model->{$relationName}(); + if (in_array($directiveType, self::DIRECTIVES_REQUIRING_LOCAL_KEY)) { + $selectColumns[] = self::getLocalKey($relation); + } elseif (in_array($directiveType, self::DIRECTIVES_REQUIRING_FOREIGN_KEY)) { + $selectColumns[] = self::getForeignKey($relation); + } elseif (in_array($directiveType, self::DIRECTIVES_REQUIRING_MORPH_KEY)) { + $selectColumns[] = self::getForeignKey($relation); + $selectColumns[] = $relation->getMorphType(); } } @@ -120,6 +106,16 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); $selectColumns[] = $renamedAttribute; + } elseif ($model->isRelation($field) || $model->relationLoaded($field)) { + $relation = $model->{$field}(); + if ($relation instanceof MorphTo) { + $selectColumns[] = self::getForeignKey($relation); + $selectColumns[] = $relation->getMorphType(); + } elseif ($relation instanceof BelongsTo) { + $selectColumns[] = self::getForeignKey($relation); + } elseif ($relation instanceof HasOneOrMany) { + $selectColumns[] = self::getLocalKey($relation); + } } else { // fallback to selecting the field name $selectColumns[] = $field; @@ -127,11 +123,26 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } - /** @var array $selectColumns */ - $selectColumns = array_filter($selectColumns, function ($column) use ($model): bool { - return ! $model->hasGetMutator($column) && ! method_exists($model, $column); - }); - return array_unique($selectColumns); } + + /** + * Get the local key. + */ + protected static function getLocalKey(HasOneOrMany $relation): string + { + return AppVersion::below(5.7) + ? Utils::accessProtected($relation, 'localKey') + : $relation->getLocalKeyName(); + } + + /** + * Get the foreign key. + */ + protected static function getForeignKey(BelongsTo $relation): string + { + return AppVersion::below(5.8) + ? $relation->getForeignKey() // @phpstan-ignore-line only be executed on Laravel < 5.8 + : $relation->getForeignKeyName(); + } } From e5b071526316fd1d1da21afe36d21ba9471f9e83 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Thu, 15 Dec 2022 17:21:43 +0800 Subject: [PATCH 35/45] cleanup --- src/Schema/Directives/AllDirective.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index 045926be23..be25de842f 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -90,9 +90,7 @@ public function resolveField(FieldValue $fieldValue): FieldValue $builder = $builder->select(array_unique(array_merge($selectColumns, $expressions))); - foreach ($bindings as $type => $binding) { - $builder = $builder->addBinding($binding, $type); - } + $builder = $builder->addBinding($bindings['select'], 'select'); } else { $builder = $builder->select($selectColumns); } From 2d9f3860cbc6f6f733f67c124589546010e3ed40 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 13:43:01 +0800 Subject: [PATCH 36/45] fix paginate schema correlates --- src/Pagination/PaginateDirective.php | 16 +++++++++++++--- src/Pagination/PaginationManipulator.php | 13 +++++++++++++ src/Select/SelectHelper.php | 10 ++++------ .../Schema/Directives/BelongsToDirectiveTest.php | 4 ++++ 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/Pagination/PaginateDirective.php b/src/Pagination/PaginateDirective.php index 0f49564279..a725257729 100644 --- a/src/Pagination/PaginateDirective.php +++ b/src/Pagination/PaginateDirective.php @@ -143,14 +143,24 @@ public function resolveField(FieldValue $fieldValue): FieldValue /** @var array $fieldSelection */ $fieldSelection = array_keys($data); + $model = $query->getModel(); + $selectColumns = SelectHelper::getSelectColumns( $this->definitionNode, $fieldSelection, - get_class($query->getModel()) + get_class($model) ); - if (! empty($selectColumns)) { - $query = $query->select($selectColumns); + $query = $query->select($selectColumns); + + /** @var string|string[] $keyName */ + $keyName = $model->getKeyName(); + if (is_string($keyName)) { + $keyName = [$keyName]; + } + + foreach ($keyName as $name) { + $query->orderBy($name); } } } diff --git a/src/Pagination/PaginationManipulator.php b/src/Pagination/PaginationManipulator.php index 6109f43952..5c59d7eb5f 100644 --- a/src/Pagination/PaginationManipulator.php +++ b/src/Pagination/PaginationManipulator.php @@ -4,6 +4,7 @@ use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\NamedTypeNode; +use GraphQL\Language\AST\Node; use GraphQL\Language\AST\NonNullTypeNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use GraphQL\Language\AST\TypeNode; @@ -15,6 +16,8 @@ use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\Directives\ModelDirective; +use function Safe\preg_replace; + class PaginationManipulator { /** @@ -273,4 +276,14 @@ protected function paginationResultType(string $typeName): TypeNode return $typeNode; } + + public static function getReturnTypeName(Node $fieldDefinition): ?string + { + if (! ASTHelper::directiveDefinition($fieldDefinition, 'paginate')) { + return null; + } + $fieldTypeName = ASTHelper::getUnderlyingTypeName($fieldDefinition); + + return preg_replace('/(Connection|SimplePaginator|Paginator)$/', '', $fieldTypeName); + } } diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index eefbc854bf..7b0642bcea 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -10,7 +10,7 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\HasOneOrMany; use Illuminate\Database\Eloquent\Relations\MorphTo; -use Illuminate\Support\Str; +use Nuwave\Lighthouse\Pagination\PaginationManipulator; use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\ASTHelper; use Nuwave\Lighthouse\Schema\AST\DocumentAST; @@ -51,7 +51,9 @@ class SelectHelper */ public static function getSelectColumns(Node $definitionNode, array $fieldSelection, string $modelName): array { - $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); + if (! ($returnTypeName = PaginationManipulator::getReturnTypeName($definitionNode))) { + $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); + } $astBuilder = Container::getInstance()->make(ASTBuilder::class); assert($astBuilder instanceof ASTBuilder); @@ -59,10 +61,6 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $documentAST = $astBuilder->documentAST(); assert($documentAST instanceof DocumentAST); - if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { - $returnTypeName = str_replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); - } - $type = $documentAST->types[$returnTypeName]; $fieldDefinitions = $type->fields; diff --git a/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php b/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php index 63d024c291..8140233681 100644 --- a/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php +++ b/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php @@ -179,6 +179,10 @@ public function testResolveBelongsToRelationshipWhenMainModelHasCompositePrimary } '; + $products = $products + ->sortBy(function ($product) {return $product->barcode; }) + ->values(); + $this->graphQL(/** @lang GraphQL */ ' { products(first: 2) { From aafd02c7a0caa87ae21c532ebacb9d6ec368214d Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 14:53:21 +0800 Subject: [PATCH 37/45] order by primary key by default --- src/Schema/Directives/AllDirective.php | 18 +++++++++++++----- src/Schema/Directives/FindDirective.php | 14 ++++++++++++-- src/Select/SelectHelper.php | 2 ++ .../Schema/Directives/HasManyDirectiveTest.php | 4 ++-- .../Schema/Directives/LimitDirectiveTest.php | 4 ++-- .../WhereConditionsDirectiveTest.php | 8 ++++---- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index be25de842f..29d6ad96a1 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -69,16 +69,14 @@ public function resolveField(FieldValue $fieldValue): FieldValue if ($builder instanceof EloquentBuilder) { $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); + $model = $builder->getModel(); + $selectColumns = SelectHelper::getSelectColumns( $this->definitionNode, $fieldSelection, - get_class($builder->getModel()) + get_class($model) ); - if (empty($selectColumns)) { - return $builder->get(); - } - $query = $builder->getQuery(); if (null !== $query->columns) { @@ -94,6 +92,16 @@ public function resolveField(FieldValue $fieldValue): FieldValue } else { $builder = $builder->select($selectColumns); } + + /** @var string|string[] $keyName */ + $keyName = $model->getKeyName(); + if (is_string($keyName)) { + $keyName = [$keyName]; + } + + foreach ($keyName as $name) { + $query->orderBy($name); + } } } diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index f110388704..ea379e2b04 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -52,8 +52,18 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->getModelClass() ); - if (! empty($selectColumns)) { - $builder = $builder->select($selectColumns); + $builder = $builder->select($selectColumns); + + $model = $builder->getModel(); + + /** @var string|string[] $keyName */ + $keyName = $model->getKeyName(); + if (is_string($keyName)) { + $keyName = [$keyName]; + } + + foreach ($keyName as $name) { + $builder->orderBy($name); } } diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 7b0642bcea..1616ef1a17 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -89,6 +89,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } elseif (in_array($directiveType, self::DIRECTIVES_REQUIRING_MORPH_KEY)) { $selectColumns[] = self::getForeignKey($relation); $selectColumns[] = $relation->getMorphType(); + } else { + $selectColumns[] = $model->getKeyName(); } } diff --git a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php index 565089b68b..c5b87621f3 100644 --- a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php @@ -146,7 +146,7 @@ public function testQueryHasManyWithConditionInDifferentAliases(): void } type Query { - users: [User!]! @all @orderBy(column: "id") + users: [User!]! @all } '; @@ -222,7 +222,7 @@ public function testQueryPaginatedHasManyWithConditionInDifferentAliases(): void } type Query { - users: [User!]! @all @orderBy(column: "id") + users: [User!]! @all } '; diff --git a/tests/Integration/Schema/Directives/LimitDirectiveTest.php b/tests/Integration/Schema/Directives/LimitDirectiveTest.php index 86693d3701..0c8e676a30 100644 --- a/tests/Integration/Schema/Directives/LimitDirectiveTest.php +++ b/tests/Integration/Schema/Directives/LimitDirectiveTest.php @@ -104,7 +104,7 @@ public function testLimitsRelations(): void } type Query { - users: [User!]! @all @orderBy(column: "id") + users: [User!]! @all } '; @@ -148,7 +148,7 @@ public function testLimitsWithCache(): void } type Query { - user: [User!]! @all @orderBy(column: "id") + user: [User!]! @all } '; diff --git a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php index 7f76910908..acdea07f08 100644 --- a/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php +++ b/tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php @@ -30,14 +30,14 @@ final class WhereConditionsDirectiveTest extends DBTestCase } type Query { - posts(where: _ @whereConditions): [Post!]! @all @orderBy(column: "id") - users(where: _ @whereConditions): [User!]! @all @orderBy(column: "id") + posts(where: _ @whereConditions): [Post!]! @all + users(where: _ @whereConditions): [User!]! @all whitelistedColumns( where: _ @whereConditions(columns: ["id", "camelCase"]) - ): [User!]! @all @orderBy(column: "id") + ): [User!]! @all enumColumns( where: _ @whereConditions(columnsEnum: "UserColumn") - ): [User!]! @all @orderBy(column: "id") + ): [User!]! @all } enum UserColumn { From f0c484656c1845545b23cc25bf1e9468083ffd3b Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 15:07:52 +0800 Subject: [PATCH 38/45] throw errors --- src/Pagination/PaginateDirective.php | 5 +++++ src/Schema/Directives/AllDirective.php | 5 +++++ src/Schema/Directives/FindDirective.php | 4 ++++ 3 files changed, 14 insertions(+) diff --git a/src/Pagination/PaginateDirective.php b/src/Pagination/PaginateDirective.php index a725257729..8183c7d9ec 100644 --- a/src/Pagination/PaginateDirective.php +++ b/src/Pagination/PaginateDirective.php @@ -2,6 +2,7 @@ namespace Nuwave\Lighthouse\Pagination; +use GraphQL\Error\Error; use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use GraphQL\Type\Definition\ResolveInfo; @@ -151,6 +152,10 @@ public function resolveField(FieldValue $fieldValue): FieldValue get_class($model) ); + if (empty($selectColumns)) { + throw new Error('The select column is empty.'); + } + $query = $query->select($selectColumns); /** @var string|string[] $keyName */ diff --git a/src/Schema/Directives/AllDirective.php b/src/Schema/Directives/AllDirective.php index 29d6ad96a1..4f3e3e81ba 100644 --- a/src/Schema/Directives/AllDirective.php +++ b/src/Schema/Directives/AllDirective.php @@ -2,6 +2,7 @@ namespace Nuwave\Lighthouse\Schema\Directives; +use GraphQL\Error\Error; use GraphQL\Type\Definition\ResolveInfo; use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Relations\Relation; @@ -77,6 +78,10 @@ public function resolveField(FieldValue $fieldValue): FieldValue get_class($model) ); + if (empty($selectColumns)) { + throw new Error('The select column is empty.'); + } + $query = $builder->getQuery(); if (null !== $query->columns) { diff --git a/src/Schema/Directives/FindDirective.php b/src/Schema/Directives/FindDirective.php index ea379e2b04..4a71fbbfa9 100644 --- a/src/Schema/Directives/FindDirective.php +++ b/src/Schema/Directives/FindDirective.php @@ -52,6 +52,10 @@ public function resolveField(FieldValue $fieldValue): FieldValue $this->getModelClass() ); + if (empty($selectColumns)) { + throw new Error('The select column is empty.'); + } + $builder = $builder->select($selectColumns); $model = $builder->getModel(); From 5171228e9e88f76b7d3b4c5d657a5e1d7a1a4d8e Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 15:18:56 +0800 Subject: [PATCH 39/45] set default config to false --- src/lighthouse.php | 2 +- tests/TestCase.php | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lighthouse.php b/src/lighthouse.php index ace2574899..7d267f7df2 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -365,7 +365,7 @@ | */ - 'optimized_selects' => true, + 'optimized_selects' => false, /* |-------------------------------------------------------------------------- diff --git a/tests/TestCase.php b/tests/TestCase.php index afaab6dac8..701a719a14 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -176,6 +176,8 @@ protected function getEnvironmentSetUp($app): void $config->set('lighthouse.cache.enable', false); $config->set('lighthouse.unbox_bensampo_enum_enum_instances', true); + + $config->set('lighthouse.optimized_selects', true); } /** From 1d570bf7b1c5f1774aad7a14467ae0a35873fd30 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 16:14:57 +0800 Subject: [PATCH 40/45] fix isRelation for laravel version below 8.0 --- src/Select/SelectHelper.php | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 1616ef1a17..54369f862e 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -106,7 +106,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); $selectColumns[] = $renamedAttribute; - } elseif ($model->isRelation($field) || $model->relationLoaded($field)) { + } elseif (self::isRelation($model, $field)) { $relation = $model->{$field}(); if ($relation instanceof MorphTo) { $selectColumns[] = self::getForeignKey($relation); @@ -145,4 +145,18 @@ protected static function getForeignKey(BelongsTo $relation): string ? $relation->getForeignKey() // @phpstan-ignore-line only be executed on Laravel < 5.8 : $relation->getForeignKeyName(); } + + protected static function isRelation(Model $model, string $field): bool + { + if (AppVersion::below(7.0)) { + return method_exists($model, $field) || $model->relationLoaded($field); + } + if (AppVersion::below(8.0)) { + return method_exists($model, $field) + || (Utils::accessProtected($model, 'relationResolvers')[get_class($model)][$field] ?? false) + || $model->relationLoaded($field); + } + + return $model->isRelation($field) || $model->relationLoaded($field); + } } From f385f5d6446f62e420864e69950869746af3e7c2 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 17:03:19 +0800 Subject: [PATCH 41/45] fix relations --- src/Select/SelectHelper.php | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 54369f862e..f453df3d36 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -106,7 +106,7 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); $selectColumns[] = $renamedAttribute; - } elseif (self::isRelation($model, $field)) { + } elseif (method_exists($model, $field)) { $relation = $model->{$field}(); if ($relation instanceof MorphTo) { $selectColumns[] = self::getForeignKey($relation); @@ -145,18 +145,4 @@ protected static function getForeignKey(BelongsTo $relation): string ? $relation->getForeignKey() // @phpstan-ignore-line only be executed on Laravel < 5.8 : $relation->getForeignKeyName(); } - - protected static function isRelation(Model $model, string $field): bool - { - if (AppVersion::below(7.0)) { - return method_exists($model, $field) || $model->relationLoaded($field); - } - if (AppVersion::below(8.0)) { - return method_exists($model, $field) - || (Utils::accessProtected($model, 'relationResolvers')[get_class($model)][$field] ?? false) - || $model->relationLoaded($field); - } - - return $model->isRelation($field) || $model->relationLoaded($field); - } } From 2aa0e4a18ae3335460d8448dcac3d6569f2e83a8 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 22:17:56 +0800 Subject: [PATCH 42/45] fix tests --- src/Select/SelectHelper.php | 15 +++++++++++---- .../Schema/Directives/HasManyDirectiveTest.php | 5 ++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index f453df3d36..97edee7bec 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -72,9 +72,18 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $selectColumns = []; foreach ($fieldSelection as $field) { + $foundSelect = false; $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); if ($fieldDefinition) { + // the priority of select directive is highest + if ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'select')) { + // append selected columns in select directive to selection + $selectFields = ASTHelper::directiveArgValue($directive, 'columns', []); + $selectColumns = array_merge($selectColumns, $selectFields); + $foundSelect = true; + } + foreach (self::DIRECTIVES as $directiveType) { if ($directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType)) { assert($directive instanceof DirectiveNode); @@ -98,10 +107,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect } } - if ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'select')) { - // append selected columns in select directive to selection - $selectFields = ASTHelper::directiveArgValue($directive, 'columns', []); - $selectColumns = array_merge($selectColumns, $selectFields); + if ($foundSelect) { + continue; } elseif ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename')) { // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); diff --git a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php index 6d97d564d4..5ff39879d0 100644 --- a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php @@ -302,7 +302,9 @@ public function testQueryPaginatedHasManyWithNonUniqueForeignKey(): void $this->schema /** @lang GraphQL */ = ' type Post { - roles: [RoleUser!]! @hasMany(relation: "roles", type: PAGINATOR, defaultCount: 10) + roles: [RoleUser!]! + @hasMany(relation: "roles", type: PAGINATOR, defaultCount: 10) + @select(columns: ["id"]) } type RoleUser { @@ -334,6 +336,7 @@ public function testQueryPaginatedHasManyWithNonUniqueForeignKey(): void } $this->assertCount(3, $user->roles); + self::trackQueries(); $this ->graphQL(/** @lang GraphQL */ ' query { From f4fb5d8a0a4fcc8bc5f6faee98680f6f95bc4768 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 14:19:33 +0000 Subject: [PATCH 43/45] Apply php-cs-fixer changes --- src/Select/SelectHelper.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index 97edee7bec..dc1ccc7876 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -109,7 +109,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect if ($foundSelect) { continue; - } elseif ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename')) { + } + if ($directive = ASTHelper::directiveDefinition($fieldDefinition, 'rename')) { // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); $selectColumns[] = $renamedAttribute; From 24ee8d495465d3f6d18521b213e93bf9dc6a1195 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Fri, 16 Dec 2022 22:24:24 +0800 Subject: [PATCH 44/45] cleanup --- tests/Integration/Schema/Directives/HasManyDirectiveTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php index 5ff39879d0..59cd794df8 100644 --- a/tests/Integration/Schema/Directives/HasManyDirectiveTest.php +++ b/tests/Integration/Schema/Directives/HasManyDirectiveTest.php @@ -336,7 +336,6 @@ public function testQueryPaginatedHasManyWithNonUniqueForeignKey(): void } $this->assertCount(3, $user->roles); - self::trackQueries(); $this ->graphQL(/** @lang GraphQL */ ' query { From 06357334ff389b6789f409e5ceab49367e8afdb3 Mon Sep 17 00:00:00 2001 From: Lyrisbee Date: Mon, 19 Dec 2022 09:30:23 +0800 Subject: [PATCH 45/45] set primary key if no select column --- src/Select/SelectHelper.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Select/SelectHelper.php b/src/Select/SelectHelper.php index dc1ccc7876..e2ffd1000c 100644 --- a/src/Select/SelectHelper.php +++ b/src/Select/SelectHelper.php @@ -114,8 +114,10 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect // append renamed attribute to selection $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); $selectColumns[] = $renamedAttribute; - } elseif (method_exists($model, $field)) { - $relation = $model->{$field}(); + } elseif (($directive = ASTHelper::directiveDefinition($fieldDefinition, 'method')) || method_exists($model, $field)) { + $relation = null !== $directive + ? $model->{ASTHelper::directiveArgValue($directive, 'name')}() + : $model->{$field}(); if ($relation instanceof MorphTo) { $selectColumns[] = self::getForeignKey($relation); $selectColumns[] = $relation->getMorphType(); @@ -123,6 +125,8 @@ public static function getSelectColumns(Node $definitionNode, array $fieldSelect $selectColumns[] = self::getForeignKey($relation); } elseif ($relation instanceof HasOneOrMany) { $selectColumns[] = self::getLocalKey($relation); + } else { + $selectColumns[] = $model->getKeyName(); } } else { // fallback to selecting the field name