Skip to content

Commit

Permalink
FIX eagerLoad crash with nonterminal hasOne relation
Browse files Browse the repository at this point in the history
  • Loading branch information
@MasonD
MasonD authored and Mason Dechaineux committed Dec 13, 2024
1 parent 61244b7 commit 18392e7
Show file tree
Hide file tree
Showing 6 changed files with 182 additions and 24 deletions.
14 changes: 14 additions & 0 deletions src/ORM/DataList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1055,7 +1055,14 @@ private function fetchEagerLoadRelations(Query $query): void
$parentIDs = $topLevelIDs;
$parentRelationData = $query;
$chainToDate = [];
$polymorphicEncountered = false;
foreach (explode('.', $relationChain) as $relationName) {
if ($polymorphicEncountered) {
$polymorphicRelation = $chainToDate[array_key_last($chainToDate)];
throw new InvalidArgumentException(
"Invalid relation passed to eagerLoad() - $relationChain. Further nested relations are not supported after polymorphic has_one relation $polymorphicRelation."
);
}
/** @var Query|array<DataObject|EagerLoadedList> $parentRelationData */
$chainToDate[] = $relationName;
list(
Expand All @@ -1074,6 +1081,9 @@ private function fetchEagerLoadRelations(Query $query): void
$relationName,
$relationType
);
if ($relationComponent['joinClass']) {
$polymorphicEncountered = true;
}
break;
case 'belongs_to':
list($parentRelationData, $parentIDs) = $this->fetchEagerLoadBelongsTo(
Expand Down Expand Up @@ -1205,6 +1215,10 @@ private function fetchEagerLoadHasOne(
// into the has_one components - DataObject does that for us in getComponent() without any extra
// db calls.

// fetchEagerLoadRelations expects these to be flat arrays if the relation is not polymorphic
if (!$hasOneClassField) {
return [$fetchedRecords, $fetchedIDs[$relationDataClass] ?? []];
}
return [$fetchedRecords, $fetchedIDs];
}

Expand Down
130 changes: 106 additions & 24 deletions tests/php/ORM/DataListEagerLoadingTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\BelongsManyManyEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\BelongsManyManySubEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\BelongsManyManySubSubEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedBackwardsHasManyEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedBackwardsHasOneEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedBackwardsManyManyEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedHasManyEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedHasOneEagerLoadObject;
use SilverStripe\ORM\Tests\DataListTest\EagerLoading\MixedManyManyEagerLoadObject;
Expand Down Expand Up @@ -76,6 +79,9 @@ public static function getExtraDataObjects()
MixedHasManyEagerLoadObject::class,
MixedHasOneEagerLoadObject::class,
MixedManyManyEagerLoadObject::class,
MixedBackwardsHasOneEagerLoadObject::class,
MixedBackwardsHasManyEagerLoadObject::class,
MixedBackwardsManyManyEagerLoadObject::class,
];
}

