Compute entity-level commit order for entity insertions #10689

mpdude · 2023-05-08T16:02:47Z

Make the UoW find a commit order for entity insertions not on the class, but at the entity level. This is the third step to break #10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from #10592 and the refactoring from #10651.

Current situation

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

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 1310 to 1325 in 70477d8

    
           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']));

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.

Extra bonus

This is what the DALL·E AI thinks it looks like when the UnitOfWork is scheduling the sequence of entity insertions.

derrabus · 2023-05-08T21:17:30Z

lib/Doctrine/ORM/UnitOfWork.php

+                // 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.


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.

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?

lib/Doctrine/ORM/UnitOfWork.php

tests/Doctrine/Tests/Models/PersistentObject/PersistentEntity.php

tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php

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.

mpdude · 2023-05-31T07:33:47Z

Rebased and resolved conflicts after #10566 has been merged.

I wanted to spare clean-ups (remove the now unused UoW::getCommitOrder() method) for another PR, but static analysis checks forced me to remove it right away.

mpdude · 2023-05-31T07:35:04Z

lib/Doctrine/ORM/UnitOfWork.php

-     *
-     * @return list<ClassMetadata>
-     */
-    private function getCommitOrder(): array


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

mpdude · 2023-05-31T07:42:09Z

@greg0ire and @SenseException

I think this is the last PR regarding changes in implementation. Once we've got this merged, I can start making PRs to pick/collect/provide (regression) tests for all the fixed issues.

That means the hard work would be done, hopefully reviewing tests added is easy :-)

mpdude · 2023-06-02T05:53:52Z

@greg0ire could you 👀 please? 🙏🏻

mpdude · 2023-06-02T13:20:42Z

Please merge this, so I can continue with more tests on the entity-level-commit-order branch.

mpdude mentioned this pull request May 8, 2023

Compute the commit order (inserts/deletes) on the entity level #10547

Merged

21 tasks

mpdude force-pushed the insert-entity-level-topo-sort branch 3 times, most recently from e007a7a to 2723132 Compare May 8, 2023 16:38

derrabus reviewed May 8, 2023

View reviewed changes

mpdude force-pushed the insert-entity-level-topo-sort branch 2 times, most recently from 3394356 to 38f3faf Compare May 9, 2023 11:08

SenseException reviewed May 25, 2023

View reviewed changes

tests/Doctrine/Tests/ORM/Functional/OneToOneSelfReferentialAssociationTest.php Show resolved Hide resolved

SenseException requested a review from derrabus May 28, 2023 20:54

SenseException approved these changes May 28, 2023

View reviewed changes

SenseException requested a review from greg0ire May 28, 2023 20:55

mpdude mentioned this pull request May 30, 2023

Commit order for removals has to consider SET NULL, not nullable #10566

Merged

mpdude force-pushed the insert-entity-level-topo-sort branch from 38f3faf to d76fc4e Compare May 31, 2023 07:17

Remove the now unused UnitOfWork::getCommitOrder() method

aa27b3a

mpdude force-pushed the insert-entity-level-topo-sort branch from 2e02bbd to aa27b3a Compare May 31, 2023 07:32

mpdude commented May 31, 2023

View reviewed changes

greg0ire approved these changes Jun 2, 2023

View reviewed changes

greg0ire merged commit 330c0bc into doctrine:entity-level-commit-order Jun 2, 2023

mpdude deleted the insert-entity-level-topo-sort branch June 2, 2023 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute entity-level commit order for entity insertions #10689

Compute entity-level commit order for entity insertions #10689

mpdude commented May 8, 2023 •

edited

Loading

derrabus May 8, 2023

mpdude May 9, 2023

mpdude commented May 31, 2023

mpdude May 31, 2023

mpdude commented May 31, 2023 •

edited

Loading

mpdude commented Jun 2, 2023

mpdude commented Jun 2, 2023

	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']));

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

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

Compute entity-level commit order for entity insertions #10689

Compute entity-level commit order for entity insertions #10689

Conversation

mpdude commented May 8, 2023 • edited Loading

Current situation

Suggested change

Extra bonus

derrabus May 8, 2023

Choose a reason for hiding this comment

mpdude May 9, 2023

Choose a reason for hiding this comment

mpdude commented May 31, 2023

mpdude May 31, 2023

Choose a reason for hiding this comment

mpdude commented May 31, 2023 • edited Loading

mpdude commented Jun 2, 2023

mpdude commented Jun 2, 2023

mpdude commented May 8, 2023 •

edited

Loading

mpdude commented May 31, 2023 •

edited

Loading