Skip to content
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

Support optimized select for top-level query #2235

Draft
wants to merge 46 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
2f13537
optimize select
Lyrisbee Nov 11, 2022
290e464
add reference
Lyrisbee Nov 11, 2022
03d4124
optimize find directive and fix tests
Lyrisbee Nov 11, 2022
cacc576
add select directvie
Lyrisbee Nov 13, 2022
d49f0ec
move select directive
Lyrisbee Nov 13, 2022
6f1a04a
add optimize select config
Lyrisbee Nov 13, 2022
ed0bee9
filter non-array document
Lyrisbee Nov 15, 2022
3ef48bd
remove Schema request
Lyrisbee Nov 15, 2022
23237a1
handle RelationOrderBy problems
Lyrisbee Nov 16, 2022
1b73b78
lint
Lyrisbee Nov 16, 2022
bcf8f8c
add tests
Lyrisbee Nov 21, 2022
6ca50ae
rename tests
Lyrisbee Nov 21, 2022
c9a7343
use str_replace
Lyrisbee Nov 21, 2022
7e75f24
Apply php-cs-fixer changes
Lyrisbee Nov 21, 2022
45d12f1
lint
bepsvpt Nov 21, 2022
cd30eea
Apply php-cs-fixer changes
bepsvpt Nov 21, 2022
fa6f99a
add old version compatibility
Lyrisbee Nov 21, 2022
7bc7c76
fix localKeyName for laravel version below 5.7
bepsvpt Nov 21, 2022
f983dec
Apply php-cs-fixer changes
bepsvpt Nov 21, 2022
25e0ecc
fix reflection issue
bepsvpt Nov 21, 2022
abf875f
Apply php-cs-fixer changes
bepsvpt Nov 21, 2022
61ed5cd
fix ReflectionClass getValue
bepsvpt Nov 21, 2022
768b28b
wip, fix tests
bepsvpt Nov 21, 2022
6e21e28
wip, fix tests
bepsvpt Nov 21, 2022
0990919
cleanup debug code
bepsvpt Nov 21, 2022
deabb67
lint
Lyrisbee Nov 22, 2022
667f222
update select directive doc
Lyrisbee Nov 22, 2022
6b456f6
only `EloquentBuilder` can be optimized and lint
Lyrisbee Nov 22, 2022
c3ff536
use container
Lyrisbee Nov 22, 2022
9ec7c71
update tests
Lyrisbee Nov 22, 2022
ee2442f
remove dead code
Lyrisbee Nov 22, 2022
3221dd2
lint
Lyrisbee Dec 2, 2022
982f4c1
Apply suggestions
Lyrisbee Dec 14, 2022
5cd2304
select without relation directives and fix morphTo
Lyrisbee Dec 15, 2022
e5b0715
cleanup
Lyrisbee Dec 15, 2022
2d9f386
fix paginate schema correlates
Lyrisbee Dec 16, 2022
aafd02c
order by primary key by default
Lyrisbee Dec 16, 2022
f0c4846
throw errors
Lyrisbee Dec 16, 2022
5171228
set default config to false
Lyrisbee Dec 16, 2022
1d570bf
fix isRelation for laravel version below 8.0
Lyrisbee Dec 16, 2022
f385f5d
fix relations
Lyrisbee Dec 16, 2022
b170698
Merge branch 'master' into optimize-query
Lyrisbee Dec 16, 2022
2aa0e4a
fix tests
Lyrisbee Dec 16, 2022
f4fb5d8
Apply php-cs-fixer changes
Lyrisbee Dec 16, 2022
24ee8d4
cleanup
Lyrisbee Dec 16, 2022
0635733
set primary key if no select column
Lyrisbee Dec 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/Pagination/PaginateDirective.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -129,6 +131,31 @@ public function resolveField(FieldValue $fieldValue): FieldValue
$this->directiveArgValue('scopes', [])
);

if (config('lighthouse.optimized_selects')) {
if ($query instanceof EloquentBuilder) {
$fieldSelection = $resolveInfo->getFieldSelection(2);

if (($hasData = Arr::has($fieldSelection, 'data')) || Arr::has($fieldSelection, 'edges')) {
$data = $hasData
? $fieldSelection['data']
: $fieldSelection['edges']['node'];
Comment on lines +166 to +169
Copy link
Collaborator

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.


/** @var array<int, string> $fieldSelection */
$fieldSelection = array_keys($data);

$selectColumns = SelectHelper::getSelectColumns(
$this->definitionNode,
$fieldSelection,
get_class($query->getModel())
);

if (! empty($selectColumns)) {
$query = $query->select($selectColumns);
}
}
}
}

$paginationArgs = PaginationArgs::extractArgs($args, $this->paginationType(), $this->paginateMaxCount());

$paginationArgs->type = $this->optimalPaginationType($resolveInfo);
Expand Down
43 changes: 40 additions & 3 deletions src/Schema/Directives/AllDirective.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases,$selectColumns will be empty, and it shouldn’t throw exceptions because this is an expected behavior.

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 posts: [Post!]! will not be optimized due to empty $selectColumns. In the current implementation, the @hasMany directive is a must-have directive to let the optimized select work normally.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, the query would have to select the users id so that posts can be resolved without @hasMany, right?

In that case, I believe this check is insufficient in order to not break the query. If the client queries an additional field from User, $selectColumns would no longer be empty and result in SELECT name FROM users, thus breaking the resolution of posts.


$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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Contributor Author

@Lyrisbee Lyrisbee Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only want to replace * with id. The sub-query and bindings should be preserved.

From testOrderByRelationCount()

{
  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" => []
  ]

Copy link
Collaborator

Choose a reason for hiding this comment

The 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, ->select() only clears columns and bindings['select'].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it only needs to re-set the bindings['select'] but not the whole bindings?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these 3 lines and $bindings = $query->getRawBindings(); are completely unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above schema is the case. Before ->select(), the raw query is below. The subquery has a select binding cleaning that name != ? needs. If we don't re-set the bindings, it will throw Illuminate\Database\QueryException: SQLSTATE[HY093]: Invalid parameter number. Because the select bindings were clear by ->select()

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it only needs to re-set the bindings['select'] but not the whole bindings?

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;
Expand Down
22 changes: 19 additions & 3 deletions src/Schema/Directives/FindDirective.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
}

$results = $builder->get();

if ($results->count() > 1) {
throw new Error('The query returned more than one result.');
Expand Down
21 changes: 21 additions & 0 deletions src/Schema/Directives/SelectDirective.php
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;
}
}
141 changes: 141 additions & 0 deletions src/Select/SelectHelper.php
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given a field definition node, resolve info

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.
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Show resolved Hide resolved
});
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved

return array_unique($selectColumns);
}
}
12 changes: 12 additions & 0 deletions src/lighthouse.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the problems I outlined in SelectHelper, I think we need to change the approach to this entirely. I believe we need to require that every field in a model that needs to select any columns has an @select configuration. I know that this is cumbersome, but perhaps we can allow iterative adoption by marking some models as being optimizable and only applying the optimization to those.

type Foo @select {
  id: ID @select(columns: ["id"])
}

@select on the model type signifies it can be considered for optimization.

|
*/

'optimized_selects' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider starting this setting with false and wait for reports of forgotten edge cases to come in.


/*
|--------------------------------------------------------------------------
| Shortcut Foreign Key Selection
Expand Down
39 changes: 39 additions & 0 deletions tests/Integration/Pagination/PaginateDirectiveDBTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,45 @@ public function testPaginateWithScopes(): void
]);
}

public function testPaginateOptimizedSelect(): 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');
Expand Down
Loading