Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compute entity-level commit order for entity insertions #10689

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Compute entity-level commit order for entity insertions #10689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d76fc4e
Compute entity-level commit order for entity insertions
mpdude May 8, 2023
aa27b3a
Remove the now unused UnitOfWork::getCommitOrder() method
mpdude May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 39 additions & 84 deletions lib/Doctrine/ORM/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
use function array_key_exists;
use function array_map;
use function array_merge;
use function array_pop;
use function array_sum;
use function array_values;
use function assert;
Expand Down Expand Up @@ -1266,20 +1265,51 @@ private function executeDeletions(array $entities): void
/** @return list<object> */
private function computeInsertExecutionOrder(): array
{
$commitOrder = $this->getCommitOrder();
$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.
Comment on lines +1296 to +1298
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, join columns are nullable by default for one-to-many relations and not nullable for many-to-many relations. If at this point you need to know these defaults, maybe ClassMetadata should amend the nullable key if it's missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have similar code here...

orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

Lines 2079 to 2083 in 70477d8

foreach ($joinColumns as $joinColumn) {
if (! isset($joinColumn['nullable']) || $joinColumn['nullable']) {
return 'LEFT JOIN';
}
}

... or here ...

orm/lib/Doctrine/ORM/Tools/EntityGenerator.php

Lines 1260 to 1264 in 70477d8

foreach ($joinColumns as $joinColumn) {
if (isset($joinColumn['nullable']) && ! $joinColumn['nullable']) {
return false;
}
}

... and for fields in general (not only join columns) for example here:

orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Line 1394 in 70477d8

return $mapping !== false && isset($mapping['nullable']) && $mapping['nullable'];

I agree that it would be better if the nullable (and possibly other keys in those join column definition or field mapping configuration arrays) were initialized with defaults in the CMF; but that's out of scope here, so can we leave it for another PR?

//
// 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();
}

/** @return list<object> */
Expand Down Expand Up @@ -1336,81 +1366,6 @@ private function computeDeleteExecutionOrder(): array
return $sort->sort();
}

/**
* Gets the commit order.
*
* @return list<ClassMetadata>
*/
private function getCommitOrder(): array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of this PR, this method is no longer used.

{
$calc = $this->getCommitOrderCalculator();

// See if there are any new classes in the changeset, that are not in the
// commit order graph yet (don't have a node).
// We have to inspect changeSet to be able to correctly build dependencies.
// It is not possible to use IdentityMap here because post inserted ids
// are not yet available.
$newNodes = [];

foreach (array_merge($this->entityInsertions, $this->entityUpdates, $this->entityDeletions) as $entity) {
$class = $this->em->getClassMetadata(get_class($entity));

if ($calc->hasNode($class->name)) {
continue;
}

$calc->addNode($class->name, $class);

$newNodes[] = $class;
}

// Calculate dependencies for new nodes
while ($class = array_pop($newNodes)) {
foreach ($class->associationMappings as $assoc) {
if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) {
continue;
}

$targetClass = $this->em->getClassMetadata($assoc['targetEntity']);

if (! $calc->hasNode($targetClass->name)) {
$calc->addNode($targetClass->name, $targetClass);

$newNodes[] = $targetClass;
}

$joinColumns = reset($assoc['joinColumns']);

$calc->addDependency($targetClass->name, $class->name, (int) empty($joinColumns['nullable']));

// If the target class has mapped subclasses, these share the same dependency.
if (! $targetClass->subClasses) {
continue;
}

foreach ($targetClass->subClasses as $subClassName) {
$targetSubClass = $this->em->getClassMetadata($subClassName);

if (! $calc->hasNode($subClassName)) {
$calc->addNode($targetSubClass->name, $targetSubClass);

$newNodes[] = $targetSubClass;
}

$calc->addDependency($targetSubClass->name, $class->name, 1);
}
}
}

// Remove duplicate class entries
$result = [];
foreach ($calc->sort() as $classMetadata) {
$result[$classMetadata->name] = $classMetadata;
}

return array_values($result);
}

/**
* Schedules an entity for insertion into the database.
* If the entity already has an identifier, it will be added to the identity map.
Expand Down
1 change: 0 additions & 1 deletion psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3031,7 +3031,6 @@
<code>setValue</code>
</PossiblyNullReference>
<PossiblyUndefinedArrayOffset>
<code><![CDATA[$assoc['joinColumns']]]></code>
<code><![CDATA[$assoc['orphanRemoval']]]></code>
<code><![CDATA[$assoc['targetToSourceKeyColumns']]]></code>
</PossiblyUndefinedArrayOffset>
Expand Down
6 changes: 4 additions & 2 deletions tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php
View file
Open in desktop
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);
SenseException marked this conversation as resolved.
Show resolved Hide resolved

$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
View file
Open in desktop
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
Loading