Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2165796143 Thx for the update @shodaaan I'm reviewing the changes and actually would like to find a test that verifies the opposite of the new MoveVersionableNodeWithNodeRepositoryTest to better understand the context etc. Might take me some more time. If someone else wants to chime in and approve in the meantime, pls feel free. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2164915319 > two more comments that I have drafted in [this temporary branch](https://github.com/apache/jackrabbit-oak/compare/OAK-8848-review-illustration?expand=1) for illustration: > > * we could move the `nodeWasMovedOrCopied()` check for the 2nd case into that if block, to make it more limited to just that case. Previously it was throwing the exception - now I think we want it to not throw it anymore. With the check being in the outer if there is possibility it might still throw if the next if case hits (perhaps unlikely, but by doing it the above drafted way we'd reduce that risk) > * some reformatting suggestion to use `//` instead of `/*` - I think that's a bit more following how inline comments are frequently done > > wdyt? Thank you for the suggestions. I have made the changes in the latest commit. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2163615840 two more comments that I have drafted in [this temporary branch](https://github.com/apache/jackrabbit-oak/compare/OAK-8848-review-illustration?expand=1) for illustration: * we could move the `nodeWasMovedOrCopied()` check for the 2nd case into that if block, to make it more limited to just that case. Previously it was throwing the exception - now I think we want it to not throw it anymore. With the check being in the outer if there is possibility it might still throw if the next if case hits (perhaps unlikely, but by doing it the above drafted way we'd reduce that risk) * some reformatting suggestion to use `//` instead of `/*` - I think that's a bit more following how inline comments are frequently done wdyt? -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1635977258
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,73 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when the JCR_ISCHECKEDOUT property
+// change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
+
+ /*Completes the restore of a version from version history.
+
+ When the JCR_BASEVERSION property is processed, a check is made for the
current base version property.
+ If a restore is currently in progress for the current base version (the
check for this is that the
+ current base version name has the format "restore-[UUID of the version
to restore to]"), then the restore
+ is completed for the current node to the version specified by the UUID.
+
+ If a node that was moved or copied to the location of a deleted node is
currently being processed
+ (see OAK-8848 for context), the restore operation must NOT be performed
when the JCR_BASEVERSION property change
+ is processed for the node.
+
+ TODO: check if this code ever runs when
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX) is FALSE,
Review Comment:
No, it is not, I will remove the comment. I will remove the second check in
the version restore code block - the one for
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX) - since it is not
necessary in order for all of the restore logic and the fix to work fine.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1635977258
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,73 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when the JCR_ISCHECKEDOUT property
+// change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
+
+ /*Completes the restore of a version from version history.
+
+ When the JCR_BASEVERSION property is processed, a check is made for the
current base version property.
+ If a restore is currently in progress for the current base version (the
check for this is that the
+ current base version name has the format "restore-[UUID of the version
to restore to]"), then the restore
+ is completed for the current node to the version specified by the UUID.
+
+ If a node that was moved or copied to the location of a deleted node is
currently being processed
+ (see OAK-8848 for context), the restore operation must NOT be performed
when the JCR_BASEVERSION property change
+ is processed for the node.
+
+ TODO: check if this code ever runs when
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX) is FALSE,
Review Comment:
No, it is not, I remove the comment. I will remove the second check in the
version restore code block - the one for
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX) - since it is not
necessary in order for all of the restore logic and the fix to work fine.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1635974692
##
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:
You are right, thank you for the comment. I will remove the second check.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1634517169
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/VersionManagementTest.java:
##
@@ -45,7 +45,7 @@ public class VersionManagementTest extends
AbstractEvaluationTest {
@Override
@Before
-protected void setUp() throws Exception {
+public void setUp() throws Exception {
Review Comment:
I reverted all of the changes, since the new unit tests are now in a
different class.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1634515733
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithNodeRepositoryTest.java:
##
@@ -0,0 +1,230 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeWithNodeRepositoryTest extends
AbstractRepositoryTest {
+
+private static final String mixVersionable = "mix:versionable";
Review Comment:
I used JcrConstants.MIX_VERSIONABLE. Done.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1634514892
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,73 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when the JCR_ISCHECKEDOUT property
+// change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
+
+ /*Completes the restore of a version from version history.
Review Comment:
Done.
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1634514112
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,73 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when the JCR_ISCHECKEDOUT property
+// change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
+
+ /*Completes the restore of a version from version history.
+
+ When the JCR_BASEVERSION property is processed, a check is made for the
current base version property.
+ If a restore is currently in progress for the current base version (the
check for this is that the
+ current base version name has the format "restore-[UUID of the version
to restore to]"), then the restore
+ is completed for the current node to the version specified by the UUID.
+
+ If a node that was moved or copied to the location of a deleted node is
currently being processed
+ (see OAK-8848 for context), the restore operation must NOT be performed
when the JCR_BASEVERSION property change
+ is processed for the node.
+
+ TODO: check if this code ever runs when
after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX) is FALSE,
Review Comment:
Done.
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithNodeRepositoryTest.java:
##
@@ -0,0 +1,230 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeWithNodeRepositoryTest extends
AbstractRepositoryTest {
+
+private static final String mixVersionable = "mix:versionable";
+
+public MoveVersionableNodeWithNodeRepositoryTest(NodeStoreFixture fixture)
{
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(mixVersionable)) {
+newNode.addMixin(mixVersionable);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, property
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
reschke commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1629067216
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##
@@ -145,27 +146,73 @@ public void propertyChanged(PropertyState before,
PropertyState after)
return;
}
String propName = after.getName();
+
+// Updates the checked-out / checked-in state of the currently
processed node when the JCR_ISCHECKEDOUT property
+// change is processed.
if (propName.equals(JCR_ISCHECKEDOUT)) {
if (wasCheckedIn()) {
vMgr.checkout(node);
} else {
vMgr.checkin(node);
}
+
+ /*Completes the restore of a version from version history.
Review Comment:
```suggestion
/* Completes the restore of a version from version history.
```
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithNodeRepositoryTest.java:
##
@@ -0,0 +1,230 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeWithNodeRepositoryTest extends
AbstractRepositoryTest {
+
+private static final String mixVersionable = "mix:versionable";
+
+public MoveVersionableNodeWithNodeRepositoryTest(NodeStoreFixture fixture)
{
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(mixVersionable)) {
+newNode.addMixin(mixVersionable);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, propertyValue);
+node.getSession().save();
+node.checkin();
+}
+
+/*
+ * 1. Create a versionable unstructured node at
nodeName1/nodeName2/sourceNode
+ * 2. Create a versionable unstructured node at
nodeName1/nodeName3/sourceNode
+ * 3. create version histories for both nodes
+ * 4. remove nodeName1/nodeName3/nodeName1 (that's because move(src,dest)
throws an exception if dest already exists)
+ * 5. move nodeName1/nodeName2/sourceNode to
nodeName1/nodeName3/sourceNode and call session.save()
+ * 6. should work according to JCR specification - reproduces bug for
OAK-8848: will throw ConstraintViolationException
+ * - "Property is protected: jcr:versionHistory
+
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2147389103 Regarding the refactoring of `propertyChanged` : so far I am not seeing the advantage of the refactoring. That code isn't reused elsewhere, it is merely cutting a larger method into 4 parts that have historically been together. I think the same could be achieved with adding more // comments where useful, the additional methods for me at least aren't improving things. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1625889552
##
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:
Thx for the very detailed description!
> Regarding the after.getValue(Type.REFERENCE).startsWith(RESTORE_PREFIX))
condition:
As the code is now `restore()` is only called if this condition is true.
Before this change however, it was also called in other cases. Now if those
other cases were wrong, then that's a bug. I would however prefer to look into
that and have a test cases that proofs that is a bug. I'm suspecting that with
this change we'll introduce a regression...
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
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 A
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1624467573
##
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:
I'm a bit lost at this condition here and wonder if it needs to be at this
complexity. I'm also wondering if all permutations of this are covered in unit
tests. Let's say nodeWasMovedOrCopied is false, why is the second part of the
condition needed then? Can you please elaborate a bit on this?
--
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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2144993219 @shodaaan `MoveVersionableNodeWithNodeRepositoryTest` is missing the license header -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1619342145 ## oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithMemoryRepositoryTest.java: ## @@ -0,0 +1,26 @@ +package org.apache.jackrabbit.oak.jcr; Review Comment: fixed in latest commit -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
reschke commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1613082105 ## oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithMemoryRepositoryTest.java: ## @@ -0,0 +1,26 @@ +package org.apache.jackrabbit.oak.jcr; Review Comment: So, IIUC, you have created multiple test classes so that the different mixtures are covered automatically. That was certainly helpful for testing, but it wouldn't scale if we did this everywhere. So I would propose to simplify that again; and we also should wortk on the orthogonal issue of documenting and potentially fixing the CI pipelines. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
reschke commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1613082105 ## oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeWithMemoryRepositoryTest.java: ## @@ -0,0 +1,26 @@ +package org.apache.jackrabbit.oak.jcr; Review Comment: So, IIUC, you have created mutliple test classes so that the different mixtures are covered automatically. That was certainly helpful for testing, but it wouldn't scale if we did this everywhere. So I would propose to simplify that again; and we also should wortk on the orthogonal issue of documenting and potentially fixing the CI pipelines. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2127197933 @stefan-egli: I created another unit test where the 2 nodes have version histories and the code flow is the same and throws ConstraintViolationException. So if the 2 nodes have only "mix:versionable" or version histories, the behaviour and result is the same. Apparently the "jcr:versionHistory" property is created automatically for mix:versionable nodes even if no check-out / check-in operations are performed on them, and the VersionEditor.propertyChanged method throws the exception for the after node's "jcr:versionHistory" property. The fix I propose in the latest commit works for both cases and was tested in unit tests using Segment, DocumentNodeStore (Mongo) and MemoryNodeStore as underlying storage when running the unit tests. -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
mbaedke commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2126720239 > Would it be possible to try to reproduce a constraint violation exception when replacing a node with another node when both have a version history? (to see if this is can be reproduced - and is the same or a different issue) @stefan-egli I don't understand, isn't that exactly what the new test does? -- 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
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#issuecomment-2121908591 - included the unit test created by Adelina Marian, which reproduces the bug - added a fix to VersionEditor that allows a node with mix:versionable to be moved over a deleted node -- 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
[PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan opened a new pull request, #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474 (no comment) -- 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
