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.

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

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

Merged

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Mar 6, 2023

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.

DALL·E 2023-05-09 13 09 58

@mpdude mpdude force-pushed the delete-commit-order branch from 0fc3be1 to 8b17ea2 Compare March 6, 2023 09:25
@mpdude mpdude force-pushed the delete-commit-order branch from 8b17ea2 to 66a7984 Compare May 8, 2023 20:06
@mpdude mpdude changed the base branch from 2.14.x to entity-level-commit-order May 8, 2023 20:06
@mpdude mpdude marked this pull request as ready for review May 8, 2023 20:06
@mpdude 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
}
}

$targetEntity = $class->getFieldValue($entity, $assoc['fieldName']);
Copy link
Contributor Author

@mpdude mpdude May 8, 2023

Choose a reason for hiding this comment

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

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

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

mpdude commented May 17, 2023

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

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

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

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

@mpdude
Copy link
Contributor Author

mpdude commented May 23, 2023

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 mpdude force-pushed the delete-commit-order branch from 5c80c73 to e86a47d Compare May 23, 2023 10:48
@mpdude
Copy link
Contributor Author

mpdude commented May 23, 2023

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

@greg0ire greg0ire requested a review from SenseException May 23, 2023 12:11
@mpdude
Copy link
Contributor Author

mpdude commented May 30, 2023

@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
Copy link
Member

Yeah, seems like no other review will be done.

@SenseException SenseException merged commit ae60cf0 into doctrine:entity-level-commit-order May 30, 2023
@mpdude mpdude deleted the delete-commit-order branch May 30, 2023 21:24
@mpdude
Copy link
Contributor Author

mpdude commented May 30, 2023

Thank you

mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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 added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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 added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants