Support optimized select for top-level query #2235

Lyrisbee · 2022-11-21T08:27:41Z

Added or updated tests
Documented user facing changes
Updated CHANGELOG.md

I made some updates from #1626.

Tests passed directives

@all
@find
@paginate

The query builder and scout builder are not optimized.

type Query {
  postsSearch(search: String @search): [Post!]! @paginate // scout builder
}

Tests passed relation types

hasOne
hasMany
morphOne
morphMany
belongsTo
belongsToMany

Only the root query will be optimized. The nested queries are not optimized.

type User {
  name: String!
  posts: [Post!]! @hasMany
}

type Post {
  id: ID!
  title: String!
}

type Query {
  users: [User!]! @all
}

{
  users {
    name
    posts {
      id
      title
    }
  }
}

# query logs
# optimized
select `id`, `name` from `users`
# the nested query is not optimized
select * from `posts` where `posts`.`user_id` in (1, 2) and `posts`.`deleted_at` is null

Changes
Only query the selection fields.

Breaking changes

Custom attribute needs to use @select

public function getCompanyNameAttribute(): string
{
    return $this->company->name;
}

type User {
    id: ID!
    companyName: String! @select(columns: ["company_id"])
}

The result may change when using optimized select. For predictable results, you should use @orderBy
https://dba.stackexchange.com/a/6053

Note
This PR was only tested in our own project, and there may be some edge cases that are not considered.

src/ClientDirectives/ClientDirective.php

spawnia

Great work on this, the tests look comprehensive and the overall direction looks right.

src/Pagination/PaginateDirective.php

src/Select/SelectHelper.php

tests/AssertsQueryCounts.php

tests/Integration/Schema/Directives/FindDirectiveTest.php

tests/Integration/Schema/Directives/HasManyDirectiveTest.php

tests/Integration/Schema/Directives/LimitDirectiveTest.php

tests/Integration/WhereConditions/WhereConditionsDirectiveTest.php

src/Select/SelectHelper.php

spawnia · 2022-12-02T08:48:06Z

src/Schema/Directives/AllDirective.php

+                        foreach ($bindings as $type => $binding) {
+                            $builder = $builder->addBinding($binding, $type);
+                        }


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.

spawnia · 2022-12-02T08:49:37Z

src/lighthouse.php

+    |
+    */
+
+    'optimized_selects' => true,


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

tests/Integration/Schema/Directives/FindDirectiveTest.php

tests/Integration/Schema/Directives/HasManyDirectiveTest.php

Co-authored-by: Benedikt Franke <benedikt@franke.tech>

bepsvpt · 2023-01-27T08:08:28Z

@spawnia

Is there anything that needs to change for this pull request to be merged?

spawnia · 2023-01-27T08:32:06Z

src/Pagination/PaginateDirective.php

+                    if (($hasData = Arr::has($fieldSelection, 'data')) || Arr::has($fieldSelection, 'edges')) {
+                        $data = $hasData
+                            ? $fieldSelection['data']
+                            : $fieldSelection['edges']['node'];


We can derive which field to check from $this->directiveArgValue('type'), no need to check both.

spawnia · 2023-01-27T08:34:08Z

src/Pagination/PaginateDirective.php

+                        /** @var string|string[] $keyName */
+                        $keyName = $model->getKeyName();
+                        if (is_string($keyName)) {
+                            $keyName = [$keyName];
+                        }
+
+                        foreach ($keyName as $name) {
+                            $query->orderBy($name);
+                        }


This seems to be unrelated to optimizing the select.

spawnia · 2023-01-27T08:36:07Z

src/Pagination/PaginationManipulator.php

+        }
+        $fieldTypeName = ASTHelper::getUnderlyingTypeName($fieldDefinition);
+
+        return preg_replace('/(Connection|SimplePaginator|Paginator)$/', '', $fieldTypeName);


We can look at the argument of @paginate(type: ?) to know exactly which one to remove. That way, a model called ProductSimple of type PAGINATOR will not wrongly be seen as Product.

spawnia · 2023-01-27T08:36:31Z

src/Schema/Directives/AllDirective.php

+                    /** @var string|string[] $keyName */
+                    $keyName = $model->getKeyName();
+                    if (is_string($keyName)) {
+                        $keyName = [$keyName];
+                    }
+
+                    foreach ($keyName as $name) {
+                        $query->orderBy($name);
+                    }


Again, this seems like an unnecessary addition that is orthogonal to optimizing select.

