Skip to content

Added some tests for version restore functionality #160

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

Closed
wants to merge 12 commits into from
Closed

Added some tests for version restore functionality #160

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fa8f1cd
added test case for checked out property after restore
danrot Jun 11, 2015
0c5ed9f
added restore test for mixins
danrot Jun 11, 2015
ec9fe87
added test for restore with meanwhile deleted property
danrot Jun 11, 2015
288f52e
added restore test for mixins
danrot Jun 11, 2015
2479c9e
added restore test for mixins
danrot Jun 11, 2015
6894431
added test for restoring with new properties
danrot Jun 13, 2015
2eeba8e
Merge branch 'version-restore' of git://github.com/danrot/phpcr-api-t…
danrot Jun 13, 2015
09c9620
added test case for restoring with children
danrot Jun 13, 2015
152f93e
added another child layer to restore test
danrot Jun 25, 2015
8a39b45
added test for restoring with new child nodes
danrot Jun 25, 2015
ba9b7fc
adjusted test to be more specific
danrot Jun 25, 2015
fea4c21
added check for removed property
danrot Aug 16, 2015
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
138 changes: 138 additions & 0 deletions tests/15_Versioning/VersionManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php
namespace PHPCR\Tests\Versioning;

use PHPCR\Version\VersionManagerInterface;

require_once(__DIR__ . '/../../inc/BaseCase.php');

/**
Expand All @@ -10,6 +12,11 @@
*/
class VersionManagerTest extends \PHPCR\Test\BaseCase
{
/**
* @var VersionManagerInterface
*/
private $vm;

public static function setupBeforeClass($fixtures = '15_Versioning/base')
{
parent::setupBeforeClass($fixtures);
Expand Down Expand Up @@ -275,6 +282,137 @@ public function testRestorePendingChanges()
$this->vm->restore(true, $version);
}

public function testRestoreWithDeletedProperties()
{
$nodePath = '/tests_version_base/versioned';
$version = $this->vm->checkpoint($nodePath);
$this->vm->checkout($nodePath);

$node = $this->session->getNode($nodePath);
$node->getProperty('foo')->remove();

$this->session->save();

$this->assertFalse($node->hasProperty('foo'));

$this->vm->restore(true, $version);
$node = $this->session->getNode($nodePath);

$this->assertTrue($node->hasProperty('foo'));
}

public function testRestoreWithNewProperties()
{
$nodePath = '/tests_version_base/versioned';
$version = $this->vm->checkpoint($nodePath);
$this->vm->checkout($nodePath);

$node = $this->session->getNode($nodePath);
$node->setProperty('bar', 'value');

$this->session->save();

$this->assertTrue($node->hasProperty('bar'));

$this->vm->restore(true, $version);
$node = $this->session->getNode($nodePath);

$this->assertFalse($node->hasProperty('bar'));
}

public function testRestoreIsCheckedIn()
{
$nodePath = '/tests_version_base/versioned';
$version = $this->vm->checkpoint($nodePath);

$this->vm->checkout($nodePath);
$this->assertTrue($this->vm->isCheckedOut($nodePath));

$this->vm->restore(true, $version);
$this->assertFalse($this->vm->isCheckedOut($nodePath));
}

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

$nodePath = '/tests_version_base/versioned';
$version = $this->vm->checkpoint($nodePath);
$this->vm->checkout($nodePath);

$node = $this->session->getNode($nodePath);
$node->addMixin('mix:created');

$this->session->save();

$node = $this->session->getNode($nodePath);
$this->assertContains('mix:created', $node->getPropertyValue('jcr:mixinTypes'));
$this->assertTrue($node->hasProperty('jcr:created'));

$this->vm->restore(true, $version);

$node = $this->session->getNode($nodePath);
$this->assertEquals(array('mix:versionable'), $node->getPropertyValue('jcr:mixinTypes'));
Copy link
Member

Choose a reason for hiding this comment

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

you could also assert that there is no created property anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added this check.

$this->assertFalse($node->hasProperty('jcr:created'));
}

public function testRestoreWithChildren()
{
$nodePath = '/tests_version_base/versioned';
$this->vm->checkout($nodePath);
$node = $this->session->getNode($nodePath);
$childNode1 = $node->addNode('childNode1');
$childNode1->setProperty('foo', 'child1');
$childNode2 = $node->addNode('childNode2');
$childNode2->setProperty('foo', 'child2');
$childNode3 = $childNode1->addNode('childNode3');

$this->session->save();
$version = $this->vm->checkin($nodePath);

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

$this->assertCount(1, $node->getNode('childNode1')->getNodes());

$this->vm->checkout($nodePath);

$childNode1->remove();
$childNode2->setProperty('foo', 'child1');

$this->session->save();

$this->assertCount(1, $node->getNodes());
$this->assertEquals('child1', $node->getNode('childNode2')->getPropertyValue('foo'));

$this->vm->restore(true, $version);
$node = $this->session->getNode($nodePath);

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

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

i will have to investigate this.

Copy link
Member

Choose a reason for hiding this comment

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

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.

$this->assertCount(1, $node->getNode('childNode1')->getNodes());
}

public function testRestoreWithNewChildren()
{
$nodePath = '/tests_version_base/versioned';
$version = $this->vm->checkin($nodePath);
$this->vm->checkout($nodePath);
$node = $this->session->getNode($nodePath);
$node->addNode('newChildNode');
$this->session->save();

$node = $this->session->getNode($nodePath);
$this->assertTrue($node->hasNode('newChildNode'));

$this->vm->restore(true, $version);
$node = $this->session->getNode($nodePath);

$this->assertFalse($node->hasNode('newChildNode'));
}

// TODO: test restore with removeExisting false and having an id clash

// TODO: testRestoreByVersionArray, testRestoreVersionToPath, testRestoreVersionToExistingPath (expect exception)
Expand Down