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

mpdude · 2023-08-16T20:48:46Z

In #10864 (comment) and subsequent comments, @Dallas62 reported that since the upgrade to 2.16.1, they have an issue with the order of DELETE statements. The order of DELETES leads to a foreign key constraint violation.

Since the report does not exactly match the original issue topic in #10864, I am creating this one here so we can track it individually.

In order to reproduce, a demo Symfony application was provided in https://github.com/Dallas62/doctrine-orm-reproducer-deletion-issue, but obviously we cannot include that in the ORM test suite.

I have picked the two entities I deemed relevant, but as of now, this is not sufficient to reproduce the bug.

@Dallas62

In doctrine#10864 (comment) and subsequent comments, @Dallas62 reported that since the upgrade to 2.16.1, they have an issue with the order of `DELETE` statements. The order of `DELETES` leads to a foreign key constraint violation. Since the report does not exactly match the original issue topic in doctrine#10864, I am creating this one here so we can track it individually. In order to reproduce, a demo Symfony application was provided in https://github.com/Dallas62/doctrine-orm-reproducer-deletion-issue, but obviously we cannot include that in the ORM test suite. I have picked the two entities I deemed relevant, but as of now, this is not sufficient to reproduce the bug.

mpdude · 2023-08-16T21:00:38Z

@Dallas62 I think what you're seeing is different from #10864. So let's continue the discussion here.

I have tried to pick the relevant classes from your repo and strip it down to the bare minimum needed to run as a test. But, as you can see, this test currently passes.

So, there must be more to it...

Can you experiment with the test as it can be found in https://github.com/mpdude/doctrine2/tree/delete-foreign-key-issue, and feel free to open PRs against it? We need a failing test to proceed, and it has to be a test that we can include and run in the ORM test suite.

Thanks!

mpdude · 2023-08-16T21:06:12Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php

+    public $id;
+
+    /**
+     * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user", cascade={"remove"})


If I remove cascade: remove, I'll get the error

Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException : An exception occurred while executing a query: SQLSTATE[23000]: Integrity constraint violation: 1451 Cannot delete or update a parent row: a foreign key constraint fails (doctrine_tests.GH10912Room, CONSTRAINT FK_7FCB83EEA76ED395 FOREIGN KEY (user_id) REFERENCES GH10912User (id))

This is because we have a GH10912Room pointing to the user that is to be removed, the association is not nullable and we have no database-level ON DELETE setting.

TIL that cascade seemingly also works on the inverse side of an association (@derrabus )

Adding orphanRemoval, as can be seen in

https://github.com/Dallas62/doctrine-orm-reproducer-deletion-issue/blob/6e14b758be3484273ddddd2366f465a3d2eead30/src/Entity/User.php#L33

does not make difference.

Dallas62 · 2023-08-17T04:58:46Z

Thanks @mpdude for your time, I will take a look later this day. I found during making the reproducer, it only happen when the OneToOne relationship between User and UserProfile is added. In this particular structure I could have merge both tables but there might be some other impact in the orm.

mpdude · 2023-08-17T06:40:31Z

I have added the Profile entity and the two OneToOne associations between User and Profile (side note: that's two individual, unidirectional associations). Seems it still works.

@Dallas62 looking forward to your suggestions!

Dallas62 · 2023-08-17T12:30:31Z

Ok, so I have continue digging on this subject.

I tried to set all classes on your test case => No change.
I tried to reproduce it by switching the pdo_mysql to pdo_sqlite in_memory on the reproducer repository => The issue is not present.
So the driver seems to be important, not sure how to mock it without changing the driver.

I will try to reduce the number of entities in the reproducer to have the minimal exemple.
Then try to reproduce it on your test case (at least with pdo_mysql).

mpdude · 2023-08-17T15:22:55Z

Not sure whether SQLite and/or in_memory implements referential integrity. Without that, the issue won't surface.

Dallas62 · 2023-08-17T16:00:38Z

I simplified the reproducer:

Remove #[HasLifecycleCallbacks]
Remove Plant and PlantPicture
Use integer instead of ulid and default GeneratedValue configuration
remove unnecessary arguments
Still able to reproduce, but not in this PR (with pdo_mysql) 🤔

Dallas62 · 2023-08-17T17:09:02Z

@mpdude After more digging into this issue, I finally got the cause of the reproducibility ! 🎉

The $this->_em->remove($userReloaded); seems to push data inside $unitOfWork->entityDeletions based on the list of properties.

So:

if profile is before rooms, it fails
if profile is after rooms, everything is working fine.

Good news, working with pdo_sqlite in_memory.

Dallas62 · 2023-08-17T17:12:02Z

@@ -80,13 +80,6 @@ class GH10912User
      */
     public $id;
 
