No ForeignKeyConstraintViolationException exception when trying to remove an entity

[jakubtobiasz]

Bug Report

Q	A
BC Break	kinda
Version	2.15.2

Summary

Since the following change

Doctrine stopped throwing ForeignKeyConstraintViolationException and (from what I noticed) it removes the entity from the database. What's more interesting, if we add the deleted lines, and leave the same code at the end of the method – it doesn't work. So putting this if statement somehow breaks the expected behavior.

Current behavior

No ForeignKeyConstraintViolationException 🤷🏼‍♂️.

How to reproduce

#10753

Expected behavior

ForeignKeyConstraintViolationException should be thrown.

[mpdude]

#10486 implemented that change

[mpdude]

Looking at the code in #10753, why should it throw? Where is the foreign key?

[jakubtobiasz]

Hi @mpdude,
this example is based on our Promotion-Order relation in Sylius. The foreign key is created under the hood.

Configuration is the same (in terms of relation between these two entities), and there are two foreign keys in the db.

[mpdude]

Yes, that’s the foreign keys in the m2m association table.

But where is the foreign key restricting removal of the element from the many-to-many collection?

[jakubtobiasz]

@mpdude considering on 2.15.1 it's working I guess it's a default. Anyway, while debugging, I set Restrict on delete and the same behavior occurred. I can update the reproducing PR if you'd like :).

Somehow between 2.15.1 and 2.15.2 the behavior changed without any configuration changes.

P.S. Also, trying to delete via MySQL CLI resulted in the error, so there's a constraint preventing removing the record.

[mpdude]

Ok, I think I begin to understand the situation.

You have an unidirectional Order *-->* Promotion many-to-many association.

Already before a951369, when you directly remove a Promotion entity that is part of an Order's collection, the collection will be updated as well:

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 960 to 967 in 76b8a21

	case self::STATE_REMOVED:
	// Consume the $value as array (it's either an array or an ArrayAccess)
	// and remove the element from Collection.
	if ($assoc['type'] & ClassMetadata::TO_MANY) {
	unset($value[$key]);
	}
	break;

The intention is that the collection should no longer point to/contain an entity that no longer exists.

pre a951369, this collection update went unnoticed for the current flush() operation. The UoW would not try to synchronize the collection change to the database.

This is why you got the ForeignKeyConstraintViolationException (for a database that supports it), since the Promotion will be deleted, but it is still pointed to by the m2m association table. You probably rely on this, since you do not want Promotions to be removed that are still being referred to from Orders.

The problem with that was reported as #10485: After the flush() operation, we are left with a "dirty" collection. That is, a collection that knows one of its elements has been removed and that this still needs to be synchronized to the database. The next time flush() is called, the UoW notices a collection entry is missing that was previously there. It tries to DELETE the row from the m2m association table but... the entity it needs to qualify the table row (the Promotion) is no longer in the identity map.

The a951369 change makes sure to include the collection update in the same flush() operation as the removal of the entity. That way, we can first remove the association row (as the entity is still known), right before the entity itself is deleted and removed from the identity map.

This is why you no longer get the exception: The ORM now removes the join table row before the Promotion entity is removed; no FK violation anymore!

Out of curiosity, I removed the lines shown above. The only tests that fail are those added in #10486. So, it seems that we did not have any tests at all covering this collection-updating code.

To make matters worse and the situation a bit more complicated, the tests from #10753 pass even with the a951369 change if you clear() the entity manager after the setup phase and load only the Promotion entity into memory.

