Adding restore functionality for versioning support #289

danrot · 2015-06-11T14:34:28Z

This PR adds restoring capabilities to the VersionHandler.

In order to use this functionality with jackalope-doctrine-dbal jackalope/jackalope-doctrine-dbal#283 also needs to be merged.

dbu · 2015-06-11T15:49:13Z

src/Jackalope/Version/VersionHandler.php

+
+        if ($frozenNode->hasProperty('jcr:frozenMixinTypes')) {
+            $node->setProperty('jcr:mixinTypes', $frozenNode->getPropertyValue('jcr:frozenMixinTypes'));
+        }


is that property not always set? if not, you would need to delete the mixinTypes from the node in case it has some.

No, this property is not always set. Removing non-existing properties is a todo a few lines below, it's an extra section in the specification, and also covers other attributes not set.

yes, but you treat frozenMixinTypes as special case so you need an elseif to remove mixins if the current node has any but frozen has not

Ok, you are right, will add that.

danrot · 2015-06-25T14:12:49Z

There are still some TODOs in the code. Almost all of them belong the onParentVersion value.

dbu · 2015-07-24T14:39:28Z

src/Jackalope/Version/VersionHandler.php

+        }
+    }
+
+    public function restoreItem($removeExisting, $versionPath, $path)


please use @inheritDoc when implementing interface methods.

dbu · 2015-07-24T16:50:20Z

sorry for the long silence. i mostly commented on some details. can you look at those?

for the TODOs i would prefer NotImplementedExceptions. its fine when not all onParentVersion behaviours are implemented in this PR, but throw exceptions when something should happen but does not. ideally explaining what should happen or at least identifying the spec section that is relevant.

it looks to me like it should be possible to quickly wrap this PR up when just throwing exceptions in those cases.

danrot · 2015-07-25T06:51:09Z

I guess the tests won't work if the OPVs are not implemented... Hopefully I can have a detailed look at your comment next week.

dbu · 2015-07-31T15:22:30Z

ping @danrot

danrot · 2015-08-10T06:46:50Z

Sry, I was on holiday, will have a look at this again soonish.

dbu · 2015-08-16T19:19:15Z

can you rebase this one on the upstream versioning branch please? it is currently in conflict with it.

danrot · 2015-09-22T18:15:32Z

@dbu I have rebased to the versioning branch, is that one up to date? Hope I did not crash anything while solving the conflicts...

dbu · 2015-10-09T15:53:55Z

@danrot can you adress the code style comments i made and ping me again? i think this is almost ready to merge.

danrot · 2015-10-14T16:47:20Z

@dbu Fixed the comments, I think the only thing left now is the correct implementation of the onParentVersion attributes, right? Then the entire feature should be ready to merge, or am I missing something?

dbu · 2015-10-17T16:31:26Z

yep, sounds right. can you do that?

danrot · 2015-10-19T17:39:24Z

Yeah, but I can't promise you a due date... should I do that on this PR, or create a new one?

danrot · 2015-11-23T06:57:31Z

src/Jackalope/Version/VersionHandler.php

+                    break;
+                case OnParentVersionAction::INITIALIZE:
+                case OnParentVersionAction::COMPUTE: // implementation-specific, decided to handle as INITIALIZE
+                    // TODO how to get default value of property?


I hope this one is ok: When there is a OPV of initialize, the default value of the property should be restored. Compute is implementation specific, so I decided to just do the same as for initialize.

The question that remains is how to get this default value, I couldn't find a method on the definition.

afaik the default value is in the property definition. and usually it would be null...

Ok, there default value is in the property definition, but the handling is not that easy. It's always an array, which should be directly applied to multifield properties, and it should be an array with only one element in case it is a single field property. Is that logic already handled somewhere, so that I could reuse it now?

maybe look for the code around creating a node or search the codebase for the specific property that defines the default value?

Found it:

jackalope/src/Jackalope/NodeType/NodeProcessor.php

Line 203 in 5f59291

$defaultValues = $propertyDef->getDefaultValues();

But I don't really know where this should be extracted...

we could just push the logic into the PropertyDefinition class as a method determineDefaultValue, with an annotation that this is @internal. the typehint on

jackalope/src/Jackalope/NodeType/NodeProcessor.php

Lines 169 to 170 in 5f59291

