From 0f6d2106021add81fbe8bd5fd4e143cd74633e45 Mon Sep 17 00:00:00 2001 From: Dominik Beerbohm Date: Wed, 8 May 2024 01:12:51 +0200 Subject: [PATCH] FIX Correctly eagerload polymorphic has_one relations (#11204) --- src/ORM/DataList.php | 70 +++++++--- tests/php/ORM/DataListEagerLoadingTest.php | 128 +++++++++++++++--- .../EagerLoading/EagerLoadObject.php | 3 +- .../EagerLoading/EagerLoadSubClassObject.php | 8 ++ 4 files changed, 166 insertions(+), 43 deletions(-) create mode 100644 tests/php/ORM/DataListTest/EagerLoading/EagerLoadSubClassObject.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index db63ee866fd..d69c5a7899e 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -979,7 +979,10 @@ private function getEagerLoadVariables(string $relationChain, string $relationNa return [ $hasOneComponent, 'has_one', - $relationName . 'ID', + [ + 'joinField' => $relationName . 'ID', + 'joinClass' => $hasOneComponent == DataObject::class ? $relationName . 'Class' : null, + ], ]; } $belongsToComponent = $schema->belongsToComponent($parentDataClass, $relationName); @@ -1109,7 +1112,7 @@ private function fetchEagerLoadRelations(Query $query): void private function fetchEagerLoadHasOne( Query|array $parents, - string $hasOneIDField, + array $hasOneRelation, string $relationDataClass, string $relationChain, string $relationName, @@ -1120,6 +1123,9 @@ private function fetchEagerLoadHasOne( throw new LogicException("Cannot manipulate eagerloading query for $relationType relation $relationName"); } + $hasOneIDField = $hasOneRelation['joinField']; + $hasOneClassField = $hasOneRelation['joinClass']; + $fetchedIDs = []; $addTo = []; @@ -1128,41 +1134,63 @@ private function fetchEagerLoadHasOne( if (is_array($parentData)) { // $parentData represents a record in this DataList $hasOneID = $parentData[$hasOneIDField]; - $fetchedIDs[] = $hasOneID; - $addTo[$hasOneID][] = $parentData['ID']; + + if ($hasOneID) { + // Class field is only set for polymorphic has_one relations + $hasOneClass = $hasOneClassField ? $parentData[$hasOneClassField] : $relationDataClass; + + $fetchedIDs[$hasOneClass][$hasOneID] = $hasOneID; + $addTo[$hasOneClass][$hasOneID][] = $parentData['ID']; + } } elseif ($parentData instanceof DataObject) { // $parentData represents another has_one record $hasOneID = $parentData->$hasOneIDField; - $fetchedIDs[] = $hasOneID; - $addTo[$hasOneID][] = $parentData; + + if ($hasOneID) { + // Class field is only set for polymorphic has_one relations + $hasOneClass = $hasOneClassField ? $parentData->$hasOneClassField : $relationDataClass; + + $fetchedIDs[$hasOneClass][$hasOneID] = $hasOneID; + $addTo[$hasOneClass][$hasOneID][] = $parentData; + } } elseif ($parentData instanceof EagerLoadedList) { // $parentData represents a has_many or many_many relation foreach ($parentData->getRows() as $parentRow) { + // $parentData represents another has_one record $hasOneID = $parentRow[$hasOneIDField]; - $fetchedIDs[] = $hasOneID; - $addTo[$hasOneID][] = ['ID' => $parentRow['ID'], 'list' => $parentData]; + + if ($hasOneID) { + // Class field is only set for polymorphic has_one relations + $hasOneClass = $hasOneClassField ? $parentRow[$hasOneClassField] : $relationDataClass; + + $fetchedIDs[$hasOneClass][$hasOneID] = $hasOneID; + $addTo[$hasOneClass][$hasOneID][] = ['ID' => $parentRow['ID'], 'list' => $parentData]; + } } } else { throw new LogicException("Invalid parent for eager loading $relationType relation $relationName"); } } - $fetchedRecords = DataObject::get($relationDataClass)->byIDs($fetchedIDs)->toArray(); + $fetchedRecords = []; - // Add each fetched record to the appropriate place - foreach ($fetchedRecords as $fetched) { - if (isset($addTo[$fetched->ID])) { - foreach ($addTo[$fetched->ID] as $addHere) { - if ($addHere instanceof DataObject) { - $addHere->setEagerLoadedData($relationName, $fetched); - } elseif (is_array($addHere)) { - $addHere['list']->addEagerLoadedData($relationName, $addHere['ID'], $fetched); - } else { - $this->eagerLoadedData[$relationChain][$addHere][$relationName] = $fetched; + foreach ($fetchedIDs as $class => $ids) { + foreach (DataObject::get($class)->byIDs($ids) as $fetched) { + $fetchedRecords[] = $fetched; + + if (isset($addTo[$class][$fetched->ID])) { + foreach ($addTo[$class][$fetched->ID] as $addHere) { + if ($addHere instanceof DataObject) { + $addHere->setEagerLoadedData($relationName, $fetched); + } elseif (is_array($addHere)) { + $addHere['list']->addEagerLoadedData($relationName, $addHere['ID'], $fetched); + } else { + $this->eagerLoadedData[$relationChain][$addHere][$relationName] = $fetched; + } } + } else { + throw new LogicException("Couldn't find parent for record $class on $relationType relation $relationName"); } - } else { - throw new LogicException("Couldn't find parent for record {$fetched->ID} on $relationType relation $relationName"); } } diff --git a/tests/php/ORM/DataListEagerLoadingTest.php b/tests/php/ORM/DataListEagerLoadingTest.php index 7709048a0f6..65950a9ae89 100644 --- a/tests/php/ORM/DataListEagerLoadingTest.php +++ b/tests/php/ORM/DataListEagerLoadingTest.php @@ -14,6 +14,7 @@ use SilverStripe\ORM\ManyManyThroughList; use SilverStripe\ORM\SS_List; use SilverStripe\ORM\Tests\DataListTest\EagerLoading\EagerLoadObject; +use SilverStripe\ORM\Tests\DataListTest\EagerLoading\EagerLoadSubClassObject; use SilverStripe\ORM\Tests\DataListTest\EagerLoading\HasOneEagerLoadObject; use SilverStripe\ORM\Tests\DataListTest\EagerLoading\HasOneSubEagerLoadObject; use SilverStripe\ORM\Tests\DataListTest\EagerLoading\HasOneSubSubEagerLoadObject; @@ -783,7 +784,11 @@ public function provideEagerLoadRelationsEmpty(): array return [ 'has_one' => [ 'eagerLoad' => 'HasOneEagerLoadObject', - 'expectedNumQueries' => 2, + 'expectedNumQueries' => 1, + ], + 'polymorph_has_one' => [ + 'eagerLoad' => 'HasOnePolymorphObject', + 'expectedNumQueries' => 1, ], 'belongs_to' => [ 'eagerLoad' => 'BelongsToEagerLoadObject', @@ -1645,28 +1650,12 @@ public function testManipulatingEagerloadingQuery(string $relationType, array $r public function testHasOneMultipleAppearance(): void { - $this->provideHasOneObjects(); - $this->validateMultipleAppearance(6, EagerLoadObject::get()); - $this->validateMultipleAppearance(2, EagerLoadObject::get()->eagerLoad('HasOneEagerLoadObject')); + $items = $this->provideHasOneObjects(); + $this->validateMultipleAppearance($items, 6, EagerLoadObject::get()); + $this->validateMultipleAppearance($items, 2, EagerLoadObject::get()->eagerLoad('HasOneEagerLoadObject')); } - protected function validateMultipleAppearance(int $expected, DataList $list): void - { - try { - $this->startCountingSelectQueries(); - - /** @var EagerLoadObject $item */ - foreach ($list as $item) { - $item->HasOneEagerLoadObject()->Title; - } - - $this->assertSame($expected, $this->stopCountingSelectQueries()); - } finally { - $this->resetShowQueries(); - } - } - - protected function provideHasOneObjects(): void + protected function provideHasOneObjects(): array { $subA = new HasOneEagerLoadObject(); $subA->Title = 'A'; @@ -1709,5 +1698,102 @@ protected function provideHasOneObjects(): void $baseF->Title = 'F'; $baseF->HasOneEagerLoadObjectID = 0; $baseF->write(); + + return [ + $baseA->ID => [$subA->ClassName, $subA->ID], + $baseB->ID => [$subA->ClassName, $subA->ID], + $baseC->ID => [$subB->ClassName, $subB->ID], + $baseD->ID => [$subC->ClassName, $subC->ID], + $baseE->ID => [$subB->ClassName, $subB->ID], + $baseF->ID => [null, 0], + ]; + } + + public function testPolymorphEagerLoading(): void + { + $items = $this->providePolymorphHasOne(); + $this->validateMultipleAppearance($items, 5, EagerLoadObject::get(), 'HasOnePolymorphObject'); + $this->validateMultipleAppearance($items, 4, EagerLoadObject::get()->eagerLoad('HasOnePolymorphObject'), 'HasOnePolymorphObject'); + } + + protected function providePolymorphHasOne(): array + { + $subA = new HasOneEagerLoadObject(); + $subA->Title = 'A'; + $subA->write(); + + $subB = new HasOneEagerLoadObject(); + $subB->Title = 'B'; + $subB->write(); + + $subC = new HasOneSubSubEagerLoadObject(); + $subC->Title = 'C'; + $subC->write(); + + $subD = new EagerLoadSubClassObject(); + $subD->Title = 'D'; + $subD->write(); + + $baseA = new EagerLoadObject(); + $baseA->Title = 'A'; + $baseA->HasOnePolymorphObjectClass = $subA->ClassName; + $baseA->HasOnePolymorphObjectID = $subA->ID; + $baseA->write(); + + $baseB = new EagerLoadObject(); + $baseB->Title = 'B'; + $baseB->HasOnePolymorphObjectClass = $subB->ClassName; + $baseB->HasOnePolymorphObjectID = $subB->ID; + $baseB->write(); + + $baseC = new EagerLoadObject(); + $baseC->Title = 'C'; + $baseC->HasOnePolymorphObjectClass = $subC->ClassName; + $baseC->HasOnePolymorphObjectID = $subC->ID; + $baseC->write(); + + $baseD = new EagerLoadObject(); + $baseD->Title = 'D'; + $baseD->HasOnePolymorphObjectClass = $subD->ClassName; + $baseD->HasOnePolymorphObjectID = $subD->ID; + $baseD->write(); + + $baseE = new EagerLoadObject(); + $baseE->Title = 'E'; + $baseE->HasOnePolymorphObjectClass = null; + $baseE->HasOnePolymorphObjectID = 0; + $baseE->write(); + + return [ + $baseA->ID => [$subA->ClassName, $subA->ID], + $baseB->ID => [$subB->ClassName, $subB->ID], + $baseC->ID => [$subC->ClassName, $subC->ID], + $baseD->ID => [$subD->ClassName, $subD->ID], + $baseE->ID => [null, 0], + ]; + } + + protected function validateMultipleAppearance( + array $expectedRelations, + int $expected, + DataList $list, + string $relation = 'HasOneEagerLoadObject', + ): void { + try { + $this->startCountingSelectQueries(); + + /** @var EagerLoadObject $item */ + foreach ($list as $item) { + $rel = $item->{$relation}(); + + $this->assertArrayHasKey($item->ID, $expectedRelations, $relation . ' should be loaded'); + $this->assertEquals($expectedRelations[$item->ID][0], $rel?->ID ? $rel?->ClassName : null); + $this->assertEquals($expectedRelations[$item->ID][1], $rel?->ID ?? 0); + } + + $this->assertSame($expected, $this->stopCountingSelectQueries()); + } finally { + $this->resetShowQueries(); + } } } diff --git a/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php index 9d753dc9f3f..568d715204d 100644 --- a/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php +++ b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php @@ -14,7 +14,8 @@ class EagerLoadObject extends DataObject implements TestOnly ]; private static $has_one = [ - 'HasOneEagerLoadObject' => HasOneEagerLoadObject::class + 'HasOneEagerLoadObject' => HasOneEagerLoadObject::class, + 'HasOnePolymorphObject' => DataObject::class, ]; private static $belongs_to = [ diff --git a/tests/php/ORM/DataListTest/EagerLoading/EagerLoadSubClassObject.php b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadSubClassObject.php new file mode 100644 index 00000000000..cff8de9526a --- /dev/null +++ b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadSubClassObject.php @@ -0,0 +1,8 @@ +