-    /**
-     * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user", cascade={"remove"})
-     *
-     * @var Collection<int, GH10912Room>
-     */
-    public $rooms;
-
     /**
      * @ORM\OneToOne(targetEntity=GH10912Profile::class, cascade={"remove"})
      * @ORM\JoinColumn(onDelete="cascade")

@@ -95,6 +88,13 @@ class GH10912User
      */
     public $profile;
 
+    /**
+     * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user", cascade={"remove"})
+     *
+     * @var Collection<int, GH10912Room>
+     */
+    public $rooms;
+
     public function __construct()
     {
         $this->rooms = new ArrayCollection();

mpdude · 2023-08-17T19:43:22Z

X-ref #10566; maybe there’s more conditions we need to consider?

(description to be written) Fixes doctrine#10912.

mpdude · 2023-08-17T22:23:15Z

@Dallas62 it's still preliminary, but you might want to give #10913 a try.

Dallas62 · 2023-08-18T06:21:52Z

Thanks, I will take a look asap.

I will also extends your tests based on some particular cases I have in mind.

Dallas62 · 2023-08-18T14:55:32Z

I tried with the given repository (at commit Dallas62/doctrine-orm-reproducer-deletion-issue@6e14b75)
And with an extra ManyToOne on Profile entity, everything is working fine. The order of deletion is still based on order of property but in tested cases, this as no impact on the result.

For me it seems ok, let some more experimented contributors review changes and give feedbacks.

Just in case:

<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\Tests\OrmFunctionalTestCase;

use function array_filter;
use function array_values;
use function strpos;

class GH10912Case2Test extends OrmFunctionalTestCase
{
    protected function setUp(): void
    {
        parent::setUp();

        $this->setUpEntitySchema([
            GH10912User::class,
            GH10912Profile::class,
            GH10912Room::class,
            GH10912AnotherDependency::class
        ]);
    }

    public function testIssue(): void
    {
        $user       = new GH10912User();
        $profile    = new GH10912Profile();
        $room       = new GH10912Room();
        $dependency = new GH10912AnotherDependency();

        $user->rooms->add($room);
        $user->profile = $profile;
        $profile->user = $user;
        $room->user = $user;
        $dependency->profile = $profile;
        $profile->dependencies->add($dependency);

        $this->_em->persist($room);
        $this->_em->persist($user);
        $this->_em->persist($profile);
        $this->_em->persist($dependency);
        $this->_em->flush();
        $this->_em->clear();

        $userReloaded = $this->_em->find(GH10912User::class, $user->id);

        $queryLog = $this->getQueryLog();
        $queryLog->reset()->enable();

        $this->_em->remove($userReloaded);
        $this->_em->flush();

        $queries = array_values(array_filter($queryLog->queries, static function ($entry) {
          return strpos($entry['sql'], 'DELETE') === 0;
        }));

        self::assertCount(4, $queries);
        self::assertSame('DELETE FROM GH10912AnotherDependency WHERE id = ?', $queries[0]['sql']);
        self::assertSame('DELETE FROM GH10912Room WHERE id = ?', $queries[1]['sql']);
        self::assertSame('DELETE FROM GH10912User WHERE id = ?', $queries[2]['sql']);
        self::assertSame('DELETE FROM GH10912Profile WHERE id = ?', $queries[3]['sql']);

        // The EntityManager is aware that all three entities have been deleted
        $im = $this->_em->getUnitOfWork()->getIdentityMap();
        self::assertEmpty($im[GH10912Profile::class]);
        self::assertEmpty($im[GH10912User::class]);
        self::assertEmpty($im[GH10912Room::class]);
        self::assertEmpty($im[GH10912AnotherDependency::class]);
    }
}

/** @ORM\Entity */
class GH10912User
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue
     *
     * @var int
     */
    public $id;

    /**
     * @ORM\OneToOne(targetEntity=GH10912Profile::class, cascade={"remove"})
     * @ORM\JoinColumn(onDelete="cascade")
     *
     * @var GH10912Profile
     */
    public $profile;

    /**
     * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user", cascade={"remove"})
     *
     * @var Collection<int, GH10912Room>
     */
    public $rooms;

    public function __construct()
    {
        $this->rooms = new ArrayCollection();
    }
}

