Commit order for removals has to consider `SET NULL`, not `nullable` by mpdude · Pull Request #10566 · doctrine/orm

mpdude · 2023-03-06T09:25:10Z

When computing the commit order for entity removals, we have to look out for @ORM\JoinColumn(onDelete="SET NULL") to find places where cyclic associations can be broken.

This is part of the efforts in #10547 to solve various commit-order related problems.

Background

The UoW computes a "commit order" to find the sequence in which tables shall be processed when inserting entities into the database or performing delete operations.

For the insert case, the ORM is able to schedule extra updates that will be performed after all entities have been inserted. Associations which are configured as @ORM\JoinColumn(nullable=true, ...) can be left as NULL in the database when performing the initial INSERT statements, and will be updated once all new entities have been written to the database. This can be used to break cyclic associations between entity instances.

For removals, the ORM does not currently implement up-front UPDATE statements to NULL out associations before DELETE statements are executed. That means when associations form a cycle, users have to configure @ORM\JoinColumn(onDelete="SET NULL", ...) on one of the associations involved. This transfers responsibility to the DBMS to break the cycle at that place.

But, we still have to perform the delete statements in an order that makes this happen early enough. This may be a different order than the one required for the insert case. We can find it only by looking at the onDelete behaviour. We must ignore the nullable property, which is irrelevant, since we do not even try to NULL anything.

Example

Assume three entity classes A, B, C. There are unidirectional one-to-one associations A -> B, B -> C, C -> A. All those associations are nullable= true.

Three entities $a, $b, $c are created from these respective classes and associations are set up.

All operations cascade at the ORM level. So we can test what happens when we start the operations at the three individual entities, but in the end, they will always involve all three of them.

Any insert order will work, so the improvements necessary to solve #10531 or #10532 are not needed here. Since all associations are between different tables, the current table-level computation is good enough.

For the removal case, only the A -> B association has onDelete="SET NULL". So, the only possible execution order is $b, $c, $a. We have to find that regardless of where we start the cascade operation.

The DBMS will set the A -> B association on $a to NULL when we remove $b. We can then remove $c since it is no longer being referred to, then $a.

Related cases

These cases ask for the ORM to perform the extra update before the delete by itself, without DBMS-level support:

Extra bonus

This is what the DALL·E AI thinks it looks like when the UnitOfWork is sequencing entity deletions with cascade.

mpdude · 2023-05-08T20:10:33Z

lib/Doctrine/ORM/UnitOfWork.php

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


Discussed with ppl on Slack that we need not worry about objects being proxies here

mpdude · 2023-05-17T19:34:30Z

@greg0ire @SenseException and @derrabus what do you think about this?

We need to agree on this one to make progress for the #10547 epic.

SenseException

Seems okay, but shouldn't the tests explicitly check that the entities were all 3 deleted?

mpdude · 2023-05-23T05:35:00Z

Thank you @SenseException! I’ll amend the tests to cover that as well.

When computing the commit order for entity removals, we have to look out for `@ORM\JoinColumn(onDelete="SET NULL")` to find places where cyclic associations can be broken. #### Background The UoW computes a "commit order" to find the sequence in which tables shall be processed when inserting entities into the database or performing delete operations. For the insert case, the ORM is able to schedule _extra updates_ that will be performed after all entities have been inserted. Associations which are configured as `@ORM\JoinColumn(nullable=true, ...)` can be left as `NULL` in the database when performing the initial `INSERT` statements, and will be updated once all new entities have been written to the database. This can be used to break cyclic associations between entity instances. For removals, the ORM does not currently implement up-front `UPDATE` statements to `NULL` out associations before `DELETE` statements are executed. That means when associations form a cycle, users have to configure `@ORM\JoinColumn(onDelete="SET NULL", ...)` on one of the associations involved. This transfers responsibility to the DBMS to break the cycle at that place. _But_, we still have to perform the delete statements in an order that makes this happen early enough. This may be a _different_ order than the one required for the insert case. We can find it _only_ by looking at the `onDelete` behaviour. We must ignore the `nullable` property, which is irrelevant, since we do not even try to `NULL` anything. #### Example Assume three entity classes `A`, `B`, `C`. There are unidirectional one-to-one associations `A -> B`, `B -> C`, `C -> A`. All those associations are `nullable= true`. Three entities `$a`, `$b`, `$c` are created from these respective classes and associations are set up. All operations `cascade` at the ORM level. So we can test what happens when we start the operations at the three individual entities, but in the end, they will always involve all three of them. _Any_ insert order will work, so the improvements necessary to solve doctrine#10531 or doctrine#10532 are not needed here. Since all associations are between different tables, the current table-level computation is good enough. For the removal case, only the `A -> B` association has `onDelete="SET NULL"`. So, the only possible execution order is `$b`, `$c`, `$a`. We have to find that regardless of where we start the cascade operation. The DBMS will set the `A -> B` association on `$a` to `NULL` when we remove `$b`. We can then remove `$c` since it is no longer being referred to, then `$a`. #### Related cases These cases ask for the ORM to perform the extra update before the delete by itself, without DBMS-level support: * doctrine#5665 * doctrine#10548

