diff --git a/core/src/main/java/apoc/refactor/GraphRefactoring.java b/core/src/main/java/apoc/refactor/GraphRefactoring.java index da962a9c3..c387e8ebb 100644 --- a/core/src/main/java/apoc/refactor/GraphRefactoring.java +++ b/core/src/main/java/apoc/refactor/GraphRefactoring.java @@ -475,32 +475,89 @@ public Stream setType( @Description("Redirects the given `RELATIONSHIP` to the given end `NODE`.") public Stream to( @Name(value = "rel", description = "The relationship to redirect.") Relationship rel, - @Name(value = "endNode", description = "The new end node the relationship should point to.") Node newNode) { + @Name(value = "endNode", description = "The new end node the relationship should point to.") Node newNode, + @Name( + value = "config", + defaultValue = "{}", + description = + """ + { + failOnErrors = false :: BOOLEAN + } + """) + Map config) { + if (config == null) config = Map.of(); if (rel == null || newNode == null) return Stream.empty(); - UpdatedRelationshipResult result = new UpdatedRelationshipResult(rel.getId()); + + final var failOnErrors = Boolean.TRUE.equals(config.getOrDefault("failOnErrors", false)); + final var result = new UpdatedRelationshipResult(rel.getId()); + try { - Relationship newRel = rel.getStartNode().createRelationshipTo(newNode, rel.getType()); - copyProperties(rel, newRel); + final var type = rel.getType(); + final var properties = rel.getAllProperties(); + final var startNode = rel.getStartNode(); + + // Delete first to not break constraints. rel.delete(); + + final var newRel = startNode.createRelationshipTo(newNode, 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 ever 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)); + } } } @Procedure(name = "apoc.refactor.invert", mode = Mode.WRITE, eager = true) @Description("Inverts the direction of the given `RELATIONSHIP`.") public Stream invert( - @Name(value = "rel", description = "The relationship to reverse.") Relationship rel) { + @Name(value = "rel", description = "The relationship to reverse.") Relationship rel, + @Name( + value = "config", + defaultValue = "{}", + description = + """ + { + failOnErrors = false :: BOOLEAN + } + """) + Map config) { + if (config == null) config = Map.of(); if (rel == null) return Stream.empty(); - RefactorRelationshipResult result = new RefactorRelationshipResult(rel.getId()); + + final var failOnErrors = Boolean.TRUE.equals(config.getOrDefault("failOnErrors", false)); + final var result = new RefactorRelationshipResult(rel.getId()); + try { - Relationship newRel = rel.getEndNode().createRelationshipTo(rel.getStartNode(), rel.getType()); - copyProperties(rel, newRel); + final var type = rel.getType(); + final var properties = rel.getAllProperties(); + final var startNode = rel.getStartNode(); + final var endNode = rel.getEndNode(); + + // Delete first to not break constraints. rel.delete(); + + final var newRel = endNode.createRelationshipTo(startNode, 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 ever 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)); + } } } @@ -511,16 +568,44 @@ public Stream invert( @Description("Redirects the given `RELATIONSHIP` to the given start `NODE`.") public Stream 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 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 ever 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)); + } } } @@ -789,37 +874,21 @@ private void mergeNodes(Node source, Node target, RefactorConfig conf, List(b:B {id:'B'}), (c:C {id:'C'})"); + db.executeTransactionally("CALL db.awaitIndexes()"); + + try (final var tx = db.beginTx()) { + final var refactorToQuery = + "MATCH (a:A)-[r:R]->(), (c:C) CALL apoc.refactor.to(r, c) YIELD error RETURN error"; + assertThat(tx.execute(refactorToQuery).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", "R", "b.id", "C"), + Map.of("a.id", "B", "r.id", "null", "b.id", "null"), + Map.of("a.id", "C", "r.id", "null", "b.id", "null")); + tx.commit(); + } + } + + @Test + public void refactorToWithErrorHandling() { + 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.to(r, c, $conf) YIELD error + RETURN error"""; + try (final var tx = db.beginTx()) { + final var params = Map.of("conf", Map.of("failOnErrors", true)); + assertThatThrownBy(() -> tx.execute(refactorQuery, params).resultAsString()) + .hasMessageContaining( + "Failed to invoke procedure `apoc.refactor.to`: Caused by: org.neo4j.internal.kernel.api.exceptions.EntityNotFoundException"); + 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 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.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"); + db.executeTransactionally("CREATE (a:A {id:'A'})-[r:R {id:'R'}]->(b:B {id:'B'})"); + db.executeTransactionally("CALL db.awaitIndexes()"); + + try (final var tx = db.beginTx()) { + assertThat(tx.execute("MATCH ()-[r:R]->() CALL apoc.refactor.invert(r) 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", "R", "b.id", "A")); + tx.commit(); + } + } + + @Test + public void invertErrorHandling() { + db.executeTransactionally("CREATE (a:A {id:'A'})-[r:R {id:'R'}]->(b:B {id:'B'})"); + + final var invertQuery = + "MATCH ()-[r:R]->() DELETE r WITH r CALL apoc.refactor.invert(r, $conf) YIELD error RETURN error"; + try (final var tx = db.beginTx()) { + final var params = Map.of("conf", Map.of("failOnErrors", true)); + assertThatThrownBy(() -> tx.execute(invertQuery, params).resultAsString()) + .hasMessageContaining( + "Failed to invoke procedure `apoc.refactor.invert`: 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")); + tx.commit(); + } + + try (final var tx = db.beginTx()) { + assertThat(tx.execute(invertQuery, 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 mergeNodesWithConstraints() { + db.executeTransactionally("CREATE CONSTRAINT foo_uniq FOR ()-[r:MY_REL]-() REQUIRE r.foo IS UNIQUE"); + db.executeTransactionally( + """ + CREATE + (n1 {name: "n1"})-[r1:MY_REL {foo: "a"}]->(n2 {name: "n2"}), + (n3 {name: "n3"})-[r2:MY_REL {foo: "b"}]->(n4 {name: "n4"})"""); + db.executeTransactionally("CALL db.awaitIndexes()"); + db.executeTransactionally( + """ + MATCH (n1 {name: "n1"}), (n3 {name: "n3"}) + CALL apoc.refactor.mergeNodes([n1, n3], {properties: 'discard'}) YIELD node + FINISH"""); + + try (final var tx = db.beginTx()) { + final var query = + """ + MATCH (a) + OPTIONAL MATCH (a)-[r]->(b) + RETURN + a.name, + CASE WHEN r.foo IS NULL THEN 'null' ELSE r.foo END AS `r.foo`, + CASE WHEN b.name IS NULL THEN 'null' ELSE b.name END AS `b.name` + ORDER BY `a.name`, `r.foo`, `b.name` + """; + assertThat(tx.execute(query).stream()) + .containsExactly( + Map.of("a.name", "n1", "r.foo", "a", "b.name", "n2"), + Map.of("a.name", "n1", "r.foo", "b", "b.name", "n4"), + Map.of("a.name", "n2", "r.foo", "null", "b.name", "null"), + Map.of("a.name", "n4", "r.foo", "null", "b.name", "null")); + tx.commit(); + } + } + private void issue2797Common(String extractQuery) { db.executeTransactionally("CREATE CONSTRAINT unique_book FOR (book:MyBook) REQUIRE book.name IS UNIQUE"); db.executeTransactionally("CREATE (n:MyBook {name: 1})");