From 367e90f2cd0dc0310d414cddd975913b337693e0 Mon Sep 17 00:00:00 2001 From: Guilherme Blanco Date: Wed, 11 Nov 2015 04:12:38 +0000 Subject: [PATCH] Fixed support for inverse side second level cache --- .../ORM/Cache/DefaultCollectionHydrator.php | 1 + .../AbstractCollectionPersister.php | 11 +++++++- .../ReadWriteCachedCollectionPersister.php | 1 + ...onStrictReadWriteCachedEntityPersister.php | 8 ++++-- .../Entity/ReadWriteCachedEntityPersister.php | 12 +++++--- .../AbstractCollectionPersister.php | 6 +--- .../Collection/CollectionPersister.php | 2 +- lib/Doctrine/ORM/UnitOfWork.php | 8 ++---- tests/Doctrine/Tests/Models/Cache/City.php | 2 +- tests/Doctrine/Tests/Models/Cache/State.php | 2 +- .../Doctrine/Tests/Models/Cache/Traveler.php | 2 +- .../Functional/ExtraLazyCollectionTest.php | 2 ++ .../SecondLevelCacheManyToOneTest.php | 22 ++++++++++++++- .../SecondLevelCacheOneToManyTest.php | 28 ++++++++++--------- 14 files changed, 71 insertions(+), 36 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultCollectionHydrator.php b/lib/Doctrine/ORM/Cache/DefaultCollectionHydrator.php index f6583cbd2ca..9199ec6dc15 100644 --- a/lib/Doctrine/ORM/Cache/DefaultCollectionHydrator.php +++ b/lib/Doctrine/ORM/Cache/DefaultCollectionHydrator.php @@ -67,6 +67,7 @@ public function buildCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key foreach ($collection as $index => $entity) { $data[$index] = new EntityCacheKey($metadata->name, $this->uow->getEntityIdentifier($entity)); } + return new CollectionCacheEntry($data); } diff --git a/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php b/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php index 3172fe9994b..abaef17a680 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php @@ -32,6 +32,7 @@ /** * @author Fabio B. Silva + * @author Guilherme Blanco * @since 2.5 */ abstract class AbstractCollectionPersister implements CachedCollectionPersister @@ -164,10 +165,18 @@ public function loadCollectionCache(PersistentCollection $collection, Collection public function storeCollectionCache(CollectionCacheKey $key, $elements) { /* @var $targetPersister CachedEntityPersister */ + $associationMapping = $this->sourceEntity->associationMappings[$key->association]; $targetPersister = $this->uow->getEntityPersister($this->targetEntity->rootEntityName); $targetRegion = $targetPersister->getCacheRegion(); $targetHydrator = $targetPersister->getEntityHydrator(); - $entry = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements); + + // Only preserve ordering if association configured it + if ( ! (isset($associationMapping['indexBy']) && $associationMapping['indexBy'])) { + // Elements may be an array or a Collection + $elements = array_values(is_array($elements) ? $elements : $elements->getValues()); + } + + $entry = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements); foreach ($entry->identifiers as $index => $entityKey) { if ($targetRegion->contains($entityKey)) { diff --git a/lib/Doctrine/ORM/Cache/Persister/Collection/ReadWriteCachedCollectionPersister.php b/lib/Doctrine/ORM/Cache/Persister/Collection/ReadWriteCachedCollectionPersister.php index a92ed20d302..660aa3d0d04 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Collection/ReadWriteCachedCollectionPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Collection/ReadWriteCachedCollectionPersister.php @@ -28,6 +28,7 @@ /** * @author Fabio B. Silva + * @author Guilherme Blanco * @since 2.5 */ class ReadWriteCachedCollectionPersister extends AbstractCollectionPersister diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php index fd296c63716..54c814a9a22 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php @@ -26,6 +26,7 @@ * Specific non-strict read/write cached entity persister * * @author Fabio B. Silva + * @author Guilherme Blanco * @since 2.5 */ class NonStrictReadWriteCachedEntityPersister extends AbstractEntityPersister @@ -77,13 +78,16 @@ public function afterTransactionRolledBack() */ public function delete($entity) { - $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); + $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); + $deleted = $this->persister->delete($entity); - if ($this->persister->delete($entity)) { + if ($deleted) { $this->region->evict($key); } $this->queuedCache['delete'][] = $key; + + return $deleted; } /** diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php index 8413082deac..07cac510d4c 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php @@ -30,6 +30,7 @@ * Specific read-write entity persister * * @author Fabio B. Silva + * @author Guilherme Blanco * @since 2.5 */ class ReadWriteCachedEntityPersister extends AbstractEntityPersister @@ -100,21 +101,24 @@ public function afterTransactionRolledBack() */ public function delete($entity) { - $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); - $lock = $this->region->lock($key); + $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); + $lock = $this->region->lock($key); + $deleted = $this->persister->delete($entity); - if ($this->persister->delete($entity)) { + if ($deleted) { $this->region->evict($key); } if ($lock === null) { - return; + return $deleted; } $this->queuedCache['delete'][] = array( 'lock' => $lock, 'key' => $key ); + + return $deleted; } /** diff --git a/lib/Doctrine/ORM/Persisters/Collection/AbstractCollectionPersister.php b/lib/Doctrine/ORM/Persisters/Collection/AbstractCollectionPersister.php index 094895671c5..2e85b67f3b2 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/AbstractCollectionPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/AbstractCollectionPersister.php @@ -90,10 +90,6 @@ protected function isValidEntityState($entity) // If Entity is scheduled for inclusion, it is not in this collection. // We can assure that because it would have return true before on array check - if ($entityState === UnitOfWork::STATE_MANAGED && $this->uow->isScheduledForInsert($entity)) { - return false; - } - - return true; + return ! ($entityState === UnitOfWork::STATE_MANAGED && $this->uow->isScheduledForInsert($entity)); } } diff --git a/lib/Doctrine/ORM/Persisters/Collection/CollectionPersister.php b/lib/Doctrine/ORM/Persisters/Collection/CollectionPersister.php index 36b5706a078..2eede798980 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/CollectionPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/CollectionPersister.php @@ -66,7 +66,7 @@ public function count(PersistentCollection $collection); * @param integer $offset * @param integer $length * - * @return array + * @return array */ public function slice(PersistentCollection $collection, $offset, $length = null); diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index a58e687eff6..3ba860b8917 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -733,7 +733,6 @@ public function computeChangeSet(ClassMetadata $class, $entity) // Look for changes in associations of the entity foreach ($class->associationMappings as $field => $assoc) { - if (($val = $class->reflFields[$field]->getValue($entity)) === null) { continue; } @@ -799,7 +798,7 @@ public function computeChangeSets() // Only MANAGED entities that are NOT SCHEDULED FOR INSERTION OR DELETION are processed here. $oid = spl_object_hash($entity); - if ( ! isset($this->entityInsertions[$oid]) && ! isset($this->entityDeletions[$oid]) && isset($this->entityStates[$oid])) { + if ( ! isset($this->entityInsertions[$oid]) && ! isset($this->entityDeletions[$oid]) && isset($this->entityStates[$oid])) { $this->computeChangeSet($class, $entity); } } @@ -826,10 +825,7 @@ private function computeAssociationChanges($assoc, $value) if ($value instanceof PersistentCollection && $value->isDirty()) { $coid = spl_object_hash($value); - if ($assoc['isOwningSide']) { - $this->collectionUpdates[$coid] = $value; - } - + $this->collectionUpdates[$coid] = $value; $this->visitedCollections[$coid] = $value; } diff --git a/tests/Doctrine/Tests/Models/Cache/City.php b/tests/Doctrine/Tests/Models/Cache/City.php index f755e07aec9..936f0feef8a 100644 --- a/tests/Doctrine/Tests/Models/Cache/City.php +++ b/tests/Doctrine/Tests/Models/Cache/City.php @@ -5,9 +5,9 @@ use Doctrine\Common\Collections\ArrayCollection; /** - * @Cache * @Entity * @Table("cache_city") + * @Cache */ class City { diff --git a/tests/Doctrine/Tests/Models/Cache/State.php b/tests/Doctrine/Tests/Models/Cache/State.php index 64812869dd6..0d37e4d8faa 100644 --- a/tests/Doctrine/Tests/Models/Cache/State.php +++ b/tests/Doctrine/Tests/Models/Cache/State.php @@ -33,7 +33,7 @@ class State protected $country; /** - * @Cache + * @Cache("NONSTRICT_READ_WRITE") * @OneToMany(targetEntity="City", mappedBy="state") */ protected $cities; diff --git a/tests/Doctrine/Tests/Models/Cache/Traveler.php b/tests/Doctrine/Tests/Models/Cache/Traveler.php index 32d7cf9361a..3f720b46595 100644 --- a/tests/Doctrine/Tests/Models/Cache/Traveler.php +++ b/tests/Doctrine/Tests/Models/Cache/Traveler.php @@ -26,7 +26,7 @@ class Traveler protected $name; /** - * @Cache() + * @Cache("NONSTRICT_READ_WRITE") * @OneToMany(targetEntity="Travel", mappedBy="traveler", cascade={"persist", "remove"}, orphanRemoval=true) * * @var \Doctrine\Common\Collections\Collection diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php index 731a0826d60..696d330c99e 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php @@ -1140,6 +1140,8 @@ public function testRemoveOrphanedManagedElementFromOneToManyExtraLazyCollection /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); + $this->assertFalse($user->userLists->isInitialized()); + $user->userLists->removeElement($this->_em->find(UserList::CLASSNAME, $userListId)); $this->_em->clear(); diff --git a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheManyToOneTest.php b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheManyToOneTest.php index 8c0225e7f0d..75c0b7da634 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheManyToOneTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheManyToOneTest.php @@ -2,10 +2,10 @@ namespace Doctrine\Tests\ORM\Functional; +use Doctrine\Tests\Models\Cache\City; use Doctrine\Tests\Models\Cache\ComplexAction; use Doctrine\Tests\Models\Cache\Country; use Doctrine\Tests\Models\Cache\State; -use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\Tests\Models\Cache\Token; use Doctrine\Tests\Models\Cache\Action; @@ -96,6 +96,26 @@ public function testPutAndLoadManyToOneRelation() $this->assertEquals($this->states[1]->getCountry()->getId(), $c4->getCountry()->getId()); $this->assertEquals($this->states[1]->getCountry()->getName(), $c4->getCountry()->getName()); + + //evict collection on add + $prev = $c3->getCities(); + $count = $prev->count(); + $city = new City("Buenos Aires", $c3); + + $c3->addCity($city); + + $this->_em->persist($city); + $this->_em->persist($c3); + $this->_em->flush(); + $this->_em->clear(); + + $state = $this->_em->find(State::CLASSNAME, $c3->getId()); + + // Association was cleared from EM + $this->assertNotEquals($prev, $state->getCities()); + + // New association has one more item (cache was evicted) + $this->assertEquals($count + 1, $state->getCities()->count()); } public function testShouldNotReloadWhenAssociationIsMissing() diff --git a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php index 3e4833dc826..829cc4056b6 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php @@ -14,18 +14,18 @@ */ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest { - public function testShouldNotPutCollectionInverseSideOnPersist() + public function testShouldPutCollectionInverseSideOnPersist() { $this->loadFixturesCountries(); $this->loadFixturesStates(); $this->loadFixturesCities(); + $this->_em->clear(); $this->assertTrue($this->cache->containsEntity(State::CLASSNAME, $this->states[0]->getId())); $this->assertTrue($this->cache->containsEntity(State::CLASSNAME, $this->states[1]->getId())); - - $this->assertFalse($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[0]->getId())); - $this->assertFalse($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[1]->getId())); + $this->assertTrue($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[0]->getId())); + $this->assertTrue($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[1]->getId())); } public function testPutAndLoadOneToManyRelation() @@ -187,6 +187,7 @@ public function testOneToManyRemove() $this->loadFixturesCountries(); $this->loadFixturesStates(); $this->loadFixturesCities(); + $this->_em->clear(); $this->secondLevelCacheLogger->clearStats(); @@ -247,8 +248,8 @@ public function testOneToManyRemove() $this->_em->remove($city0); $this->_em->persist($state); $this->_em->flush(); - $this->_em->clear(); + $this->secondLevelCacheLogger->clearStats(); $queryCount = $this->getCurrentQueryCount(); @@ -261,19 +262,19 @@ public function testOneToManyRemove() $this->assertInstanceOf(City::CLASSNAME, $city1); $this->assertEquals($entity->getCities()->get(1)->getName(), $city1->getName()); - $this->assertEquals(1, $this->secondLevelCacheLogger->getHitCount()); + $this->assertEquals(2, $this->secondLevelCacheLogger->getHitCount()); $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getEntityRegion(State::CLASSNAME))); - $this->assertEquals(0, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities'))); + $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities'))); - $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount()); + $this->assertEquals($queryCount, $this->getCurrentQueryCount()); $state->getCities()->remove(0); $this->_em->remove($city1); $this->_em->persist($state); $this->_em->flush(); - $this->_em->clear(); + $this->secondLevelCacheLogger->clearStats(); $queryCount = $this->getCurrentQueryCount(); @@ -281,9 +282,9 @@ public function testOneToManyRemove() $this->assertCount(0, $state->getCities()); - $this->assertEquals(1, $this->secondLevelCacheLogger->getHitCount()); + $this->assertEquals(2, $this->secondLevelCacheLogger->getHitCount()); $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getEntityRegion(State::CLASSNAME))); - $this->assertEquals(0, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities'))); + $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities'))); } public function testOneToManyWithEmptyRelation() @@ -346,11 +347,12 @@ public function testOneToManyCount() public function testCacheInitializeCollectionWithNewObjects() { $this->_em->clear(); + $this->evictRegions(); $traveler = new Traveler("Doctrine Bot"); - for ($i=0; $i<3; ++$i) { + for ($i = 0; $i < 3; ++$i) { $traveler->getTravels()->add(new Travel($traveler)); } @@ -373,7 +375,7 @@ public function testCacheInitializeCollectionWithNewObjects() $this->assertFalse($entity->getTravels()->isInitialized()); $this->assertCount(4, $entity->getTravels()); $this->assertTrue($entity->getTravels()->isInitialized()); - $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount()); + $this->assertEquals($queryCount, $this->getCurrentQueryCount()); $this->_em->flush(); $this->_em->clear();