Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Bugfix for MODM166 - collections not handled properly in recomputeSingleDocumentChangeSet #336

Merged
merged 1 commit into from

2 participants

@superdweebie

Added test.

Added fix.

Did modest refactoring in uow to remove code duplication between computeChangeSet and recomputeSingleDocumentChangeSet.

@jmikola jmikola commented on the diff
lib/Doctrine/ODM/MongoDB/UnitOfWork.php
((10 lines not shown))
foreach ($actualData as $propName => $actualValue) {
+ // skip not saved fields
+ if (isset($class->fieldMappings[$propName]['notSaved']) && $class->fieldMappings[$propName]['notSaved'] === true) {
+ continue;
+ }
@jmikola Owner
jmikola added a note

I noticed this is also (and originally) further down in the method's $class->fieldMappings as $mapping loop. Was it a bug that the field also wasn't skipped in this loop?

@jmikola I noticed that not saved fields weren't being skipped as early as possible, so I moved it up to make execution a little faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -736,47 +802,6 @@ public function computeChangeSet(ClassMetadata $class, $document)
}
/**
- * Computes all the changes that have been done to documents and collections
- * since the last commit and stores these changes in the _documentChangeSet map
- * temporarily for access by the persisters, until the UoW commit is finished.
- */
- public function computeChangeSets()
@jmikola Owner
jmikola added a note

It doesn't look like this method was changed (only relocated). Assuming there were no changes, can you move it back to keep the diff smaller? It makes it a bit more difficult to review the actual changes in this PR.

@jmikola I moved it to try and put the methods in a logical order, but I can move it back to make the PR easier to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...e/ODM/MongoDB/Tests/Functional/Ticket/MODM166Test.php
((45 lines not shown))
+ $phonenumbers[] = $phonenumber->getPhonenumber();
+ }
+ sort($phonenumbers);
+
+ $this->assertEquals(array('1111', '2222'), $phonenumbers);
+ }
+}
+
+class MODM166EventListener
+{
+ public function onFlush(OnFlushEventArgs $eventArgs)
+ {
+ $documentManager = $eventArgs->getDocumentManager();
+ $unitOfWork = $documentManager->getUnitOfWork();
+
+ foreach ($unitOfWork->getScheduledDocumentUpdates() AS $document) {
@jmikola Owner
jmikola added a note

CS: lower-case AS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...e/ODM/MongoDB/Tests/Functional/Ticket/MODM166Test.php
((51 lines not shown))
+}
+
+class MODM166EventListener
+{
+ public function onFlush(OnFlushEventArgs $eventArgs)
+ {
+ $documentManager = $eventArgs->getDocumentManager();
+ $unitOfWork = $documentManager->getUnitOfWork();
+
+ foreach ($unitOfWork->getScheduledDocumentUpdates() AS $document) {
+ $metadata = $documentManager->getClassMetadata(get_class($document));
+ $document->addPhonenumber(new Phonenumber('2222'));
+ $unitOfWork->recomputeSingleDocumentChangeSet($metadata, $document);
+ }
+ }
+}
@jmikola Owner
jmikola added a note

CS: Newline at end of file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...e/ODM/MongoDB/Tests/Functional/Ticket/MODM166Test.php
@@ -0,0 +1,66 @@
+<?php
+
+namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;
+
+use Doctrine\ODM\MongoDB\Events;
+use Documents\User;
+use Documents\Phonenumber;
+use Doctrine\ODM\MongoDB\Event\OnFlushEventArgs;
@jmikola Owner
jmikola added a note

CS: alphabetical order for use statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...e/ODM/MongoDB/Tests/Functional/Ticket/MODM166Test.php
((7 lines not shown))
+use Documents\Phonenumber;
+use Doctrine\ODM\MongoDB\Event\OnFlushEventArgs;
+
+class MODM166Test extends \Doctrine\ODM\MongoDB\Tests\BaseTest
+{
+
+ public function setUp()
+ {
+ parent::setUp();
+
+ $this->listener = new MODM166EventListener();
+ $evm = $this->dm->getEventManager();
+ $events = array(
+ Events::onFlush
+ );
+ $evm->addEventListener($events, $this->listener);
@jmikola Owner
jmikola added a note

Let's replace these four lines with: $evm->addEventListener(Events::onFlush, $this->listener); to be concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola jmikola was assigned
@jmikola
Owner

@superdweebie: It looks like we might be stepping on each others' toes with #329. I'd like to get @jwage to review that first and merge down, so it may be a good idea to hold off on these changes until that's done.

@jmikola
Owner

@superdweebie: Evidently there's no conflict. Feel free to make any revisions.

@jmikola jmikola commented on the diff
lib/Doctrine/ODM/MongoDB/UnitOfWork.php
((28 lines not shown))
- $this->collectionDeletions[] = $orgValue;
- $this->collectionUpdates[] = $actualValue;
- $this->visitedCollections[] = $actualValue;
- }
- }
- } else if ($orgValue !== $actualValue) {
- $changeSet[$propName] = array($orgValue, $actualValue);
- }
- }
- if ($changeSet) {
- if (isset($this->documentChangeSets[$oid])) {
- $this->documentChangeSets[$oid] = $changeSet + $this->documentChangeSets[$oid];
- }
- $this->originalDocumentData[$oid] = $actualData;
- }
+ $this->computeOrRecomputeChangeSet($class, $document, true);
@jmikola Owner
jmikola added a note

