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

[2.10.2] Ignore dependencies having onDelete="SET NULL" when computing commit order for removal operations #9192

Closed
DoobleD opened this issue Nov 19, 2021 · 4 comments · Fixed by #10547

Comments

@DoobleD
Copy link

DoobleD commented Nov 19, 2021

Feature Request

Q A
New Feature yes
RFC ?
BC Break no

Summary

TLDR: Doctrine could find a valid delete commit order in some circular references cases if it were ignoring dependencies with onDelete="SET NULL".

I have 3 entities with the following relationships:

  • A contains a OneToMany to B with cascade={"remove"}.
  • B contains a OneToMany to C with cascade={"remove"}.
  • A also contains a OneToOne to C with no inverse side, nullable=true and onDelete="SET NULL".

When I remove A via the entity manager, I get a foreign key violation error because the commit order Doctrine computes is B, C, A. Indeed B can't be deleted first since it is referenced by C.

I expected the order to be C, B, A instead. That wouldn't violate FK constraints. I eventually realized I have a circular reference (A references C and C references A via B) and that Doctrine doesn't know how to deal with it.

The thing is, the circular reference is not really one when deleting, because of onDelete="SET NULL". If Doctrine were to ignore such dependencies on delete, the commit order would end up correct.

Is there any chance this could be implemented?

Disclaimer: I'm not very familiar with Doctrine and DB engines in general, there could be downsides I'm not seeing.

More precisely what happens

UnitOfWork::getCommitOrder correctly lists the dependencies for each nodes:

  • Node C with dep A,
  • Node B with dep C,
  • Node A with deps B and C (in that order, because B is the first entity encountered which is the owning side of A).

The order in which nodes are listed (C, B, A), comes from UnitOfWork::doRemove's recursion putting the most deeply nested child first in the deletion list, and working back towards the parents.

Note in particular the dep A in C. It comes from the OneToOne relationship between A and C (the one with onDelete="SET NULL").

A bit later in CommitOrderCalculator::visit (which computes the actual commit order from the dependency list):

  • C is processed first.
  • Then A (being a dependency of C).
  • Then B (being the first dependency of A).
  • C, the dependency of B, is in progress already so there's no new recursion on it.
  • B is added to the sorted list.
  • Being done with B we bubble back to A but its next dependency C is in progress already so we bubble back to C.
  • C is added to the sorted list (it has no other dependency).
  • Finally A, of which all dependencies have been visited already, is added to the sorted list.

Thus the order of CommitOrderCalculator::sortedNodeList ends up being: B, C, A. That order is reversed then again is used in reversed (i.e. original order).

When dependencies are first listed, Doctrine could ignore the dep A in C because that dep has onDelete="SET NULL". The whole process would then compute a valid commit order C, B, A.

@DoobleD
Copy link
Author

DoobleD commented Nov 19, 2021

Another way of making this work would be to support deferrable constraints, but Doctrine seems to have issues with that.

@DoobleD
Copy link
Author

DoobleD commented Nov 19, 2021

Another (probably bad) workaround is to add onDelete="CASCADE" on the ManyToOne link to B in C. That seems to work, though I keep cascade={"remove"} on the OneToMany side because I need lifecycle events.

@DoobleD
Copy link
Author

DoobleD commented Nov 19, 2021

I ended up removing all onDelete annotations and set deferred constraints on all my foreign keys instead.

This is much simpler to manage, and hopefully #3423 will get solved soon (there seem to be a PR with recent activity at least).

@mpdude
Copy link
Contributor

mpdude commented Feb 27, 2023

Could you please test if #10547 solves the problem for you?

mpdude added a commit to mpdude/doctrine2 that referenced this issue Feb 28, 2023
mpdude added a commit to mpdude/doctrine2 that referenced this issue Mar 11, 2023
mpdude added a commit to mpdude/doctrine2 that referenced this issue Mar 11, 2023
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jun 4, 2023
This test implements the situation described in doctrine#9192. The commit order computation will be fixed by doctrine#10547.
greg0ire added a commit that referenced this issue Jun 4, 2023
Add test to show #9192 has been fixed
@greg0ire greg0ire closed this as completed Aug 1, 2023
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 a pull request may close this issue.

3 participants