nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.36k stars 438 forks source link

Deduplicate nested relations using dot notation with equivalent fields #1873

Open spawnia opened 3 years ago

spawnia commented 3 years ago

What problem does this feature proposal attempt to solve?

See https://github.com/nuwave/lighthouse/pull/1871. Database queries are unnecessarily duplicated.

Which possible solutions should be considered?

When using dot notation to batch load a relation, e.g. @with(relation: "foo.bar.baz"), we would have to pick apart the individual relation segments and resolve a batch loader for each one separately.

Rough draft of how that might look:

        /** @var array<int, \Nuwave\Lighthouse\Execution\BatchLoader\RelationBatchLoader> $relationBatchLoader */
        $loaders = [];

        // Dot notation may be used to eager load nested relations
        $parts = explode('.', $this->relation());

        // Includes the field we are loading the relation for
        $path = $resolveInfo->path;

        // In case we have no args, we can combine eager loads that are the same
        if ($args === []) {
            array_pop($path);
        }

        $lastRelation = array_pop($parts);

        foreach ($parts as $intermediaryRelation) {
            $path []= $intermediaryRelation;
            $loaders []= BatchLoaderRegistry::instance(
                $intermediaryRelation,
                function () use ($resolveInfo, $intermediaryRelation): RelationBatchLoader {
                    return new RelationBatchLoader($this->relationLoader($resolveInfo, $intermediaryRelation));
                }
            );
        }

        $path []= $lastRelation;
        $path = array_merge($path, $this->scopes());

        $loaders []= BatchLoaderRegistry::instance(
            $path,
            function () use ($resolveInfo, $lastRelation): RelationBatchLoader {
                return new RelationBatchLoader($this->relationLoader($resolveInfo, $lastRelation));
            }
        );

        return new Deferred(static function () use ($loaders, $parent) {
            /** @var RelationBatchLoader $loader */
            foreach ($loaders as $loader) {
                $loader->load($parent);
            }
        });

Scopes and additional constraints only apply to the last relation query, so the intermediary queries can safely be batched together with other unconstrained queries on the same relation:

image

vgedzius commented 1 year ago

Eloquent provides a method to load relations only when they are not loaded already loadMissing. Could it be utilised here?

spawnia commented 1 year ago

Unfortunately, changing https://github.com/nuwave/lighthouse/blob/4dd08990889ecf996a2a955434852c967cb9c2ee/src/Execution/ModelsLoader/SimpleModelsLoader.php#L29 to use loadMissing causes many tests to fail - the underlying issue is that relations can be loaded multiple times in a single GraphQL query, but they may apply different conditions.

See failing test logs ``` 1) Tests\Integration\Execution\DataLoader\RelationBatchLoaderTest::testSplitsEagerLoadsByScopes Failed asserting that 2 matches expected 3. /workdir/tests/AssertsQueryCounts.php:47 /workdir/tests/Integration/Execution/DataLoader/RelationBatchLoaderTest.php:308 2) Tests\Integration\Execution\DataLoader\RelationBatchLoaderTest::testSplitsEagerLoadsWithArguments Failed asserting that 2 matches expected 3. /workdir/tests/AssertsQueryCounts.php:47 /workdir/tests/Integration/Execution/DataLoader/RelationBatchLoaderTest.php:341 3) Tests\Integration\Schema\Directives\BelongsToDirectiveTest::testDoesNotShortcutForeignKeyIfQueryHasConditions Unable to find JSON: [{ "data": { "user": { "company": null } } }] within response JSON: [{ "data": { "user": { "company": { "id": "1" } } } }]. Failed asserting that an array has the subset Array &0 ( 'data' => Array &1 ( 'user' => Array &2 ( 'company' => null ) ) ). --- Expected +++ Actual @@ @@ array ( 'user' => array ( - 'company' => NULL, + 'company' => + array ( + 'id' => '1', + ), ), ), ) /workdir/vendor/laravel/framework/src/Illuminate/Testing/Constraints/ArraySubset.php:85 /workdir/vendor/laravel/framework/src/Illuminate/Testing/Assert.php:40 /workdir/vendor/laravel/framework/src/Illuminate/Testing/AssertableJsonString.php:295 /workdir/vendor/laravel/framework/src/Illuminate/Testing/TestResponse.php:673 /workdir/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php:403 /workdir/tests/AssertsQueryCounts.php:44 /workdir/tests/Integration/Schema/Directives/BelongsToDirectiveTest.php:407 4) Tests\Integration\Schema\Directives\HasManyDirectiveTest::testQueryHasManyWithConditionInDifferentAliases Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ "id": 1\n }\n ],\n - "lastTasks": []\n - },\n - {\n - "firstTasks": [],\n "lastTasks": [\n {\n - "id": 6\n + "id": 1\n }\n ]\n + },\n + {\n + "firstTasks": [],\n + "lastTasks": []\n }\n ]\n }\n }' /workdir/vendor/laravel/framework/src/Illuminate/Testing/AssertableJsonString.php:102 /workdir/vendor/laravel/framework/src/Illuminate/Testing/TestResponse.php:709 /workdir/tests/Integration/Schema/Directives/HasManyDirectiveTest.php:202 5) Tests\Integration\SoftDeletes\SoftDeletesAndTrashedDirectiveTest::testNested Failed to assert that the response count matched the expected 2 Failed asserting that actual size 3 matches expected size 2. /workdir/vendor/laravel/framework/src/Illuminate/Testing/AssertableJsonString.php:74 /workdir/vendor/laravel/framework/src/Illuminate/Testing/TestResponse.php:803 /workdir/tests/Integration/SoftDeletes/SoftDeletesAndTrashedDirectiveTest.php:359 ```