Added some tests for version restore functionality #160

danrot · 2015-06-11T14:21:15Z

No description provided.

danrot · 2015-06-11T14:22:50Z

tests/15_Versioning/VersionManagerTest.php

+
+    public function testRestoreBaseProperties()
+    {
+        // TODO also check for primary node type and uuid once it can be changed


@dbu I am not sure about this one. The specification says how to handle these properties on restore, but if I try to write a test for it jackalope says it's not allowed to change these properties. Am I missing something? Is this just a rarely happening edge case?

i would think that the primary type never changes, or can it? if it does not change it also does not need to be reverted...

and the uuid of the original node should stay the same when restoring, no?

I've got the same understanding until now, but after having a look at the specification I am not sure if there are some edge cases at which this properties can change. Otherwise 15.7.2 on http://www.day.com/specs/jcr/2.0/15_Versioning.html doesn't make any sense to me.

i would propose we assume the uuid immutable and you can add a check that throws an exception when the frozen uuid is not the one of the current node, saying something unexpected happened... for mixins, they certainly can change. about the primary type, this seems to be another jackrabbit restriction that is actually a mistake jackalope/jackalope#247 but in my opinion we can just leave a TODO here referencing that issue, and a comment in that issue that it would also have to be fixed in version restore if this is ever changed. as its now, its not possible to change the primary type so no need (yet) to restore it.

dbu · 2015-06-11T15:44:51Z

tests/15_Versioning/VersionManagerTest.php

+        $this->vm->restore(true, $version);
+
+        $node = $this->session->getNode($nodePath);
+        $this->assertEquals(array('mix:versionable'), $node->getPropertyValue('jcr:mixinTypes'));


you could also assert that there is no created property anymore

But then it might be possible that all mixinTypes are gone, which would be a wrong behavior. Maybe I also should add the complete array check instead of assertContains a few lines earlier?

i mean the jcr:created property that the mix:created made jackalope write.

for the mixins i think its fine like this, or you will need to make sure not to rely on a particular order of the mixins as that is not determined...

Is it possible that the properties are not deleted after the mixin is removed? Because it seems to me like that... And actually that would also make sense to me.

i would expect that if the frozen node has no jcr:created property and it is restored, that property should be removed.

are you saying jackrabbit does not do that? even when refreshing the session after restoring?

I thought so, it has been some time since I've checked the last time 😉 Will check hopefully again next week.

Do you really would expect that? Isn't it possible that the jcr:created property has been there before the mixin has been added? Would feel strange to me if removing the mixin is then also removing the property...

is jcr:created not backed up? if it is backed up, restoring the old version should have deleted it like it deletes other properties that where not existing when the version was created.

can you please check this bit? otherwise i think this would be ready to merge, the issue below is indeed a bug in the code and the test is correct.

Have added this check.

dbu · 2015-06-11T16:05:36Z

jackalope-jackrabbit is happy with these tests so far. so the assumptions seem correct.

danrot · 2015-06-11T17:08:59Z

Yeah, I write them in jackalope-jackrabbit first, and take over the changes and required implementations to jackalope-doctrine-dbal afterwards :-)

…ests into version-restore

danrot · 2015-06-13T14:00:59Z

tests/15_Versioning/VersionManagerTest.php

+
+        $this->assertCount(2, $node->getNodes());
+        $this->assertEquals('child1', $node->getNode('childNode1')->getPropertyValue('foo'));
+        $this->assertEquals('child2', $node->getNode('childNode2')->getPropertyValue('foo'));


@dbu @lsmith77 This is failing for jackrabbit. But I guess this should be the behavior as I read in 15.7.6 in http://www.day.com/specs/jcr/2.0/15_Versioning.html. It basically says that each child from the frozen node should replace any existing child in the actual node.

This is interesting, it looks to me like this is an error in the jackalope jackrabbit transport layer, since jackrabbit itself (I have tested it via the cli in jackrabbit-standalone.jar) behaves as expected, as you can see in the following script:

[/] > addnode test [/*] > addmixin mix:versionable test [/*] > cd test [/test*] > addnode childnode1 [/test*] > cd childnode1 [/test/childnode1*] > setprop foo child1 [/test/childnode1*] > cd .. [/test*] > addnode childnode2 [/test*] > cd childnode2 [/test/childnode2*] > setprop foo child2 [/test/childnode2*] > cd .. [/test*] > save [/test] > checkin [/test] > cd childnode1 [/test/childnode1] > lsprop name multiple type length preview ------------------------------ --------- --------- -------- ------------------ jcr:primaryType false Name 15 nt:unstructured foo false String 6 child1 total 2 nodes [/test/childnode1] > cd ../childnode2 [/test/childnode2] > lsprop name multiple type length preview ------------------------------ --------- --------- -------- ------------------ foo false String 6 child2 jcr:primaryType false Name 15 nt:unstructured total 2 nodes [/test/childnode2] > cd .. [/test] > checkout [/test] > rm childnode1 [/test*] > cd childnode2 [/test/childnode2*] > setprop foo child1 [/test/childnode2*] > save [/test/childnode2] > cd .. [/test] > ls name type node new modified ------------------------------ --------------- --------- --------- --------- childnode2 nt:unstructured true false false jcr:isCheckedOut Boolean false false false jcr:versionHistory Reference false false false jcr:mixinTypes Name false false false jcr:predecessors Reference false false false jcr:baseVersion Reference false false false jcr:uuid String false false false jcr:primaryType Name false false false total [/test] > cd childnode2 [/test/childnode2] > lsprop name multiple type length preview ------------------------------ --------- --------- -------- ------------------ foo false String 6 child1 jcr:primaryType false Name 15 nt:unstructured total 2 nodes [/test/childnode2] > cd .. [/test] > lsversions version labels -------------------- -------------------------------------------------- jcr:rootVersion 1.0 [/test] > restore 1.0 [/test] > ls name type node new modified ------------------------------ --------------- --------- --------- --------- childnode1 nt:unstructured true false false childnode2 nt:unstructured true false false jcr:isCheckedOut Boolean false false false jcr:versionHistory Reference false false false jcr:mixinTypes Name false false false jcr:predecessors Reference false false false jcr:baseVersion Reference false false false jcr:uuid String false false false jcr:primaryType Name false false false total [/test] > cd childnode1 [/test/childnode1] > lsprop name multiple type length preview ------------------------------ --------- --------- -------- ------------------ jcr:primaryType false Name 15 nt:unstructured foo false String 6 child1 jcr:mixinTypes true[0] Name 0 total 3 nodes [/test/childnode1] > cd ../childnode2 [/test/childnode2] > lsprop name multiple type length preview ------------------------------ --------- --------- -------- ------------------ jcr:mixinTypes true[0] Name 0 foo false String 6 child2 jcr:primaryType false Name 15 nt:unstructured total 3 nodes

@dbu I am missing a comment of yours on this one :-)

i will have to investigate this.

ok. the expectations seem correct, this is a problem with jackalope-jackrabbit not noticing that the children also changed. fixed in jackalope/jackalope#294

can you check if things still work out when you have these changes with your code? i wonder if marking nodes as dirty could cause side effects when you restore nodes in memory instead.

dbu · 2015-08-17T06:11:34Z

rebased on master and will create a new PR

dbu · 2015-08-17T06:13:03Z

see #166

danrot added 2 commits June 11, 2015 15:21

added test case for checked out property after restore

fa8f1cd

added restore test for mixins

0c5ed9f

lsmith77 added the wip/poc label Jun 11, 2015

danrot reviewed Jun 11, 2015
View reviewed changes

danrot mentioned this pull request Jun 11, 2015

add version restore functionality to transport layer jackalope/jackalope-doctrine-dbal#283

Open

dbu reviewed Jun 11, 2015
View reviewed changes

danrot mentioned this pull request Jun 11, 2015

Implement $node->setPrimaryType jackalope/jackalope#247

Open

danrot added 6 commits June 11, 2015 19:26

added test for restore with meanwhile deleted property

ec9fe87

added restore test for mixins

288f52e

added restore test for mixins

2479c9e

added test for restoring with new properties

6894431

Merge branch 'version-restore' of git://github.com/danrot/phpcr-api-t…

2eeba8e

…ests into version-restore

added test case for restoring with children

09c9620

danrot reviewed Jun 13, 2015
View reviewed changes

danrot added 4 commits June 25, 2015 13:54

added another child layer to restore test

152f93e

added test for restoring with new child nodes

8a39b45

adjusted test to be more specific

ba9b7fc

added check for removed property

fea4c21

dbu closed this Aug 17, 2015

lsmith77 removed the wip/poc label Aug 17, 2015

dbu mentioned this pull request Aug 17, 2015

Added some tests for version restore functionality #166

Merged

danrot deleted the version-restore branch September 22, 2015 18:17

danrot restored the version-restore branch September 22, 2015 18:18

Added some tests for version restore functionality #160

Added some tests for version restore functionality #160

Uh oh!

Conversation

danrot commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu Jun 11, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jun 11, 2015

Uh oh!

danrot commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 17, 2015

Uh oh!

dbu commented Aug 17, 2015

Uh oh!

Uh oh!