Expand Down Expand Up @@ -189,154 +195,175 @@ public function provideEagerLoadRelations(): array
[
'iden' => 'lazy-load',
'eagerLoad' => [],
'expected' => 83
'expected' => 91
],
[
'iden' => 'has-one-a',
'eagerLoad' => [
'HasOneEagerLoadObject',
],
'expected' => 82
'expected' => 90
],
[
'iden' => 'has-one-b',
'eagerLoad' => [
'HasOneEagerLoadObject.HasOneSubEagerLoadObject',
],
'expected' => 81
'expected' => 89
],
[
'iden' => 'has-one-c',
'eagerLoad' => [
'HasOneEagerLoadObject.HasOneSubEagerLoadObject.HasOneSubSubEagerLoadObject',
],
'expected' => 80
'expected' => 88
],
[
'iden' => 'belongs-to-a',
'eagerLoad' => [
'BelongsToEagerLoadObject',
],
'expected' => 82
'expected' => 90
],
[
'iden' => 'belongs-to-b',
'eagerLoad' => [
'BelongsToEagerLoadObject.BelongsToSubEagerLoadObject',
],
'expected' => 81
'expected' => 89
],
[
'iden' => 'belongs-to-c',
'eagerLoad' => [
'BelongsToEagerLoadObject.BelongsToSubEagerLoadObject.BelongsToSubSubEagerLoadObject',
],
'expected' => 80
'expected' => 88
],
[
'iden' => 'has-many-a',
'eagerLoad' => [
'HasManyEagerLoadObjects',
],
'expected' => 82
'expected' => 90
],
[
'iden' => 'has-many-b',
'eagerLoad' => [
'HasManyEagerLoadObjects.HasManySubEagerLoadObjects',
],
'expected' => 79
'expected' => 87
],
[
'iden' => 'has-many-c',
'eagerLoad' => [
'HasManyEagerLoadObjects.HasManySubEagerLoadObjects.HasManySubSubEagerLoadObjects',
],
'expected' => 72
'expected' => 80
],
[
'iden' => 'many-many-a',
'eagerLoad' => [
'ManyManyEagerLoadObjects',
],
'expected' => 83 // same number as lazy-load, though without an INNER JOIN
'expected' => 91 // same number as lazy-load, though without an INNER JOIN
],
[
'iden' => 'many-many-b',
'eagerLoad' => [
'ManyManyEagerLoadObjects.ManyManySubEagerLoadObjects',
],
'expected' => 81
'expected' => 89
],
[
'iden' => 'many-many-c',
'eagerLoad' => [
'ManyManyEagerLoadObjects.ManyManySubEagerLoadObjects.ManyManySubSubEagerLoadObjects',
],
'expected' => 75
'expected' => 83
],
[
'iden' => 'many-many-through-a',
'eagerLoad' => [
'ManyManyThroughEagerLoadObjects',
],
'expected' => 83
'expected' => 91
],
[
'iden' => 'many-many-through-b',
'eagerLoad' => [
'ManyManyThroughEagerLoadObjects.ManyManyThroughSubEagerLoadObjects',
],
'expected' => 81
'expected' => 89
],
[
'iden' => 'many-many-through-c',
'eagerLoad' => [
'ManyManyThroughEagerLoadObjects.ManyManyThroughSubEagerLoadObjects.ManyManyThroughSubSubEagerLoadObjects',
],
'expected' => 75
'expected' => 83
],
[
'iden' => 'belongs-many-many-a',
'eagerLoad' => [
'BelongsManyManyEagerLoadObjects',
],
'expected' => 83
'expected' => 91
],
[
'iden' => 'belongs-many-many-b',
'eagerLoad' => [
'BelongsManyManyEagerLoadObjects.BelongsManyManySubEagerLoadObjects',
],
'expected' => 81
'expected' => 89
],
[
'iden' => 'belongs-many-many-c',
'eagerLoad' => [
'BelongsManyManyEagerLoadObjects.BelongsManyManySubEagerLoadObjects.BelongsManyManySubSubEagerLoadObjects',
],
'expected' => 75
'expected' => 83
],
[
'iden' => 'mixed-a',
'eagerLoad' => [
'MixedManyManyEagerLoadObjects',
],
'expected' => 83
'expected' => 91
],
[
'iden' => 'mixed-b',
'eagerLoad' => [
'MixedManyManyEagerLoadObjects.MixedHasManyEagerLoadObjects',
],
'expected' => 80
'expected' => 88
],
[
'iden' => 'mixed-c',
'eagerLoad' => [
'MixedManyManyEagerLoadObjects.MixedHasManyEagerLoadObjects.MixedHasOneEagerLoadObject',
],
'expected' => 73
'expected' => 81
],
[
'iden' => 'mixed-back-a',
'eagerLoad' => [
'MixedBackwardsHasOneEagerLoadObject',
],
'expected' => 90
],
[
'iden' => 'mixed-back-b',
'eagerLoad' => [
'MixedBackwardsHasOneEagerLoadObject.MixedBackwardsHasManyEagerLoadObjects',
],
'expected' => 89
],
[
'iden' => 'mixed-back-c',
'eagerLoad' => [
'MixedBackwardsHasOneEagerLoadObject.MixedBackwardsHasManyEagerLoadObjects.MixedBackwardsManyManyEagerLoadObjects',
],
'expected' => 87
],
[
'iden' => 'duplicates',
Expand All @@ -349,7 +376,7 @@ public function provideEagerLoadRelations(): array
'BelongsManyManyEagerLoadObjects.BelongsManyManySubEagerLoadObjects',
'MixedManyManyEagerLoadObjects.MixedHasManyEagerLoadObjects.MixedHasOneEagerLoadObject',
],
'expected' => 73
'expected' => 81
],
[
'iden' => 'all',
Expand All @@ -361,8 +388,9 @@ public function provideEagerLoadRelations(): array
'ManyManyThroughEagerLoadObjects.ManyManyThroughSubEagerLoadObjects.ManyManyThroughSubSubEagerLoadObjects',
'BelongsManyManyEagerLoadObjects.BelongsManyManySubEagerLoadObjects.BelongsManyManySubSubEagerLoadObjects',
'MixedManyManyEagerLoadObjects.MixedHasManyEagerLoadObjects.MixedHasOneEagerLoadObject',
'MixedBackwardsHasOneEagerLoadObject.MixedBackwardsHasManyEagerLoadObjects.MixedBackwardsManyManyEagerLoadObjects',
],
'expected' => 32
'expected' => 36
],
];
}
Expand Down Expand Up @@ -443,6 +471,13 @@ private function expectedEagerLoadRelations(): array
'mixedHasOneObj 0 1 0 1',
'mixedHasManyObj 0 1 1',
'mixedHasOneObj 0 1 1 1',
'mixedBackwardsHasOneObj 0',
'mixedBackwardsHasManyObj 0 0',
'mixedBackwardsManyManyObj 0 0 0',
'mixedBackwardsManyManyObj 0 0 1',
'mixedBackwardsHasManyObj 0 1',
'mixedBackwardsManyManyObj 0 1 0',
'mixedBackwardsManyManyObj 0 1 1',
'obj 1',
'hasOneObj 1',
'hasOneSubObj 1',
Expand Down Expand Up @@ -516,6 +551,13 @@ private function expectedEagerLoadRelations(): array
'mixedHasOneObj 1 1 0 1',
'mixedHasManyObj 1 1 1',
'mixedHasOneObj 1 1 1 1',
'mixedBackwardsHasOneObj 1',
'mixedBackwardsHasManyObj 1 0',
'mixedBackwardsManyManyObj 1 0 0',
'mixedBackwardsManyManyObj 1 0 1',
'mixedBackwardsHasManyObj 1 1',
'mixedBackwardsManyManyObj 1 1 0',
'mixedBackwardsManyManyObj 1 1 1',
];
}

