Fix bug in apoc.refactor.from

- Better handling of constraints.
- Introduce option to fail on errors.

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

@@ -570,16 +570,44 @@ public Stream<RefactorRelationshipResult> invert(
     @Description("Redirects the given `RELATIONSHIP` to the given start `NODE`.")
     public Stream<RefactorRelationshipResult> from(
             @Name(value = "rel", description = "The relationship to redirect.") Relationship rel,
-            @Name(value = "newNode", description = "The node to redirect the given relationship to.") Node newNode) {
+            @Name(value = "newNode", description = "The node to redirect the given relationship to.") Node newNode,
+            @Name(
+                            value = "config",
+                            defaultValue = "{}",
+                            description =
+                                    """
+                    {
+                        failOnErrors = false :: BOOLEAN
+                    }
+                    """)
+                    Map<String, Object> config) {
+        if (config == null) config = Map.of();
         if (rel == null || newNode == null) return Stream.empty();
-        RefactorRelationshipResult result = new RefactorRelationshipResult(rel.getId());
+
+        final var result = new RefactorRelationshipResult(rel.getId());
+        final var failOnErrors = Boolean.TRUE.equals(config.getOrDefault("failOnErrors", false));
+
         try {
-            Relationship newRel = newNode.createRelationshipTo(rel.getEndNode(), rel.getType());
-            copyProperties(rel, newRel);
+            final var type = rel.getType();
+            final var properties = rel.getAllProperties();
+            final var endNode = rel.getEndNode();
+
+            // Delete before setting properties to not break constraints.
             rel.delete();
+
+            final var newRel = newNode.createRelationshipTo(endNode, type);
+            properties.forEach(newRel::setProperty);
+
             return Stream.of(result.withOther(newRel));
         } catch (Exception e) {
-            return Stream.of(result.withError(e));
+            if (failOnErrors) {
+                throw e;
+            } else {
+                // Note! We might now have half applied the changes, not sure why we would want to do this instead of
+                // just failing.
+                // I guess it's up to the user to explicitly rollback at this point ¯\_(ツ)_/¯.
+                return Stream.of(result.withError(e));
+            }
         }
     }
 

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

@@ -1807,6 +1807,88 @@ OPTIONAL MATCH (a)-[r]->(b)
         }
     }
 
+    @Test
+    public void refactorFromWithConstraints() {
+        db.executeTransactionally("CREATE CONSTRAINT id_unique FOR ()-[r:R]-() REQUIRE r.id IS UNIQUE");
+        db.executeTransactionally("CREATE (a:A {id:'A'})-[r:R {id:'R'}]->(b:B {id:'B'}), (c:C {id:'C'})");
+        db.executeTransactionally("CALL db.awaitIndexes()");
+
+        try (final var tx = db.beginTx()) {
+            assertThat(tx
+                            .execute(
+                                    "MATCH (a:A)-[r:R]->(), (c:C) CALL apoc.refactor.from(r, c) YIELD error RETURN error")
+                            .stream())
+                    .singleElement(InstanceOfAssertFactories.map(String.class, Object.class))
+                    .containsEntry("error", null);
+            tx.commit();
+        }
+        try (final var tx = db.beginTx()) {
+            final var query =
+                    """
+                    MATCH (a)
+                    OPTIONAL MATCH (a)-[r]->(b)
+                    RETURN
+                      a.id,
+                      CASE WHEN r.id IS NULL THEN 'null' ELSE r.id END AS `r.id`,
+                      CASE WHEN b.id IS NULL THEN 'null' ELSE b.id END AS `b.id`
+                    ORDER BY `a.id`, `r.id`, `b.id`
+                    """;
+            assertThat(tx.execute(query).stream())
+                    .containsExactly(
+                            Map.of("a.id", "A", "r.id", "null", "b.id", "null"),
+                            Map.of("a.id", "B", "r.id", "null", "b.id", "null"),
+                            Map.of("a.id", "C", "r.id", "R", "b.id", "B"));
+            tx.commit();
+        }
+    }
+
+    @Test
+    public void refactorFromWithErrorHandling() {
+        db.executeTransactionally("CREATE (a:A {id:'A'})-[r:R {id:'R'}]->(b:B {id:'B'}), (c:C {id:'C'})");
+
+        final var refactorQuery =
+                """
+                   MATCH (a:A)-[r:R]->(), (c:C)
+                   DELETE r WITH r, c
+                   CALL apoc.refactor.from(r, c, $conf) YIELD error
+                   RETURN error""";
+        try (final var tx = db.beginTx()) {
+            final var params = Map.<String, Object>of("conf", Map.of("failOnErrors", true));
+            assertThatThrownBy(() -> tx.execute(refactorQuery, params).resultAsString())
+                    .hasMessageContaining(
+                            "Failed to invoke procedure `apoc.refactor.from`: Caused by: org.neo4j.internal.kernel.api.exceptions.EntityNotFoundException")
+                    .hasRootCauseExactlyInstanceOf(
+                            org.neo4j.internal.kernel.api.exceptions.EntityNotFoundException.class);
+            tx.rollback();
+        }
+
+        try (final var tx = db.beginTx()) {
+            final var query =
+                    """
+                    MATCH (a)
+                    OPTIONAL MATCH (a)-[r]->(b)
+                    RETURN
+                      a.id,
+                      CASE WHEN r.id IS NULL THEN 'null' ELSE r.id END AS `r.id`,
+                      CASE WHEN b.id IS NULL THEN 'null' ELSE b.id END AS `b.id`
+                    ORDER BY `a.id`, `r.id`, `b.id`
+                    """;
+            assertThat(tx.execute(query).stream())
+                    .containsExactly(
+                            Map.of("a.id", "A", "r.id", "R", "b.id", "B"),
+                            Map.of("a.id", "B", "r.id", "null", "b.id", "null"),
+                            Map.of("a.id", "C", "r.id", "null", "b.id", "null"));
+            tx.commit();
+        }
+
+        try (final var tx = db.beginTx()) {
+            assertThat(tx.execute(refactorQuery, Map.of("conf", Map.of())).stream())
+                    .singleElement(InstanceOfAssertFactories.map(String.class, Object.class))
+                    .hasEntrySatisfying("error", e -> assertThat(e).asString().contains("EntityNotFoundException"));
+            tx.rollback();
+        }
+    }
+
     @Test
     public void invertWithConstraints() {
         db.executeTransactionally("CREATE CONSTRAINT id_unique FOR ()-[r:R]-() REQUIRE r.id IS UNIQUE");