/** @ORM\Entity */
class GH10912Profile
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue
     *
     * @var int
     */
    public $id;

    /**
     * @ORM\OneToOne(targetEntity=GH10912User::class)
     * @ORM\JoinColumn(onDelete="cascade")
     *
     * @var GH10912User
     */
    public $user;

    /**
     * @ORM\OneToMany(targetEntity=GH10912AnotherDependency::class, mappedBy="profile", cascade={"remove"})
     *
     * @var Collection<int, GH10912AnotherDependency>
     */
    public $dependencies;

    public function __construct()
    {
        $this->dependencies = new ArrayCollection();
    }
}

/** @ORM\Entity */
class GH10912Room
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue
     *
     * @var int
     */
    public $id;

    /**
     * @ORM\ManyToOne(targetEntity=GH10912User::class, inversedBy="rooms")
     * @ORM\JoinColumn(nullable=false)
     *
     * @var GH10912User
     */
    public $user;
}


/** @ORM\Entity */
class GH10912AnotherDependency
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue
     *
     * @var int
     */
    public $id;

    /**
     * @ORM\ManyToOne(targetEntity=GH10912Profile::class, inversedBy="dependencies")
     * @ORM\JoinColumn(nullable=false)
     *
     * @var GH10912Profile
     */
    public $profile;
}

asmafarhat2006 · 2023-10-11T19:29:13Z

@mpdude
Im facing a similar issue where deleting an entity which has a child association triggers this error

"Cannot delete or update a parent row: a foreign key constraint fails"

, in 2.16, and it used to work fine in 2.15
Adding an inverse relation on the entity seems to resolve the problem , but this solution wont work for a large codebase.
Am I looking at the right thread or is my issue slightly different. ?

eigan · 2023-10-12T21:33:11Z

@asmafarhat2006 Might not be helpful, but we got the same problem in our codebase and I dug arround and found out why. Of course, there is multiple ways to trigger this same error.

class  MyEntity {
   # ManyToOne
   public OtherFooEntity $foo;
}


# This will give you problem in 2.16 and not in earlier version (we came from 2.14):
$myEntity->foo = null;
$em->remove($myEntity->foo);
$em->remove($myEntity);

$em->flush(); // Crash

// Doctrine thinks its OK to remove OtherFooEntity before MyEntity
// because it looks at the current state and not the original and current state in db.

// Was easy for us to refactor this, as it didnt make much sense to set foo = null

asmafarhat2006 · 2023-10-12T21:36:34Z

Thanks for looking into this But my issue is slightly different .the child entity does exist but doesnt have the annotations representing the relation in the entity class. It used to work fine without adding the relation on the inverse side in 2.15 but it crashes in 2.16

…

On Thu, 12 Oct 2023, 22:33 Einar, ***@***.***> wrote: @asmafarhat2006 <https://github.com/asmafarhat2006> Might not be helpful, but we got the same problem in our codebase and I dug arround and found out why. class MyEntity { # ManyToOne public OtherFooEntity $foo; } # This will give you problem in 2.16 and not in earlier version (we came from 2.14):$myEntity->foo = null;$em->remove($myEntity->foo);$em->remove($myEntity); $em->flush(); // Crash // Doctrine thinks its OK to remove OtherFooEntity before MyEntity// because it looks at the current state and not the original and current state in db. // Was easy for us to refactor this, as it didnt make much sense to set foo = null — Reply to this email directly, view it on GitHub <#10912 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQ5MNFRIKW3GPOD35L7T73X7BO2HAVCNFSM6AAAAAA3TCP3SWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRQGM4TGMRVGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…ion (#10913) In order to resolve #10348, some changes were included in #10547 to improve the computed _delete_ order for entities. One assumption was that foreign key references with `ON DELETE SET NULL` or `... CASCADE` need not need to be taken into consideration when planning the deletion order, since the RDBMS would unset or cascade-delete such associations by itself when necessary. Only associations that do _not_ use RDBMS-level cascade handling would be sequenced, to make sure the referring entity is deleted before the referred-to one. This assumption is wrong for `ON DELETE CASCADE`. The following examples give reasons why we need to also consider such associations, and in addition, we need to be able to deal with cycles formed by them. In the following diagrams, `odc` means `ON DELETE CASCADE`, and `ref` is a regular foreign key with no extra `ON DELETE` semantics. ```mermaid graph LR; C-->|ref| B; B-->|odc| A; ``` In this example, C must be removed before B and A. If we ignore the B->A dependency in the delete order computation, the result may not to be correct. ACB is not a working solution. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; C-->|ref| B; ``` This is the situation in #10912. We have to deal with a cycle in the graph. C must be removed before A as well as B. If we ignore the B->A dependency (e.g. because we set it to "optional" to get away with the cycle), we might end up with an incorrect order ACB. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; A-->|ref| C; C-->|ref| B; ``` This example has no possible remove order. But, if we treat `odc` edges as optional, A -> C -> B would wrongly be deemed suitable. ```mermaid graph LR; A-->|ref| B; B-->|odc| C; C-->|odc| B; D-->|ref| C; ``` Here, we must first remove A and D in any order; then, B and C in any order. If we treat one of the `odc` edges as optional, we might find the invalid solutions ABDC or DCAB. #### Solution implemented in this PR First, build a graph with a node for every to-be-removed entity, and edges for `ON DELETE CASCADE` associations between those entities. Then, use [Tarjan's algorithm](https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm) to find strongly connected components (SCCs) in this graph. The significance of SCCs is that whenever we remove one of the entities in a SCC from the database (no matter which one), the DBMS will immediately remove _all_ the other entities of that group as well. For every SCC, pick one (arbitrary) entity from the group to represent all entities of that group. Then, build a second graph. Again we have nodes for all entities that are to be removed. This time, we insert edges for all regular (foreign key) associations and those with `ON DELETE CASCADE`. `ON DELETE SET NULL` can be left out. The edges are not added between the entities themselves, but between the entities representing the respective SCCs. Also, for all non-trivial SCCs (those containing more than a single entity), add dependency edges to indicate that all entities of the SCC shall be processed _after_ the entity representing the group. This is to make sure we do not remove a SCC inadvertedly by removing one of its entities too early. Run a topological sort on the second graph to get the actual delete order. Cycles in this second graph are a problem, there is no delete order. Fixes #10912.