foreach ($nodeTypeDefinition->getDeclaredPropertyDefinitions() as $propertyDef) {

/* @var $propertyDef PropertyDefinitionInterface */

should be changed to the concrete class (and move above the foreach and get a second * so its seen by IDEs)

check if you always need the exception to be thrown or if the method should return false and we check the return value to know what to do.

erm, actually i guess its the whole isAutocreated part, right? and that only makes sense if the property actually is auto created? we could move the whole switch statement into he node type then.

I don't think so, because the values for the other properties, which have a special handling in the switch block are not handled by the version at all.

I also dont think it's about the isAutoCreated part, since the spec doesn't even mention it:

If P has an OPV of INITIALIZE then, if N/P has a default value (either defined in the node type of N or implementation-defined) its value is changed to that default value. If N/P has no default value then it is left unchanged.

I think it might also be possible that a property is not auto-created but still have a default value, isn't it? So I think I would go with your first suggestion (the internal determineDefaultValue method on the PropertyDefinition).

right, makes sense to me. i did not check the spec, but your reasoning makes sense to me.

Ok, then I'll implement it like that. But I have another question: I would also like to write a test for that, and I would have to add my own node type for this. Hasn't there something like this be done already? I don't know where in the tests I can create my own node-types...

dbu · 2015-11-23T16:16:16Z

btw, did you see that label support is being improved? #302 this will be implemented for jackalope-jackrabbit in master, but could also affect the dbal implementation.

danrot · 2016-01-05T10:32:15Z

@dbu I will consider the new label support, and see if it changes anything for me.

However, I wanted to continue today, but I am having the same issue now as in a different PR. When I am trying to run the tests I am getting loads of these error:

DOMDocument::loadXML(): attributes construct error in Entity, line: 1

According to php -i I still have the libxml version which should not be problematic:

libxml

libXML support => active
libXML Compiled Version => 2.9.2
libXML Loaded Version => 20902
libXML streams => enabled

Do you still have no idea what could be causing this?

danrot · 2016-01-05T11:00:42Z

@dbu Maybe I should try to getting my branches up to date first... Can you maybe update the versioning branches my PRs in jackalope/jackalope and jackalope/doctrine-dbal are targeting?

danrot · 2016-01-05T18:23:16Z

src/Jackalope/Version/VersionHandler.php

+                throw new NotImplementedException('Check for $removeExisting not implemented yet');
+            }
+
+            // TODO handle different onParentVersion values


@dbu I still have to get here the onParentVersion attribute of the $frozenChildNode. The $frozenChildNode says correctly that it is a nt:unstructure (at least that's correct for the VersionManagerTest::testRestoreWithChildren test, I am currently investigating), but $frozenChildNode->getDefinition() returns the definition for nt:frozenNode.

It looks to me like this is caused by the algorithm used in getDefinition, it takes the parent into account. Looks to me like this doesn't work for frozen nodes.

From what I've seen until now the onParentVersion attribute also depends on the parent, so that's probably the reason getDefinition makes use of the parent. So what I probably would need is to set the parent on my own. Any elegant way to do so?

does the spec say anything about node types of children of that frozenNode? maybe the whole subtree should be frozen? or the children would retain their original node type.

either way, getNodeType and getDefinition have to be consistent, i am surprised this is not the case. smells like we have code duplication between getDefinition and getNodeType... do you have the java jackrabbit client running? then you could try to recreate the situation and see what jackrabbit is doing. either the children just keep their node types, or they are frozen too and their original node type is in a separate property, just like for the main frozen node. (the more i think about this, the more sense it would make to freeze the whole subtree. otherwise you could change a child of a frozen node which makes no sense)

You were right, the children have also the node type nt:frozenNode... But that doesn't make my issue easier to solve. I think that means that the logic from getDefinition has to be duplicated in some way, because the one existing does not work with all the frozen* properties, or am I missing something?

it probably means that when restoring, you need to walk the subtree and restore each of the nodes. i don't have in my mind how getDefinition comes into that here? do you need the definition to restore the node? isn't the frozen primary type and the (frozen?) mixins enough? then you could create the child node with those.

You are right again :-) The specification is just written a bit strange IMO. [Here](http://www.day.com/specs/jcr/2.0/15_Versioning.html#15.7.6 Restoring Child Nodes) it says that nodes are only copied over if the OPV of the node is COPY or VERSION, but later it says that there will never be a different value on these nodes. So there is no need to check this.

It would make things even easier if the subtree of this node is not frozen at all, since it would be a simple copy command then. But I guess this is still required, because it might be possible that the child of the frozen node is changed by some userland code otherwise, right?

yes, i think the whole subtree must be frozen to avoid accidentally overwriting. if you want to be sure, best reproduce the situation with jackalope-jackrabbit and check the node types of the children. but afaik a read-only can not be enforced by a parent node so all sub nodes need to be transformed to frozen and back to not frozen.

ahilles107 · 2016-07-28T07:51:54Z

Any news here?

danrot · 2016-07-28T09:00:38Z

I am sry, currently I am very busy... IIRC it's not too much work to finish this, but I think I won't find time to finish it before october 😞

dbu · 2016-07-28T09:09:54Z

@ahilles107 would you be interested in trying to take over? if you want to, i can take this branch over into the jackalope repository so you can do pull requests against it.

danrot · 2016-12-13T10:27:27Z

Good News: Since I've been implementing the versioning feature for Sulu, I will now also get some time to work on this, so that we can use the versioning feature also with jackalope-doctrine-dbal :-) Hope I can finish it...

danrot · 2016-12-13T10:28:09Z

@dbu Is there anything new that should be rebased into the versioning branch?

dbu · 2016-12-13T10:44:51Z

there is one conflict with master in the versioning branch. but i guess its better to wrap this PR up first before we rebase. otherwise we might get a huge mess with the old versions of the commits that are here in your restore branch.

danrot · 2016-12-13T14:15:30Z

src/Jackalope/Version/VersionHandler.php

+                $this->session->removeItem($childNodePath);
+            }
+
+            // TODO handle onParentVersion?