This time, the UoW does not know about the m2m collection on the Order, since it is not in memory. And, I am not surprised that the UoW does not go out to find and load all the collections that possibly contain the Promotion when you remove the Promotion. The consequence is that, this time, the UoW will not update the collection (it's not in memory), so not try to synchronize that change, not delete the association row in the database and you get the foreign key reference violation you're expecting.

Now, at that point, I don't know how to proceed. I think it is necessary to agree on how the ORM should behave, whether it should update the collections or not. I've checked the documentation on this, but found nothing really distinct.

I am not sure if that might mean the ORM would have to implement configuration or switches to support CASCADE vs. RESTRICT behaviour on collections? I don't know if that is already there?

One thing I would assume is that, at least, the behaviour should be consistent regardless of whether a collection has been loaded into memory or not.

🤷🏻

[greg0ire]

One thing is unclear to me about the current behavior. If there is a foreign key pointing to the entity being removed, has is it even possible to remove the referenced entity? I would expect the RDBMS to make that impossible.

[mpdude]

Depending on the database level CASCADE/RESTRICT behavior, the RDBMS would abort or remove the n/m table row.

The point is that Doctrine removes the to-be-deleted entity from the collection and flushes that first, so the DB level foreign key never notices a problem.

[greg0ire]

So right now, there is no exception, and nothing gets removed, or there is an exception, just not the expected one? @jakubtobiasz didn't mention an exception, so I guess it cascades right now?

[jakubtobiasz]

Hello,
in terms of "whether anything is removed":

On 2.15.1; state of the sylius_promotion_order; after running a Behat scenario:

---

On 2.15.2:

The promotion is also deleted:

---

When I try to delete the promotion with using a SQL:

---

From what I noticed yesterday, no exception is thrown.

[greg0ire]

Ok this means Doctrine removes the n-m table row before deleting the promotion row, whereas before, it forced users to be explicit about this.

Well if ensuring a consistent behavior is too complicated, let's say that the behavior is undefined: it may throw or may succeed, and if users want a reliable result, then they should explicitly delete the link between Order and Promotion. If you cannot find documentation stating what should happen, then it is undefined, and if you want you can contribute a documentation paragraph making it explicitly undefined, and stating how to be sure not to get an exception (here, I think it's by removing the promotion from the collection of promotions in the order object).

Maybe the best place to document that would be here: https://www.doctrine-project.org/projects/doctrine-orm/en/stable/reference/working-with-associations.html#removing-associations

It can always been changed afterwards if you somehow manage to get a consistent behavior for this (removing the target entity of an m2m unidirectional association, i.e. removing entity on the inverse side). Sounds fair?

I am not sure if that might mean the ORM would have to implement configuration or switches to support CASCADE vs. RESTRICT behaviour on collections? I don't know if that is already there?

I don't know either, maybe another maintainer will.

[mpdude]

For the first time, I've noticed the following documentation section:

orm/docs/en/reference/working-with-objects.rst

Lines 289 to 294 in 76b8a21

	Removing an entity will also automatically delete any existing
	records in many-to-many join tables that link this entity. The
	action taken depends on the value of the ``@joinColumn`` mapping
	attribute "onDelete". Either Doctrine issues a dedicated ``DELETE``
	statement for records of each join table or it depends on the
	foreign key semantics of onDelete="CASCADE".

How do you interpret that?

[greg0ire]

The first sentence makes it clear what is suppose to happen. According to it, there should never have been a ForeignKeyConstraintViolationException in the first place, just silent deletions. The other 2 sentence seem to say it's guaranteed to happen, but not necessarily thanks to a DELETE. A database-level cascade might take care of it. That's my interpretation anyway, what's yours?

[mpdude]

Well, yes, that's pretty unambiguous. But.

• This part of the docs has not seen any relevant changes since the early days of 2.0. We cannot assume that it is "living" documentation, really up to date with the implementation.
• It suggests the ORM would always make sure the join table is cleaned up. Either this happens in the DB automatically, or the code will run DELETE statements. But, I cannot see in the code where is difference would be made.
• Assume you have a unidirectional m2m association, let's stick with the Order *-->* Promotion example. Now, you have a "clean" entity manager, and all you do is to load one Promotion and remove it. How is the EM even supposed to find out about all the potential m2m join tables that might exist, in order to perform this cleanup? Remember, we don't have a "full" view of all classes that (might) exist, we only know about what we loaded so far and what associations those classes declare. In this example, Promotion does not declare anything (and does not have to!).

