Skip to content

[WIP] Prevent creating same name siblings #94

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.

Sign up for GitHub

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

Merged
dbu merged 4 commits into from
Apr 16, 2013
Merged

[WIP] Prevent creating same name siblings #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a76c97c
Added clone test cases for existing, non-corresponding nodes
Apr 12, 2013
7d587af
Expect an ItemExistsException when trying to clone over a non-referen…
Apr 15, 2013
7186ac6
Skip affected tests where SNS support is present (not yet implemented)
Apr 15, 2013
cfcadbe
Test cloning non-referenceable node over a referenceable node
Apr 16, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions fixtures/10_Writing/clone.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,33 @@
</sv:property>
</sv:node>
</sv:node>
<sv:node sv:name="testWorkspaceCloneNonCorresponding">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:node sv:name="sourceRemoveExisting">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:property sv:name="jcr:mixinTypes" sv:type="Name">
<sv:value>mix:referenceable</sv:value>
</sv:property>
<sv:property sv:name="jcr:uuid" sv:type="String">
<sv:value>b33fbe21-d3d8-4f52-8188-ad18a62dea9b</sv:value>
</sv:property>
</sv:node>
<sv:node sv:name="sourceNoRemoveExisting">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:property sv:name="jcr:mixinTypes" sv:type="Name">
<sv:value>mix:referenceable</sv:value>
</sv:property>
<sv:property sv:name="jcr:uuid" sv:type="String">
<sv:value>1ef2d543-a889-48a3-ae46-a97cf4767e95</sv:value>
</sv:property>
</sv:node>
</sv:node>
<sv:node sv:name="testWorkspaceCorrespondingNode">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
Expand Down
45 changes: 44 additions & 1 deletion fixtures/general/additionalWorkspace.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,51 @@
xmlns:sv="http://www.jcp.org/jcr/sv/1.0"
xmlns:rep="internal"

sv:name="tests_general_additional_workspace">
sv:name="tests_additional_workspace">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:node sv:name="testWorkspaceCloneNonCorresponding">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:node sv:name="destRemoveExisting">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:property sv:name="jcr:mixinTypes" sv:type="Name">
<sv:value>mix:referenceable</sv:value>
</sv:property>
<sv:property sv:name="jcr:uuid" sv:type="String">
<sv:value>f8019868-3533-4519-a077-9c8601950627</sv:value>
</sv:property>
</sv:node>
<sv:node sv:name="destNoRemoveExisting">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:property sv:name="jcr:mixinTypes" sv:type="Name">
<sv:value>mix:referenceable</sv:value>
</sv:property>
<sv:property sv:name="jcr:uuid" sv:type="String">
<sv:value>9aee563c-b5b7-44f7-a16d-f94116f4dfba</sv:value>
</sv:property>
</sv:node>
</sv:node>
<sv:node sv:name="testWorkspaceCloneReferenceable">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:node sv:name="destExistingNode">
<sv:property sv:name="jcr:primaryType" sv:type="Name">
<sv:value>nt:unstructured</sv:value>
</sv:property>
<sv:property sv:name="jcr:mixinTypes" sv:type="Name">
<sv:value>mix:referenceable</sv:value>
</sv:property>
<sv:property sv:name="jcr:uuid" sv:type="String">
<sv:value>a6e94d5f-6aee-44c8-878e-afca80d3e41c</sv:value>
</sv:property>
</sv:node>
</sv:node>
</sv:node>
98 changes: 72 additions & 26 deletions tests/10_Writing/CloneMethodsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ public function testCloneReferenceableWithChild()
}

/**
* Clone a referenceable node, then clone again with 'remove existing' feature.
* Clone a referenceable node, then clone again with removeExisting = true
* This should overwrite the existing, corresponding node (same UUID)
*/
public function testCloneReferenceableRemoveExisting()
{
Expand Down Expand Up @@ -127,6 +128,9 @@ public function testCloneReferenceableRemoveExisting()
}

/**
* Clone a referenceable node, then clone again with removeExisting = false
* This should cause an exception, even with a corresponding node (same UUID)
*
* @expectedException \PHPCR\ItemExistsException
*/
public function testCloneReferenceableNoRemoveExisting()
Expand All @@ -149,6 +153,10 @@ public function testCloneReferenceableNoRemoveExisting()
}