Is it a problem if recomputeSingleDocumentChangeSet() never had the "Look for changes in associations of the document" loop that was in computeChangeSet() and is now in computeOrRecomputeChangeSet()? That is the only difference in execution path that I can see.

@jmikola Yes, this is the fix for the bug. If recomputeSingleDocumentChangeSet() doesn't have the extra loop, then changes to associations aren't processed properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@superdweebie

@jmikola After stuffing around with git (and learning a few new things), I think I have all your requested changes in the commit.

@jmikola jmikola commented on the diff
lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -895,7 +920,7 @@ private function computeAssociationChanges($parentDocument, $mapping, $value)
* @param object $document The document for which to (re)calculate the change set.
* @throws InvalidArgumentException If the passed document is not MANAGED.
*/
- public function recomputeSingleDocumentChangeSet($class, $document)
+ public function recomputeSingleDocumentChangeSet(ClassMetadata $class, $document)
@jmikola Owner
jmikola added a note

@jwage: Was there any particular reason many of the UoW methods didn't type-hint for ClassMetadata? Is it a micro-optimization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola
Owner

Thanks @superdweebie; the diff is much more concise now :)

@jwage: I left one question for you above. Beyond that, this PR has my blessing so feel free to merge down if it looks alright to you.

@jmikola
Owner

@superdweebie: I was just re-testing this and noticed that your test case (MODM166Test) currently passes in the master branch without this commit merged. If that's the case, is there really a bug being fixed in this commit or is it just refactoring?

Based on the diff, the only functional change (not refactoring) that I can see is skipping NotSaved fields in the first foreach loop of computeChangeSet() -- and that's certainly unrelated to the MODM-166.

@superdweebie

@jmikola Are you sure? I just loaded up the latest (commit e2c1fbf), and test for MODM166 was failing against it.

@jmikola
Owner

My mistake, I must have been on the wrong branch when I tested that.

@jmikola jmikola merged commit 11ddc65 into doctrine:master
@superdweebie

@jmikola :) No problem

@jmikola
Owner

I checked ORM, and they also have type-hinting in the changeset compute methods (but not elsewhere). That was the only outstanding question, so we're merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 25, 2012
  1. @superdweebie

    Added test and fix for bug MODM166

    superdweebie authored
     with CS cleanup
