Defer removing removed entities from to-many collections until after transaction commit

[mpdude]

This PR fixes regressions introduced in 2.15.1 related to how the UnitOfWork and EntityPersisters implement removal of entities that are part of many-to-many collections.

Background

To better understand the problem and regressions caused, we need to take a look at how the ORM deals with many-to-many associations when an entity shall be removed that is part of such an association.

BasicEntityPersister::deleteJoinTableRecords() is the method responsible for efficient removal of join-table rows in the database when a single entity is removed. It will DELETE all join-table rows referring to the entity in question before the entity is removed itself. This only works when the many-to-many association is declared on the entity's side (ie. its a bi-directional many-to-many association, or the entity is the owning side). Otherwise, since the ORM cannot detect associations that are declared in other classes only, join tables are left alone and database-level constraints apply.

BasicEntityPersister::deleteJoinTableRecords() implements a rather efficient join-table row removal, since a single DELETE query per removed entity and association can be used to possibly delete multiple rows (for associations with different other entities).

Besides deleting join-table rows in the database, we also want to update collections to no longer contain the removed entity. Removing the entity from these collections is the in-memory equivalent to removing the join table rows in the database.

Early during the commit phase, the UnitOfWork::computeAssociationChanges() method is responsible for looking at all associations in all in-memory entities. Among a few other tasks, it will identify all "dirty" collections – those that have been changed and need to be flushed to the database. This method will also check all collections for entities pending removal, and it will remove (unset) these entities from the collections.

Current situation

#10485 complained that the computeAssociationChanges would first take note of the "dirty" collections and then unset entities pending removal. The consequences of this were that

• in collections that were already dirty (changed) before flush() was called, the join-table row would be removed with a dedicated DELETE statement, while bringing the collection and database in sync
• more importantly, previously clean collections would be dirty after flush() returns (the opposite of what flush() is supposed to achieve), and subsequent flush() operations would fail since a DELETE statement would be issued for an entity that was no longer in the identity map.

The fix in #10486 (contained in 2.15.1) tried to address this by first unsetting the to-be-removed entities before taking note of (then) dirty collections. This caused unintended behavior changes that were not spotted by tests:

• Since collection updates are done first, we now use single DELETE statements for every join-table row deletion. BasicEntityPersister::deleteJoinTableRecords() was effectively bypassed for those collections that were loaded into memory at that time. More queries are used than necessary.
• No ForeignKeyConstraintViolationException exception when trying to remove an entity #10752 reports a use case where the to-be-removed entity does not declare the many-to-many association. Pre Fix to-many collections left in dirty state after entities are removed by the UoW #10486, this would always mean database-level constraints apply, since the ORM does not touch join-table rows in that case. But with the Fix to-many collections left in dirty state after entities are removed by the UoW #10486 changes and given a collection containing the to-be-removed entity is in memory, we ended up doing join-table row removals first – circumventing database-level constraints.

Suggested fix

• Improve ManyToManyBasicAssociationTest to make sure collections are clean after flush() returns, and include assertions regarding the expected query count. This would have spotted per-row DELETEs instead of using the optimized persister method
• Do not modify collections during computeAssociationChanges, but instead track which entity needs to be removed from which collection in a dedicated array
• This way, join-tables will be dealt with by the deleteJoinTableRecords method only
• After all changes have been flushed successfully, in the finishing phase of the UoW commit, go back to the list of tracked entity removals. Remove the entities from the respective collections immediately before taking new snapshots (= defining the new "clean" state). This makes sure we only do in-memory cleanups and cause no additional database operations due to removals.
• Improve documentation to raise awareness of how this proess works

Fixes #10752, closes #10753.

[mpdude]

This reverts the main change from #10486.

[mpdude]

Do you think there should be a mapping configuration setting/switch to tell the ORM not to handle the join table itself and always leave it to the database layer?

Probably that has to be a completely new setting. Might be beneficial for performance, to get consistent behavior regardless of using uni/bidirectional associations and to be able to RESTRICT removing entities that have m2m links.

[greg0ire]

@mpdude I don't know what level of flexibility people expect, or what's the sensible thing to expect here. I think we should keep the ORM as simple as possible. If people let us know they need this, we can always implement it.

[greg0ire]

Thanks @mpdude !