FIX Don't throw exception for empty eagerloaded relation (#11220)

silverstripe · May 6, 2024 · a198c91 · a198c91
1 parent a92baea
commit a198c91
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 12 deletions.
diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php
@@ -1162,7 +1162,7 @@ private function fetchEagerLoadHasOne(
                     }
                 }
             } else {
-                throw new LogicException("Couldn't find parent for record $fetchedID on $relationType relation $relationName");
+                throw new LogicException("Couldn't find parent for record {$fetched->ID} on $relationType relation $relationName");
             }
         }
 
@@ -1321,17 +1321,21 @@ private function fetchEagerLoadManyMany(
         }
 
         // Get the join records so we can correctly identify which children belong to which parents
-        // This also holds extra fields data
-        $fetchedIDsAsString = implode(',', $fetchedIDs);
-        $joinRows = DB::query(
-            'SELECT * FROM "' . $joinTable
-            // Only get joins relevant for the parent list
-            . '" WHERE "' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')'
-            // Exclude any children that got filtered out
-            . ' AND ' . $childIDField . ' IN (' . $fetchedIDsAsString . ')'
-            // Respect sort order of fetched items
-            . ' ORDER BY FIELD(' . $childIDField . ', ' . $fetchedIDsAsString . ')'
-        );
+        // If there are no parents and no children, skip this to avoid an error (and to skip an unnecessary DB call)
+        // Note that $joinRows also holds extra fields data
+        $joinRows = [];
+        if (!empty($parentIDs) && !empty($fetchedIDs)) {
+            $fetchedIDsAsString = implode(',', $fetchedIDs);
+            $joinRows = DB::query(
+                'SELECT * FROM "' . $joinTable
+                // Only get joins relevant for the parent list
+                . '" WHERE "' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')'
+                // Exclude any children that got filtered out
+                . ' AND ' . $childIDField . ' IN (' . $fetchedIDsAsString . ')'
+                // Respect sort order of fetched items
+                . ' ORDER BY FIELD(' . $childIDField . ', ' . $fetchedIDsAsString . ')'
+            );
+        }
 
         // Store the children in an EagerLoadedList against the correct parent
         foreach ($joinRows as $row) {

diff --git a/tests/php/ORM/DataListEagerLoadingTest.php b/tests/php/ORM/DataListEagerLoadingTest.php
@@ -12,6 +12,7 @@
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\EagerLoadedList;
 use SilverStripe\ORM\ManyManyThroughList;
+use SilverStripe\ORM\SS_List;
 use SilverStripe\ORM\Tests\DataListTest\EagerLoading\EagerLoadObject;
 use SilverStripe\ORM\Tests\DataListTest\EagerLoading\HasOneEagerLoadObject;
 use SilverStripe\ORM\Tests\DataListTest\EagerLoading\HasOneSubEagerLoadObject;
@@ -754,6 +755,59 @@ private function iterateEagerLoadData(DataList $dataList, int $chunks = 0): arra
         return [$results, $selectCount];
     }
 
+    /**
+     * @dataProvider provideEagerLoadRelationsEmpty
+     */
+    public function testEagerLoadRelationsEmpty(string $eagerLoadRelation, int $expectedNumQueries): void
+    {
+        EagerLoadObject::create(['Title' => 'test object'])->write();
+        $dataList = EagerLoadObject::get()->eagerLoad($eagerLoadRelation);
+        $this->startCountingSelectQueries();
+        foreach ($dataList as $record) {
+            $relation = $record->$eagerLoadRelation();
+            if ($relation instanceof SS_List) {
+                // The list should be an empty eagerloaded list
+                $this->assertInstanceOf(EagerLoadedList::class, $relation);
+                $this->assertCount(0, $relation);
+            } elseif ($relation !== null) {
+                // There should be no record here
+                $this->assertSame($relation->ID, 0);
+            }
+        }
+        $numQueries = $this->stopCountingSelectQueries();
+        $this->assertSame($expectedNumQueries, $numQueries);
+    }
+
+    public function provideEagerLoadRelationsEmpty(): array
+    {
+        return [
+            'has_one' => [
+                'eagerLoad' => 'HasOneEagerLoadObject',
+                'expectedNumQueries' => 2,
+            ],
+            'belongs_to' => [
+                'eagerLoad' => 'BelongsToEagerLoadObject',
+                'expectedNumQueries' => 2,
+            ],
+            'has_many' => [
+                'eagerLoad' => 'HasManyEagerLoadObjects',
+                'expectedNumQueries' => 2,
+            ],
+            'many_many' => [
+                'eagerLoad' => 'ManyManyEagerLoadObjects',
+                'expectedNumQueries' => 2,
+            ],
+            'many_many through' => [
+                'eagerLoad' => 'ManyManyThroughEagerLoadObjects',
+                'expectedNumQueries' => 2,
+            ],
+            'belongs_many_many' => [
+                'eagerLoad' => 'BelongsManyManyEagerLoadObjects',
+                'expectedNumQueries' => 2,
+            ],
+        ];
+    }
+
     public function testEagerLoadFourthLevelException(): void
     {
         $eagerLoadRelation = implode('.', [