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.

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

ORM Collection OneToMany cache issue on update #5821

Closed
mpoiriert opened this issue May 12, 2016 · 9 comments
Closed

ORM Collection OneToMany cache issue on update #5821

mpoiriert opened this issue May 12, 2016 · 9 comments
Assignees

Comments

@mpoiriert
Copy link

Following

http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/reference/second-level-cache.html

Under 33.6.1. Cache usage

There is a example of OneToMany collection cache (State->cities).

From this documentation

http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/reference/unitofwork-associations.html

In a OneToMany the owner side is the inverse side (City in the example mention above).

From the UnitOfWork

 private function computeAssociationChanges($assoc, $value)
    {
        if ($value instanceof Proxy && ! $value->__isInitialized__) {
            return;
        }

        if ($value instanceof PersistentCollection && $value->isDirty()) {
            $coid = spl_object_hash($value);

            if ($assoc['isOwningSide']) {
                $this->collectionUpdates[$coid] = $value;
            }

            $this->visitedCollections[$coid] = $value;
        }
 ///...///
}

This mean the collection will not be added to the collectionUpdates since it's not the owning side.

Not being in the collectionUpdates the AbstractCollectionPersister->update will not get called so the cache entry will not be updated (or deleted) resulting of corrupted data in the cache.

It's all a big theory but looking in the code it make sense that this is my problem (unless there is something I didn't configure properly).

Clearing the cache after the flush does make all the cache entry getting updated and it's working properly...

Also I was trying to find automation test of those classes and I didn't find any, could I be mistaken or there is really no automated integration test for this ?

@janlanger
Copy link
Contributor

I just came across same issue. Inverse side configured with cascade persist and orphan removal, but adding or removing of item from collection does not refresh collection cache.

Workaround is

$this->entityManager->getCache()->evictCollection(get_class($entity), 'property', $identifier);

@mpoiriert
Copy link
Author

I am a bit late but were did you put this line ? Right now I have a listener of the OnFlush event, I was clearing everything a bit to evelly (the region namespace). Using your code will be more precise just want to make sure the "listener" approach is good.

It's really hacky but here is what I have right now:

/**
 * Auto clear cache system until a doctrine bug is fixed
 *
 * @see https://github.com/doctrine/doctrine2/issues/5821
 */
class ClearCacheListener
{
    public function onFlush(OnFlushEventArgs $args)
    {
        $unitOfWork = $args->getEntityManager()->getUnitOfWork();

        $reflectionObject = new \ReflectionObject($unitOfWork);
        $reflectionProperty = $reflectionObject->getProperty('visitedCollections');
        $reflectionProperty->setAccessible(true);
        $visitedCollections = $reflectionProperty->getValue($unitOfWork);

        $collections = array_merge(
            $unitOfWork->getScheduledCollectionDeletions(),
            $unitOfWork->getScheduledCollectionUpdates()
        );

        $evicted = [];
        foreach ($visitedCollections as $key => $visitedCollection) {
            if (array_key_exists($key, $collections)) {
                continue;
            }
            /* @var $visitedCollection \Doctrine\ORM\PersistentCollection */
            if (!$visitedCollection->isDirty()) {
                continue;
            }

            $entityPersister = $unitOfWork->getCollectionPersister($visitedCollection->getMapping());

            if (!$entityPersister instanceof CachedPersister) {
                continue;
            }

            if (in_array($entityPersister->getCacheRegion()->getName(), $evicted)) {
                continue;
            }

            $entityPersister->getCacheRegion()->evictAll();

            $evicted[] = $entityPersister->getCacheRegion()->getName();
        }
    }
}

@mpoiriert
Copy link
Author

mpoiriert commented Nov 1, 2016

Using your method this is what I have right now

/**
 * Auto clear cache system until a doctrine bug is fixed
 *
 * @see https://github.com/doctrine/doctrine2/issues/5821
 */
class ClearCacheListenerBu
{
    public function onFlush(OnFlushEventArgs $args)
    {
        $entityManager = $args->getEntityManager();
        $unitOfWork = $args->getEntityManager()->getUnitOfWork();

        $reflectionObject = new \ReflectionObject($unitOfWork);
        $reflectionProperty = $reflectionObject->getProperty('visitedCollections');
        $reflectionProperty->setAccessible(true);
        $visitedCollections = $reflectionProperty->getValue($unitOfWork);

        $collections = array_merge(
            $unitOfWork->getScheduledCollectionDeletions(),
            $unitOfWork->getScheduledCollectionUpdates()
        );

        foreach ($visitedCollections as $key => $visitedCollection) {
            if (array_key_exists($key, $collections)) {
                continue;
            }
            /* @var $visitedCollection \Doctrine\ORM\PersistentCollection */
            if (!$visitedCollection->isDirty()) {
                continue;
            }
            $entityPersister = $unitOfWork->getCollectionPersister($visitedCollection->getMapping());

            if (!$entityPersister instanceof CachedPersister) {
                continue;
            }

            $entity = $visitedCollection->getOwner();
            $mapping = $visitedCollection->getMapping();
            $identifier = $entityManager->getClassMetadata($class = get_class($entity))->getIdentifierValues($entity);

            $entityManager->getCache()->evictCollection($class, $mapping['fieldName'], $identifier);
        }
    }
}