This page is out of date. Refresh to see the latest.
View
73 lib/Doctrine/ODM/MongoDB/UnitOfWork.php
@@ -643,6 +643,18 @@ public function computeChangeSet(ClassMetadata $class, $document)
$class->invokeLifecycleCallbacks(Events::preFlush, $document);
}
+ $this->computeOrRecomputeChangeSet($class, $document);
+ }
+
+ /**
+ * Used to do the common work of computeChangeSet and recomputeSingleDocumentChangeSet
+ *
+ * @param \Doctrine\ODM\MongoDB\Mapping\ClassMetadata $class
+ * @param object $document
+ * @param boolean $recompute
+ */
+ private function computeOrRecomputeChangeSet(ClassMetadata $class, $document, $recompute = false)
+ {
$oid = spl_object_hash($document);
$actualData = $this->getDocumentActualData($document);
$isNewDocument = ! isset($this->originalDocumentData[$oid]);
@@ -660,9 +672,18 @@ public function computeChangeSet(ClassMetadata $class, $document)
// and we have a copy of the original data
$originalData = $this->originalDocumentData[$oid];
$isChangeTrackingNotify = $class->isChangeTrackingNotify();
- $changeSet = $isChangeTrackingNotify ? $this->documentChangeSets[$oid] : array();
+ if ($isChangeTrackingNotify && !$recompute) {
+ $changeSet = $this->documentChangeSets[$oid];
+ } else {
+ $changeSet = array();
+ }
foreach ($actualData as $propName => $actualValue) {
+ // skip not saved fields
+ if (isset($class->fieldMappings[$propName]['notSaved']) && $class->fieldMappings[$propName]['notSaved'] === true) {
+ continue;
+ }
@jmikola Owner
jmikola added a note

I noticed this is also (and originally) further down in the method's $class->fieldMappings as $mapping loop. Was it a bug that the field also wasn't skipped in this loop?

@jmikola I noticed that not saved fields weren't being skipped as early as possible, so I moved it up to make execution a little faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
$orgValue = isset($originalData[$propName]) ? $originalData[$propName] : null;
// skip if value has not changed
@@ -732,7 +753,11 @@ public function computeChangeSet(ClassMetadata $class, $document)
$changeSet[$propName] = array($orgValue, $actualValue);
}
if ($changeSet) {
- $this->documentChangeSets[$oid] = $changeSet;
+ if ($recompute) {
+ $this->documentChangeSets[$oid] = $changeSet + $this->documentChangeSets[$oid];
+ } else {
+ $this->documentChangeSets[$oid] = $changeSet;
+ }
$this->originalDocumentData[$oid] = $actualData;
$this->documentUpdates[$oid] = $document;
}
@@ -895,7 +920,7 @@ private function computeAssociationChanges($parentDocument, $mapping, $value)
* @param object $document The document for which to (re)calculate the change set.
* @throws InvalidArgumentException If the passed document is not MANAGED.
*/
- public function recomputeSingleDocumentChangeSet($class, $document)
+ public function recomputeSingleDocumentChangeSet(ClassMetadata $class, $document)
@jmikola Owner
jmikola added a note

@jwage: Was there any particular reason many of the UoW methods didn't type-hint for ClassMetadata? Is it a micro-optimization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
$oid = spl_object_hash($document);
@@ -907,45 +932,7 @@ public function recomputeSingleDocumentChangeSet($class, $document)
$class = $this->dm->getClassMetadata(get_class($document));
}
- $actualData = $this->getDocumentActualData($document);
- $isChangeTrackingNotify = $class->isChangeTrackingNotify();
-
- $originalData = isset($this->originalDocumentData[$oid]) ? $this->originalDocumentData[$oid] : array();
- $changeSet = array();
- foreach ($actualData as $propName => $actualValue) {
- // skip not saved fields
- if (isset($class->fieldMappings[$propName]['notSaved']) && $class->fieldMappings[$propName]['notSaved'] === true) {
- continue;
- }
- $orgValue = isset($originalData[$propName]) ? $originalData[$propName] : null;
- if (isset($class->fieldMappings[$propName]['embedded']) && $class->fieldMappings[$propName]['type'] === 'one' && $orgValue !== $actualValue) {
- if ($orgValue !== null) {
- $this->scheduleOrphanRemoval($orgValue);
- }
- $changeSet[$propName] = array($orgValue, $actualValue);
- } else if (isset($class->fieldMappings[$propName]['reference']) && $class->fieldMappings[$propName]['type'] === 'one' && $orgValue !== $actualValue) {
- $changeSet[$propName] = array($orgValue, $actualValue);
- } else if ($isChangeTrackingNotify) {
- continue;
- } else if (isset($class->fieldMappings[$propName]['type']) && $class->fieldMappings[$propName]['type'] === 'many') {
- if ($orgValue !== $actualValue) {
- $changeSet[$propName] = array($orgValue, $actualValue);
- if ($orgValue instanceof PersistentCollection) {
- $this->collectionDeletions[] = $orgValue;
- $this->collectionUpdates[] = $actualValue;
- $this->visitedCollections[] = $actualValue;
- }
- }
- } else if ($orgValue !== $actualValue) {
- $changeSet[$propName] = array($orgValue, $actualValue);
- }
- }
- if ($changeSet) {
- if (isset($this->documentChangeSets[$oid])) {
- $this->documentChangeSets[$oid] = $changeSet + $this->documentChangeSets[$oid];
- }
- $this->originalDocumentData[$oid] = $actualData;
- }
+ $this->computeOrRecomputeChangeSet($class, $document, true);
@jmikola Owner
jmikola added a note