I rewrote the code, but leave the comment here for now. The onParentVersion has to be handled in some way, but maybe this information can be inferred in a different way. The only possible OPVs here is COPY and VERSION, and when it is VERSION the resulting node has a different node type, if I understand the specification on 3.13.9 correct:

Under full versioning, if C is mix:versionable, then a special nt:versionedChild node with a reference to the version history of C is substituted in place of C as a child of the frozen node.

sounds right and would need special handling on restore.

danrot · 2016-12-13T15:52:18Z

@dbu We already said that the children of the frozen nodes must also be frozen nodes, but that is not that easy. Should that be done in a separate PR, or should I fix it in here?

dbu · 2016-12-13T16:09:23Z

lets try to keep this step as small as possible - its big enough already. so if we don't create the children as frozen nodes yet, lets postpone that to a separate PR and do both in a later PR

danrot · 2016-12-13T16:20:07Z

Ok, so the following steps won't be addressed in this PR:

restore primary type (we said we won't do that as long as the primary type can't change)
handle identifier collisions
handle remove existing
Create children of frozen node also as frozen node

And the following should be fixed before this is being merged:

handle OPVs when traversing child nodes on current node
restore the default value on an OPV of INITIALIZE or COMPUTE (I think that should go into this PR)

Would be nice to know if you agree.

danrot · 2016-12-14T09:54:12Z

src/Jackalope/Version/VersionHandler.php

+            if (!$frozenChildNode->isNodeType('nt:versionedChild')) {
+                $this->restoreFromNode($node, $frozenChildNode);
+            } else {
+                // TODO handle restoring from frozenChildNode with OPV of VERSION


I think the check is the correct one, but I would not implement restoring from a child with an OPV of VERSION at the moment, because it would make the PR too big.

@dbu Another question is if this must be implemented for the entire Versioning stuff to be merged, or if just throwing a NotImplementedException here would be enough.

I can't imagine that a lot of system want to rely on that anyway, because the specification is not really clear about what happens here:

Under full versioning each child node C of N where C has an OPV of VERSION and C is versionable, is represented in F not as a copy of N/C but as special node of type nt:versionedChild containing a reference to the version history of C. On restore, N/C in the workspace is replaced by a version of C. The determination of which version of C to use is implementation-dependent (see §15.7.5 Chained Versions on Restore).

(See 15.7.6 on https://docs.adobe.com/content/docs/en/spec/jcr/2.0/15_Versioning.html)

i am ok with leaving parts out, as long as we throw the NotImplementedException and document what we support or not.

danrot · 2016-12-14T10:01:58Z

src/Jackalope/Version/VersionHandler.php

+                continue;
+            }
+
+            // TODO That's the correct behavior for the OPVs COPY, VERSION and ABORT, handle others as well


I am not a 100% sure if there is anything needed. The specification at https://docs.adobe.com/content/docs/en/spec/jcr/2.0/15_Versioning.html chapter 15.7.6 says this:

If C has an OPV of COPY, VERSION or ABORT then N/C is removed. Note that while a node with a child item of OPV ABORT cannot be versioned, it is legal for a previously versioned node to have such a child item added to it and then for it to be restored to the state that it had before that item was added, as this step indicates.

If C has an OPV of IGNORE then no change is made to N/C.

If C has an OPV of INITIALIZE then N/C is re-initialized as if it were newly created, as defined in its node type.

If C has an OPV of COMPUTE then N/C may be re-initialized according to an implementation-specific mechanism.

But when I check 3.13.9 on https://docs.adobe.com/content/docs/en/spec/jcr/2.0/3_Repository_Model.html it only handles the OPV of COPY and VERSION, so checking this value here is probably not necessary, am I right?

does 15.7.6 not apply here? i would think that IGNORE would mean that we don't do the delete that is below. initialize/compute should delete it and then create a new one with default values. why do you think that would not be needed?

Sry, you were right, I mixed that up with a frozen node, but this is not what is handled here.

danrot · 2016-12-15T09:00:29Z

src/Jackalope/Version/VersionHandler.php

+                case OnParentVersionAction::ABORT:
+                    $property->remove();
+                    break;
+                case OnParentVersionAction::INITIALIZE:


I removed the COMPUTE value which was here before, because it made problems with some internal fields (in my case jcr:mixinTypes has an OPV of COMPUTE, and when changing these fields I got a few error message later on).

Another thing is that the spec states following:

If P has an OPV of COMPUTE then the value of N/P may be changed according to an implementation-specific mechanism.

Since I also don't see a need to change these values, I think we are fine to don't do so. @dbu Do you agree?

implementation-specific means we are free to do whatever the easiest solution is :-)

