Skip to content

Commit

Permalink
Fixes #2797 and #2780: Graph Refactoring procedures internally handle…
Browse files Browse the repository at this point in the history
… errors instead of throwing them, leaving half-created results (#2845) (#2921)
  • Loading branch information
neo4j-oss-build authored May 20, 2022
1 parent 919672c commit 067b113
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 19 deletions.
47 changes: 29 additions & 18 deletions core/src/main/java/apoc/refactor/GraphRefactoring.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

import static apoc.refactor.util.PropertiesManager.mergeProperties;
import static apoc.refactor.util.RefactorConfig.RelationshipSelectionStrategy.MERGE;
import static apoc.refactor.util.RefactorUtil.*;
import static apoc.util.Util.withTransactionAndRebind;

public class GraphRefactoring {
@Context
Expand All @@ -45,16 +47,20 @@ private Stream<NodeRefactorResult> doCloneNodes(@Name("nodes") List<Node> nodes,
return nodes.stream().map(node -> Util.rebind(tx, node)).map(node -> {
NodeRefactorResult result = new NodeRefactorResult(node.getId());
try {
Node newNode = copyLabels(node, tx.createNode());
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);
Map<String, Object> properties = node.getAllProperties();
if (skipProperties != null && !skipProperties.isEmpty())
for (String skip : skipProperties) properties.remove(skip);

Node copy = copyProperties(properties, newNode);
if (withRelationships) {
copyRelationships(node, copy, false, true);
}
newNode = copyProperties(properties, newNode);
copyLabels(node, newNode);
if (withRelationships) {
copyRelationships(node, newNode, false, true);
}
return newNode;
});
return result.withOther(copy);
} catch (Exception e) {
return result.withError(e);
Expand All @@ -68,8 +74,11 @@ public Stream<NodeRefactorResult> extractNode(@Name("relationships") Object rels
return Util.relsStream(tx, rels).map((rel) -> {
NodeRefactorResult result = new NodeRefactorResult(rel.getId());
try {
Node copy = copyProperties(rel, tx.createNode(Util.labels(labels)));
copy.createRelationshipTo(rel.getEndNode(), RelationshipType.withName(outType));
Node copy = withTransactionAndRebind(db, tx, transaction -> {
Node copyNode = copyProperties(rel, transaction.createNode(Util.labels(labels)));
copyNode.createRelationshipTo(rel.getEndNode(), RelationshipType.withName(outType));
return copyNode;
});
rel.getStartNode().createRelationshipTo(copy, RelationshipType.withName(inType));
rel.delete();
return result.withOther(copy);
Expand Down Expand Up @@ -209,14 +218,16 @@ public Stream<NodeRefactorResult> cloneSubgraph(@Name("nodes") List<Node> nodes,

NodeRefactorResult result = new NodeRefactorResult(node.getId());
try {
Node copy = copyLabels(node, tx.createNode());

Map<String, Object> properties = node.getAllProperties();
if (skipProperties != null && !skipProperties.isEmpty()) {
for (String skip : skipProperties) properties.remove(skip);
}
copy = copyProperties(properties, copy);

Node copy = withTransactionAndRebind(db, tx, transaction -> {
Node copyTemp = transaction.createNode();
Map<String, Object> properties = node.getAllProperties();
if (skipProperties != null && !skipProperties.isEmpty()) {
for (String skip : skipProperties) properties.remove(skip);
}
copyProperties(properties, copyTemp);
copyLabels(node, copyTemp);
return copyTemp;
});
resultStream.add(result.withOther(copy));
copyMap.put(node, copy);
} catch (Exception e) {
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/apoc/util/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.neo4j.internal.kernel.api.security.SecurityContext;
import org.neo4j.kernel.internal.GraphDatabaseAPI;
import org.neo4j.logging.Log;
import org.neo4j.logging.NullLog;
import org.neo4j.procedure.TerminationGuard;
import org.neo4j.values.storable.CoordinateReferenceSystem;
import org.neo4j.values.storable.PointValue;
Expand Down Expand Up @@ -995,4 +996,9 @@ public static String toCypherMap(Map<String, Object> map) {
final StringBuilder builder = formatProperties(map);
return "{" + formatToString(builder) + "}";
}

public static <T extends Entity> T withTransactionAndRebind(GraphDatabaseService db, Transaction transaction, Function<Transaction, T> action) {
T result = retryInTx(NullLog.getInstance(), db, action, 0, 0, r -> {});
return rebind(transaction, result);
}
}
114 changes: 114 additions & 0 deletions core/src/test/java/apoc/refactor/GraphRefactoringEnterpriseTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package apoc.refactor;

import apoc.util.Neo4jContainerExtension;
import apoc.util.TestUtil;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.neo4j.driver.Record;
import org.neo4j.driver.Session;
import org.neo4j.driver.internal.value.NullValue;

import java.util.Map;

import static apoc.refactor.GraphRefactoringTest.CLONE_NODES_QUERY;
import static apoc.refactor.GraphRefactoringTest.CLONE_SUBGRAPH_QUERY;
import static apoc.refactor.GraphRefactoringTest.EXTRACT_QUERY;
import static apoc.util.TestContainerUtil.createEnterpriseDB;
import static apoc.util.TestContainerUtil.testCall;
import static apoc.util.TestUtil.isRunningInCI;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeFalse;
import static org.junit.Assume.assumeNotNull;
import static org.junit.Assume.assumeTrue;

public class GraphRefactoringEnterpriseTest {
private static final String CREATE_REL_FOR_EXTRACT_NODE = "CREATE (:Start)-[r:TO_MOVE {name: 'foobar', surname: 'baz'}]->(:End)";
private static final String DELETE_REL_FOR_EXTRACT_NODE = "MATCH p=(:Start)-[r:TO_MOVE]->(:End) DELETE p";
private static Neo4jContainerExtension neo4jContainer;
private static Session session;

@BeforeClass
public static void beforeAll() {
assumeFalse(isRunningInCI());
TestUtil.ignoreException(() -> {
neo4jContainer = createEnterpriseDB(true);
neo4jContainer.start();
}, Exception.class);
assumeNotNull(neo4jContainer);
assumeTrue("Neo4j Instance should be up-and-running", neo4jContainer.isRunning());
session = neo4jContainer.getSession();
}

@AfterClass
public static void afterAll() {
if (neo4jContainer != null && neo4jContainer.isRunning()) {
session.close();
neo4jContainer.close();
}
}

@Test
public void testCloneNodesWithNodeKeyConstraint() {
nodeKeyCommons(CLONE_NODES_QUERY);
}

@Test
public void testCloneNodesWithBothExistenceAndUniqueConstraint() {
uniqueNotNullCommons(CLONE_NODES_QUERY);
}

@Test
public void testCloneSubgraphWithNodeKeyConstraint() {
nodeKeyCommons(CLONE_SUBGRAPH_QUERY);
}

@Test
public void testCloneSubgraphWithBothExistenceAndUniqueConstraint() {
uniqueNotNullCommons(CLONE_SUBGRAPH_QUERY);
}

@Test
public void testExtractNodesWithNodeKeyConstraint() {
session.writeTransaction(tx -> tx.run(CREATE_REL_FOR_EXTRACT_NODE));
nodeKeyCommons(EXTRACT_QUERY);
session.writeTransaction(tx -> tx.run(DELETE_REL_FOR_EXTRACT_NODE));
}

@Test
public void testExtractNodesWithBothExistenceAndUniqueConstraint() {
session.writeTransaction(tx -> tx.run(CREATE_REL_FOR_EXTRACT_NODE));
uniqueNotNullCommons(EXTRACT_QUERY);
session.writeTransaction(tx -> tx.run(DELETE_REL_FOR_EXTRACT_NODE));
}

private void nodeKeyCommons(String query) {
session.writeTransaction(tx -> tx.run("CREATE CONSTRAINT nodeKey ON (p:MyBook) ASSERT (p.name, p.surname) IS NODE KEY"));
cloneNodesAssertions(query, "already exists with label `MyBook` and properties `name` = 'foobar', `surname` = 'baz'");
session.writeTransaction(tx -> tx.run("DROP CONSTRAINT nodeKey"));

}

private void uniqueNotNullCommons(String query) {
session.writeTransaction(tx -> tx.run("CREATE CONSTRAINT unique ON (p:MyBook) ASSERT (p.name) IS UNIQUE"));
session.writeTransaction(tx -> tx.run("CREATE CONSTRAINT notNull ON (p:MyBook) ASSERT (p.name) IS NOT NULL"));

cloneNodesAssertions(query, "already exists with label `MyBook` and property `name` = 'foobar'");
session.writeTransaction(tx -> tx.run("DROP CONSTRAINT unique"));
session.writeTransaction(tx -> tx.run("DROP CONSTRAINT notNull"));
}

private void cloneNodesAssertions(String query, String message) {
session.writeTransaction(tx -> tx.run("CREATE (n:MyBook {name: 'foobar', surname: 'baz'})"));
testCall(session, query,
r -> {
final String error = (String) r.get("error");
assertTrue(error.contains(message));
assertNull(r.get("output"));

});
session.writeTransaction(tx -> tx.run("MATCH (n:MyBook) DELETE n"));
}
}
44 changes: 43 additions & 1 deletion core/src/test/java/apoc/refactor/GraphRefactoringTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import org.neo4j.graphdb.QueryExecutionException;
import org.neo4j.graphdb.Relationship;
import org.neo4j.graphdb.RelationshipType;
import org.neo4j.graphdb.config.Setting;
import org.neo4j.internal.helpers.collection.Iterators;
import org.neo4j.test.rule.DbmsRule;
import org.neo4j.test.rule.ImpermanentDbmsRule;
Expand Down Expand Up @@ -47,6 +46,7 @@
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.neo4j.configuration.SettingImpl.newBuilder;
import static org.neo4j.configuration.SettingValueParsers.BOOL;
Expand All @@ -57,6 +57,11 @@
* @since 25.03.16
*/
public class GraphRefactoringTest {
protected static final String CLONE_NODES_QUERY = "match (n:MyBook) with n call apoc.refactor.cloneNodes([n], true) " +
"YIELD output, error RETURN output, error";
protected static final String CLONE_SUBGRAPH_QUERY = "MATCH (n:MyBook) with n call apoc.refactor.cloneSubgraph([n], [], {}) YIELD output, error RETURN output, error";
protected static final String EXTRACT_QUERY = "MATCH p=(:Start)-[r:TO_MOVE]->(:End) with r call apoc.refactor.extractNode([r], ['MyBook'], 'OUT', 'IN') " +
"YIELD output, error RETURN output, error";

@Rule
public DbmsRule db = new ImpermanentDbmsRule()
Expand Down Expand Up @@ -1245,6 +1250,43 @@ public void testMergeRelsTrueAndProduceSelfRelFalse() {
});
}

@Test
public void issue2797WithCloneNodes() {
issue2797Common(CLONE_NODES_QUERY);
}

@Test
public void issue2797WithExtractNode() {
db.executeTransactionally("CREATE (:Start)-[r:TO_MOVE {name: 1}]->(:End)");
issue2797Common(EXTRACT_QUERY);
}

@Test
public void issue2797WithCloneSubgraph() {
issue2797Common(CLONE_SUBGRAPH_QUERY);
}

private void issue2797Common(String extractQuery) {
db.executeTransactionally(("CREATE CONSTRAINT unique_book ON (book:MyBook) ASSERT book.name IS UNIQUE"));

db.executeTransactionally(("CREATE (n:MyBook {name: 1})"));

testCall(db, extractQuery, r -> {
final String actualError = (String) r.get("error");
assertTrue(actualError.contains("already exists with label `MyBook` and property `name` = 1"));
assertNull(r.get("output"));
});

testCall(db, "MATCH (n:MyBook) RETURN properties(n) AS props",
r -> {
final Map<String, Long> expected = Map.of("name", 1L);
assertEquals(expected, r.get("props"));
});

db.executeTransactionally("DROP CONSTRAINT unique_book");
db.executeTransactionally("MATCH (n:MyBook) DELETE n");
}

private void assertSelfRel(Relationship next) {
assertSelfRel(next, null);
}
Expand Down

0 comments on commit 067b113

Please sign in to comment.