lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Deduplicate nested relations using dot notation with equivalent fields

Open spawnia opened this issue 4 years ago • 2 comments

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

spawnia avatar Jun 09 '21 15:06 spawnia

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

vgedzius avatar Oct 24 '22 14:10 vgedzius

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

spawnia avatar Oct 25 '22 10:10 spawnia