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 22 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
22 changes: 22 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,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')) {
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
$fieldSelection = $resolveInfo->getFieldSelection(4);
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved

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.
Show resolved Hide resolved

$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);
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 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();
}
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 instanceof EloquentBuilder ? $builder->getQuery() : $builder;
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved

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 columns names to pass to the Eloquent query builder
"""
columns: [String!]
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
) on FIELD_DEFINITION
GRAPHQL;
}
}
137 changes: 137 additions & 0 deletions src/Select/SelectHelper.php
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.
Show resolved Hide resolved

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.
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 mixed[] $fieldSelection
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
*
* @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.
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];

if ($type instanceof UnionTypeDefinitionNode) {
$type = $documentAST->types[ASTHelper::getUnderlyingTypeName($type->types[0])];
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
}

/** @var iterable<FieldDefinitionNode> $fieldDefinitions */
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
$fieldDefinitions = $type->fields;

/** @var Model $model */
$model = new $modelName();
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved

$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.
Show resolved Hide resolved

foreach ($directivesRequiringKeys as $directiveType) {
if (ASTHelper::hasDirective($fieldDefinition, $directiveType)) {
/** @var DirectiveNode $directive */
$directive = ASTHelper::directiveDefinition($fieldDefinition, $directiveType);
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved

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.
Show resolved Hide resolved
} 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.
Show resolved Hide resolved
} 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.
Show resolved Hide resolved
$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.
Show resolved Hide resolved
return ! $model->hasGetMutator($column) && ! method_exists($model, $column);
spawnia marked this conversation as resolved.
Show resolved Hide resolved
});

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

'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
2 changes: 1 addition & 1 deletion tests/AssertsQueryCounts.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ trait AssertsQueryCounts
{
protected function countQueries(?int &$count): void
{
DB::listen(function () use (&$count): void {
DB::listen(function ($query) use (&$count): void {
Lyrisbee marked this conversation as resolved.
Show resolved Hide resolved
++$count;
});
}
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