Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[WIP] remove duplicate original data tracking (PHPCR-57) #118

Open
wants to merge 6 commits into from

3 participants

@lsmith77
Owner

the tests pass .. but there is a chance this doesn't cover all edge cases

@dbu
Collaborator
dbu commented

i agree with this. to compensate for the limited test coverage, did you run this in your big project? or in the sandbox? if it does not fail in those, i would be pretty optimistic.

@lsmith77
Owner

no i have not .. but i agree we should before merging this .. maybe @chregu can try this as part of his current round of performance improvements. like i said, i think this could potentially improve memory consumption, though i am not sure if it will. i am fairly confident that it shouldn't hurt performance.

@lsmith77
Owner

@chregu has run some tests in our app and found some issues related to binary handling. guess i need to dive into this deeper than i had hoped was necessary :-/

@dbu
Collaborator
dbu commented

they probably can't be part of the diff but need to be pushed separatly i guess. or somehow be encoded for json.

@lsmith77
Owner

umm maybe i just misunderstood you .. but sounds like you are commenting on the JSOP stuff?

@dbu
Collaborator
dbu commented

indeed. sorry for the confusion, completely out of context.

@chregu

JFTR. they can be part of the diff and they don't have to be encoded. multi-part posts FTW (and therefore saves some bytes, since we don't have to encode it in base64 ;))

But still out of context ;)

@lsmith77 lsmith77 Merge remote-tracking branch 'origin/master' into remove_duplicate_or…
…iginal_data

Conflicts:
	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
79fd881
@lsmith77
Owner

the issue i see in our project is really bizarre. we iterate over a list and in each iteration we do a flush. basically on the 5th iteration i get the following error:
[PHPCR\ValueFormatException]

Cannot convert value into a binary resource

this is caused by a resource suddenly being an "unknown type". not sure what could cause this.
however one thing to note here is that the node in question didn't really change. all that is being done is that a reference to the grandparent node is being created.

@chregu

"unknown type" is usually after an fclose, JFYI :)

@lsmith77
Owner

so in general the issue might be more related to the fact that we are incorrectly detecting a change.
furthermore i wonder why we do "all or nothing" aka either we update everything .. or nothing.

@lsmith77
Owner

i changed the code to more precisely compute which fields actually need to be updated. it doesn't fix the issue i was seeing.

i have also made the same change based on current master in #123

@dbu
Collaborator

i am currently refactoring the PHPCR PropertyType::convertType method. not sure if you are using that. but afaikt it should not affect you yet, i did only push minor cleanups.

lsmith77 added some commits
@lsmith77 lsmith77 Merge branch 'master' into remove_duplicate_original_data
Conflicts:
	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
3eb8ca8
@lsmith77 lsmith77 Merge branch 'master' into remove_duplicate_original_data
Conflicts:
	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
d202d1a
@dbu
Collaborator

should we merge now? if the tests pass, the cmf sandbox is not broken and you think its correct now, then lets just do it and see what happens :-)

@lsmith77
Owner

no, the issue i noticed in the test suite of our project isn't fixed yet. or at least last time i tried it wasn't fixed.

@dbu
Collaborator

is this affected by the fix from uwe in #130 about duplicating calculating changesets?

@lsmith77
Owner

i will have to take another look .. there were a lot of changes that might have a positive effect on the goals of this sprint.

@dbu
Collaborator

is this still relevant? if so it needs a rebase.

@dbu
Collaborator
dbu commented

ping

@dbu
Collaborator

ping

@lsmith77 lsmith77 added ready and removed in progress labels
@lsmith77 lsmith77 added the wip/poc label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 4, 2012
  1. @lsmith77
Commits on Mar 12, 2012
  1. @lsmith77

    Merge remote-tracking branch 'origin/master' into remove_duplicate_or…

    lsmith77 authored
    …iginal_data
    
    Conflicts:
    	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
  2. @lsmith77
  3. @lsmith77

    Merge branch 'master' into remove_duplicate_original_data

    lsmith77 authored
    Conflicts:
    	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
Commits on Mar 14, 2012
  1. @lsmith77

    Merge branch 'master' into remove_duplicate_original_data

    lsmith77 authored
    Conflicts:
    	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
Commits on Mar 16, 2012
  1. @lsmith77

    Merge branch 'master' into remove_duplicate_original_data

    lsmith77 authored
    Conflicts:
    	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
This page is out of date. Refresh to see the latest.
Showing with 39 additions and 63 deletions.
  1. +39 −63 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