Expand Down Expand Up @@ -670,6 +712,25 @@ private function createEagerLoadData(
}
}
}
$mixedBackwardsHasOneObj = new MixedBackwardsHasOneEagerLoadObject();
$mixedBackwardsHasOneObj->Title = "mixedBackwardsHasOneObj $i";
$mixedBackwardsHasOneObjID = $mixedBackwardsHasOneObj->write();

$obj->MixedBackwardsHasOneEagerLoadObjectID = $mixedBackwardsHasOneObjID;
$obj->write();

for ($j = 0; $j < $numLevel2Records; $j++) {
$mixedBackwardsHasManyObj = new MixedBackwardsHasManyEagerLoadObject();
$mixedBackwardsHasManyObj->Title = "mixedBackwardsHasManyObj $i $j";
$mixedBackwardsHasManyObj->MixedBackwardsHasOneEagerLoadObjectID = $mixedBackwardsHasOneObjID;
$mixedBackwardsHasManyObjID = $mixedBackwardsHasManyObj->write();
$mixedBackwardsHasOneObj->MixedBackwardsHasManyEagerLoadObjects()->add($mixedBackwardsHasManyObj);
for ($k = 0; $k < $numLevel3Records; $k++) {
$mixedBackwardsManyManyObj = new MixedBackwardsManyManyEagerLoadObject();
$mixedBackwardsManyManyObj->Title = "mixedBackwardsManyManyObj $i $j $k";
$mixedBackwardsHasManyObj->MixedBackwardsManyManyEagerLoadObjects()->add($mixedBackwardsManyManyObj);
}
}
}
}

