-
-
Notifications
You must be signed in to change notification settings - Fork 468
Support optimized select for top-level query #2235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 22 commits
2f13537
290e464
03d4124
cacc576
d49f0ec
6f1a04a
ed0bee9
3ef48bd
23237a1
1b73b78
bcf8f8c
6ca50ae
c9a7343
7e75f24
45d12f1
cd30eea
fa6f99a
7bc7c76
f983dec
25e0ecc
abf875f
61ed5cd
768b28b
6e21e28
0990919
deabb67
667f222
6b456f6
c3ff536
9ec7c71
ee2442f
3221dd2
982f4c1
5cd2304
e5b0715
2d9f386
aafd02c
f0c4846
5171228
1d570bf
f385f5d
b170698
2aa0e4a
f4fb5d8
24ee8d4
0635733
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,26 @@ public function resolveField(FieldValue $fieldValue): FieldValue | |
| $this->directiveArgValue('scopes', []) | ||
| ); | ||
|
|
||
| if (config('lighthouse.optimized_selects')) { | ||
| if (($query instanceof QueryBuilder || $query instanceof EloquentBuilder) && ! $this->directiveHasArgument('builder')) { | ||
| $fieldSelection = $resolveInfo->getFieldSelection(4); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| if (Arr::has($fieldSelection, 'data') || Arr::has($fieldSelection, 'edges')) { | ||
| $fieldSelection = array_keys(Arr::has($fieldSelection, 'data') ? $fieldSelection['data'] : $fieldSelection['edges']['node']); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| $selectColumns = SelectHelper::getSelectColumns( | ||
| $this->definitionNode, | ||
| $fieldSelection, | ||
| $this->getModelClass() | ||
| ); | ||
|
|
||
| if (! empty($selectColumns)) { | ||
| $query = $query->select($selectColumns); | ||
| } | ||
|
Comment on lines
+188
to
+196
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be unrelated to optimizing the select. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| $paginationArgs = PaginationArgs::extractArgs($args, $this->paginationType(), $this->paginateMaxCount()); | ||
|
|
||
| $paginationArgs->type = $this->optimalPaginationType($resolveInfo); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,11 @@ | |
| 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; | ||
| use Nuwave\Lighthouse\Select\SelectHelper; | ||
| use Nuwave\Lighthouse\Support\Contracts\FieldResolver; | ||
| use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; | ||
|
|
||
|
|
@@ -56,13 +58,48 @@ public function resolveField(FieldValue $fieldValue): FieldValue | |
| $query = $this->getModelClass()::query(); | ||
| } | ||
|
|
||
| return $resolveInfo | ||
| $builder = $resolveInfo | ||
| ->argumentSet | ||
| ->enhanceBuilder( | ||
| $query, | ||
| $this->directiveArgValue('scopes', []) | ||
| ) | ||
| ->get(); | ||
| ); | ||
|
|
||
| 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() | ||
| ); | ||
|
|
||
| if (empty($selectColumns)) { | ||
| return $builder->get(); | ||
| } | ||
|
Comment on lines
+78
to
+89
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this throw?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In some cases, type User {
posts: [Post!]! #@hasMany
}
type Post {
id: ID!
}
type Query {
users: [User!]! @all
}{
users {
posts {
id
}
}
}Use the above schema as an example. The The optimized select is an improvement feature, even if this function is not working normally due to insufficient directives, it shouldn’t break original queries.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this example, the query would have to select the users In that case, I believe this check is insufficient in order to not break the query. If the client queries an additional field from |
||
|
|
||
| $query = $builder instanceof EloquentBuilder ? $builder->getQuery() : $builder; | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| if (null !== $query->columns) { | ||
| $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); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only want to replace From {
users(
orderBy: [
{
tasks: { aggregate: COUNT }
order: ASC
}
]
) {
id
}
}query dump: columns: array:2 [
0 => "users.*"
1 => Illuminate\Database\Query\Expression^ {#7717
#value: "(select count(*) from `tasks` where `users`.`id` = `tasks`.`user_id` and `tasks`.`deleted_at` is null and `name` != ?) as `tasks_count`"
}
],
bindings: array:9 [
"select" => array:1 [
0 => "cleaning"
]
"from" => []
"join" => []
"where" => []
"groupBy" => []
"having" => []
"order" => []
"union" => []
"unionOrder" => []
]
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not understand why you need to first get and then re-set the bindings. As far as I can tell,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean it only needs to re-set the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these 3 lines and
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The above schema is the case. Before SELECT
`users.*`,
(SELECT
COUNT(*)
FROM
`tasks`
WHERE
`users`.`id` = `tasks`.`user_id`
AND `tasks`.`deleted_at` IS NULL
AND `name` != ?
) AS `tasks_count`
FROM
`users`
ORDER BY `tasks_count` ASC
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable then. Saves unnecessary work and is more explicit about what is happening and why. |
||
| } else { | ||
| $builder = $builder->select($selectColumns); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $builder->get(); | ||
| }); | ||
|
|
||
| return $fieldValue; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| use GraphQL\Type\Definition\ResolveInfo; | ||
| use Illuminate\Database\Eloquent\Model; | ||
| 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 +36,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 (config('lighthouse.optimized_selects')) { | ||
| $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); | ||
|
|
||
| $selectColumns = SelectHelper::getSelectColumns( | ||
| $this->definitionNode, | ||
| $fieldSelection, | ||
| $this->getModelClass() | ||
| ); | ||
|
|
||
| if (! empty($selectColumns)) { | ||
| $builder = $builder->select($selectColumns); | ||
| } | ||
|
Comment on lines
+61
to
+70
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary |
||
| } | ||
|
|
||
| $results = $builder->get(); | ||
|
|
||
| if ($results->count() > 1) { | ||
| throw new Error('The query returned more than one result.'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <?php | ||
|
|
||
| namespace Nuwave\Lighthouse\Schema\Directives; | ||
|
|
||
| class SelectDirective extends BaseDirective | ||
| { | ||
| public static function definition(): string | ||
| { | ||
| return /** @lang GraphQL */ <<<'GRAPHQL' | ||
| """ | ||
| Specify the SQL column dependencies of this field. | ||
| """ | ||
| directive @select( | ||
| """ | ||
| SQL columns names to pass to the Eloquent query builder | ||
| """ | ||
| columns: [String!] | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| ) on FIELD_DEFINITION | ||
| GRAPHQL; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| <?php | ||
|
|
||
| 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; | ||
| use Nuwave\Lighthouse\Support\AppVersion; | ||
|
|
||
| class SelectHelper | ||
| { | ||
| public const DirectivesRequiringLocalKey = ['hasOne', 'hasMany', 'count', 'morphOne', 'morphMany']; | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| public const DirectivesRequiringForeignKey = ['belongsTo']; | ||
|
|
||
| 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
does not match the signature |
||
| * Accounts for relationships and to rename and select directives. | ||
| * | ||
| * @param mixed[] $fieldSelection | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| * | ||
| * @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 DocumentAST $documentAST */ | ||
| $documentAST = app(ASTBuilder::class)->documentAST(); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { | ||
| $returnTypeName = str_replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); | ||
| } | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| $type = $documentAST->types[$returnTypeName]; | ||
|
|
||
| if ($type instanceof UnionTypeDefinitionNode) { | ||
| $type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])]; | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** @var iterable<FieldDefinitionNode> $fieldDefinitions */ | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| $fieldDefinitions = $type->fields; | ||
|
|
||
| /** @var Model $model */ | ||
| $model = new $modelName(); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| $selectColumns = []; | ||
|
|
||
| foreach ($fieldSelection as $field) { | ||
| $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); | ||
|
|
||
| if ($fieldDefinition) { | ||
| $directivesRequiringKeys = array_merge(self::DirectivesRequiringLocalKey, self::DirectivesRequiringForeignKey, self::DirectivesReturn, self::DirectivesIgnore); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| foreach ($directivesRequiringKeys as $directiveType) { | ||
| if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) { | ||
| /** @var DirectiveNode $directive */ | ||
| $directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
|
|
||
| if (in_array($directiveType, self::DirectivesReturn)) { | ||
| return []; | ||
| } | ||
|
|
||
| if (in_array($directiveType, self::DirectivesRequiringLocalKey)) { | ||
| $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)); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| array_push($selectColumns, $model->{$relationName}()->getLocalKeyName()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (in_array($directiveType, self::DirectivesRequiringForeignKey)) { | ||
| $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field); | ||
|
|
||
| if (method_exists($model, $relationName)) { | ||
| if (AppVersion::below(5.8)) { | ||
| array_push($selectColumns, $model->{$relationName}()->getForeignKey()); | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| } 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') ?? []; | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| $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); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $selectColumns = array_filter($selectColumns, function ($column) use ($model) { | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| return ! $model->hasGetMutator($column) && ! method_exists($model, $column); | ||
|
spawnia marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| return array_unique($selectColumns); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Lyrisbee marked this conversation as resolved.
Outdated
|
||
| | | ||
| */ | ||
|
|
||
| 'optimized_selects' => true, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider starting this setting with |
||
|
|
||
| /* | ||
| |-------------------------------------------------------------------------- | ||
| | Shortcut Foreign Key Selection | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.