[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #159: JCRVLT-551 try to set UUID from package

[GitBox <gi...@apache.org>]

kwin commented on a change in pull request #159:
URL: https://github.com/apache/jackrabbit-filevault/pull/159#discussion_r694557448



##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewSAXImporter.java
##########
@@ -809,89 +813,47 @@ private void handleAuthorizable(Node node, DocViewNode ni) throws RepositoryExce
     private StackElement addNode(DocViewNode ni) throws RepositoryException, IOException {
         final Node currentNode = stack.getNode();
 
-        // find old node
-        Node oldNode = null;
-        Node node = null;
+        Node existingNode = null;
         if ("".equals(ni.label)) {
             // special case for root node update
-            node = currentNode;
-        } else if (ni.uuid == null) {
-            if (stack.checkForNode() && currentNode.hasNode(ni.label)) {
-                node = currentNode.getNode(ni.label);
-            }
+            existingNode = currentNode;
         } else {
-            try {
-                node = session.getNodeByUUID(ni.uuid);
-                if (!node.getParent().isSame(currentNode)) {
-                    log.warn("Packaged node at {} is referenceable and collides with existing node at {}. Will create new UUID.",
-                            currentNode.getPath() + "/" + ni.label,
-                            node.getPath());
-                    ni.uuid = null;
-                    ni.props.remove(JcrConstants.JCR_UUID);
-                    ni.props.remove(JcrConstants.JCR_BASEVERSION);
-                    ni.props.remove(JcrConstants.JCR_PREDECESSORS);
-                    ni.props.remove(JcrConstants.JCR_SUCCESSORS);
-                    ni.props.remove(JcrConstants.JCR_VERSIONHISTORY);
-                    node = null;
-                }
-            } catch (ItemNotFoundException e) {
-                // ignore
-            }
-            if (node == null) {
-                if (stack.checkForNode() && currentNode.hasNode(ni.label)) {
-                    node = currentNode.getNode(ni.label);
-                }
-            } else {
-                if (!node.getName().equals(ni.name)) {
-                    // if names mismatches => replace
-                    oldNode = node;
-                    node = null;
-                }
-
+            if (stack.checkForNode() && currentNode.hasNode(ni.label)) {
+                existingNode = currentNode.getNode(ni.label);
             }
-        }
-        // if old node is not included in the package, ignore rewrite
-        if (oldNode != null && !isIncluded(oldNode, oldNode.getDepth() - rootDepth)) {
-            node = oldNode;
-            oldNode = null;
-        }
-
-        // TODO: under which condition do we end up here?
-        if (oldNode != null) {
-            // check versionable
-            new VersioningState(stack, oldNode).ensureCheckedOut();
-
-            String oldNodePath = oldNode.getPath();
-            ImportMode importMode = wspFilter.getImportMode(oldNodePath);
-            NodeStash recovery = new NodeStash(session, oldNodePath, importMode);
-            recovery.stash();
-
-            // ensure that existing binaries are not sourced from a property
-            // that is about to be removed
-            Map<String, DocViewSAXImporter.BlobInfo> blobs = binaries.get(oldNode.getPath());
-            if (blobs != null) {
-                for (DocViewSAXImporter.BlobInfo info : blobs.values()) {
-                    info.detach();
+            if (ni.uuid != null) {
+                if (idConflictPolicy == IdConflictPolicy.FAIL) {
+                    try {
+                        // does uuid already exist in the repo?
+                        Node sameIdNode = session.getNodeByIdentifier(ni.uuid);
+                        // edge-case: same node path -> uuid is kept
+                        if (existingNode != null && existingNode.getPath().equals(sameIdNode.getPath())) {
+                            log.debug("Node with existing identifier is being updated without modifying its uuid");
+                        } else {
+                            // uuid found in path covered by filter
+                            if (isIncluded(sameIdNode, 0)) {
+                                log.warn("Node identifier {} for to-be imported node {} already taken by {}, trying to release it.", ni.uuid, currentNode.getPath() + '/' + ni.label, sameIdNode.getPath());
+                                removeReferences(sameIdNode);

Review comment:
       IMHO if a node with a conflicting id already exists in the repo at a different path, one should either
   - not touch it (if not contained in filter rules)
   - remove it and its references (if contained in the filter rules)
   Just assuming that this node should be moved because its id is the same is very dangerous as conflicts may occur for totally unrelated nodes (although unlikely)




-- 
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-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org