Expand Down Expand Up @@ -748,6 +809,16 @@ private function iterateEagerLoadData(DataList $dataList, int $chunks = 0): arra
$results[] = $mixedHasManyObj->MixedHasOneEagerLoadObject()->Title;
}
}
$mixedBackwardsHasOneObj = $obj->MixedBackwardsHasOneEagerLoadObject();
if ($mixedBackwardsHasOneObj) {
$results[] = $mixedBackwardsHasOneObj->Title;
foreach ($mixedBackwardsHasOneObj->MixedBackwardsHasManyEagerLoadObjects() as $mixedBackwardsHasManyObj) {
$results[] = $mixedBackwardsHasManyObj->Title;
foreach ($mixedBackwardsHasManyObj->MixedBackwardsManyManyEagerLoadObjects() as $mixedBackwardsManyManyObj) {
$results[] = $mixedBackwardsManyManyObj->Title;
}
}
}
}
$selectCount = $this->stopCountingSelectQueries();
} finally {
Expand Down Expand Up @@ -1716,6 +1787,17 @@ public function testPolymorphEagerLoading(): void
$this->validateMultipleAppearance($items, 4, EagerLoadObject::get()->eagerLoad('HasOnePolymorphObject'), 'HasOnePolymorphObject');
}

/**
* Tests that attempting to eager load a sub relation to a polymorphic relation will throw an exception.
*/
public function testEagerLoadingSubRelationToPolymorphicException(): void
{
$items = $this->providePolymorphHasOne();
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage("Invalid relation passed to eagerLoad() - HasOnePolymorphObject.ManyManySubEagerLoadObjects. Further nested relations are not supported after polymorphic has_one relation HasOnePolymorphObject.");
EagerLoadObject::get()->eagerLoad("HasOnePolymorphObject.ManyManySubEagerLoadObjects")->toArray();
}

protected function providePolymorphHasOne(): array
{
$subA = new HasOneEagerLoadObject();
Expand Down
1 change: 1 addition & 0 deletions tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class EagerLoadObject extends DataObject implements TestOnly
private static $has_one = [
'HasOneEagerLoadObject' => HasOneEagerLoadObject::class,
'HasOnePolymorphObject' => DataObject::class,
'MixedBackwardsHasOneEagerLoadObject' => MixedBackwardsHasOneEagerLoadObject::class,
];

private static $belongs_to = [
Expand Down
25 changes: 25 additions & 0 deletions tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasManyEagerLoadObject.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

namespace SilverStripe\ORM\Tests\DataListTest\EagerLoading;

use SilverStripe\ORM\DataObject;
use SilverStripe\Dev\TestOnly;

class MixedBackwardsHasManyEagerLoadObject extends DataObject implements TestOnly
{
// Table names become too long when using class name
private static $table_name = 'MixedBackHasManyEagerLoad';

private static $db = [
'Title' => 'Varchar'
];

private static $has_one = [
'MixedBackwardsHasOneEagerLoadObject' => MixedBackwardsHasOneEagerLoadObject::class
];


private static $many_many = [
'MixedBackwardsManyManyEagerLoadObjects' => MixedBackwardsManyManyEagerLoadObject::class
];
}
Loading

0 comments on commit 18392e7

Please sign in to comment.