mreutegg commented on code in PR #293:
URL: https://github.com/apache/jackrabbit-oak/pull/293#discussion_r1315662326


##########
oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/xml/ImporterImpl.java:
##########
@@ -336,138 +339,150 @@ public void startNode(@NotNull NodeInfo nodeInfo, 
@NotNull List<PropInfo> propIn
             throws RepositoryException {
         Tree parent = parents.peek();
         Tree tree = null;
-        String id = nodeInfo.getUUID();
-        String nodeName = nodeInfo.getName();
-        String ntName = nodeInfo.getPrimaryTypeName();
-
-        if (parent == null) {
-            log.debug("Skipping node: {}", nodeName);
-            // parent node was skipped, skip this child node too
-            parents.push(null); // push null onto stack for skipped node
-            // notify the p-i-importer
-            if (pnImporter != null) {
-                pnImporter.startChildInfo(nodeInfo, propInfos);
-            }
-            return;
-        }
-
-        NodeDefinition parentDef = getDefinition(parent);
-        if (parentDef.isProtected()) {
-            // skip protected node
-            parents.push(null);
-            log.debug("Skipping protected node: {}", nodeName);
-
-            if (pnImporter != null) {
-                // pnImporter was already started (current nodeInfo is a 
sibling)
-                // notify it about this child node.
-                pnImporter.startChildInfo(nodeInfo, propInfos);
-            } else {
-                // no importer defined yet:
-                // test if there is a ProtectedNodeImporter among the 
configured
-                // importers that can handle this.
-                // if there is one, notify the ProtectedNodeImporter about the
-                // start of a item tree that is protected by this parent. If it
-                // potentially is able to deal with it, notify it about the 
child node.
-                for (ProtectedNodeImporter pni : getNodeImporters()) {
-                    if (pni.start(parent)) {
-                        log.debug("Protected node -> delegated to 
ProtectedNodeImporter");
-                        pnImporter = pni;
-                        pnImporter.startChildInfo(nodeInfo, propInfos);
-                        break;
-                    } /* else: p-i-Importer isn't able to deal with the 
protected tree.
-                     try next. and if none can handle the passed parent the
-                     tree below will be skipped */
+        try {
+            String id = nodeInfo.getUUID();
+            String nodeName = nodeInfo.getName();
+            String ntName = nodeInfo.getPrimaryTypeName();
+
+            if (parent == null) {
+                log.debug("Skipping node: {}", nodeName);
+                // parent node was skipped, skip this child node too
+                parents.push(null); // push null onto stack for skipped node
+                // notify the p-i-importer
+                if (pnImporter != null) {
+                    pnImporter.startChildInfo(nodeInfo, propInfos);
                 }
+                return;
             }
-            return;
-        }
 
-        if (parent.hasChild(nodeName)) {
-            // a node with that name already exists...
-            Tree existing = parent.getChild(nodeName);
-            NodeDefinition def = getDefinition(existing);
-            if (!def.allowsSameNameSiblings()) {
-                // existing doesn't allow same-name siblings,
-                // check for potential conflicts
-                if (def.isProtected() && isNodeType(existing, ntName)) {
-                    /*
-                     use the existing node as parent for the possible 
subsequent
-                     import of a protected tree, that the protected node 
importer
-                     may or may not be able to deal with.
-                     -> upon the next 'startNode' the check for the parent 
being
-                        protected will notify the protected node importer.
-                     -> if the importer is able to deal with that node it needs
-                        to care of the complete subtree until it is notified
-                        during the 'endNode' call.
-                     -> if the import can't deal with that node or if that node
-                        is the a leaf in the tree to be imported 'end' will
-                        not have an effect on the importer, that was never 
started.
-                    */
-                    log.debug("Skipping protected node: {}", existing);
-                    parents.push(existing);
-                    /**
-                     * let ProtectedPropertyImporters handle the properties
-                     * associated with the imported node. this may include 
overwriting,
-                     * merging or just adding missing properties.
-                     */
-                    importProperties(existing, propInfos, true);
-                    return;
-                }
-                if (def.isAutoCreated() && isNodeType(existing, ntName)) {
-                    // this node has already been auto-created, no need to 
create it
-                    tree = existing;
+            NodeDefinition parentDef = getDefinition(parent);
+            if (parentDef.isProtected()) {
+                // skip protected node
+                parents.push(null);
+                log.debug("Skipping protected node: {}", nodeName);
+
+                if (pnImporter != null) {
+                    // pnImporter was already started (current nodeInfo is a 
sibling)
+                    // notify it about this child node.
+                    pnImporter.startChildInfo(nodeInfo, propInfos);
                 } else {
-                    // edge case: colliding node does have same uuid
-                    // (see http://issues.apache.org/jira/browse/JCR-1128)
-                    String existingIdentifier = 
IdentifierManager.getIdentifier(existing);
-                    if (!(existingIdentifier.equals(id)
-                            && (uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING
-                            || uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING))) {
-                        throw new ItemExistsException(
-                                "Node with the same UUID exists:" + existing);
+                    // no importer defined yet:
+                    // test if there is a ProtectedNodeImporter among the 
configured
+                    // importers that can handle this.
+                    // if there is one, notify the ProtectedNodeImporter about 
the
+                    // start of a item tree that is protected by this parent. 
If it
+                    // potentially is able to deal with it, notify it about 
the child node.
+                    for (ProtectedNodeImporter pni : getNodeImporters()) {
+                        if (pni.start(parent)) {
+                            log.debug("Protected node -> delegated to 
ProtectedNodeImporter");
+                            pnImporter = pni;
+                            pnImporter.startChildInfo(nodeInfo, propInfos);
+                            break;
+                        } /* else: p-i-Importer isn't able to deal with the 
protected tree.
+                         try next. and if none can handle the passed parent the
+                         tree below will be skipped */
                     }
-                    // fall through
                 }
+                return;
             }
-        }
 
-        if (tree == null) {
-            // create node
-            if (id == null) {
-                // no potential uuid conflict, always add new node
-                tree = createTree(parent, nodeInfo, null);
-            } else if (uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_CREATE_NEW) {
-                // always create a new UUID even if no
-                // conflicting node exists. see OAK-1244
-                tree = createTree(parent, nodeInfo, 
UUID.randomUUID().toString());
-                // remember uuid mapping
-                if (isNodeType(tree, JcrConstants.MIX_REFERENCEABLE)) {
-                    refTracker.put(nodeInfo.getUUID(), 
TreeUtil.getString(tree, JcrConstants.JCR_UUID));
+            if (parent.hasChild(nodeName)) {
+                // a node with that name already exists...
+                Tree existing = parent.getChild(nodeName);
+                NodeDefinition def = getDefinition(existing);
+                if (!def.allowsSameNameSiblings()) {
+                    // existing doesn't allow same-name siblings,
+                    // check for potential conflicts
+                    if (def.isProtected() && isNodeType(existing, ntName)) {
+                        /*
+                         use the existing node as parent for the possible 
subsequent
+                         import of a protected tree, that the protected node 
importer
+                         may or may not be able to deal with.
+                         -> upon the next 'startNode' the check for the parent 
being
+                            protected will notify the protected node importer.
+                         -> if the importer is able to deal with that node it 
needs
+                            to care of the complete subtree until it is 
notified
+                            during the 'endNode' call.
+                         -> if the import can't deal with that node or if that 
node
+                            is the a leaf in the tree to be imported 'end' will
+                            not have an effect on the importer, that was never 
started.
+                        */
+                        log.debug("Skipping protected node: {}", existing);
+                        parents.push(existing);
+                        /**
+                         * let ProtectedPropertyImporters handle the properties
+                         * associated with the imported node. this may include 
overwriting,
+                         * merging or just adding missing properties.
+                         */
+                        importProperties(existing, propInfos, true);
+                        return;
+                    }
+                    if (def.isAutoCreated() && isNodeType(existing, ntName)) {
+                        // this node has already been auto-created, no need to 
create it
+                        tree = existing;
+                    } else {
+                        // edge case: colliding node does have same uuid
+                        // (see http://issues.apache.org/jira/browse/JCR-1128)
+                        String existingIdentifier = 
IdentifierManager.getIdentifier(existing);
+                        if (!(existingIdentifier.equals(id)
+                                && (uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING
+                                || uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING))) {
+                            throw new ItemExistsException(
+                                    "Node with the same UUID exists:" + 
existing);
+                        }
+                        // fall through
+                    }
                 }
-            } else {
+            }
 
-                Tree conflicting = idLookup.getConflictingTree(id);
-                if (conflicting != null && conflicting.exists()) {
-                    // resolve uuid conflict
-                    tree = resolveUUIDConflict(parent, conflicting, id, 
nodeInfo);
-                    if (tree == null) {
-                        // no new node has been created, so skip this node
-                        parents.push(null); // push null onto stack for 
skipped node
-                        log.debug("Skipping existing node {}", 
nodeInfo.getName());
-                        return;
+            if (tree == null) {
+                // create node
+                if (id == null) {
+                    // no potential uuid conflict, always add new node
+                    tree = createTree(parent, nodeInfo, null);
+                } else if (uuidBehavior == 
ImportUUIDBehavior.IMPORT_UUID_CREATE_NEW) {
+                    // always create a new UUID even if no
+                    // conflicting node exists. see OAK-1244
+                    tree = createTree(parent, nodeInfo, 
UUID.randomUUID().toString());
+                    // remember uuid mapping
+                    if (isNodeType(tree, JcrConstants.MIX_REFERENCEABLE)) {
+                        refTracker.put(nodeInfo.getUUID(), 
TreeUtil.getString(tree, JcrConstants.JCR_UUID));
                     }
                 } else {
-                    // create new with given uuid
-                    tree = createTree(parent, nodeInfo, id);
+
+                    Tree conflicting = idLookup.getConflictingTree(id);
+                    if (conflicting != null && conflicting.exists()) {
+                        // resolve uuid conflict
+                        tree = resolveUUIDConflict(parent, conflicting, id, 
nodeInfo);
+                        if (tree == null) {
+                            // no new node has been created, so skip this node
+                            parents.push(null); // push null onto stack for 
skipped node
+                            log.debug("Skipping existing node {}", 
nodeInfo.getName());
+                            return;
+                        }
+                    } else {
+                        // create new with given uuid
+                        tree = createTree(parent, nodeInfo, id);
+                    }
                 }
             }
-        }
 
-        // process properties
-        importProperties(tree, propInfos, false);
+            // process properties
+            importProperties(tree, propInfos, false);
 
-        if (tree.exists()) {
-            parents.push(tree);
+            if (tree.exists()) {
+                EffectiveNodeType parentEnt = 
effectiveNodeTypeProvider.getEffectiveNodeType(tree.getParent());
+                EffectiveNodeType ent = 
effectiveNodeTypeProvider.getEffectiveNodeType(tree);
+                // this throws ConstraintViolationException
+                parentEnt.getNodeDefinition(tree.getName(), ent);
+                parents.push(tree);
+            }
+        } catch (ConstraintViolationException e) {

Review Comment:
   Actually GitHub can. Just add ?w=1 to the URL or check the 'Hide whitespace' 
box in the Diff view settings.



-- 
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

Reply via email to