Skip to content

Commit

Permalink
FIX Correctly eagerload polymorphic has_one relations (#11204)
Browse files Browse the repository at this point in the history
  • Loading branch information
@beerbohmdo
beerbohmdo authored May 7, 2024
1 parent 241d03b commit 0f6d210
Show file tree
Hide file tree
Showing 4 changed files with 166 additions and 43 deletions.
70 changes: 49 additions & 21 deletions src/ORM/DataList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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 = [];

Expand All @@ -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");
}
}

Expand Down
128 changes: 107 additions & 21 deletions tests/php/ORM/DataListEagerLoadingTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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();
}
}
}
3 changes: 2 additions & 1 deletion tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
8 changes: 8 additions & 0 deletions tests/php/ORM/DataListTest/EagerLoading/EagerLoadSubClassObject.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace SilverStripe\ORM\Tests\DataListTest\EagerLoading;

class EagerLoadSubClassObject extends HasOneSubSubEagerLoadObject
{

}

0 comments on commit 0f6d210

Please sign in to comment.