spawnia · 2023-01-27T08:38:45Z

src/Schema/Directives/FindDirective.php

+
+                /** @var string|string[] $keyName */
+                $keyName = $model->getKeyName();
+                if (is_string($keyName)) {
+                    $keyName = [$keyName];
+                }
+
+                foreach ($keyName as $name) {
+                    $builder->orderBy($name);
+                }


unnecessary

spawnia · 2023-01-27T08:50:36Z

src/Select/SelectHelper.php

+                        $relationName = ASTHelper::directiveArgValue($directive, 'relation', $field);
+                        if (method_exists($model, $relationName)) {
+                            $relation = $model->{$relationName}();


Calling methods equivalent to the name of a field just because they exist is potentially dangerous.

type MyModel { launchTheNukes: Boolean! @method(name: "checkIfUserIsAllowedToLaunchTheNukesButNotActuallyLaunchThem") }

Consider

lighthouse/src/Execution/Arguments/ArgPartitioner.php

Lines 171 to 200 in 46a57e3

/**

* Does a method on the model return a relation of the given class?

*/

public static function methodReturnsRelation(

\ReflectionClass $modelReflection,

string $name,

string $relationClass

): bool {

if (! $modelReflection->hasMethod($name)) {

return false;

}

$relationMethodCandidate = $modelReflection->getMethod($name);

$returnType = $relationMethodCandidate->getReturnType();

if (! $returnType instanceof \ReflectionNamedType) {

return false;

}

if ($returnType->isBuiltin()) {

return false;

}

if (! class_exists($returnType->getName())) {

throw new DefinitionException('Class ' . $returnType->getName() . ' does not exist, did you forget to import the Eloquent relation class?');

}

return is_a($returnType->getName(), $relationClass, true);

}

}

for an actually safe check.

spawnia · 2023-01-27T08:51:33Z

src/Select/SelectHelper.php

+                    $renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute');
+                    $selectColumns[] = $renamedAttribute;


There is no guarantee that the renamed attribute is actually a column, it could just as well be a virtual property.

spawnia · 2023-01-27T08:57:28Z

src/Select/SelectHelper.php

+                    $relation = null !== $directive
+                        ? $model->{ASTHelper::directiveArgValue($directive, 'name')}()
+                        : $model->{$field}();


Again, this is dangerously calling methods of the model. Due to the possibility of there being custom resolver directives, we don't know at all if they would be called during field resolution.

Even with @method(name: ?), there are still multiple ways this could go wrong:

the field might have required arguments that would be passed to the method when actually resolved

there might be middleware that prevents the current user from actually resolving the field

There are probably more, but once again this proves my point:

I don't see how we could reliably determine if a field is a column without explicit configuration.

It is not that I don't want to add this feature, I just need to make sure it is done in a way that does not cause random runtime crashes, security breaches, unintended side effects or a multitude of other problems. We really need to limit the magic to cases where we can be absolutely sure nothing can go wrong, otherwise we can only depend on explicit configuration.

spawnia · 2023-01-27T09:05:10Z

src/Select/SelectHelper.php

+                    // fallback to selecting the field name
+                    $selectColumns[] = $field;


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.

spawnia · 2023-01-27T09:12:03Z

src/lighthouse.php

+    |--------------------------------------------------------------------------
+    |
+    | 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.


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.

spawnia · 2023-01-27T09:14:37Z

Thank you for your continued efforts on this. Unfortunately this pull request needs significant rework for the reasons I outlined in my review.

spawnia · 2023-02-22T08:44:47Z

You can add the following entry to CHANGELOG.md:

### Added

- Allow optimizing `SELECT` https://github.com/nuwave/lighthouse/pull/2235 by @Lyrisbee, @bepsvpt

HelloAlexPan · 2023-02-22T09:28:49Z

Thanks @spawnia, could we add a @storipress tag as well — we'll get back to this one sometime next month as we're working on some other features right now. We expect this to be done EO March

spawnia · 2023-02-22T10:57:29Z

Thanks @spawnia, could we add a @storipress tag as well — we'll get back to this one sometime next month as we're working on some other features right now. We expect this to be done EO March

Sure, that sounds fine to me.

Lyrisbee marked this pull request as draft November 21, 2022 08:32

spawnia reviewed Nov 21, 2022

View reviewed changes

src/ClientDirectives/ClientDirective.php Outdated Show resolved Hide resolved

Lyrisbee and others added 22 commits November 21, 2022 18:36

optimize select

