[oIVz2Y7V] apoc.refactor.cloneNodes does not need to be in its own tr…

…ansaction (#605)

common/src/main/java/apoc/util/Util.java

@@ -169,6 +169,19 @@ public static Label[] labels(Object labelNames) {
         return new Label[] {Label.label(labelNames.toString())};
     }
 
+    public static Label[] getLabelsArray(Node node) {
+        List<Label> labels = getLabels(node);
+        return labels.toArray(new Label[0]);
+    }
+
+    public static List<Label> getLabels(Node node) {
+        return getLabelsAsStream(node).collect(Collectors.toList());
+    }
+
+    private static Stream<Label> getLabelsAsStream(Node node) {
+        return StreamSupport.stream(node.getLabels().spliterator(), false);
+    }
+
     public static Stream<Object> stream(Object values) {
         return ConvertUtils.convertToList(values).stream();
     }

core/src/main/java/apoc/refactor/GraphRefactoring.java

@@ -58,35 +58,6 @@ public class GraphRefactoring {
     @Context
     public Pools pools;
 
-    private Stream<NodeRefactorResult> doCloneNodes(
-            @Name("nodes") List<Node> nodes,
-            @Name("withRelationships") boolean withRelationships,
-            List<String> skipProperties) {
-        if (nodes == null) return Stream.empty();
-        return nodes.stream().map(node -> Util.rebind(tx, node)).map(node -> {
-            NodeRefactorResult result = new NodeRefactorResult(node.getId());
-            try {
-                Node copy = withTransactionAndRebind(db, tx, transaction -> {
-                    Node newNode = copyLabels(node, transaction.createNode());
-
-                    Map<String, Object> properties = node.getAllProperties();
-                    if (skipProperties != null && !skipProperties.isEmpty())
-                        for (String skip : skipProperties) properties.remove(skip);
-
-                    newNode = copyProperties(properties, newNode);
-                    copyLabels(node, newNode);
-                    return newNode;
-                });
-                if (withRelationships) {
-                    copyRelationships(node, copy, false, true);
-                }
-                return result.withOther(copy);
-            } catch (Exception e) {
-                return result.withError(e);
-            }
-        });
-    }
-
     @Procedure(name = "apoc.refactor.extractNode", mode = Mode.WRITE)
     @Description("Expands the given `RELATIONSHIP` VALUES into intermediate `NODE` VALUES.\n"
             + "The intermediate `NODE` values are connected by the given `outType` and `inType`.")
@@ -154,7 +125,32 @@ public Stream<NodeRefactorResult> cloneNodes(
             @Name("nodes") List<Node> nodes,
             @Name(value = "withRelationships", defaultValue = "false") boolean withRelationships,
             @Name(value = "skipProperties", defaultValue = "[]") List<String> skipProperties) {
-        return doCloneNodes(nodes, withRelationships, skipProperties);
+        if (nodes == null) return Stream.empty();
+        return nodes.stream().map(node -> {
+            NodeRefactorResult result = new NodeRefactorResult(node.getId());
+            Node newNode = tx.createNode(Util.getLabelsArray(node));
+            Map<String, Object> properties = node.getAllProperties();
+            if (skipProperties != null && !skipProperties.isEmpty()) {
+                for (String skip : skipProperties) properties.remove(skip);
+            }
+            try {
+                copyProperties(properties, newNode);
+                if (withRelationships) {
+                    copyRelationships(node, newNode, false, true);
+                }
+            } catch (Exception e) {
+                // If there was an error, the procedure still passes, but this node + its rels should not
+                // be created. Instead, an error is returned to the user in the output.
+                if (withRelationships) {
+                    for (Relationship rel: newNode.getRelationships()) {
+                        rel.delete();
+                    }
+                }
+                newNode.delete();
+                return result.withError(e);
+            }
+            return result.withOther(newNode);
+        });
     }
 
     /**

core/src/test/java/apoc/refactor/GraphRefactoringTest.java

@@ -54,6 +54,8 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
+
+import junit.framework.TestCase;
 import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.hamcrest.Matchers;
 import org.junit.After;
@@ -1673,6 +1675,55 @@ public void testMergeRelsTrueAndProduceSelfRelFalse() {
                 });
     }
 
+    @Test
+    public void issue3960WithCloneNodes() {
+    // Any node created in cloneNodes should not be committed if the entire query fails.
+        String query = """
+                CREATE (original:Person {uid: "original"}), (original2:Person {uid: "original"})
+                WITH original, original2
+                CALL apoc.refactor.cloneNodes([original, original2], false, ["uid"])
+                YIELD input, output AS clone, error
+                SET clone.uid = "clone"
+                RETURN 1/0
+                """;
+
+        QueryExecutionException e = assertThrows(
+                QueryExecutionException.class,
+                () -> testCall(
+                        db,
+                        query,
+                        (r) -> {}));
+        Throwable except = ExceptionUtils.getRootCause(e);
+        TestCase.assertTrue(except instanceof RuntimeException);
+        TestCase.assertEquals("/ by zero", except.getMessage());
+
+        testCall(db, "MATCH (n) RETURN count(*) AS count", r -> assertEquals(0L, r.get("count")));
+    }
+
+    @Test
+    public void ShouldErrorOnConstraintsFailedCommon() {
+        db.executeTransactionally(("CREATE CONSTRAINT unique_id FOR ()-[r:HAS_PET]-() REQUIRE r.id IS UNIQUE"));
+
+        String query = """
+                CREATE (a:Person {name: 'Mark', city: 'London'})-[:HAS_PET {id: 1}]->(:Cat {name: "Mittens"})
+                WITH a
+                CALL apoc.refactor.cloneNodes([a], true, ["city"])
+                YIELD input, output, error
+                RETURN *
+                """;
+
+        testCall(db, query, r -> {
+            final String actualError = (String) r.get("error");
+            assertTrue(actualError.contains("already exists with type `HAS_PET` and property `id` = 1"));
+            assertNull(r.get("output"));
+        });
+
+        testCall(db, "MATCH (n) RETURN count(*) AS count", r -> assertEquals(2L, r.get("count")));
+
+        db.executeTransactionally("DROP CONSTRAINT unique_id");
+        db.executeTransactionally("MATCH (n) DETACH DELETE n");
+    }
+
     @Test
     public void issue2797WithCloneNodes() {
         issue2797Common(CLONE_NODES_QUERY);
@@ -1705,6 +1756,10 @@ private void issue2797Common(String extractQuery) {
             assertEquals(expected, r.get("props"));
         });
 
+        testCall(db, "MATCH (n:MyBook) RETURN count(n) as count", r -> {
+            assertEquals(1L, r.get("count"));
+        });
+
         db.executeTransactionally("DROP CONSTRAINT unique_book");
         db.executeTransactionally("MATCH (n:MyBook) DELETE n");
     }