/**
* Clone a referenceable node, then clone again with removeExisting = false.
* Even though the second clone is to a new location, because a corresponding node (same UUID)
* already exists in the destination workspace, an exception should still be thrown.
*
* @expectedException \PHPCR\ItemExistsException
*/
public function testCloneNoRemoveExistingNewLocation()
Expand All @@ -171,6 +179,55 @@ public function testCloneNoRemoveExistingNewLocation()
self::$destWs->cloneFrom($this->srcWsName, $srcNode, $secondDstNode, false);
}

/**
* Check that we don't inadvertently create same name siblings (SNS) with removeExisting = true.
* This can happen when cloning from one workspace to another, when a node already exists at the
* destination but is not a corresponding node (the nodes have different UUIDs)
*
* @expectedException \PHPCR\ItemExistsException
*/
public function testExistingNonCorrespondingNodeRemoveExisting()
Copy link
Author

@fazy fazy Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test currently fails, until we manage to fix the problem in jackalope-jackrabbit.

{
$this->skipIfSameNameSiblingsSupported();

$srcNode = '/tests_write_manipulation_clone/testWorkspaceCloneNonCorresponding/sourceRemoveExisting';
$dstNode = '/tests_additional_workspace/testWorkspaceCloneNonCorresponding/destRemoveExisting';

self::$destWs->cloneFrom($this->srcWsName, $srcNode, $dstNode, true);
}

/**
* Check that we don't inadvertently create same name siblings (SNS) with removeExisting = false.
* This can happen when cloning from one workspace to another, when a node already exists at the
* destination but is not a corresponding node (the nodes have different UUIDs)
*
* @expectedException \PHPCR\ItemExistsException
*/
public function testExistingNonCorrespondingNodeNoRemoveExisting()
{
$this->skipIfSameNameSiblingsSupported();

$srcNode = '/tests_write_manipulation_clone/testWorkspaceCloneNonCorresponding/sourceNoRemoveExisting';
$dstNode = '/tests_additional_workspace/testWorkspaceCloneNonCorresponding/destNoRemoveExisting';

self::$destWs->cloneFrom($this->srcWsName, $srcNode, $dstNode, false);
}

/**
* Test when source node is non-referenceable but a referenceable node exists at destination path
*
* @expectedException \PHPCR\ItemExistsException
*/
public function testReferenceableDestNodeWithNonReferenceableSourceNode()
{
$this->skipIfSameNameSiblingsSupported();

$srcNode = '/tests_write_manipulation_clone/testWorkspaceClone/nonReferenceable';
$dstNode = '/tests_additional_workspace/testWorkspaceCloneReferenceable/destExistingNode';

self::$destWs->cloneFrom($this->srcWsName, $srcNode, $dstNode, true);
}

/**
* @expectedException \PHPCR\NoSuchWorkspaceException
*/
Expand Down Expand Up @@ -256,10 +313,14 @@ public function testCloneNonReferenceable()
}