2f13537

add reference

290e464

optimize find directive and fix tests

03d4124

add select directvie

cacc576

move select directive

d49f0ec

add optimize select config

6f1a04a

filter non-array document

ed0bee9

remove Schema request

3ef48bd

handle RelationOrderBy problems

23237a1

lint

1b73b78

add tests

bcf8f8c

rename tests

6ca50ae

use str_replace

c9a7343

Apply php-cs-fixer changes

7e75f24

lint

45d12f1

Apply php-cs-fixer changes

cd30eea

add old version compatibility

fa6f99a

fix localKeyName for laravel version below 5.7

7bc7c76

Apply php-cs-fixer changes

f983dec

fix reflection issue

25e0ecc

Apply php-cs-fixer changes

abf875f

fix ReflectionClass getValue

61ed5cd

bepsvpt force-pushed the optimize-query branch from 3f9cc91 to 61ed5cd Compare November 21, 2022 10:37

spawnia added the enhancement A feature or improvement label Nov 21, 2022

wip, fix tests

768b28b

spawnia requested changes Nov 21, 2022

View reviewed changes

bepsvpt added 2 commits November 21, 2022 21:47

wip, fix tests

6e21e28

cleanup debug code

0990919

Lyrisbee added 6 commits November 22, 2022 17:18

update select directive doc

667f222

only EloquentBuilder can be optimized and lint

6b456f6

use container

c3ff536

update tests

9ec7c71

remove dead code

ee2442f

lint

3221dd2

spawnia reviewed Dec 2, 2022

View reviewed changes

Lyrisbee and others added 14 commits December 14, 2022 14:16

Apply suggestions

982f4c1

Co-authored-by: Benedikt Franke <benedikt@franke.tech>

select without relation directives and fix morphTo

5cd2304

cleanup

e5b0715

fix paginate schema correlates

2d9f386

order by primary key by default

aafd02c

throw errors

f0c4846

set default config to false

5171228

fix isRelation for laravel version below 8.0

1d570bf

fix relations

f385f5d

Merge branch 'master' into optimize-query

b170698

fix tests

2aa0e4a

Apply php-cs-fixer changes

f4fb5d8

cleanup

24ee8d4

set primary key if no select column

0635733

Lyrisbee marked this pull request as ready for review December 19, 2022 02:13

spawnia requested changes Jan 27, 2023

View reviewed changes

spawnia marked this pull request as draft February 20, 2023 09:27

spawnia mentioned this pull request Aug 29, 2023

Select only necessary columns for optimized queries #1356

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support optimized select for top-level query #2235

Support optimized select for top-level query #2235

Lyrisbee commented Nov 21, 2022 •

edited

Loading

spawnia left a comment

spawnia Dec 2, 2022

spawnia Dec 2, 2022

bepsvpt commented Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia Jan 27, 2023

spawnia commented Jan 27, 2023

spawnia commented Feb 22, 2023

HelloAlexPan commented Feb 22, 2023

spawnia commented Feb 22, 2023

	/**
	* Does a method on the model return a relation of the given class?
	*/
	public static function methodReturnsRelation(
	\ReflectionClass $modelReflection,
	string $name,
	string $relationClass
	): bool {
	if (! $modelReflection->hasMethod($name)) {
	return false;
	}

	$relationMethodCandidate = $modelReflection->getMethod($name);

	$returnType = $relationMethodCandidate->getReturnType();
	if (! $returnType instanceof \ReflectionNamedType) {
	return false;
	}

	if ($returnType->isBuiltin()) {
	return false;
	}

	if (! class_exists($returnType->getName())) {
	throw new DefinitionException('Class ' . $returnType->getName() . ' does not exist, did you forget to import the Eloquent relation class?');
	}

	return is_a($returnType->getName(), $relationClass, true);
	}
	}

		$renamedAttribute = ASTHelper::directiveArgValue($directive, 'attribute');
		$selectColumns[] = $renamedAttribute;

		// fallback to selecting the field name
		$selectColumns[] = $field;

Support optimized select for top-level query #2235

Are you sure you want to change the base?

Support optimized select for top-level query #2235

Conversation

Lyrisbee commented Nov 21, 2022 • edited Loading

spawnia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bepsvpt commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spawnia commented Jan 27, 2023

spawnia commented Feb 22, 2023

HelloAlexPan commented Feb 22, 2023

spawnia commented Feb 22, 2023

Lyrisbee commented Nov 21, 2022 •

edited

Loading