@beberlei maybe you have thoughts on that? (sorry for pinging)

[mpdude]

Maybe we can say that when an entity is removed, what happens entirely depends on the onDelete behaviour and whether the database supports it?

• So, do not try to update join tables by modifying collections beforehand.
• Try to remove the entity in the DB.
• When the DB causes foreign key violations, throw. No entity removed, no collection modified.
• If the previous step passed, remove the entity from all collections we have in memory. It does no longer exists, and you would not get it either if the collection was loaded from the database now. And since we just flushed, everything should be in sync.
• All collections have to be clean when flush() returns.
• In databases that do not support foreign keys (is that relevant at all?), join rows might be left behind.
• We need not care about collections that are not in memory
• When an entity is removed, no lifecycle events or similar can be triggered for the collections (= entities with collections) referencing the removed entity.

[mpdude]

I have found

orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

Line 538 in 2afe2dc

protected function deleteJoinTableRecords(array $identifier, array $types): void

– that is where the EntityPersister cleans up m2m join-table rows with a DELETE statement when an entity is removed, unless onDeleteCascade is set. It works at the SQL level and can remove all join table rows referring to that particular entity in a single query.

That code only works when the association is declared on the side of the entity being removed, either as the owning or inverse side. That is not the case for the Promotion in the OP. Otherwise, the ORM cannot see/detect which other entities might possibly have a m2m association against the entity in question.

\Doctrine\ORM\UnitOfWork::computeAssociationChanges is more concerned with keeping in-memory collections updated. Since #10486, it will flush those changes to the DB as well – and that happens before the EntityPersister.

→ Need to check if that causes an amount of extra queries.

[mpdude]

Without the change from #10486, the tests in #10753 pass... unless you make the association bidirectional.

Note that the database-level foreign keys are still at their default, RESTRICT. IMHO, adding an inverse side like this should also not remove foreign key protections the application might rely on – by suddenly starting to clean up join table rows in the ORM/BasicEntityPersister, non?

diff --git a/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php b/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php
index e508e06a86..b154ab8ef5 100644
--- a/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php
+++ b/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php
@@ -88,7 +88,7 @@ class GH10752Order
     private $id = null;

     /**
-     * @ORM\ManyToMany(targetEntity="GH10752Promotion", cascade={"persist"})
+     * @ORM\ManyToMany(targetEntity="GH10752Promotion", inversedBy="orders", cascade={"persist"})
      * @ORM\JoinTable(name="order_promotion",
      *      joinColumns={@ORM\JoinColumn(name="order_id", referencedColumnName="id", onDelete="CASCADE")},
      *      inverseJoinColumns={@ORM\JoinColumn(name="promotion_id", referencedColumnName="id")}
@@ -142,6 +142,13 @@ class GH10752Promotion
      */
     private $id = null;

+    /**
+     * @ORM\ManyToMany(targetEntity="GH10752Order", mappedBy="promotions")
+     *
+     * @var Collection
+     */
+    private $orders;
+
     public function getId(): ?int
     {
         return $this->id;

[greg0ire]

It's not that weird IMO. If Promotion is suddenly aware of the link it has with Order, it might make sense to anybody taking a look at Promotion that removing it will result in the link with Order being removed (because the link between the 2 is now obvious).

[mpdude]

#10486 had another side effect: We started to take care of the dirty collections (where to-be-removed entities have been removed), which happens before we reach \Doctrine\ORM\Persisters\Entity\BasicEntityPersister::deleteJoinTableRecords. So, it takes one query per join table row (for every single collection entry), instead of being able to remove all join table rows in a single step per removed entity.

I'll add query count expectations to \Doctrine\Tests\ORM\Functional\ManyToManyBasicAssociationTest that would have uncovered this.

[mpdude]

A possible fix including documentation updates is in #10763.