forked from doctrine/orm
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve topological sort result order
This PR changes a detail in the commit order computation for depended-upon entities. We have a parent-child relationship between two entity classes. The association is parent one-to-many children, with the child entities containing the (owning side) back-reference. Cascade-persist is not used, so all entities have to be passed to `EntityManager::persist()`. Before v2.16.0, two child entities C1 and C2 will be inserted in the same order in which they are passed to `persist()`, and that is regardless of whether the parent entity was passed to `persist()` before or after the child entities. As of v2.16.0, passing the parent entity to `persist()` _after_ the child entities will lead to an insert order that is _reversed_ compared to the order of `persist()` calls. This PR makes the order consistent in both cases, as it was before v2.16.0. #### Cause When the parent is passed to `persist()` after the children, commit order computation has to re-arrange the entities. The parent must be inserted first since it is referred to by the children. The implementation of the topological sort from doctrine#10547 processed entities in reverse `persist()` order and unshifted finished nodes to an array to obtain the final result. That leads to dependencies (parent → before c1, parent → before c2) showing up in the result in the reverse order of which they were added. This PR changes the topological sort to produce a result in the opposite order ("all edges pointing left"), which helps to avoid the duplicate array order reversal. #### Discussion * This PR _does not_ change semantics of the `persist()` so that entities would (under all ciscumstances) be inserted in the order of `persist()` calls. * It fixes an unnecessary inconsistency between versions before 2.16.0 and after. In particular, it may be surprising that the insert order for the child entities depends on whether another referred-to entity (the parent) was added before or after them. * _Both_ orders (c1 before or after c2) are technically and logically correct with regard to the agreement that `commit()` is free to arrange entities in a way that allows for efficient insertion into the database. Fixes doctrine#11058.
- Loading branch information
Showing
4 changed files
with
223 additions
and
29 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
146 changes: 146 additions & 0 deletions
146
tests/Doctrine/Tests/ORM/Functional/Ticket/GH11058Test.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Doctrine\Tests\ORM\Functional\Ticket; | ||
|
|
||
| use Doctrine\Common\Collections\ArrayCollection; | ||
| use Doctrine\Common\Collections\Collection; | ||
| use Doctrine\ORM\Mapping as ORM; | ||
| use Doctrine\Tests\OrmFunctionalTestCase; | ||
|
|
||
| class GH11058Test extends OrmFunctionalTestCase | ||
| { | ||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| $this->setUpEntitySchema([ | ||
| GH11058Parent::class, | ||
| GH11058Child::class, | ||
| ]); | ||
| } | ||
|
|
||
| public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedLast(): void | ||
| { | ||
| [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); | ||
|
|
||
| $this->_em->persist($child1); | ||
| $this->_em->persist($child2); | ||
| $this->_em->persist($parent); | ||
| $this->_em->flush(); | ||
|
|
||
| self::assertTrue($child1->id < $child2->id); | ||
| } | ||
|
|
||
| public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedLast(): void | ||
| { | ||
| [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); | ||
|
|
||
| $this->_em->persist($child2); | ||
| $this->_em->persist($child1); | ||
| $this->_em->persist($parent); | ||
| $this->_em->flush(); | ||
|
|
||
| self::assertTrue($child2->id < $child1->id); | ||
| } | ||
|
|
||
| public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedFirst(): void | ||
| { | ||
| [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); | ||
|
|
||
| $this->_em->persist($parent); | ||
| $this->_em->persist($child1); | ||
| $this->_em->persist($child2); | ||
| $this->_em->flush(); | ||
|
|
||
| self::assertTrue($child1->id < $child2->id); | ||
| } | ||
|
|
||
| public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedFirst(): void | ||
| { | ||
| [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities(); | ||
|
|
||
| $this->_em->persist($parent); | ||
| $this->_em->persist($child2); | ||
| $this->_em->persist($child1); | ||
| $this->_em->flush(); | ||
|
|
||
| self::assertTrue($child2->id < $child1->id); | ||
| } | ||
|
|
||
| private function createParentWithTwoChildEntities(): array | ||
| { | ||
| $parent = new GH11058Parent(); | ||
| $child1 = new GH11058Child(); | ||
| $child2 = new GH11058Child(); | ||
|
|
||
| $parent->addChild($child1); | ||
| $parent->addChild($child2); | ||
|
|
||
| return [$parent, $child1, $child2]; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @ORM\Entity() | ||
| */ | ||
| class GH11058Parent | ||
| { | ||
| /** | ||
| * @ORM\Id | ||
| * @ORM\Column(type="integer") | ||
| * @ORM\GeneratedValue | ||
| * | ||
| * @var int | ||
| */ | ||
| public $id; | ||
|
|
||
| /** | ||
| * @ORM\OneToMany(targetEntity="GH11058Child", mappedBy="parent") | ||
| * | ||
| * @var Collection<int, GH11058Child> | ||
| */ | ||
| public $children; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->children = new ArrayCollection(); | ||
| } | ||
|
|
||
| public function addChild(GH11058Child $child): void | ||
| { | ||
| if (! $this->children->contains($child)) { | ||
| $this->children->add($child); | ||
| $child->setParent($this); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @ORM\Entity() | ||
| */ | ||
| class GH11058Child | ||
| { | ||
| /** | ||
| * @ORM\Id | ||
| * @ORM\Column(type="integer") | ||
| * @ORM\GeneratedValue | ||
| * | ||
| * @var int | ||
| */ | ||
| public $id; | ||
|
|
||
| /** | ||
| * @ORM\ManyToOne(targetEntity="GH11058Parent", inversedBy="children") | ||
| * | ||
| * @var GH11058Parent | ||
| */ | ||
| public $parent; | ||
|
|
||
| public function setParent(GH11058Parent $parent): void | ||
| { | ||
| $this->parent = $parent; | ||
| $parent->addChild($this); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters