Prevent creation of new MANAGED entity instance by reloading REMOVED entity from database #11428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
According to #11208, you should target 2.x rightaway |
…ted. If the entity gets reloaded from database before the deletions are executed UnitOfWork needs to be able to return the original instance in REMOVED state.
xificurk
force-pushed
the
keep-removed-entity-in-identity-map
branch
from
87f2f05 to
bb36d49
Compare
|
@greg0ire I've overlooked that, thanks. PR is now rebased and targets 2.19.x. |
greg0ire
approved these changes
Apr 26, 2024
SenseException
approved these changes
Apr 26, 2024
|
Thanks @xificurk ! |
This was referenced May 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6123, #9463
Replaces: #6126
Current behavior:
EntityManager::remove()called with MANAGED entity schedules the entity for deletion at a later time, but immediately removes it from identity map.The immediate removal from identity map is problematic. Until the changes are flushed, the entity may get loaded again from database - either directly as part of a query result, or indirectly through a reference from another entity. This leads to the creation of a new entity (proxy) instance that is then added to identity map, i.e. the
UnitOfWorkwill have both - MANAGED entity instance in identity map, and REMOVED entity scheduled for deletion.This leads to all sorts of unexpected behaviors, e.g.:
===entity comparisonspersistbehavior depending on which of the instances is passed (including indirect throughcascade: persist)Proposed fix: Postpone the removal from identity map to the time when the scheduled deletes are executed.
This change caused failure of
UnitOfWorkTest::testRemovedAndRePersistedEntitiesAreInTheIdentityMapAndAreNotGarbageCollected(), which directly asserted the problematic immediate removal from identity map. From the original PR #1338, it seems this is rather an unintended by-product than actually desired behavior. The test was introduced to ensure that REMOVED entity can become MANAGED again (and unscheduled from deletion) by re-persisting it. This functionality does not require the immediate removal from identity map. In fact, the immediate removal from identity map may break it as demonstrated inGH6123Test::testRemovedEntityCanBePersistedAgain().Although the PR targets 3.1.x branch, it would be definitely a good candidate for backporting.(changed to target 2.19.x)