mpdude · 2024-01-12T21:45:33Z

The fix from #10913 will be included in the next 2.17 bugfix release.

mpdude force-pushed the delete-foreign-key-issue branch from af73761 to 9f84c51 Compare August 16, 2023 20:54

mpdude commented Aug 16, 2023

View reviewed changes

mpdude mentioned this pull request Aug 16, 2023

Creation order parent-children inversed #10864

Closed

Include a "Profile" entity as well

94f8e68

mpdude mentioned this pull request Aug 17, 2023

Include ON DELETE CASCADE associations in the delete order computation #10913

Merged

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Aug 17, 2023

Include ON DELETE CASCADE associations in the delete order computation

49b9ee7

(description to be written) Fixes doctrine#10912.

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 2, 2024

Add issue reproduce code as discussed in doctrine#10912

4e60559

mpdude closed this in #10913 Jan 12, 2024

mpdude deleted the delete-foreign-key-issue branch January 12, 2024 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mpdude commented Aug 16, 2023 •

edited

Loading

mpdude commented Aug 16, 2023 •

edited

Loading

mpdude Aug 16, 2023

mpdude Aug 16, 2023

Dallas62 commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

Dallas62 commented Aug 17, 2023 •

edited

Loading

mpdude commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 18, 2023

Dallas62 commented Aug 18, 2023 •

edited

Loading

asmafarhat2006 commented Oct 11, 2023 •

edited

Loading

eigan commented Oct 12, 2023 •

edited

Loading

asmafarhat2006 commented Oct 12, 2023 via email

mpdude commented Jan 12, 2024

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

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

Conversation

mpdude commented Aug 16, 2023 • edited Loading

mpdude commented Aug 16, 2023 • edited Loading

mpdude Aug 16, 2023

Choose a reason for hiding this comment

mpdude Aug 16, 2023

Choose a reason for hiding this comment

Dallas62 commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

Dallas62 commented Aug 17, 2023

Dallas62 commented Aug 17, 2023 • edited Loading

mpdude commented Aug 17, 2023

mpdude commented Aug 17, 2023

Dallas62 commented Aug 18, 2023

Dallas62 commented Aug 18, 2023 • edited Loading

asmafarhat2006 commented Oct 11, 2023 • edited Loading

eigan commented Oct 12, 2023 • edited Loading

asmafarhat2006 commented Oct 12, 2023 via email

mpdude commented Jan 12, 2024

mpdude commented Aug 16, 2023 •

edited

Loading

mpdude commented Aug 16, 2023 •

edited

Loading

Dallas62 commented Aug 17, 2023 •

edited

Loading

Dallas62 commented Aug 18, 2023 •

edited

Loading

asmafarhat2006 commented Oct 11, 2023 •

edited

Loading

eigan commented Oct 12, 2023 •

edited

Loading