View
102 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -99,21 +99,6 @@ class UnitOfWork
private $documentLocales = array();
/**
- * PHPCR always returns and updates the whole data of a document. If on update data is "missing"
- * this means the data is deleted. This also applies to attachments. This is why we need to ensure
- * that data that is not mapped is not lost. This map here saves all the "left-over" data and keeps
- * track of it if necessary.
- *
- * @var array
- */
- private $nonMappedData = array();
-
- /**
- * @var array
- */
- private $originalData = array();
-
- /**
* @var array
*/
private $documentChangesets = array();
@@ -232,7 +217,6 @@ public function createDocument($className, $node, array &$hints = array())
$class = $this->dm->getClassMetadata($className);
$documentState = array();
- $nonMappedData = array();
$id = $node->getPath();
// second param is false to get uuid rather than dereference reference properties to node instances
@@ -352,11 +336,9 @@ public function createDocument($className, $node, array &$hints = array())
}
if ($overrideLocalValuesOid) {
- $this->nonMappedData[$overrideLocalValuesOid] = $nonMappedData;
foreach ($class->reflFields as $prop => $reflFields) {
$value = isset($documentState[$prop]) ? $documentState[$prop] : null;
$reflFields->setValue($document, $value);
- $this->originalData[$overrideLocalValuesOid][$prop] = $value;
}
}
@@ -801,29 +783,24 @@ private function computeChangeSet(ClassMetadata $class, $document)
}
$actualData = $this->getDocumentActualData($class, $document);
+ $id = $this->getDocumentId($document);
$oid = spl_object_hash($document);
- $isNew = !isset($this->originalData[$oid]);
- if ($isNew) {
- // Document is New and should be inserted
- $this->originalData[$oid] = $actualData;
- $this->documentChangesets[$oid] = $actualData;
- $this->scheduledInserts[$oid] = $document;
- }
if ($class->parentMapping && isset($actualData[$class->parentMapping])) {
- $parent = $actualData[$class->parentMapping];
- $parentClass = $this->dm->getClassMetadata(get_class($parent));
- $state = $this->getDocumentState($parent);
+ $parentClass = $this->dm->getClassMetadata(get_class($actualData[$class->parentMapping]));
+ }
+ if (isset($parentClass)) {
+ $state = $this->getDocumentState($actualData[$class->parentMapping]);
if ($state === self::STATE_MANAGED || $state === self::STATE_MOVED) {
- $this->computeChangeSet($parentClass, $parent);
+ $this->computeChangeSet($parentClass, $actualData[$class->parentMapping]);
}
}
- $id = $this->getDocumentId($document);
foreach ($class->childMappings as $name => $childMapping) {
if ($actualData[$name]) {
- if ($this->originalData[$oid][$name] && $this->originalData[$oid][$name] !== $actualData[$name]) {
+ $child = $this->getDocumentById($id.'/'.$childMapping['name']);
+ if ($child && $child !== $actualData[$name]) {
throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the DocumentManager::move() or PHPCR\Session::copy() operations for this: '.self::objToStr($document, $this->dm));
}
$this->computeChildChanges($childMapping, $actualData[$name], $id);
@@ -844,24 +821,26 @@ private function computeChangeSet(ClassMetadata $class, $document)
}
}
- if (!$isNew) {
- if (isset($this->originalData[$oid][$class->nodename])
- && isset($actualData[$class->nodename])
- && $this->originalData[$oid][$class->nodename] !== $actualData[$class->nodename]
+ if (!$this->session->nodeExists($id)) {
+ // Document is new and should be inserted
+ $this->documentChangesets[$oid] = $actualData;
+ $this->scheduledInserts[$oid] = $document;
+ } else {
+ $node = $this->session->getNode($id);
+ $originalData = $node->getPropertiesValues();
+
+ if (isset($actualData[$class->nodename])
+ && $node->getName() !== $actualData[$class->nodename]
) {
- throw new PHPCRException('The Nodename property is immutable ('.$this->originalData[$oid][$class->nodename].' !== '.$actualData[$class->nodename].'). Please use DocumentManager::move to rename the document: '.self::objToStr($document, $this->dm));
+ throw new PHPCRException('The Nodename property is immutable ('.$node->getName().' !== '.$actualData[$class->nodename].'). Please use DocumentManager::move to rename the document: '.self::objToStr($document, $this->dm));
}
- if (isset($this->originalData[$oid][$class->parentMapping])
- && isset($actualData[$class->parentMapping])
- && $this->originalData[$oid][$class->parentMapping] !== $actualData[$class->parentMapping]
- ) {
- throw new PHPCRException('The ParentDocument property is immutable ('.$this->getDocumentId($this->originalData[$oid][$class->parentMapping]).' !== '.$this->getDocumentId($actualData[$class->parentMapping]).'). Please use PHPCR\Session::move to move the document: '.self::objToStr($document, $this->dm));
+ if (isset($parentClass) && ($parent = $this->getDocumentById(dirname($id))) && $parent !== $actualData[$class->parentMapping]) {
+ throw new PHPCRException('The ParentDocument property is immutable ('.self::objToStr($parent, $this->dm).' !== '.self::objToStr($actualData[$class->parentMapping], $this->dm).'). Please use PHPCR\Session::move to move the document: '.self::objToStr($document, $this->dm));
}
- if (isset($this->originalData[$oid][$class->identifier])
- && isset($actualData[$class->identifier])
- && $this->originalData[$oid][$class->identifier] !== $actualData[$class->identifier]
+ if (isset($actualData[$class->identifier])
+ && $id !== $actualData[$class->identifier]
) {
- throw new PHPCRException('The Id is immutable ('.$this->originalData[$oid][$class->identifier].' !== '.$actualData[$class->identifier].'). Please use DocumentManager::move to move the document: '.self::objToStr($document, $this->dm));
+ throw new PHPCRException('The Id is immutable ('.$id.' !== '.$actualData[$class->identifier].'). Please use DocumentManager::move to move the document: '.self::objToStr($document, $this->dm));
}
if (!isset($this->documentLocales[$oid])
@@ -884,7 +863,9 @@ private function computeChangeSet(ClassMetadata $class, $document)
$this->visitedCollections[] = $fieldValue;
continue;
}
- } elseif ($this->originalData[$oid][$fieldName] !== $fieldValue) {
+ } elseif (!array_key_exists($fieldName, $originalData)
+ || $originalData[$fieldName] !== $fieldValue
+ ) {
continue;
} elseif ($fieldValue instanceof ReferenceManyCollection && $fieldValue->changed()) {
continue;
@@ -901,8 +882,8 @@ private function computeChangeSet(ClassMetadata $class, $document)
}
foreach ($class->referrersMappings as $name => $referrerMapping) {
- if ($this->originalData[$oid][$name]) {
- foreach ($this->originalData[$oid][$name] as $referrer) {
+ if (isset($originalData[$name])) {
+ foreach ($originalData[$name] as $referrer) {
$this->computeReferrerChanges($referrer);
}
}
@@ -1380,16 +1361,16 @@ private function executeUpdates($documents, $dispatchEvents = true)
}
}
}
- } elseif (isset($class->childMappings[$fieldName])) {
- if ($fieldValue === null) {
- if ($node->hasNode($class->childMappings[$fieldName]['name'])) {
- $child = $node->getNode($class->childMappings[$fieldName]['name']);
- $childDocument = $this->createDocument(null, $child);
- $this->purgeChildren($childDocument);
- $child->remove();
- }
- } elseif ($this->originalData[$oid][$fieldName] && $this->originalData[$oid][$fieldName] !== $fieldValue) {
- throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the DocumentManager::move() or PHPCR\Session::copy() operations for this.');
+ } elseif (isset($class->childMappings[$fieldName])
+ && $node->hasNode($class->childMappings[$fieldName]['name'])
+ && $fieldValue === null
+ ) {
+ $childDocument = $this->getDocumentById($node->getPath().'/'.$class->childMappings[$fieldName]['name']);
+ if (empty($childDocument) || $childDocument instanceof Proxy) {
+ $child = $node->getNode($class->childMappings[$fieldName]['name']);
+ $childDocument = $this->createDocument(null, $child);
+ $this->purgeChildren($childDocument);
+ $child->remove();
}
}
}
@@ -1450,7 +1431,6 @@ private function executeMoves($documents)
$class = $this->dm->getClassMetadata(get_class($document));
if ($class->identifier) {
$class->setIdentifierValue($document, $newId);
- $this->originalData[$oid][$class->identifier] = $newId;
}
}
}
@@ -1659,12 +1639,10 @@ private function unregisterDocument($document)
$this->scheduledMoves[$oid],
$this->scheduledInserts[$oid],
$this->scheduledAssociationUpdates[$oid],
- $this->originalData[$oid],
$this->documentIds[$oid],
$this->documentState[$oid],
$this->documentTranslations[$oid],
$this->documentLocales[$oid],
- $this->nonMappedData[$oid],
$this->documentChangesets[$oid],
$this->documentHistory[$oid],
$this->documentVersion[$oid]
@@ -1823,8 +1801,6 @@ public function clear()
$this->documentState =
$this->documentTranslations =
$this->documentLocales =
- $this->nonMappedData =
- $this->originalData =
$this->documentChangesets =
$this->scheduledUpdates =
$this->scheduledAssociationUpdates =
Something went wrong with that request. Please try again.