Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644584397 ## oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java: ## @@ -0,0 +1,229 @@ +/* + * 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.JcrConstants; +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 MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest { + +public MoveVersionableNodeRepositoryTest(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(JcrConstants.MIX_VERSIONABLE)) { +newNode.addMixin(JcrConstants.MIX_VERSIONABLE); +} 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 + */ +@Test +public void testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws Exception { + +Session session = getAdminSession(); +Node testRootNode = session.getRootNode().addNode("node1"); + +String newNodeName = "sourceNode"; +Node sourceParent = testRootNode.addNode("node2"); // nodeName1/nodeName2 +Node sourceNode = createVersionableNode(sourceParent, newNodeName); // nodeName1/nodeName2/sourceNode +Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3 +Node destNode = createVersionableNode(destParent, "destNode"); // nodeName1/nodeName3/sourceNode + +String destPath = destNode.getPath(); + +// add version histories for sourceNode and destNode +setNodePropertyAndCheckIn(sourceNode,
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_r1644565491 ## oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java: ## @@ -0,0 +1,229 @@ +/* + * 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.JcrConstants; +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 MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest { + +public MoveVersionableNodeRepositoryTest(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(JcrConstants.MIX_VERSIONABLE)) { +newNode.addMixin(JcrConstants.MIX_VERSIONABLE); +} 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 + */ +@Test +public void testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws Exception { + +Session session = getAdminSession(); +Node testRootNode = session.getRootNode().addNode("node1"); + +String newNodeName = "sourceNode"; +Node sourceParent = testRootNode.addNode("node2"); // nodeName1/nodeName2 +Node sourceNode = createVersionableNode(sourceParent, newNodeName); // nodeName1/nodeName2/sourceNode +Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3 +Node destNode = createVersionableNode(destParent, "destNode"); // nodeName1/nodeName3/sourceNode + +String destPath = destNode.getPath(); + +// add version histories for sourceNode and destNode +setNodePropertyAndCheckIn(sourceNode,
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644246364 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: Fixed it, thank you for the observation. -- 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: oak-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_r1644227441 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: this file shouldn't be part of 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: oak-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_r1642962427 ## oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java: ## @@ -145,27 +146,71 @@ 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); } } 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); + +// 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. +if (!nodeWasMovedOrCopied()) { + +String baseVersion = after.getValue(Type.REFERENCE); +if (baseVersion.startsWith(RESTORE_PREFIX)) { +baseVersion = baseVersion.substring(RESTORE_PREFIX.length()); +node.setProperty(JCR_BASEVERSION, baseVersion, Type.REFERENCE); +} + +vMgr.restore(node, baseVersion, null); } -vMgr.restore(node, baseVersion, null); } else if (isVersionProperty(after)) { -throwProtected(after.getName()); +// Checks if a version property is being changed and throws a CommitFailedException +// with the message "Constraint Violation Exception" if this is not allowed. +// JCR_ISCHECKEDOUT and JCR_BASEVERSION properties should be ignored, since changes +// to them are allowed for specific use cases (for example, completing the check-in +// / check-out for a node or completing a node restore). +// +// The only situation when the update of a version property is allowed is when this +// occurs as a result of the current node being moved over a previously deleted node +// - see OAK-8848 for context. +// +// OAK-8848: moving a versionable node in the same location as a node deleted in the +// same session should be allowed. +// This check works because the only way that moving a node in a location is allowed +// is if there is no existing (undeleted) node in that location. +// Property comparison should not fail for two jcr:versionHistory properties in this case. +if (!nodeWasMovedOrCopied()) { +throwProtected(after.getName()); +} } else if (isReadOnly && getOPV(after) != OnParentVersionAction.IGNORE) { throwCheckedIn("Cannot change property " + after.getName() + " on checked in node"); } } +/** + * Returns true if and only if the given node was moved or copied from another location. + */ +private boolean nodeWasMovedOrCopied() { Review Comment: I just tested the copy instead of move case and it can not be done in one session, because WorkspaceDelegate.copy throws a javax.jcr.ItemExistsException. Copy was not mentioned in the ticket requirement either, it was a wrong assumption on my part that copy will behave the same as move in this case. I will rename the check method to nodeWasMoved. -- 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: oak-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_r1638425524 ## oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java: ## @@ -145,27 +146,71 @@ 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); } } 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); + +// 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. +if (!nodeWasMovedOrCopied()) { + +String baseVersion = after.getValue(Type.REFERENCE); +if (baseVersion.startsWith(RESTORE_PREFIX)) { +baseVersion = baseVersion.substring(RESTORE_PREFIX.length()); +node.setProperty(JCR_BASEVERSION, baseVersion, Type.REFERENCE); +} + +vMgr.restore(node, baseVersion, null); } -vMgr.restore(node, baseVersion, null); } else if (isVersionProperty(after)) { -throwProtected(after.getName()); +// Checks if a version property is being changed and throws a CommitFailedException +// with the message "Constraint Violation Exception" if this is not allowed. +// JCR_ISCHECKEDOUT and JCR_BASEVERSION properties should be ignored, since changes +// to them are allowed for specific use cases (for example, completing the check-in +// / check-out for a node or completing a node restore). +// +// The only situation when the update of a version property is allowed is when this +// occurs as a result of the current node being moved over a previously deleted node +// - see OAK-8848 for context. +// +// OAK-8848: moving a versionable node in the same location as a node deleted in the +// same session should be allowed. +// This check works because the only way that moving a node in a location is allowed +// is if there is no existing (undeleted) node in that location. +// Property comparison should not fail for two jcr:versionHistory properties in this case. +if (!nodeWasMovedOrCopied()) { +throwProtected(after.getName()); +} } else if (isReadOnly && getOPV(after) != OnParentVersionAction.IGNORE) { throwCheckedIn("Cannot change property " + after.getName() + " on checked in node"); } } +/** + * Returns true if and only if the given node was moved or copied from another location. + */ +private boolean nodeWasMovedOrCopied() { Review Comment: @shodaaan a question about this one : I'm not sure this is also applicable to copy - I can see the move case, as that is covered through `:source-path` - but in which case is `:source-path` also added for copy? -- 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: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org