mpdude · 2023-05-23T10:49:37Z

Tests now contain assertions that insertions/deletions did in fact happen.

mpdude · 2023-05-30T20:53:25Z

@SenseException Can we (could you?) merge this, since I addressed your remarks?

#10689 probably will have merge conflicts afterwards, so I could address those.

SenseException · 2023-05-30T20:58:23Z

Yeah, seems like no other review will be done.

mpdude · 2023-05-30T21:24:21Z

Thank you

This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant. See doctrine#9376 for the bug description; doctrine#9377 is obsoleted by doctrine#10547.

This is part of the series of issues fixed by doctrine#10547. In particular, the changes from doctrine#10566 were relevant. See doctrine#10348 for the bug description. Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

mpdude force-pushed the delete-commit-order branch from 0fc3be1 to 8b17ea2 Compare March 6, 2023 09:25

mpdude force-pushed the delete-commit-order branch from 8b17ea2 to 66a7984 Compare May 8, 2023 20:06

mpdude changed the base branch from 2.14.x to entity-level-commit-order May 8, 2023 20:06

mpdude marked this pull request as ready for review May 8, 2023 20:06

mpdude changed the title ~~Failing test: Commit order for removals has to consider SET NULL, not nullable~~ Commit order for removals has to consider SET NULL, not nullable May 8, 2023

mpdude commented May 8, 2023

View reviewed changes

mpdude force-pushed the delete-commit-order branch 3 times, most recently from e0bffda to 5c80c73 Compare May 8, 2023 20:19

greg0ire approved these changes May 19, 2023

View reviewed changes

greg0ire requested review from SenseException, beberlei and derrabus May 19, 2023 07:17

SenseException approved these changes May 22, 2023

View reviewed changes

mpdude force-pushed the delete-commit-order branch from 5c80c73 to e86a47d Compare May 23, 2023 10:48

greg0ire requested a review from SenseException May 23, 2023 12:11

SenseException merged commit ae60cf0 into doctrine:entity-level-commit-order May 30, 2023

mpdude deleted the delete-commit-order branch May 30, 2023 21:24

mpdude mentioned this pull request May 31, 2023

Compute entity-level commit order for entity insertions #10689

Merged

mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023

Add a test case to show doctrine#10348 has been fixed by doctrine#10566

972b529

This was referenced May 31, 2023

Add a test case to show #10348 has been fixed #10732

Merged

Add a test case to show #9376 and #9377 have been resolved #10733

Closed

mpdude mentioned this pull request Aug 17, 2023

(Try to) add a reproducer for a delete ordering issue #10912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Commit order for removals has to consider `SET NULL`, not `nullable`#10566
SenseException merged 1 commit intodoctrine:entity-level-commit-orderfrom
mpdude:delete-commit-order

mpdude commented Mar 6, 2023 •

edited

Loading

Uh oh!

mpdude May 8, 2023 •

edited

Loading

Uh oh!

mpdude commented May 17, 2023

Uh oh!

SenseException left a comment

Uh oh!

mpdude commented May 23, 2023

Uh oh!

mpdude commented May 23, 2023

Uh oh!

mpdude commented May 30, 2023

Uh oh!

SenseException commented May 30, 2023

Uh oh!

mpdude commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mpdude commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Example

Related cases

Extra bonus

Uh oh!

mpdude May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented May 17, 2023

Uh oh!

SenseException left a comment

Choose a reason for hiding this comment

Uh oh!

mpdude commented May 23, 2023

Uh oh!

mpdude commented May 23, 2023

Uh oh!

mpdude commented May 30, 2023

Uh oh!

SenseException commented May 30, 2023

Uh oh!

mpdude commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mpdude commented Mar 6, 2023 •

edited

Loading

mpdude May 8, 2023 •

edited

Loading