From 25ed64f1279c26fb09f72dff4247f899fa9d808f Mon Sep 17 00:00:00 2001 From: Mason Dechaineux Date: Mon, 2 Dec 2024 10:46:32 +1000 Subject: [PATCH] FIX eagerLoad crash with nonterminal hasOne relation --- src/ORM/DataList.php | 14 ++ tests/php/ORM/DataListEagerLoadingTest.php | 130 ++++++++++++++---- .../EagerLoading/EagerLoadObject.php | 1 + .../MixedBackwardsHasManyEagerLoadObject.php | 25 ++++ .../MixedBackwardsHasOneEagerLoadObject.php | 19 +++ .../MixedBackwardsManyManyEagerLoadObject.php | 16 +++ 6 files changed, 181 insertions(+), 24 deletions(-) create mode 100644 tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasManyEagerLoadObject.php create mode 100644 tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasOneEagerLoadObject.php create mode 100644 tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsManyManyEagerLoadObject.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 72ccacd7113..43824002391 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -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 $parentRelationData */ $chainToDate[] = $relationName; list( @@ -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( @@ -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]; } diff --git a/tests/php/ORM/DataListEagerLoadingTest.php b/tests/php/ORM/DataListEagerLoadingTest.php index 13c2460c8f8..ebfcdae2fb3 100644 --- a/tests/php/ORM/DataListEagerLoadingTest.php +++ b/tests/php/ORM/DataListEagerLoadingTest.php @@ -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; @@ -76,6 +79,9 @@ public static function getExtraDataObjects() MixedHasManyEagerLoadObject::class, MixedHasOneEagerLoadObject::class, MixedManyManyEagerLoadObject::class, + MixedBackwardsHasOneEagerLoadObject::class, + MixedBackwardsHasManyEagerLoadObject::class, + MixedBackwardsManyManyEagerLoadObject::class, ]; } @@ -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', @@ -349,7 +376,7 @@ public function provideEagerLoadRelations(): array 'BelongsManyManyEagerLoadObjects.BelongsManyManySubEagerLoadObjects', 'MixedManyManyEagerLoadObjects.MixedHasManyEagerLoadObjects.MixedHasOneEagerLoadObject', ], - 'expected' => 73 + 'expected' => 81 ], [ 'iden' => '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 ], ]; } @@ -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', @@ -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', ]; } @@ -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); + } + } } } @@ -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 { @@ -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(); diff --git a/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php index 568d715204d..c0022b5dd86 100644 --- a/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php +++ b/tests/php/ORM/DataListTest/EagerLoading/EagerLoadObject.php @@ -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 = [ diff --git a/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasManyEagerLoadObject.php b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasManyEagerLoadObject.php new file mode 100644 index 00000000000..e134ec060fe --- /dev/null +++ b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasManyEagerLoadObject.php @@ -0,0 +1,25 @@ + 'Varchar' + ]; + + private static $has_one = [ + 'MixedBackwardsHasOneEagerLoadObject' => MixedBackwardsHasOneEagerLoadObject::class + ]; + + + private static $many_many = [ + 'MixedBackwardsManyManyEagerLoadObjects' => MixedBackwardsManyManyEagerLoadObject::class + ]; +} diff --git a/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasOneEagerLoadObject.php b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasOneEagerLoadObject.php new file mode 100644 index 00000000000..fb3bb485585 --- /dev/null +++ b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsHasOneEagerLoadObject.php @@ -0,0 +1,19 @@ + 'Varchar' + ]; + + private static $has_many = [ + 'MixedBackwardsHasManyEagerLoadObjects' => MixedBackwardsHasManyEagerLoadObject::class + ]; +} diff --git a/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsManyManyEagerLoadObject.php b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsManyManyEagerLoadObject.php new file mode 100644 index 00000000000..e8ba574798b --- /dev/null +++ b/tests/php/ORM/DataListTest/EagerLoading/MixedBackwardsManyManyEagerLoadObject.php @@ -0,0 +1,16 @@ + 'Varchar' + ]; +}