diff --git a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java index c9f158a..78b0aab 100644 --- a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java +++ b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugValidatorTest.java @@ -48,8 +48,10 @@ public class CugValidatorTest extends AbstractCugTest { @Test public void testChangePrimaryType() { + node = new NodeUtil(root.getTree(SUPPORTED_PATH2)); try { node.setName(JcrConstants.JCR_PRIMARYTYPE, NT_REP_CUG_POLICY); + node.setStrings(REP_PRINCIPAL_NAMES, EveryonePrincipal.NAME); root.commit(); fail(); } catch (CommitFailedException e) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java index f733066..91c1aff 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditor.java @@ -36,6 +36,7 @@ import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.MISSING_NO import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.JCR_IS_ABSTRACT; import static org.apache.jackrabbit.oak.plugins.nodetype.constraint.Constraints.valueConstraint; +import java.util.Collections; import java.util.List; import java.util.Set; @@ -44,7 +45,9 @@ import javax.annotation.Nonnull; import javax.jcr.PropertyType; import javax.jcr.Value; +import com.google.common.base.Objects; import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import org.apache.jackrabbit.oak.api.CommitFailedException; import org.apache.jackrabbit.oak.api.PropertyState; @@ -55,6 +58,7 @@ import org.apache.jackrabbit.oak.spi.commit.DefaultEditor; import org.apache.jackrabbit.oak.spi.commit.Editor; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; +import org.apache.jackrabbit.oak.spi.state.NodeStateUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -75,7 +79,7 @@ class TypeEditor extends DefaultEditor { private final Set typesToCheck; - private final boolean checkThisNode; + private boolean checkThisNode; private final TypeEditor parent; @@ -87,6 +91,8 @@ class TypeEditor extends DefaultEditor { private final NodeBuilder builder; + private final boolean validate; + TypeEditor( boolean strict, Set typesToCheck, NodeState types, String primary, Iterable mixins, NodeBuilder builder) @@ -102,11 +108,13 @@ class TypeEditor extends DefaultEditor { this.types = checkNotNull(types); this.effective = createEffectiveType(null, null, primary, mixins); this.builder = checkNotNull(builder); + this.validate = false; } private TypeEditor( @Nonnull TypeEditor parent, @Nonnull String name, - @CheckForNull String primary, @Nonnull Iterable mixins, @Nonnull NodeBuilder builder) + @CheckForNull String primary, @Nonnull Iterable mixins, @Nonnull NodeBuilder builder, + boolean validate) throws CommitFailedException { this.strict = parent.strict; this.typesToCheck = parent.typesToCheck; @@ -119,6 +127,7 @@ class TypeEditor extends DefaultEditor { this.types = parent.types; this.effective = createEffectiveType(parent.effective, name, primary, mixins); this.builder = checkNotNull(builder); + this.validate = validate; } /** @@ -133,6 +142,7 @@ class TypeEditor extends DefaultEditor { this.types = EMPTY_NODE; this.effective = checkNotNull(effective); this.builder = EMPTY_NODE.builder(); + this.validate = false; } /** @@ -174,23 +184,7 @@ class TypeEditor extends DefaultEditor { public void propertyChanged(PropertyState before, PropertyState after) throws CommitFailedException { if (checkThisNode) { - NodeState definition = effective.getDefinition(after); - if (definition == null) { - constraintViolation(4, "No matching property definition found for " + after); - } else if (JCR_UUID.equals(after.getName()) - && effective.isNodeType(MIX_REFERENCEABLE)) { - // special handling for the jcr:uuid property of mix:referenceable - // TODO: this should be done in a pluggable extension - if (!isValidUUID(after.getValue(Type.STRING))) { - constraintViolation(12, "Invalid UUID value in the jcr:uuid property"); - } - } else { - int requiredType = getRequiredType(definition); - if (requiredType != PropertyType.UNDEFINED) { - checkRequiredType(after, requiredType); - checkValueConstraints(definition, after, requiredType); - } - } + checkPropertyTypeConstraints(after); } } @@ -205,31 +199,20 @@ class TypeEditor extends DefaultEditor { } @Override - public Editor childNodeAdded(String name, NodeState after) - throws CommitFailedException { - TypeEditor editor = childNodeChanged(name, MISSING_NODE, after); - - if (editor != null && editor.checkThisNode) { - // TODO: add any auto-created items that are still missing - - // verify the presence of all mandatory items - for (String property : editor.getEffective().getMandatoryProperties()) { - if (!after.hasProperty(property)) { - editor.constraintViolation( - 21, "Mandatory property " + property - + " not found in a new node"); - } - } - for (String child : editor.getEffective().getMandatoryChildNodes()) { - if (!after.hasChildNode(child)) { - editor.constraintViolation( - 25, "Mandatory child node " + child - + " not found in a new node"); - } - } + public void enter(NodeState before, NodeState after) throws CommitFailedException { + if (checkThisNode && validate) { + // when adding a new node, or changing node type on an existing + // node, we need to reverify type constraints + checkNodeTypeConstraints(after); + checkThisNode = false; } + } - return editor; + @Override + public Editor childNodeAdded(String name, NodeState after) + throws CommitFailedException { + // TODO: add any auto-created items that are still missing + return childNodeChanged(name, MISSING_NODE, after); } @Override @@ -239,7 +222,6 @@ class TypeEditor extends DefaultEditor { String primary = after.getName(JCR_PRIMARYTYPE); Iterable mixins = after.getNames(JCR_MIXINTYPES); - NodeBuilder childBuilder = builder.getChildNode(name); if (primary == null && effective != null) { // no primary type defined, find and apply a default type primary = effective.getDefaultType(name); @@ -252,8 +234,11 @@ class TypeEditor extends DefaultEditor { } } - TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder); - if (checkThisNode && !getEffective().isValidChildNode(name, editor.getEffective())) { + // if node type didn't change no need to validate child node + boolean validate = primaryChanged(before, primary) || mixinsChanged(before, mixins); + NodeBuilder childBuilder = builder.getChildNode(name); + TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder, validate); + if (checkThisNode && validate && !effective.isValidChildNode(name, editor.getEffective())) { constraintViolation( 1, "No matching definition found for child node " + name + " with effective type " + editor.getEffective()); @@ -380,4 +365,79 @@ class TypeEditor extends DefaultEditor { constraintViolation(5, "Value constraint violation in " + property); } + private static boolean primaryChanged(NodeState before, String after) { + String pre = before.getName(JCR_PRIMARYTYPE); + return !Objects.equal(pre, after); + } + + private static boolean mixinsChanged(NodeState before, Iterable after) { + List pre = Lists.newArrayList(before.getNames(JCR_MIXINTYPES)); + Collections.sort(pre); + List post = Lists.newArrayList(after); + Collections.sort(post); + if (pre.isEmpty() && post.isEmpty()) { + return false; + } else if (pre.isEmpty() || post.isEmpty()) { + return true; + } else { + return !Iterables.elementsEqual(pre, post); + } + } + + private void checkNodeTypeConstraints(NodeState after) throws CommitFailedException { + EffectiveType effective = getEffective(); + + Set properties = effective.getMandatoryProperties(); + for (PropertyState ps : after.getProperties()) { + properties.remove(ps.getName()); + checkPropertyTypeConstraints(ps); + } + // verify the presence of all mandatory items + if (!properties.isEmpty()) { + constraintViolation(21, "Mandatory property " + properties.iterator().next() + " not found in a new node"); + } + + List names = Lists.newArrayList(after.getChildNodeNames()); + for (String child : effective.getMandatoryChildNodes()) { + if (!names.remove(child)) { + constraintViolation(25, "Mandatory child node " + child + " not found in a new node"); + } + } + if (!names.isEmpty()) { + for (String name : names) { + NodeState child = after.getChildNode(name); + String primary = child.getName(JCR_PRIMARYTYPE); + Iterable mixins = child.getNames(JCR_MIXINTYPES); + NodeBuilder childBuilder = builder.getChildNode(name); + TypeEditor editor = new TypeEditor(this, name, primary, mixins, childBuilder, false); + if (!effective.isValidChildNode(name, editor.getEffective())) { + constraintViolation(25, "Unexpected child node " + names + " found in a new node"); + } + } + } + } + + private void checkPropertyTypeConstraints(PropertyState after) + throws CommitFailedException { + if (NodeStateUtils.isHidden(after.getName())) { + return; + } + NodeState definition = effective.getDefinition(after); + if (definition == null) { + constraintViolation(4, "No matching property definition found for " + after); + } else if (JCR_UUID.equals(after.getName()) && effective.isNodeType(MIX_REFERENCEABLE)) { + // special handling for the jcr:uuid property of mix:referenceable + // TODO: this should be done in a pluggable extension + if (!isValidUUID(after.getValue(Type.STRING))) { + constraintViolation(12, "Invalid UUID value in the jcr:uuid property"); + } + } else { + int requiredType = getRequiredType(definition); + if (requiredType != PropertyType.UNDEFINED) { + checkRequiredType(after, requiredType); + checkValueConstraints(definition, after, requiredType); + } + } + } + } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java index 6087328..36e02ed 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/nodetype/TypeEditorTest.java @@ -19,6 +19,7 @@ package org.apache.jackrabbit.oak.plugins.nodetype; import static org.apache.jackrabbit.JcrConstants.JCR_MIXINTYPES; import static org.apache.jackrabbit.JcrConstants.JCR_PRIMARYTYPE; import static org.apache.jackrabbit.JcrConstants.NT_FOLDER; +import static org.apache.jackrabbit.JcrConstants.MIX_REFERENCEABLE; import static org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE; import static org.apache.jackrabbit.oak.plugins.nodetype.write.InitialContent.INITIAL_CONTENT; import static org.easymock.EasyMock.createControl; @@ -225,4 +226,98 @@ public class TypeEditorTest { builder.setProperty("any", 134.34, Type.DOUBLE); hook.processCommit(after, builder.getNodeState(), CommitInfo.EMPTY); } + + @Test + public void changeNodeTypeWExtraNodes() throws CommitFailedException { + EditorHook hook = new EditorHook(new TypeEditorProvider()); + + NodeState root = INITIAL_CONTENT; + NodeBuilder builder = root.builder(); + + NodeState before = builder.getNodeState(); + + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME); + builder.child("testcontent").child("unstructured_child").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", + Type.NAME); + NodeState after = builder.getNodeState(); + root = hook.processCommit(before, after, CommitInfo.EMPTY); + + builder = root.builder(); + before = builder.getNodeState(); + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME); + try { + hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY); + fail("should not be able to change node type due to extra nodes"); + } catch (CommitFailedException e) { + assertTrue(e.isConstraintViolation()); + } + } + + @Test + public void changeNodeTypeWExtraProps() throws CommitFailedException { + EditorHook hook = new EditorHook(new TypeEditorProvider()); + + NodeState root = INITIAL_CONTENT; + NodeBuilder builder = root.builder(); + + NodeState before = builder.getNodeState(); + + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME); + builder.child("testcontent").setProperty("extra", "information"); + + NodeState after = builder.getNodeState(); + root = hook.processCommit(before, after, CommitInfo.EMPTY); + + builder = root.builder(); + before = builder.getNodeState(); + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME); + try { + hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY); + fail("should not be able to change node type due to extra properties"); + } catch (CommitFailedException e) { + assertTrue(e.isConstraintViolation()); + } + } + + @Test + public void changeNodeTypeNewBroken() throws CommitFailedException { + EditorHook hook = new EditorHook(new TypeEditorProvider()); + + NodeState root = INITIAL_CONTENT; + NodeBuilder builder = root.builder(); + + NodeState before = builder.getNodeState(); + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:folder", Type.NAME); + builder.child("testcontent").setProperty("extra", "information"); + try { + hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY); + fail("should not be able to change node type due to extra properties"); + } catch (CommitFailedException e) { + assertTrue(e.isConstraintViolation()); + } + } + + @Test + public void malformedUUID() throws CommitFailedException { + EditorHook hook = new EditorHook(new TypeEditorProvider()); + + NodeState root = INITIAL_CONTENT; + NodeBuilder builder = root.builder(); + + NodeState before = builder.getNodeState(); + builder.child("testcontent").setProperty(JCR_PRIMARYTYPE, "nt:unstructured", Type.NAME); + builder.child("testcontent").setProperty("jcr:uuid", "not-a-uuid"); + NodeState after = builder.getNodeState(); + root = hook.processCommit(before, after, CommitInfo.EMPTY); + + builder = root.builder(); + before = builder.getNodeState(); + builder.child("testcontent").setProperty(JCR_MIXINTYPES, ImmutableList.of(MIX_REFERENCEABLE), Type.NAMES); + try { + hook.processCommit(before, builder.getNodeState(), CommitInfo.EMPTY); + fail("should not be able to change mixin due to illegal uuid format"); + } catch (CommitFailedException e) { + assertTrue(e.isConstraintViolation()); + } + } } 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..6dda0bf 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; @@ -377,6 +379,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 +442,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,60 +459,83 @@ 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); - - 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 (newDefinition == null - || (getBoolean(oldDefinition, JCR_PROTECTED) - && !getBoolean(newDefinition, JCR_PROTECTED))) { + purgeNode(typeRoot, removedOakMixinNames); + } + } + + /** + * 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); + + for (PropertyState property : tree.getProperties()) { + String name = property.getName(); + Type type = property.getType(); + + Tree oldDefinition = findMatchingPropertyDefinition(removed, name, type, true); + if (oldDefinition != null) { + boolean wasProtected = getBoolean(oldDefinition, JCR_PROTECTED); + Tree newDefinition = findMatchingPropertyDefinition(remaining, name, type, true); + 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); - if (newDefinition == null - || (getBoolean(oldDefinition, JCR_PROTECTED) - && !getBoolean(newDefinition, JCR_PROTECTED))) { + Tree oldDefinition = findMatchingChildNodeDefinition(removed, name, typeNames); + if (oldDefinition != null) { + boolean wasProtected = getBoolean(oldDefinition, JCR_PROTECTED); + Tree newDefinition = findMatchingChildNodeDefinition(remaining, name, typeNames); + 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 b9c3977..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,7 +1872,38 @@ public class RepositoryTest extends AbstractRepositoryTest { } @Test - @Ignore("OAK-5229") + 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()); @@ -1882,12 +1913,12 @@ 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); } Session session2 = createAnonymousSession(); 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/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java index ad01663..99eae00 100644 --- a/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java +++ b/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/authorization/NodeTypeManagementTest.java @@ -115,7 +115,7 @@ public class NodeTypeManagementTest extends AbstractEvaluationTest { String ntName = child.getPrimaryNodeType().getName(); try { - childNode.setPrimaryType("nt:folder"); + childNode.setPrimaryType("oak:Unstructured"); testSession.save(); fail("TestSession does not have sufficient privileges to change the primary type."); } catch (AccessDeniedException e) { @@ -134,7 +134,7 @@ public class NodeTypeManagementTest extends AbstractEvaluationTest { Node child = (Node) superuser.getItem(childNode.getPath()); String ntName = child.getPrimaryNodeType().getName(); - String changedNtName = "nt:folder"; + String changedNtName = "oak:Unstructured"; child.setPrimaryType(changedNtName); testSession.save(); 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 +