Skip to content

Commit

Permalink
Compute entity-level commit order for entity insertions
Browse files Browse the repository at this point in the history
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

#### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

#### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

#### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change – so the order of entities is generally stable, equal to the previous implementation.
  • Loading branch information
mpdude committed May 9, 2023
1 parent 9ac063d commit 38f3faf
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 27 deletions.
73 changes: 48 additions & 25 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use Doctrine\ORM\Id\AssignedGenerator;
use Doctrine\ORM\Internal\CommitOrderCalculator;
use Doctrine\ORM\Internal\HydrationCompleteHandler;
use Doctrine\ORM\Internal\TopologicalSort;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\MappingException;
use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter;
Expand Down Expand Up @@ -408,9 +409,6 @@ public function commit($entity = null)

$this->dispatchOnFlushEvent();

// Now we need a commit order to maintain referential integrity
$commitOrder = $this->getCommitOrder();

$conn = $this->em->getConnection();
$conn->beginTransaction();

Expand All @@ -431,7 +429,7 @@ public function commit($entity = null)
// into account (new entities referring to other new entities), since all other types (entities
// with updates or scheduled deletions) are currently not a problem, since they are already
// in the database.
$this->executeInserts($this->computeInsertExecutionOrder($commitOrder));
$this->executeInserts($this->computeInsertExecutionOrder());
}

if ($this->entityUpdates) {
Expand All @@ -456,7 +454,7 @@ public function commit($entity = null)
// Entity deletions come last. Their order only needs to take care of other deletions
// (first delete entities depending upon others, before deleting depended-upon entities).
if ($this->entityDeletions) {
$this->executeDeletions($this->computeDeleteExecutionOrder($commitOrder));
$this->executeDeletions($this->computeDeleteExecutionOrder());
}

// Commit failed silently
Expand Down Expand Up @@ -1265,36 +1263,61 @@ private function executeDeletions(array $entities): void
}
}

/**
* @param list<ClassMetadata> $commitOrder
*
* @return list<object>
*/
private function computeInsertExecutionOrder(array $commitOrder): array
/** @return list<object> */
private function computeInsertExecutionOrder(): array
{
$result = [];
foreach ($commitOrder as $class) {
$className = $class->name;
foreach ($this->entityInsertions as $entity) {
if ($this->em->getClassMetadata(get_class($entity))->name !== $className) {
$sort = new TopologicalSort();

// First make sure we have all the nodes
foreach ($this->entityInsertions as $entity) {
$sort->addNode($entity);
}

// Now add edges
foreach ($this->entityInsertions as $entity) {
$class = $this->em->getClassMetadata(get_class($entity));

foreach ($class->associationMappings as $assoc) {
// We only need to consider the owning sides of to-one associations,
// since many-to-many associations are persisted at a later step and
// have no insertion order problems (all entities already in the database
// at that time).
if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) {
continue;
}

$result[] = $entity;
$targetEntity = $class->getFieldValue($entity, $assoc['fieldName']);

// If there is no entity that we need to refer to, or it is already in the
// database (i. e. does not have to be inserted), no need to consider it.
if ($targetEntity === null || ! $sort->hasNode($targetEntity)) {
continue;
}

// According to https://www.doctrine-project.org/projects/doctrine-orm/en/2.14/reference/annotations-reference.html#annref_joincolumn,
// the default for "nullable" is true. Unfortunately, it seems this default is not applied at the metadata driver, factory or other
// level, but in fact we may have an undefined 'nullable' key here, so we must assume that default here as well.
//
// Same in \Doctrine\ORM\Tools\EntityGenerator::isAssociationIsNullable or \Doctrine\ORM\Persisters\Entity\BasicEntityPersister::getJoinSQLForJoinColumns,
// to give two examples.
assert(isset($assoc['joinColumns']));
$joinColumns = reset($assoc['joinColumns']);
$isNullable = ! isset($joinColumns['nullable']) || $joinColumns['nullable'];

// Add dependency. The dependency direction implies that "$targetEntity has to go before $entity",
// so we can work through the topo sort result from left to right (with all edges pointing right).
$sort->addEdge($targetEntity, $entity, $isNullable);
}
}

return $result;
return $sort->sort();
}

/**
* @param list<ClassMetadata> $commitOrder
*
* @return list<object>
*/
private function computeDeleteExecutionOrder(array $commitOrder): array
/** @return list<object> */
private function computeDeleteExecutionOrder(): array
{
$result = [];
$commitOrder = $this->getCommitOrder();
$result = [];
foreach (array_reverse($commitOrder) as $class) {
$className = $class->name;
foreach ($this->entityDeletions as $entity) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,11 @@ public function testFind(): void

public function testEagerLoadsAssociation(): void
{
$this->createFixture();
$customerId = $this->createFixture();

$query = $this->_em->createQuery('select c, m from Doctrine\Tests\Models\ECommerce\ECommerceCustomer c left join c.mentor m where c.id = :id');
$query->setParameter('id', $customerId);

$query = $this->_em->createQuery('select c, m from Doctrine\Tests\Models\ECommerce\ECommerceCustomer c left join c.mentor m order by c.id asc');
$result = $query->getResult();
$customer = $result[0];
$this->assertLoadingOfAssociation($customer);
Expand Down
1 change: 1 addition & 0 deletions tests/Doctrine/Tests/OrmFunctionalTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ protected function tearDown(): void
$conn->executeStatement('DELETE FROM ecommerce_products_categories');
$conn->executeStatement('DELETE FROM ecommerce_products_related');
$conn->executeStatement('DELETE FROM ecommerce_carts');
$conn->executeStatement('DELETE FROM ecommerce_customers WHERE mentor_id IS NOT NULL');
$conn->executeStatement('DELETE FROM ecommerce_customers');
$conn->executeStatement('DELETE FROM ecommerce_features');
$conn->executeStatement('DELETE FROM ecommerce_products');
Expand Down

0 comments on commit 38f3faf

Please sign in to comment.