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.

Sign up for GitHub

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

Jump to bottom

Lost Update when using Deferred Explicit change tracking, Remove followed by Persist. #8231

Closed
skurfuerst opened this issue Aug 7, 2020 · 1 comment · Fixed by #8392
Closed

Lost Update when using Deferred Explicit change tracking, Remove followed by Persist. #8231

skurfuerst opened this issue Aug 7, 2020 · 1 comment · Fixed by #8392
Milestone
2.8.2

Comments

@skurfuerst
Copy link

... That's hard to describe, I'll try nevertheless :-)

Background

  • We use Deferred Explicit as change tracking policy; and we have a single entity which is enough to reproduce the issue.
  • The entity is persisted in the database already, and we have loaded it as $entity.

How to reproduce

  • call $entityManager->remove($entity);
  • modify the entity $entity->setFoo('bar');
  • call $entityManager->persist($entity);
  • call $entityManager->flush();
  • EXPECTED: The entity should be updated in the database.
  • ACTUAL: the entity is not updated.

Root Cause Analysis:

  • call $entityManager->remove($entity);
    • Internally, this calls UnitOfWork::doRemove(), where the entity is marked with UnitOfWork::STATE_REMOVED.
  • modify the entity $entity->setFoo('bar');
  • call $entityManager->persist($entity);
    • Internally, this calls UnitOfWork::doPersist().
    • There, the entity is marked as UnitOfWork::STATE_MANAGED again (it was marked as removed beforehand).
    • NOTE: Doctrine does NOT call UnitOfWork::scheduleForDirtyCheck(), which it would call if the entity was already
      in STATE_MANAGED. We rely on this scheduleForDirtyCheck to be called, because we use DEFERRED_EXPLICIT as change tracking policy.
  • call $entityManager->flush();
    • the entity does not appear in the changed entities.
  • EXPECTED: The entity should be updated in the database.
  • ACTUAL: the entity is not updated.

Suggested fix:

in UnitOfWork::doPersist

private function doPersist($entity, array &$visited) {
  // ... 
     switch ($entityState) {
            case self::STATE_MANAGED:
                // Nothing to do, except if policy is "deferred explicit"
                if ($class->isChangeTrackingDeferredExplicit()) {
                    $this->scheduleForDirtyCheck($entity);
                }
                break;

            case self::STATE_NEW:
                $this->persistNew($class, $entity);
                break;

            case self::STATE_REMOVED:
                // Entity becomes managed again
                unset($this->entityDeletions[$oid]);
                $this->addToIdentityMap($entity);

                $this->entityStates[$oid] = self::STATE_MANAGED;

// BEGIN CHANGE
                // Nothing to do, except if policy is "deferred explicit"
                if ($class->isChangeTrackingDeferredExplicit()) {
                    $this->scheduleForDirtyCheck($entity);
                }
// END CHANGE

                break;

            case self::STATE_DETACHED:
                // Can actually not happen right now since we assume STATE_NEW.
                throw ORMInvalidArgumentException::detachedEntityCannot($entity, "persisted");

            default:
                throw new UnexpectedValueException("Unexpected entity state: $entityState." . self::objToStr($entity));
        }
    // ... 
}

Note: this issue appeared as part of neos/neos-development-collection#3015

Thanks for your great work :)
Sebastian
skurfuerst added a commit to neos/neos-development-collection that referenced this issue Aug 7, 2020
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
4e18e47 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
skurfuerst added a commit to neos/neos-development-collection that referenced this issue Aug 7, 2020
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
b5a7d67 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
@skurfuerst skurfuerst mentioned this issue Aug 7, 2020
Merged
neos-bot pushed a commit to neos/content-repository that referenced this issue Aug 10, 2020
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
b6b75d9 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
@beberlei
Copy link
Member

@skurfuerst Valid report, please make it a pull request against the 2.7 branch.

@beberlei beberlei mentioned this issue Sep 13, 2020
Closed
neos-bot pushed a commit to neos/content-repository that referenced this issue Oct 30, 2020
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
0929bdd 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
neos-bot pushed a commit to neos/content-repository that referenced this issue Dec 4, 2020
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
0491cfa 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
@beberlei beberlei added this to the 2.8.2 milestone Dec 6, 2020
beberlei added a commit to beberlei/doctrine2 that referenced this issue Dec 13, 2020
@beberlei
[doctrineGH-8231] Bugfix: Missed dirty check synchronization check.
1b50607 
When an entity with change tracking policy "deferred explicit" gets
removed, then persisted again, it is not schedulded for a dirty check
synchronization. This is not the case for entities that are persisted
and are already in the managed state.
@beberlei beberlei mentioned this issue Dec 13, 2020
Merged
beberlei added a commit to beberlei/doctrine2 that referenced this issue Dec 13, 2020
@beberlei
[doctrineGH-8231] Bugfix: Missed dirty check synchronization check.
6137e12 
When an entity with change tracking policy "deferred explicit" gets
removed, then persisted again, it is not schedulded for a dirty check
synchronization. This is not the case for entities that are persisted
and are already in the managed state.
beberlei added a commit that referenced this issue Dec 14, 2020
@beberlei
[GH-8231] Bugfix: Missed dirty check synchronization check. (#8392)
8b74964 
When an entity with change tracking policy "deferred explicit" gets
removed, then persisted again, it is not schedulded for a dirty check
synchronization. This is not the case for entities that are persisted
and are already in the managed state.
markusguenther pushed a commit to markusguenther/neos-development-collection that referenced this issue Mar 25, 2021
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
0033226 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
neos-bot pushed a commit to neos/content-repository that referenced this issue Oct 7, 2022
@skurfuerst
BUGFIX: fix lost update when creating shadow nodes during move
8ff0758 
Fixes Behat testcases; not sure whether it happens in production; as
the workaround is to call PersistenceManager->persistAll() twice...

The corresponding Doctrine bug is at doctrine/orm#8231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.8.2
Development

Successfully merging a pull request may close this issue.

[GH-8231] Bugfix: Missed dirty check synchronization check. beberlei/doctrine2
2 participants
@beberlei @skurfuerst