Is it a problem if recomputeSingleDocumentChangeSet() never had the "Look for changes in associations of the document" loop that was in computeChangeSet() and is now in computeOrRecomputeChangeSet()? That is the only difference in execution path that I can see.

@jmikola Yes, this is the fix for the bug. If recomputeSingleDocumentChangeSet() doesn't have the extra loop, then changes to associations aren't processed properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
private function persistNew($class, $document)
@@ -1656,7 +1643,7 @@ public function tryGetById($id, $rootClassName)
return isset($this->identityMap[$rootClassName][$id]) ?
$this->identityMap[$rootClassName][$id] : false;
}
-
+
/**
* Schedules a document for dirty-checking at commit-time.
*
View
63 tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/MODM166Test.php
@@ -0,0 +1,63 @@
+<?php
+
+namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;
+
+use Doctrine\ODM\MongoDB\Event\OnFlushEventArgs;
+use Doctrine\ODM\MongoDB\Events;
+use Documents\Phonenumber;
+use Documents\User;
+
+class MODM166Test extends \Doctrine\ODM\MongoDB\Tests\BaseTest
+{
+
+ public function setUp()
+ {
+ parent::setUp();
+
+ $this->listener = new MODM166EventListener();
+ $evm = $this->dm->getEventManager();
+ $evm->addEventListener(Events::onFlush, $this->listener);
+ return $this->dm;
+ }
+
+ public function testUpdateCollectionDuringOnFlushAndRecomputSingleDocumentChangeSet()
+ {
+ // create a test document
+ $test = new User();
+ $test->setUsername('toby');
+ $test->addPhonenumber(new Phonenumber('1111'));
+
+ $this->dm->persist($test);
+ $this->dm->flush();
+
+ $test->setUsername('lucy');
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $repository = $this->dm->getRepository(get_class($test));
+ $test = $repository->findOneBy(array('username' => 'lucy'));
+
+ $phonenumbers = array();
+ foreach ($test->getPhonenumbers() as $phonenumber){
+ $phonenumbers[] = $phonenumber->getPhonenumber();
+ }
+ sort($phonenumbers);
+
+ $this->assertEquals(array('1111', '2222'), $phonenumbers);
+ }
+}
+
+class MODM166EventListener
+{
+ public function onFlush(OnFlushEventArgs $eventArgs)
+ {
+ $documentManager = $eventArgs->getDocumentManager();
+ $unitOfWork = $documentManager->getUnitOfWork();
+
+ foreach ($unitOfWork->getScheduledDocumentUpdates() as $document) {
+ $metadata = $documentManager->getClassMetadata(get_class($document));
+ $document->addPhonenumber(new Phonenumber('2222'));
+ $unitOfWork->recomputeSingleDocumentChangeSet($metadata, $document);
+ }
+ }
+}
Something went wrong with that request. Please try again.