diff --git a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegate.java b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegate.java index 4ce6f4e..e9aee17 100644 --- a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegate.java +++ b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/NodeDelegate.java @@ -24,6 +24,7 @@ import static com.google.common.collect.Iterators.transform; import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.Sets.newHashSet; import static com.google.common.collect.Sets.newLinkedHashSet; +import static org.apache.jackrabbit.JcrConstants.JCR_HASORDERABLECHILDNODES; import static org.apache.jackrabbit.JcrConstants.JCR_ISMIXIN; import static org.apache.jackrabbit.JcrConstants.JCR_LOCKISDEEP; import static org.apache.jackrabbit.JcrConstants.JCR_LOCKOWNER; @@ -38,6 +39,7 @@ import static org.apache.jackrabbit.JcrConstants.JCR_UUID; import static org.apache.jackrabbit.JcrConstants.MIX_LOCKABLE; import static org.apache.jackrabbit.JcrConstants.NT_BASE; import static org.apache.jackrabbit.oak.api.Type.BOOLEAN; +import static org.apache.jackrabbit.oak.api.Type.NAME; import static org.apache.jackrabbit.oak.api.Type.NAMES; import static org.apache.jackrabbit.oak.api.Type.UNDEFINED; import static org.apache.jackrabbit.oak.api.Type.UNDEFINEDS; @@ -96,6 +98,12 @@ import org.apache.jackrabbit.value.ValueHelper; */ public class NodeDelegate extends ItemDelegate { + /** + * Read more about this flag on OAK-5229. Enable Jackrabbit Compatibility by + * setting the system property 'oak.nodetype.purgeNodeOnTypeChange' to true + */ + private static final boolean PURGE_NODE_ON_TYPE_CHANGE = Boolean.getBoolean("oak.nodetype.purgeNodeOnTypeChange"); + /** The underlying {@link org.apache.jackrabbit.oak.api.Tree} of this node. */ private final Tree tree; @@ -377,6 +385,30 @@ public class NodeDelegate extends ItemDelegate { } } + public void setPrimaryType(String typeName) throws RepositoryException { + Tree typeRoot = sessionDelegate.getRoot().getTree(NODE_TYPES_PATH); + Tree type = typeRoot.getChild(typeName); + if (!type.exists()) { + throw new NoSuchNodeTypeException("Node type " + typeName + " does not exist"); + } else if (getBoolean(type, JCR_IS_ABSTRACT)) { + throw new ConstraintViolationException("Node type " + typeName + " is abstract"); + } else if (getBoolean(type, JCR_ISMIXIN)) { + throw new ConstraintViolationException("Node type " + typeName + " is a mixin type"); + } + Tree tree = getTree(); + String oldType = TreeUtil.getName(tree, JCR_PRIMARYTYPE); + if (typeName.equals(oldType)) { + return; + } + PropertyState state = PropertyStates.createProperty(JCR_PRIMARYTYPE, typeName, NAME); + setProperty(state, true, true); + setOrderableChildren(getBoolean(type, JCR_HASORDERABLECHILDNODES)); + TreeUtil.autoCreateItems(tree, type, typeRoot, getUserID()); + if (oldType != null) { + purgeNode(typeRoot, newHashSet(oldType)); + } + } + public boolean canAddMixin(String typeName) throws RepositoryException { Tree type = sessionDelegate.getRoot().getTree(NODE_TYPES_PATH).getChild(typeName); if (type.exists()) { @@ -416,11 +448,13 @@ public class NodeDelegate extends ItemDelegate { } } - public void updateMixins(Set addMixinNames, Set removedOakMixinNames) throws RepositoryException { + Tree tree = getTree(); + Tree typeRoot = sessionDelegate.getRoot().getTree(NODE_TYPES_PATH); + String userId = getUserID(); // 1. set all new mixin types including validation for (String oakMixinName : addMixinNames) { - addMixin(oakMixinName); + TreeUtil.addMixin(tree, oakMixinName, typeRoot, userId); } if (!removedOakMixinNames.isEmpty()) { @@ -431,61 +465,100 @@ public class NodeDelegate extends ItemDelegate { mixinNames.addAll(addMixinNames); tree.setProperty(JCR_MIXINTYPES, mixinNames, NAMES); } - - // 3. deal with locked nodes - boolean wasLockable = isNodeType(MIX_LOCKABLE); - boolean isLockable = isNodeType(MIX_LOCKABLE); - if (wasLockable && !isLockable && holdsLock(false)) { - // TODO: This should probably be done in a commit hook - unlock(); - sessionDelegate.refresh(true); - } - - // 4. clean up set of properties and child nodes such that all child items + // 3. clean up set of properties and child nodes such that all child items // have a valid item definition according to the effective node type present // after having updated the mixin property. this includes removing all // protected properties and child nodes associated with the removed mixin // type(s), as there's no way for the client to do that. Other items // defined in this mixin type might also need to be removed if there // is no longer a matching item definition available. - Tree typeRoot = sessionDelegate.getRoot().getTree(NODE_TYPES_PATH); - List removed = new ArrayList(); - for (String name : removedOakMixinNames) { - removed.add(typeRoot.getChild(name)); - } - List remaining = getNodeTypes(tree, typeRoot); + purgeNode(typeRoot, removedOakMixinNames); + } + } - for (PropertyState property : tree.getProperties()) { - String name = property.getName(); - Type type = property.getType(); + /** + * Clean up the set of properties and child nodes such that all child items + * have a valid item definition according to the effective node type present + * after having updated the mixin property or the primary type. this + * includes removing all protected properties and child nodes associated + * with the removed mixin type(s), as there's no way for the client to do + * that. Other items defined in this mixin type might also need to be + * removed if there is no longer a matching item definition available. + * @param typeRoot + * @param removedNames + */ + private void purgeNode(Tree typeRoot, Set removedNames) throws RepositoryException { + if (removedNames.isEmpty()) { + return; + } + + // deal with locked nodes + boolean wasLockable = isNodeType(MIX_LOCKABLE); + boolean isLockable = isNodeType(MIX_LOCKABLE); + if (wasLockable && !isLockable && holdsLock(false)) { + // TODO: This should probably be done in a commit hook + unlock(); + sessionDelegate.refresh(true); + } + List removed = new ArrayList(); + for (String name : removedNames) { + removed.add(typeRoot.getChild(name)); + } + Tree tree = getTree(); + List remaining = getNodeTypes(tree, typeRoot); - Tree oldDefinition = findMatchingPropertyDefinition(removed, name, type, true); - if (oldDefinition != null) { - Tree newDefinition = findMatchingPropertyDefinition(remaining, name, type, true); + for (PropertyState property : tree.getProperties()) { + String name = property.getName(); + Type type = property.getType(); + + Tree oldDefinition = findMatchingPropertyDefinition(removed, name, type, true); + if (oldDefinition != null) { + Tree newDefinition = findMatchingPropertyDefinition(remaining, name, type, true); + if(PURGE_NODE_ON_TYPE_CHANGE) { if (newDefinition == null || (getBoolean(oldDefinition, JCR_PROTECTED) && !getBoolean(newDefinition, JCR_PROTECTED))) { tree.removeProperty(name); } + } else { + boolean wasProtected = getBoolean(oldDefinition, JCR_PROTECTED); + if (newDefinition == null) { + if (wasProtected) { + tree.removeProperty(name); + } else { + throw new ConstraintViolationException("No matching property definition found for " + name); + } + } } } + } - for (Tree child : tree.getChildren()) { - String name = child.getName(); - Set typeNames = newLinkedHashSet(); - for (Tree type : getNodeTypes(child, typeRoot)) { - typeNames.add(TreeUtil.getName(type, JCR_NODETYPENAME)); - addAll(typeNames, getNames(type, REP_SUPERTYPES)); - } + for (Tree child : tree.getChildren()) { + String name = child.getName(); + Set typeNames = newLinkedHashSet(); + for (Tree type : getNodeTypes(child, typeRoot)) { + typeNames.add(TreeUtil.getName(type, JCR_NODETYPENAME)); + addAll(typeNames, getNames(type, REP_SUPERTYPES)); + } - Tree oldDefinition = findMatchingChildNodeDefinition(removed, name, typeNames); - if (oldDefinition != null) { - Tree newDefinition = findMatchingChildNodeDefinition(remaining, name, typeNames); + Tree oldDefinition = findMatchingChildNodeDefinition(removed, name, typeNames); + if (oldDefinition != null) { + Tree newDefinition = findMatchingChildNodeDefinition(remaining, name, typeNames); + if(PURGE_NODE_ON_TYPE_CHANGE) { if (newDefinition == null || (getBoolean(oldDefinition, JCR_PROTECTED) && !getBoolean(newDefinition, JCR_PROTECTED))) { child.remove(); } + } else { + boolean wasProtected = getBoolean(oldDefinition, JCR_PROTECTED); + if (newDefinition == null) { + if (wasProtected) { + child.remove(); + } else { + throw new ConstraintViolationException("No matching child node definition found for " + name); + } + } } } } diff --git a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/NodeImpl.java b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/NodeImpl.java index 15ffa7a..a6e4c50 100644 --- a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/NodeImpl.java +++ b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/NodeImpl.java @@ -934,6 +934,7 @@ public class NodeImpl extends ItemImpl implements Nod @Override public void setPrimaryType(final String nodeTypeName) throws RepositoryException { + final String oakTypeName = getOakName(checkNotNull(nodeTypeName)); sessionDelegate.performVoid(new ItemWriteOperation("setPrimaryType") { @Override public void checkPreconditions() throws RepositoryException { @@ -945,7 +946,7 @@ public class NodeImpl extends ItemImpl implements Nod @Override public void performVoid() throws RepositoryException { - internalSetPrimaryType(nodeTypeName); + dlg.setPrimaryType(oakTypeName); } }); } @@ -1341,20 +1342,6 @@ public class NodeImpl extends ItemImpl implements Nod } } - private void internalSetPrimaryType(final String nodeTypeName) throws RepositoryException { - // TODO: figure out the right place for this check - NodeType nt = getNodeTypeManager().getNodeType(nodeTypeName); // throws on not found - if (nt.isAbstract() || nt.isMixin()) { - throw new ConstraintViolationException(getNodePath()); - } - // TODO: END - - PropertyState state = PropertyStates.createProperty( - JCR_PRIMARYTYPE, getOakName(nodeTypeName), NAME); - dlg.setProperty(state, true, true); - dlg.setOrderableChildren(nt.hasOrderableChildNodes()); - } - private Property internalSetProperty( final String jcrName, final Value value, final boolean exactTypeMatch) throws RepositoryException { diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/JackrabbitNodeTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/JackrabbitNodeTest.java index ebd290c..8e24f00 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/JackrabbitNodeTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/JackrabbitNodeTest.java @@ -279,6 +279,7 @@ public class JackrabbitNodeTest extends AbstractJCRTest { superuser.save(); // 'downgrade' from test:AA to test:A + n.getProperty("test:propAA").remove(); ((JackrabbitNode) n).setMixins(new String[]{"test:A"}); superuser.save(); @@ -294,6 +295,8 @@ public class JackrabbitNodeTest extends AbstractJCRTest { assertTrue(n.hasProperty("test:propAA")); // replace test:AA with mix:title + n.getProperty("test:propA").remove(); + n.getProperty("test:propAA").remove(); ((JackrabbitNode) n).setMixins(new String[]{"mix:title"}); n.setProperty("jcr:title", "..."); n.setProperty("jcr:description", "blah blah"); diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java index 01c9706..48efd69 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java @@ -1872,6 +1872,38 @@ public class RepositoryTest extends AbstractRepositoryTest { } @Test + public void setPrimaryTypeCleanup() throws RepositoryException { + String path = TEST_PATH + "/" + "testType"; + getAdminSession().getWorkspace().copy("/jcr:system/jcr:nodeTypes/oak:Unstructured/jcr:propertyDefinition", + path); + + Node testNode = getNode(path); + assertEquals("nt:propertyDefinition", testNode.getPrimaryNodeType().getName()); + + List protectedProps = Arrays.asList("jcr:valueConstraints", "jcr:isQueryOrderable", + "jcr:isFullTextSearchable", "jcr:requiredType", "jcr:multiple", "jcr:availableQueryOperators", + "jcr:defaultValues"); + + Set pnames = new HashSet<>(); + PropertyIterator pi = testNode.getProperties(); + while (pi.hasNext()) { + pnames.add(pi.nextProperty().getName()); + } + assertTrue(pnames.containsAll(protectedProps)); + + // this should purge protected properties from original node that don't + // exist on the new type + testNode.setPrimaryType("nt:childNodeDefinition"); + + pnames = new HashSet<>(); + pi = testNode.getProperties(); + while (pi.hasNext()) { + pnames.add(pi.nextProperty().getName()); + } + assertTrue(Collections.disjoint(protectedProps, pnames)); + } + + @Test public void setPrimaryTypeShouldFail() throws RepositoryException { Node testNode = getNode(TEST_PATH); assertEquals("nt:unstructured", testNode.getPrimaryNodeType().getName()); @@ -1881,12 +1913,11 @@ public class RepositoryTest extends AbstractRepositoryTest { testNode.addNode("unstructured_child", NodeType.NT_UNSTRUCTURED); getAdminSession().save(); - testNode.setPrimaryType("nt:folder"); try { - getAdminSession().save(); + // call fails immediately due to extra child node that cannot be cleaned automatically + testNode.setPrimaryType("nt:folder"); fail("Changing the primary type to nt:folder should fail."); } catch (RepositoryException e) { - // ok getAdminSession().refresh(false); } diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/MixinTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/MixinTest.java index eb44b6e..e477410 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/MixinTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/MixinTest.java @@ -19,14 +19,11 @@ package org.apache.jackrabbit.oak.jcr.nodetype; import javax.jcr.Node; import javax.jcr.nodetype.NoSuchNodeTypeException; -import org.apache.jackrabbit.JcrConstants; import org.apache.jackrabbit.api.JackrabbitSession; import org.apache.jackrabbit.api.security.user.Authorizable; import org.apache.jackrabbit.test.AbstractJCRTest; import org.apache.jackrabbit.test.NotExecutableException; -import static org.apache.jackrabbit.JcrConstants.NT_UNSTRUCTURED; - /** * */ @@ -34,10 +31,10 @@ public class MixinTest extends AbstractJCRTest { public void testRemoveMixinWithoutMixinProperty() throws Exception { Node node = testRootNode.addNode( - "testRemoveMixinWithoutMixinProperty", NT_UNSTRUCTURED); + "testRemoveMixinWithoutMixinProperty", ntUnstructured); superuser.save(); try { - node.removeMixin(JcrConstants.MIX_REFERENCEABLE); + node.removeMixin(mixReferenceable); fail(); } catch (NoSuchNodeTypeException e) { // success @@ -50,12 +47,12 @@ public class MixinTest extends AbstractJCRTest { public void testRemoveInheritedMixin() throws Exception { Node node = testRootNode.addNode( - "testRemoveInheritedMixin", NT_UNSTRUCTURED); - node.addMixin(JcrConstants.MIX_VERSIONABLE); + "testRemoveInheritedMixin", ntUnstructured); + node.addMixin(mixVersionable); superuser.save(); try { - node.removeMixin(JcrConstants.MIX_REFERENCEABLE); + node.removeMixin(mixReferenceable); fail(); } catch (NoSuchNodeTypeException e) { // success @@ -73,8 +70,8 @@ public class MixinTest extends AbstractJCRTest { } Node node = superuser.getNode(user.getPath()); - assertTrue(node.isNodeType(JcrConstants.MIX_REFERENCEABLE)); - node.removeMixin(JcrConstants.MIX_REFERENCEABLE); + assertTrue(node.isNodeType(mixReferenceable)); + node.removeMixin(mixReferenceable); } catch (NoSuchNodeTypeException e) { // success } finally { @@ -88,6 +85,9 @@ public class MixinTest extends AbstractJCRTest { superuser.save(); node.removeMixin(mixVersionable); + node.getProperty(jcrBaseVersion).remove(); + node.getProperty(jcrPredecessors).remove(); + node.getProperty(jcrVersionHistory).remove(); superuser.save(); } @@ -98,9 +98,13 @@ public class MixinTest extends AbstractJCRTest { superuser.save(); node.removeMixin(mixVersionable); + node.getProperty(jcrBaseVersion).remove(); + node.getProperty(jcrPredecessors).remove(); + node.getProperty(jcrVersionHistory).remove(); superuser.save(); } + @SuppressWarnings("deprecation") public void testRemoveAddMixVersionable() throws Exception { Node node = testRootNode.addNode(nodeName1); node.addMixin(mixVersionable); @@ -111,9 +115,10 @@ public class MixinTest extends AbstractJCRTest { node.addMixin(mixVersionable); superuser.save(); - assertFalse(vhId.equals(node.getVersionHistory().getUUID())); + assertTrue(vhId.equals(node.getVersionHistory().getUUID())); } + @SuppressWarnings("deprecation") public void testRemoveAddMixVersionable1() throws Exception { Node node = testRootNode.addNode(nodeName1); node.addMixin(mixReferenceable); diff --git a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AccessControlManagementTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AccessControlManagementTest.java index e60ed01..9df64a1 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AccessControlManagementTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/AccessControlManagementTest.java @@ -510,7 +510,6 @@ public class AccessControlManagementTest extends AbstractEvaluationTest { n.removeMixin("rep:AccessControllable"); superuser.save(); - assertFalse(n.hasNode("rep:policy")); assertFalse(n.isNodeType("rep:AccessControllable")); } } \ No newline at end of file diff --git a/oak-jcr/src/test/resources/repositoryStubImpl.properties b/oak-jcr/src/test/resources/repositoryStubImpl.properties index d765ab0..46e7b91 100644 --- a/oak-jcr/src/test/resources/repositoryStubImpl.properties +++ b/oak-jcr/src/test/resources/repositoryStubImpl.properties @@ -527,3 +527,8 @@ javax.jcr.tck.retentionpolicyholder=/testconf/retentionTest # LIFECYCLE MANAGEMENT CONFIGURATION # ============================================================================== javax.jcr.tck.lifecycleNode=/testdata/lifecycle/node + +javax.jcr.tck.DeepLockTest.testRemoveMixLockableFromLockedNode.nodetype=nt:folder +javax.jcr.tck.SessionScopedLockTest.testRemoveMixLockableFromLockedNode.nodetype=nt:folder +javax.jcr.tck.OpenScopedLockTest.testRemoveMixLockableFromLockedNode.nodetype=nt:folder +