-
-
Notifications
You must be signed in to change notification settings - Fork 463
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 32 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 |
|---|---|---|
|
|
@@ -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 EloquentBuilder) { | ||
| $fieldSelection = array_keys($resolveInfo->getFieldSelection(1)); | ||
|
|
||
| $selectColumns = SelectHelper::getSelectColumns( | ||
| $this->definitionNode, | ||
| $fieldSelection, | ||
| get_class($builder->getModel()) | ||
| ); | ||
|
|
||
| if (empty($selectColumns)) { | ||
| return $builder->get(); | ||
| } | ||
|
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->getQuery(); | ||
|
|
||
| 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); | ||
| } | ||
|
||
| } else { | ||
| $builder = $builder->select($selectColumns); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $builder->get(); | ||
| }); | ||
|
|
||
| return $fieldValue; | ||
|
|
||
| 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 column names to include in the `SELECT` part of the query. | ||
| """ | ||
| columns: [String!]! | ||
| ) on FIELD_DEFINITION | ||
| GRAPHQL; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| <?php | ||
|
|
||
| namespace Nuwave\Lighthouse\Select; | ||
|
|
||
| use GraphQL\Language\AST\DirectiveNode; | ||
| use GraphQL\Language\AST\Node; | ||
| use GraphQL\Language\AST\NodeList; | ||
| use Illuminate\Container\Container; | ||
| 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; | ||
| use Nuwave\Lighthouse\Support\Utils; | ||
|
|
||
| class SelectHelper | ||
| { | ||
| public const DIRECTIVES_REQUIRING_LOCAL_KEY = ['hasOne', 'hasMany', 'count', 'morphOne', 'morphMany']; | ||
|
|
||
| public const DIRECTIVES_REQUIRING_FOREIGN_KEY = ['belongsTo']; | ||
|
|
||
| public const DIRECTIVES_RETURN = ['morphTo', 'morphToMany']; | ||
|
|
||
| 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. | ||
|
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 array<int, string> $fieldSelection | ||
| * | ||
| * @return array<int, string> | ||
| * | ||
| * @reference https://github.com/nuwave/lighthouse/pull/1626 | ||
| */ | ||
| public static function getSelectColumns(Node $definitionNode, array $fieldSelection, string $modelName): array | ||
| { | ||
| $returnTypeName = ASTHelper::getUnderlyingTypeName($definitionNode); | ||
|
|
||
| $astBuilder = Container::getInstance()->make(ASTBuilder::class); | ||
|
|
||
| assert($astBuilder instanceof ASTBuilder); | ||
Lyrisbee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| $documentAST = $astBuilder->documentAST(); | ||
|
|
||
| assert($documentAST instanceof DocumentAST); | ||
Lyrisbee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (Str::contains($returnTypeName, ['SimplePaginator', 'Paginator'])) { | ||
| $returnTypeName = str_replace(['SimplePaginator', 'Paginator'], '', $returnTypeName); | ||
| } | ||
Lyrisbee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| $type = $documentAST->types[$returnTypeName]; | ||
|
|
||
| $fieldDefinitions = $type->fields; | ||
|
|
||
| assert($fieldDefinitions instanceof NodeList); | ||
Lyrisbee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| $model = new $modelName(); | ||
|
|
||
| assert($model instanceof Model); | ||
Lyrisbee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| $selectColumns = []; | ||
|
|
||
| foreach ($fieldSelection as $field) { | ||
| $fieldDefinition = ASTHelper::firstByName($fieldDefinitions, $field); | ||
|
|
||
| if ($fieldDefinition) { | ||
| foreach (self::DIRECTIVES as $directiveType) { | ||
| if ($directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType)) { | ||
| assert($directive instanceof DirectiveNode); | ||
|
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 should not be necessary |
||
|
|
||
| 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; | ||
| } | ||
| } | ||
|
|
||
| continue 2; | ||
| } | ||
| } | ||
|
|
||
| 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 | ||
| $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute'); | ||
| $selectColumns[] = $renamedAttribute; | ||
|
Comment on lines
+115
to
+116
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. There is no guarantee that the renamed attribute is actually a column, it could just as well be a virtual property. |
||
| } else { | ||
| // fallback to selecting the field name | ||
| $selectColumns[] = $field; | ||
|
Comment on lines
+132
to
+133
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 just goes wrong for the very simple case of there being a custom getter for the field - or any custom directive that we can not possibly know about. Consider the following: type MyModel {
foo: Int @someCustomDirectiveThatWeDoNotKnowAbout
bar: ID @field(resolver: "SomeCustomClass@andAMethodWeCanNotPossiblyLookInto")
}I don't think any amount of magic can help us here, any approach of trying to determine columns without explicit configuration is just fundamentally flawed. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** @var array<int, string> $selectColumns */ | ||
| $selectColumns = array_filter($selectColumns, function ($column) use ($model): bool { | ||
| return ! $model->hasGetMutator($column) && ! method_exists($model, $column); | ||
spawnia marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }); | ||
Lyrisbee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| return array_unique($selectColumns); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -355,6 +355,18 @@ | |
|
|
||
| 'batchload_relations' => true, | ||
|
|
||
| /* | ||
| |-------------------------------------------------------------------------- | ||
| | Optimized Selects | ||
| |-------------------------------------------------------------------------- | ||
| | | ||
| | 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. | ||
|
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. Due to the problems I outlined in type Foo @select {
id: ID @select(columns: ["id"])
}
|
||
| | | ||
| */ | ||
|
|
||
| 'optimized_selects' => true, | ||
|
||
|
|
||
| /* | ||
| |-------------------------------------------------------------------------- | ||
| | Shortcut Foreign Key Selection | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can derive which field to check from
$this->directiveArgValue('type'), no need to check both.