shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1624611555
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##########
@@ -152,20 +153,37 @@ public void propertyChanged(PropertyState before,
PropertyState after)
vMgr.checkin(node);
}
} else if (propName.equals(JCR_BASEVERSION)) {
- String baseVersion = after.getValue(Type.REFERENCE);
- if (baseVersion.startsWith(RESTORE_PREFIX)) {
- baseVersion = baseVersion.substring(RESTORE_PREFIX.length());
- node.setProperty(JCR_BASEVERSION, baseVersion, Type.REFERENCE);
+ // OAK-8848: skip restore if this is an overwrite of a node and a
restore operation is not in progress
+ if (!nodeWasMovedOrCopied() ||
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX)) {
Review Comment:
The reason why the nodeWasMovedOrCopied check is necessary in the code path
that performs the version restore (starting with if
(propName.equals(JCR_BASEVERSION))
) is that without it a bug occurs in certain conditions when doing the move
with overwrite that is required to work in the definition of done of OAK-8848.
I will describe the flow that leads to the bug.
Prerequisites:
Start with two nodes, and create different versions for both of them. I
tested this with image files that I then modify at least once, therefore
creating versions for them.
Then run the curl command for overwriting one of the nodes with the other.
An example of using the Sling curl command for moving a node named Testfile.png
from the parent sourceDir, under the parent destinationDir, where it overwrites
another node with the same name is:
curl -u admin:admin -X MOVE
http://localhost:4502/api/assets/sourceDir/Testfile.png -H "X-Destination:
http://localhost:4502/api/assets/destinationDir/Testfile.png"; -H "X-Overwrite:
T"
If both of the nodes have version histories, i.e. they both have the
jcr:versionHistory property set, as well as the jcr:baseVersion property set,
the following flow will happen in the JCR layer of Oak, as a result of the
session.save() call:
Step 1: ModifiedNodeState.compareAgainstBaseState will be called for the
node Testfile.png (the moved node that will overwrite the previous node),
leading to calls to EditorDiff.propertyChanged for all properties of the node.
This in turn will lead to VersionEditor.propertyChanged to be called for all
properties of the Testfile.png node.
Step 2: Eventually VersionEditor.propertyChanged will be called for the
jcr:versionHistory property of the Testfile.png node.
In my integration tests, this was called before being called for the
jcr:baseVersion property. This order is very important, as will be shown in the
following steps.
Since nodeWasMovedOrCopied will return true for Testfile.png, the
throwProtected method will not be called.
This is the fix for the ConstraintViolationException that is described in
OAK-8848.
Step 3: Since the node has multiple versions, it has the jcr:baseVersion
property. Therefore, VersionEditor.propertyChanged will be called for this
property as well.
When this happens, without the nodeWasMovedOrChanged check, the restore code
will run, leading to versionManager.restore to be called for the Testfile.png
node.
This logic is wrong, since there is no restore of a version of the node that
should take place here.
Further, the VersionableState.restore method will be called for the
Testfile.png node, leading to the call
checkNotNull(versionable).setProperty(JCR_ISCHECKEDOUT, false, Type.BOOLEAN),
which will set the jcr:isCheckedOut property for the node to false.
This is wrong behavior, since the move of a node over another should not
result in the node being automatically checked in.
The fix I propose in the PR is to also add the check for
nodeWasMovedOrChanged for the code that performs the restore. Since the check
returns true, the restore logic will not run when the overwrite is performed.
Regarding the after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX))
condition:
Debugging the code when restoring previous versions of a node led me to
understand that the jcr:baseVersion property is first modified to have the
prefix “restore-” to the original UUID, and then this prefix is used in the
restore code section in VersionEditor.propertyChanged to set the
jcr:baseVersion property back to its’ original UUID, when the method runs for
the jcr:baseVersion property.
This leads me to believe that the check for startsWith(RESTORE_PREFIX)
should also be added for clarity.
Regarding the unit tests, I agree that the bug described above should have a
unit test that reproduces it. I will create it and update the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
![http://localhost:4502/api/assets/destinationDir/Testfile.png"](http://localhost:4502/api/assets/destinationDir/Testfile.png"){kind=link}