From b42cf99402f5cd6a899fcbc69cc9072a084ceea5 Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Mon, 24 Apr 2023 11:39:33 +0200 Subject: [PATCH 1/2] Prepare entity-level commit order computation in the UnitOfWork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the second chunk to break #10547 into smaller PRs suitable for reviewing. It prepares the `UnitOfWork` to work with a commit order computed on the entity level, as opposed to a class-based ordering as before. #### Background #10531 and #10532 show that it is not always possible to run `UnitOfWork::commit()` with a commit order given in terms of entity _classes_. Instead it is necessary to process entities in an order computed on the _object_ level. That may include * a particular order for multiple entities of the _same_ class * a particular order for entities of _different_ classes, possibly even going back and forth (entity `$a1` of class `A`, then `$b` of class `B`, then `$a2` of class `A` – revisiting that class). This PR is about preparing the `UnitOfWork` so that its methods will be able to perform inserts and deletions on that level of granularity. Subsequent PRs will deal with implementing that particular order computation. #### Suggested change Change the private `executeInserts` and `executeDeletions` methods so that they do not take a `ClassMetadata` identifying the class of entities that shall be processed, but pass them the list of entities directly. The lists of entities are built in two dedicated methods. That happens basically as previously, by iterating over `$this->entityInsertions` or `$this->entityDeletions` and filtering those by class. #### Potential (BC breaking?) changes that need review scrutiny * `\Doctrine\ORM\Persisters\Entity\EntityPersister::addInsert()` was previously called multiple times, before the insert would be performed by a call to `\Doctrine\ORM\Persisters\Entity\EntityPersister::executeInserts()`. With the changes here, this batching effectively no longer takes place. `executeInserts()` will always see one entity/insert only, and there may be multiple `executeInserts()` calls during a single `UoW::commit()` phase. * The caching in `\Doctrine\ORM\Cache\Persister\Entity\AbstractEntityPersister` previously would cache entities from the last executed insert batch only. Now it will collect entities across multiple batches. I don't know if that poses a problem. * Overhead in `\Doctrine\ORM\Persisters\Entity\BasicEntityPersister::executeInserts` is incurred multiple times; that may, however, only be about SQL statement preparation and might be salvageable. * The `postPersist` event previously was not dispatched before _all_ instances of a particular entity class had been written to the database. Now, it will be dispatched immediately after every single entity that has been inserted. --- .../Entity/AbstractEntityPersister.php | 9 +- lib/Doctrine/ORM/UnitOfWork.php | 180 +++++++++++------- psalm-baseline.xml | 4 - 3 files changed, 117 insertions(+), 76 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php index 2fbc63855b3..cfd625d009b 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php @@ -23,6 +23,7 @@ use Doctrine\ORM\Persisters\Entity\EntityPersister; use Doctrine\ORM\UnitOfWork; +use function array_merge; use function assert; use function serialize; use function sha1; @@ -314,7 +315,13 @@ public function getOwningTable($fieldName) */ public function executeInserts() { - $this->queuedCache['insert'] = $this->persister->getInserts(); + // The commit order/foreign key relationships may make it necessary that multiple calls to executeInsert() + // are performed, so collect all the new entities. + $newInserts = $this->persister->getInserts(); + + if ($newInserts) { + $this->queuedCache['insert'] = array_merge($this->queuedCache['insert'] ?? [], $newInserts); + } return $this->persister->executeInserts(); } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 1e949cab4ee..94331d9ba82 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -57,10 +57,10 @@ use function array_map; use function array_merge; use function array_pop; +use function array_reverse; use function array_sum; use function array_values; use function assert; -use function count; use function current; use function func_get_arg; use function func_num_args; @@ -426,32 +426,37 @@ public function commit($entity = null) } if ($this->entityInsertions) { - foreach ($commitOrder as $class) { - $this->executeInserts($class); - } + // Perform entity insertions first, so that all new entities have their rows in the database + // and can be referred to by foreign keys. The commit order only needs to take new entities + // 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)); } if ($this->entityUpdates) { - foreach ($commitOrder as $class) { - $this->executeUpdates($class); - } + // Updates do not need to follow a particular order + $this->executeUpdates(); } // Extra updates that were requested by persisters. + // This may include foreign keys that could not be set when an entity was inserted, + // which may happen in the case of circular foreign key relationships. if ($this->extraUpdates) { $this->executeExtraUpdates(); } // Collection updates (deleteRows, updateRows, insertRows) + // No particular order is necessary, since all entities themselves are already + // in the database foreach ($this->collectionUpdates as $collectionToUpdate) { $this->getCollectionPersister($collectionToUpdate->getMapping())->update($collectionToUpdate); } - // Entity deletions come last and need to be in reverse commit order + // 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) { - for ($count = count($commitOrder), $i = $count - 1; $i >= 0 && $this->entityDeletions; --$i) { - $this->executeDeletions($commitOrder[$i]); - } + $this->executeDeletions($this->computeDeleteExecutionOrder($commitOrder)); } // Commit failed silently @@ -1114,64 +1119,52 @@ public function recomputeSingleEntityChangeSet(ClassMetadata $class, $entity) } /** - * Executes all entity insertions for entities of the specified type. + * Executes entity insertions in the given order + * + * @param list $entities */ - private function executeInserts(ClassMetadata $class): void + private function executeInserts(array $entities): void { - $entities = []; - $className = $class->name; - $persister = $this->getEntityPersister($className); - $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist); - - $insertionsForClass = []; - - foreach ($this->entityInsertions as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } - - $insertionsForClass[$oid] = $entity; + foreach ($entities as $entity) { + $oid = spl_object_id($entity); + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist); $persister->addInsert($entity); unset($this->entityInsertions[$oid]); - if ($invoke !== ListenersInvoker::INVOKE_NONE) { - $entities[] = $entity; - } - } - - $postInsertIds = $persister->executeInserts(); + $postInsertIds = $persister->executeInserts(); - if ($postInsertIds) { - // Persister returned post-insert IDs - foreach ($postInsertIds as $postInsertId) { - $idField = $class->getSingleIdentifierFieldName(); - $idValue = $this->convertSingleFieldIdentifierToPHPValue($class, $postInsertId['generatedId']); + if ($postInsertIds) { + // Persister returned post-insert IDs + foreach ($postInsertIds as $postInsertId) { + $idField = $class->getSingleIdentifierFieldName(); + $idValue = $this->convertSingleFieldIdentifierToPHPValue($class, $postInsertId['generatedId']); - $entity = $postInsertId['entity']; - $oid = spl_object_id($entity); + $entity = $postInsertId['entity']; + $oid = spl_object_id($entity); - $class->reflFields[$idField]->setValue($entity, $idValue); + $class->reflFields[$idField]->setValue($entity, $idValue); - $this->entityIdentifiers[$oid] = [$idField => $idValue]; - $this->entityStates[$oid] = self::STATE_MANAGED; - $this->originalEntityData[$oid][$idField] = $idValue; + $this->entityIdentifiers[$oid] = [$idField => $idValue]; + $this->entityStates[$oid] = self::STATE_MANAGED; + $this->originalEntityData[$oid][$idField] = $idValue; - $this->addToIdentityMap($entity); - } - } else { - foreach ($insertionsForClass as $oid => $entity) { + $this->addToIdentityMap($entity); + } + } else { if (! isset($this->entityIdentifiers[$oid])) { //entity was not added to identity map because some identifiers are foreign keys to new entities. //add it now $this->addToEntityIdentifiersAndEntityMap($class, $oid, $entity); } } - } - foreach ($entities as $entity) { - $this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke); + if ($invoke !== ListenersInvoker::INVOKE_NONE) { + $this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke); + } } } @@ -1209,19 +1202,15 @@ private function addToEntityIdentifiersAndEntityMap( } /** - * Executes all entity updates for entities of the specified type. + * Executes all entity updates */ - private function executeUpdates(ClassMetadata $class): void + private function executeUpdates(): void { - $className = $class->name; - $persister = $this->getEntityPersister($className); - $preUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::preUpdate); - $postUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postUpdate); - foreach ($this->entityUpdates as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $preUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::preUpdate); + $postUpdateInvoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postUpdate); if ($preUpdateInvoke !== ListenersInvoker::INVOKE_NONE) { $this->listenersInvoker->invoke($class, Events::preUpdate, $entity, new PreUpdateEventArgs($entity, $this->em, $this->getEntityChangeSet($entity)), $preUpdateInvoke); @@ -1242,18 +1231,17 @@ private function executeUpdates(ClassMetadata $class): void } /** - * Executes all entity deletions for entities of the specified type. + * Executes all entity deletions + * + * @param list $entities */ - private function executeDeletions(ClassMetadata $class): void + private function executeDeletions(array $entities): void { - $className = $class->name; - $persister = $this->getEntityPersister($className); - $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postRemove); - - foreach ($this->entityDeletions as $oid => $entity) { - if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { - continue; - } + foreach ($entities as $entity) { + $oid = spl_object_id($entity); + $class = $this->em->getClassMetadata(get_class($entity)); + $persister = $this->getEntityPersister($class->name); + $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postRemove); $persister->delete($entity); @@ -1277,6 +1265,50 @@ private function executeDeletions(ClassMetadata $class): void } } + /** + * @param list $commitOrder + * + * @return list + */ + private function computeInsertExecutionOrder(array $commitOrder): array + { + $result = []; + foreach ($commitOrder as $class) { + $className = $class->name; + foreach ($this->entityInsertions as $entity) { + if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { + continue; + } + + $result[] = $entity; + } + } + + return $result; + } + + /** + * @param list $commitOrder + * + * @return list + */ + private function computeDeleteExecutionOrder(array $commitOrder): array + { + $result = []; + foreach (array_reverse($commitOrder) as $class) { + $className = $class->name; + foreach ($this->entityDeletions as $entity) { + if ($this->em->getClassMetadata(get_class($entity))->name !== $className) { + continue; + } + + $result[] = $entity; + } + } + + return $result; + } + /** * Gets the commit order. * @@ -1343,7 +1375,13 @@ private function getCommitOrder(): array } } - return $calc->sort(); + // Remove duplicate class entries + $result = []; + foreach ($calc->sort() as $classMetadata) { + $result[$classMetadata->name] = $classMetadata; + } + + return array_values($result); } /** diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 1c14701234a..584de36bc9f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -3043,10 +3043,6 @@ nonCascadedNewDetectedEntities]]> - - = 0 && $this->entityDeletions]]> - entityDeletions]]> - is_array($entity) From ed3432794130065a0c6d05fdf061ee8acd8bae59 Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Mon, 24 Apr 2023 13:56:59 +0200 Subject: [PATCH 2/2] More precisely state conditions for the postPersist event --- docs/en/reference/events.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/en/reference/events.rst b/docs/en/reference/events.rst index 97c3a6f339c..f91a947ec70 100644 --- a/docs/en/reference/events.rst +++ b/docs/en/reference/events.rst @@ -707,8 +707,8 @@ not directly mapped by Doctrine. ``UPDATE`` statement. - The ``postPersist`` event occurs for an entity after the entity has been made persistent. It will be invoked after the - database insert operations. Generated primary key values are - available in the postPersist event. + database insert operation for that entity. A generated primary key value for + the entity will be available in the postPersist event. - The ``postRemove`` event occurs for an entity after the entity has been deleted. It will be invoked after the database delete operations. It is not called for a DQL ``DELETE`` statement.