/**
* Clone a non-referenceable node, then clone again with 'remove existing' feature.
* Clone a non-referenceable node, then clone again with removeExisting = true
*
* @expectedException \PHPCR\ItemExistsException
Copy link
Author

@fazy fazy Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure about this change. The JCR documentation states that "The clone method clones both referenceable and non-referenceable nodes", but it doesn't explicitly says how "removeExisting=true" should behave for non-referenceable nodes.

From our discussion on the symfony-cmf-devs list I got the impression that we probably shouldn't allow removeExisting for non-referenceable nodes. However, the alternative (remove and replace the existing node) is equally arguable.

Copy link
Member

@dbu dbu Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from how i read that section, i would say that if either node is not referenceable then it has no identifier, hence removeExisting has no effect. the application would have to remove the target manually first. (unless same-name siblings are allowed, that is. probably this test first should check that capability and skip if that capability is supported by the repository. in that case a new same-name sibling would be the correct thing to happen.)

Copy link
Member

@dbu dbu Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or set the expected exception in code instead of the annotation in case same-name is not supported, otherwise expect that there are 2 children with that name now - that would be forward compatible.

Copy link
Author

@fazy fazy Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for a non-referenceable node, removeExisting should have no effect (but should throw an exception in that case?)

For the case with same name siblings/forward compatibility, I would hesitate here, because if we support SNS then there are many more tests that should be done.

Copy link
Member

@dbu dbu Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is something with the same name and we do not support SNS, we would need the exception yes. otherwise never (f.e. if internally uuid are assigned even for not referenceable nodes - then a new uuid would have to be created. but i think jackrabbit handles that for us, right?)

the thing about forward compatible is that if SNS are allowed, this test is completely wrong as no exception is to be assumed. lets check the capability and mark the test skipped if SNS are supported, saying that this needs to be done with an implementation that actually supports SNS. doing the test that is never run would be weird...

Copy link
Author

@fazy fazy Apr 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've skipped the tests that would probably be wrong if SNS support is enabled. All of those tests would need to look for different behaviour with/without SNS.

To make this possible, I needed to change the Jackalope-Jackrabbit client so that it doesn't claim to support SNS, see this PR.

*/
public function testCloneRemoveExistingNonReferenceable()
{
$this->skipIfSameNameSiblingsSupported();

$srcNode = '/tests_write_manipulation_clone/testWorkspaceClone/nonReferenceableRemoveExisting';
$dstNode = $srcNode;
$destSession = self::$destWs->getSession();
Expand All @@ -272,39 +333,17 @@ public function testCloneRemoveExistingNonReferenceable()
$this->checkNodeProperty($clonedNode, 'jcr:primaryType', 'nt:unstructured');
$this->checkNodeProperty($clonedNode, 'foo', 'bar_4');

// Update the source node after cloning it
$node = $this->srcWs->getSession()->getNode($srcNode);
$node->setProperty('foo', 'bar-updated');
$node->setProperty('newProperty', 'hello');
$this->srcWs->getSession()->save();

// Clone the updated source node
self::$destWs->cloneFrom($this->srcWsName, $srcNode, $dstNode, true);

$this->renewDestinationSession();

// Check the first cloned node again; it should not have changed
$clonedNode = $destSession->getNode($dstNode);
$this->assertInstanceOf('PHPCR\NodeInterface', $clonedNode);
$this->assertCount(2, $clonedNode->getProperties());
$this->checkNodeProperty($clonedNode, 'jcr:primaryType', 'nt:unstructured');
$this->checkNodeProperty($clonedNode, 'foo', 'bar_4');

// Second cloned node created with [2] appended to name
$replacedDstNode = $srcNode . '[2]';
$clonedReplacedNode = self::$destWs->getSession()->getNode($replacedDstNode);
$this->assertInstanceOf('PHPCR\NodeInterface', $clonedReplacedNode);
$this->assertCount(3, $clonedReplacedNode->getProperties());
$this->checkNodeProperty($clonedReplacedNode, 'jcr:primaryType', 'nt:unstructured');
$this->checkNodeProperty($clonedReplacedNode, 'foo', 'bar-updated');
$this->checkNodeProperty($clonedReplacedNode, 'newProperty', 'hello');
}

/**
* @expectedException \PHPCR\ItemExistsException
*/
public function testCloneNonReferenceableNoRemoveExisting()
{
$this->skipIfSameNameSiblingsSupported();

$srcNode = '/tests_write_manipulation_clone/testWorkspaceClone/nonReferenceableNoRemoveExisting';
$dstNode = $srcNode;
$destSession = self::$destWs->getSession();
Expand Down Expand Up @@ -498,4 +537,11 @@ private function renewDestinationSession()
$destSession = self::$loader->getRepository()->login(self::$loader->getCredentials(), self::$destWsName);
self::$destWs = $destSession->getWorkspace();
}

private function skipIfSameNameSiblingsSupported()
{
if (self::$staticSharedFixture['session']->getRepository()->getDescriptor('node.type.management.same.name.siblings.supported')) {
$this->markTestSkipped('Test does not yet cover repositories that support same name siblings.');
}
}
}