@janlanger
Copy link
Contributor

@mpoiriert Well, we have my workaround directly where insert/update/delete is done. Not very nice, but its working.

I though about listener workaround, but didn't get to it, thanks for your code! But it's still bug and should be fixed in doctrine...

@mpoiriert
Copy link
Author

yeah it should ! Just to let you know I have use the last version of what I paste above, it pass our integration test and we are releasing it in production this friday. My previous code was running since I open this issue but it was clearing the full region, ok for CMS data but less good for User Data...

@nicolas-cajelli
Copy link

I ran into the same issue today, and i was about to get my hands into the code when i found this issue (i googled a lot to get here).

It would be nice to get the fix, but in the meanwhile (since probably this will need lots of discussions), the documentation could be updated to reflect this (just for the sake of future innocents)

Ocramius added a commit that referenced this issue Dec 12, 2016
…ache-#1551-to-2.5

#5821 Backport #1551 - Fixed support for inverse side second level cache
AlexKryvets added a commit to AlexKryvets/doctrine2 that referenced this issue Dec 23, 2016
This release relaxes [`doctrine/common`](https://github.com/doctrine/common) requirements
in order to allow installation of versions that support PHP 7.1 features in proxy class
generation. Please note that a similar requirement relaxation still needs to be applied to
[`doctrine/dbal`](https://github.com/doctrine/dbal) in order to allow installation of
the latest `doctrine/common` versions. [doctrine#6156](doctrine#6156)

This version also backports some fixes around the eviction of the second level cache entries
of inverse side associations in one-to-many - many-to-one mappings. [doctrine#6159](doctrine#6159)

Further fixes were applied in order to have child classes in inheritance mapping share the
same timestamp region when the second level cache is enabled. [doctrine#6028](doctrine#6028)

Also, `Doctrine\ORM\EntityManager#merge()` now triggers `Doctrine\ORM\Events::prePersist`
listeners with the merged entity state whenever an internal `Doctrine\ORM\UnitOfWork#persist()`
call is implied. [doctrine#6177](doctrine#6177).

Total issues resolved: **8**

- [5570: Fix PrePersist EventListener when using merge instead of persist](doctrine#5570)
- [6028: Make child entity share the timestamp region with parent class](doctrine#6028)
- [6110: Clear $this->collection even when empty, to reset keys](doctrine#6110)
- [6156: Allow doctrine/common 2.7](doctrine#6156)
- [6159: doctrine#5821 Backport doctrine#1551 - Fixed support for inverse side second level cache](doctrine#6159)
- [6174: Merging a new entity with PrePersist event make changes in callback not be considered](doctrine#6174)
- [6177: Fix doctrine#6174 doctrine#5570: merging new entities should also trigger prepersist lifecycle callbacks with merged entity data](doctrine#6177)
- [6178: Backport doctrine#6177 - fix doctrine#6174 doctrine#5570: merging new entities should also trigger prepersist lifecycle callbacks with the merged data](doctrine#6178)

# gpg: directory `/c/Users/PC/.gnupg' created
# gpg: new configuration file `/c/Users/PC/.gnupg/gpg.conf' created
# gpg: WARNING: options in `/c/Users/PC/.gnupg/gpg.conf' are not yet active during this run
# gpg: keyring `/c/Users/PC/.gnupg/pubring.gpg' created
# gpg: Signature made Tue Dec 20 00:49:05 2016 FLEST using DSA key ID 12EC2DF8
# gpg: Can't check signature: public key not found

# Conflicts:
#	lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php
#	lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php
#	tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php
#	tests/Doctrine/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php
#	tests/Doctrine/Tests/ORM/Tools/Pagination/PaginationTestCase.php
@lcobucci lcobucci self-assigned this Jan 19, 2017
@lcobucci
Copy link
Member

@mpoiriert could you check if this still happens on 2.6.x-dev?

@janlanger
Copy link
Contributor

@lcobucci this issue was fixed in 2.5.6 by #6159

@lcobucci
Copy link
Member

@janlanger thanks! Closing then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants