From 9839c400b8af173f30b8ad83df2a2e1c26285d18 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 06:19:26 +0100 Subject: [PATCH 01/10] #1169 DDC-3343 - updating test expectations - one-to-many changes should be no-op unless orphan removal is specified. --- .../Functional/ExtraLazyCollectionTest.php | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php index 6021a9efd..f196654a6 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php @@ -528,22 +528,37 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase */ public function testRemovalOfManagedElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt() { + /* @var $otherClass DDC2504OtherClass */ $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId); + /* @var $childClass DDC2504ChildClass */ $childClass = $this->_em->find(DDC2504ChildClass::CLASSNAME, $this->ddc2504ChildClassId); $queryCount = $this->getCurrentQueryCount(); $otherClass->childClasses->removeElement($childClass); + $childClass->other = null; // updating owning side $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.'); $this->assertEquals( - $queryCount + 1, + $queryCount, $this->getCurrentQueryCount(), - 'The owning side of the association is updated' + 'No queries have been executed' ); - $this->assertFalse($otherClass->childClasses->contains($childClass)); + $this->assertTrue( + $otherClass->childClasses->contains($childClass), + 'Collection item still not updated (needs flushing)' + ); + + $this->_em->flush(); + + $this->assertFalse( + $otherClass->childClasses->contains($childClass), + 'Referenced item was removed in the transaction' + ); + + $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.'); } /** @@ -551,6 +566,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase */ public function testRemovalOfNonManagedElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt() { + /* @var $otherClass DDC2504OtherClass */ $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId); $queryCount = $this->getCurrentQueryCount(); @@ -568,6 +584,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase */ public function testRemovalOfNewElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt() { + /* @var $otherClass DDC2504OtherClass */ $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId); $childClass = new DDC2504ChildClass(); @@ -1035,7 +1052,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase /** * @group DDC-3343 */ - public function testRemovesManagedElementFromOneToManyExtraLazyCollection() + public function testRemoveManagedElementFromOneToManyExtraLazyCollectionIsNoOp() { list($userId, $tweetId) = $this->loadTweetFixture(); @@ -1049,13 +1066,13 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); - $this->assertCount(0, $user->tweets); + $this->assertCount(1, $user->tweets, 'Element was not removed - need to update the owning side first'); } /** * @group DDC-3343 */ - public function testRemovesManagedElementFromOneToManyExtraLazyCollectionWithoutDeletingTheTargetEntityEntry() + public function testRemoveManagedElementFromOneToManyExtraLazyCollectionWithoutDeletingTheTargetEntityEntryIsNoOp() { list($userId, $tweetId) = $this->loadTweetFixture(); @@ -1076,7 +1093,11 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase 'Even though the collection is extra lazy, the tweet should not have been deleted' ); - $this->assertNull($tweet->author, 'Tweet author link has been removed'); + $this->assertInstanceOf( + User::CLASSNAME, + $tweet->author, + 'Tweet author link has not been removed - need to update the owning side first' + ); } /** @@ -1107,7 +1128,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); - $this->assertCount(0, $user->tweets); + $this->assertCount(1, $user->tweets, 'Element was not removed - need to update the owning side first'); } /** From 539c364ca6e48a08cd5c6f88ec9c209ac227bb0d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 06:26:55 +0100 Subject: [PATCH 02/10] #1169 DDC-3343 - updating test expectations - one-to-many changes should be no-op unless orphan removal is specified. --- .../Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php index f196654a6..353f0a52a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php @@ -485,7 +485,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized."); // NOTE: +2 queries because CmsArticle is a versioned entity, and that needs to be handled accordingly - $this->assertEquals($queryCount + 2, $this->getCurrentQueryCount()); + $this->assertEquals($queryCount, $this->getCurrentQueryCount()); // Test One to Many removal with Entity state as new $article = new \Doctrine\Tests\Models\CMS\CmsArticle(); @@ -1123,7 +1123,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase 'Even though the collection is extra lazy, the tweet should not have been deleted' ); - $this->assertNull($tweet->author); + $this->assertInstanceOf(User::CLASSNAME, $tweet->author); /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); From 9eaac1361574937b6dd473b0f4d1aa9c2aaf1e03 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 06:27:37 +0100 Subject: [PATCH 03/10] #1169 DDC-3343 - correcting one-to-many persister - association should not be updated directly if no orphan removal is involved --- .../ORM/Persisters/Collection/OneToManyPersister.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php index 9dc2fad54..fab5e9393 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php @@ -157,11 +157,17 @@ class OneToManyPersister extends AbstractCollectionPersister */ public function removeElement(PersistentCollection $collection, $element) { + $mapping = $collection->getMapping(); + + if ( ! $mapping['orphanRemoval']) { + // no-op: this is not the owning side, therefore no operations should be applied + return false; + } + if ( ! $this->isValidEntityState($element)) { return false; } - $mapping = $collection->getMapping(); $persister = $this->uow->getEntityPersister($mapping['targetEntity']); $targetMetadata = $this->em->getClassMetadata($mapping['targetEntity']); From a0eb6005f38d0d5cc3fe058d1ff955d6a5b326e8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 07:42:48 +0100 Subject: [PATCH 04/10] #1169 DDC-3343 - adding tests for orphan-removal + extra-lazy + one-to-many element removal behavior --- tests/Doctrine/Tests/Models/Tweet/User.php | 14 ++- .../Doctrine/Tests/Models/Tweet/UserList.php | 29 +++++ .../Functional/ExtraLazyCollectionTest.php | 104 +++++++++++++++++- .../Doctrine/Tests/OrmFunctionalTestCase.php | 9 +- 4 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 tests/Doctrine/Tests/Models/Tweet/UserList.php diff --git a/tests/Doctrine/Tests/Models/Tweet/User.php b/tests/Doctrine/Tests/Models/Tweet/User.php index 4722e6317..68cf137f2 100644 --- a/tests/Doctrine/Tests/Models/Tweet/User.php +++ b/tests/Doctrine/Tests/Models/Tweet/User.php @@ -29,9 +29,15 @@ class User */ public $tweets; + /** + * @OneToMany(targetEntity="UserList", mappedBy="owner", fetch="EXTRA_LAZY", orphanRemoval=true) + */ + public $userLists; + public function __construct() { - $this->tweets = new ArrayCollection(); + $this->tweets = new ArrayCollection(); + $this->userLists = new ArrayCollection(); } public function addTweet(Tweet $tweet) @@ -39,4 +45,10 @@ class User $tweet->setAuthor($this); $this->tweets->add($tweet); } + + public function addUserList(UserList $userList) + { + $userList->owner = $this; + $this->userLists->add($userList); + } } diff --git a/tests/Doctrine/Tests/Models/Tweet/UserList.php b/tests/Doctrine/Tests/Models/Tweet/UserList.php new file mode 100644 index 000000000..1eb3e141f --- /dev/null +++ b/tests/Doctrine/Tests/Models/Tweet/UserList.php @@ -0,0 +1,29 @@ +_em->find(User::CLASSNAME, $userId); + $tweet = $this->_em->find(Tweet::CLASSNAME, $tweetId); - $e = $this->_em->find(Tweet::CLASSNAME, $tweetId); - - $user->tweets->removeElement($e); + $user->tweets->removeElement($tweet); $this->_em->clear(); @@ -1131,6 +1131,83 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase $this->assertCount(1, $user->tweets, 'Element was not removed - need to update the owning side first'); } + /** + * @group DDC-3343 + */ + public function testRemoveOrphanedManagedElementFromOneToManyExtraLazyCollection() + { + list($userId, $userListId) = $this->loadUserListFixture(); + + /* @var $user User */ + $user = $this->_em->find(User::CLASSNAME, $userId); + + $user->tweets->removeElement($this->_em->find(UserList::CLASSNAME, $userListId)); + + $this->_em->clear(); + + /* @var $user User */ + $user = $this->_em->find(User::CLASSNAME, $userId); + + $this->assertCount(0, $user->userLists, 'Element was removed from association due to orphan removal'); + $this->assertNull( + $this->_em->find(UserList::CLASSNAME, $userListId), + 'Element was deleted due to orphan removal' + ); + } + + /** + * @group DDC-3343 + */ + public function testRemoveOrphanedUnManagedElementFromOneToManyExtraLazyCollection() + { + list($userId, $userListId) = $this->loadUserListFixture(); + + /* @var $user User */ + $user = $this->_em->find(User::CLASSNAME, $userId); + + $user->userLists->removeElement(new UserList()); + + $this->_em->clear(); + + /* @var $userList UserList */ + $userList = $this->_em->find(UserList::CLASSNAME, $userListId); + $this->assertInstanceOf( + UserList::CLASSNAME, + $userList, + 'Even though the collection is extra lazy + orphan removal, the user list should not have been deleted' + ); + + $this->assertInstanceOf( + User::CLASSNAME, + $userList->owner, + 'User list to owner link has not been removed' + ); + } + + /** + * @group DDC-3343 + */ + public function testRemoveOrphanedManagedLazyProxyFromExtraLazyOneToMany() + { + list($userId, $userListId) = $this->loadUserListFixture(); + + /* @var $user User */ + $user = $this->_em->find(User::CLASSNAME, $userId); + + $user->tweets->removeElement($this->_em->getReference(UserList::CLASSNAME, $userListId)); + + $this->_em->clear(); + + /* @var $user User */ + $user = $this->_em->find(User::CLASSNAME, $userId); + + $this->assertCount(0, $user->userLists, 'Element was removed from association due to orphan removal'); + $this->assertNull( + $this->_em->find(UserList::CLASSNAME, $userListId), + 'Element was deleted due to orphan removal' + ); + } + /** * @return int[] ordered tuple: user id and tweet id */ @@ -1151,4 +1228,25 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase return array($user->id, $tweet->id); } + + /** + * @return int[] ordered tuple: user id and user list id + */ + private function loadUserListFixture() + { + $user = new User(); + $userList = new UserList(); + + $user->name = 'ocramius'; + $userList->listName = 'PHP Developers to follow closely'; + + $user->addUserList($userList); + + $this->_em->persist($user); + $this->_em->persist($userList); + $this->_em->flush(); + $this->_em->clear(); + + return array($user->id, $userList->id); + } } diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index f73ac54fc..fedeed734 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -186,7 +186,8 @@ abstract class OrmFunctionalTestCase extends OrmTestCase ), 'tweet' => array( 'Doctrine\Tests\Models\Tweet\User', - 'Doctrine\Tests\Models\Tweet\Tweet' + 'Doctrine\Tests\Models\Tweet\Tweet', + 'Doctrine\Tests\Models\Tweet\UserList', ), 'ddc2504' => array( 'Doctrine\Tests\Models\DDC2504\DDC2504RootClass', @@ -387,6 +388,12 @@ abstract class OrmFunctionalTestCase extends OrmTestCase $conn->executeUpdate('DELETE FROM taxi_driver'); } + if (isset($this->_usedModelSets['tweet'])) { + $conn->executeUpdate('DELETE FROM tweet_tweet'); + $conn->executeUpdate('DELETE FROM tweet_user_list'); + $conn->executeUpdate('DELETE FROM tweet_user'); + } + if (isset($this->_usedModelSets['cache'])) { $conn->executeUpdate('DELETE FROM cache_attraction_location_info'); $conn->executeUpdate('DELETE FROM cache_attraction_contact_info'); From 3f28adf9b6a330efd25d312060a752c1f35a4dc1 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 07:45:58 +0100 Subject: [PATCH 05/10] #1169 DDC-3343 - correcting collection name used in tests --- .../Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php index 2d36e9957..b60eb0286 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php @@ -1141,7 +1141,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); - $user->tweets->removeElement($this->_em->find(UserList::CLASSNAME, $userListId)); + $user->userLists->removeElement($this->_em->find(UserList::CLASSNAME, $userListId)); $this->_em->clear(); @@ -1194,7 +1194,7 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase /* @var $user User */ $user = $this->_em->find(User::CLASSNAME, $userId); - $user->tweets->removeElement($this->_em->getReference(UserList::CLASSNAME, $userListId)); + $user->userLists->removeElement($this->_em->getReference(UserList::CLASSNAME, $userListId)); $this->_em->clear(); From f32766c00d735d6fc501d9869ccddd8e534c4e55 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 07:46:31 +0100 Subject: [PATCH 06/10] #1169 DDC-3343 - when using one-to-many extra-lazy with orphan-removal, referenced entities should be deleted directly --- .../Collection/OneToManyPersister.php | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php index fab5e9393..c18fdbbb9 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php @@ -168,22 +168,10 @@ class OneToManyPersister extends AbstractCollectionPersister return false; } - $persister = $this->uow->getEntityPersister($mapping['targetEntity']); - - $targetMetadata = $this->em->getClassMetadata($mapping['targetEntity']); - - if ($element instanceof Proxy && ! $element->__isInitialized()) { - $element->__load(); - } - - // clearing owning side value - $targetMetadata->reflFields[$mapping['mappedBy']]->setValue($element, null); - - $this->uow->computeChangeSet($targetMetadata, $element); - - $persister->update($element); - - return true; + return $this + ->uow + ->getEntityPersister($mapping['targetEntity']) + ->delete($element); } /** From c4019d96b9f369ef1bc4c651c2eea0917ace1426 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 09:03:36 +0100 Subject: [PATCH 07/10] #1169 DDC-3343 - when a deletion fails for some reason, related cache entries should be evicted to avoid collisions with DB state --- .../Tests/ORM/Functional/SecondLevelCacheTest.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php index 37dadd26c..56735840a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php @@ -296,10 +296,12 @@ class SecondLevelCacheTest extends SecondLevelCacheAbstractTest $this->_em->clear(); - $this->assertTrue($this->cache->containsEntity(Country::CLASSNAME, $countryId)); + $this->assertFalse( + $this->cache->containsEntity(Country::CLASSNAME, $countryId), + 'Removal attempts should clear the cache entry corresponding to the entity' + ); - $country = $this->_em->find(Country::CLASSNAME, $countryId); - $this->assertInstanceOf(Country::CLASSNAME, $country); + $this->assertInstanceOf(Country::CLASSNAME, $this->_em->find(Country::CLASSNAME, $countryId)); } public function testCachedNewEntityExists() From a9671fdc2e211e9e6490f2989a361905691be751 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 09:04:00 +0100 Subject: [PATCH 08/10] #1169 DDC-3343 - eagerly evicting cache if a persister passes a delete operation down to the DB --- .../Entity/NonStrictReadWriteCachedEntityPersister.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php index b120ae2fa..d50d8c500 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php @@ -78,9 +78,13 @@ class NonStrictReadWriteCachedEntityPersister extends AbstractEntityPersister */ public function delete($entity) { - $this->persister->delete($entity); + $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); - $this->queuedCache['delete'][] = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); + if ($this->persister->delete($entity)) { + $this->region->evict($key); + } + + $this->queuedCache['delete'][] = $key; } /** From c7a6352b0872549c6336c4dfe839f7bec9ecdd80 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 27 Jan 2015 09:04:28 +0100 Subject: [PATCH 09/10] #1169 DDC-3343 - eagerly evicting cache if a persister passes a delete operation down to the DB --- .../Cache/Persister/Entity/ReadWriteCachedEntityPersister.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php index 0f65e4c9b..0d0a1af9d 100644 --- a/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php +++ b/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php @@ -103,7 +103,9 @@ class ReadWriteCachedEntityPersister extends AbstractEntityPersister $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity)); $lock = $this->region->lock($key); - $this->persister->delete($entity); + if ($this->persister->delete($entity)) { + $this->region->evict($key); + } if ($lock === null) { return; From e76b20b1098731a82eadfed53e4e28a70e29e90d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 28 Jan 2015 20:01:57 +0000 Subject: [PATCH 10/10] #1169 DDC-3343 - removing note about query count assertion --- tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php index b60eb0286..731a0826d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php @@ -485,7 +485,6 @@ class ExtraLazyCollectionTest extends OrmFunctionalTestCase $user->articles->removeElement($article); $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized."); - // NOTE: +2 queries because CmsArticle is a versioned entity, and that needs to be handled accordingly $this->assertEquals($queryCount, $this->getCurrentQueryCount()); // Test One to Many removal with Entity state as new