danrot · 2016-12-15T09:48:59Z

src/Jackalope/Version/VersionHandler.php

+                    $childNode->remove();
+                    break;
+                case OnParentVersionAction::INITIALIZE:
+                    // TODO reinitialize node as defined in its node type


I think that's the only thing left before we can merge this. This behavior here should be similar to the one for properties. The code autocreating the child nodes is currently in the NodeProcessor. Should I also extract that to a determineDefaultValue to the NodeDefinition?

is it as easy as for a property? what is the "default value" of a node? a node is not just a value.

Yeah, if I look at the code of the NodeProcessor I'd say it's just about adding a node and apply some value to the required properties. Was thinking if it would be enough to just add the node with the primary NodeType and let the system do the rest. Not sure if that will crash something, but then the NodeProcessor also wouldn't work. I guess I just give it a try.

My idea here was to simply call $childNode->remove() and recreate the node. But the problem is that if the $childNode is defined as mandatory, I can't remove it... Any idea how else I could handle that?

dbu · 2016-12-15T10:42:23Z

there is NodeTypeTest in phpcr-api-tests. does that help as an inspiration? if its a generic feature you want to test, we should have it in phpcr-api-tests. if its about our implementation specific behaviour, jackalope would be the right place i guess. you can rely on the phpcr-api-tests infrastructure if you need, i think

lsmith77 added the wip/poc label Jun 11, 2015

dbu reviewed Jun 11, 2015
View reviewed changes

dbu reviewed Jul 24, 2015
View reviewed changes

dbu force-pushed the versioning branch from 0dc3dc0 to 5913317 Compare August 16, 2015 19:14

danrot mentioned this pull request Oct 9, 2015

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

Open

danrot reviewed Nov 23, 2015
View reviewed changes

danrot added 5 commits January 5, 2016 18:02

handled case with deleted property

22fb674

remove properties from node on restore

eddb549

implemented restoring of child nodes

d6f3e26

fixed comments

7c76730

write versioned child property to node

fcdfb22

danrot reviewed Jan 5, 2016
View reviewed changes

fixed restoring of child nodes

9bdbe21

danrot commented Dec 13, 2016

View reviewed changes

fixed code style

e2b5b0c

danrot added 2 commits December 14, 2016 08:12

added rep namespace

40b08e6

added check for onParentVersion when restoring nodes from frozenNode

5074b42

danrot commented Dec 14, 2016

View reviewed changes

danrot added 2 commits December 15, 2016 09:57

extract determination of default value

de32f5a

throw NotImplementedException for restoring nodes with OPV of VERSION

91ced8a

danrot commented Dec 15, 2016

View reviewed changes

implemented OPV for child nodes not existing in frozen node

5d1ddf5

danrot commented Dec 15, 2016

View reviewed changes

danrot mentioned this pull request Dec 15, 2016

added tests for OPV of initialize phpcr/phpcr-api-tests#181

Closed

implemented restoring of childnodes with OPV of INITIALIZE

e805a9a

	foreach ($nodeTypeDefinition->getDeclaredPropertyDefinitions() as $propertyDef) {
	/* @var $propertyDef PropertyDefinitionInterface */

Adding restore functionality for versioning support #289

Are you sure you want to change the base?

Adding restore functionality for versioning support #289

Conversation

danrot commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot commented Jun 25, 2015

Choose a reason for hiding this comment

dbu commented Jul 24, 2015

danrot commented Jul 25, 2015

dbu commented Jul 31, 2015

danrot commented Aug 10, 2015

dbu commented Aug 16, 2015

danrot commented Sep 22, 2015

dbu commented Oct 9, 2015

danrot commented Oct 14, 2015

dbu commented Oct 17, 2015

danrot commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Nov 23, 2015

danrot commented Jan 5, 2016

danrot commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahilles107 commented Jul 28, 2016

danrot commented Jul 28, 2016

dbu commented Jul 28, 2016

danrot commented Dec 13, 2016

danrot commented Dec 13, 2016

dbu commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot commented Dec 13, 2016

dbu commented Dec 13, 2016

danrot commented Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Dec 15, 2016 via email

danrot commented Dec 13, 2016 •

edited

danrot Dec 15, 2016 •

edited