Fixes #1582: rebind flag in apoc.periodic.iterate (#3098)

docs/asciidoc/modules/ROOT/nav.adoc

@@ -71,6 +71,7 @@ include::partial$generated-documentation/nav.adoc[]
 * xref:operational/index.adoc[]
     ** xref::operational/init-script.adoc[]
     ** xref::operational/log.adoc[]
+    ** xref::operational/rebind.adoc[]
 
 * xref:misc/index.adoc[]
     ** xref::misc/static-values.adoc[]

docs/asciidoc/modules/ROOT/pages/operational/index.adoc

@@ -8,3 +8,4 @@ For more information on how to use these procedures, see:
 
 * xref::operational/init-script.adoc[]
 * xref::operational/log.adoc[]
+* xref::operational/rebind.adoc[]

docs/asciidoc/modules/ROOT/pages/operational/rebind.adoc

@@ -0,0 +1,84 @@
+APOC provides a list of functions to rebind nodes and relationships:
+
+* `apoc.node.rebind(node)`
+* `apoc.rel.rebind(rel)`
+* `apoc.any.rebind(map/list/paths/...)`
+
+== Why use these functions
+
+Unlike versions up to 3.5, 
+in Neo4j 4+ the entities hold a reference to their originating transaction.
+
+This can cause problems when for example, we create a node (called n1) in a transaction, open a new transaction in which we create another node (called n2) and execute
+via `org.neo4j.graphdb.Node.createRelationshipTo()`,
+that is `n1.createRelationshipTo(n2)`.
+
+
+Is what happens when we perform the following operation:
+
+[source,cypher]
+----
+// node creation
+CREATE (:Article {content: 'contentBody'});
+
+// iterate all (:Article) nodes and new transaction and rel creation
+CALL apoc.periodic.iterate('MATCH (art:Article) RETURN art',
+'CREATE (node:Category) with art, node call apoc.create.relationship(art, "CATEGORY", {b: 1}, node) yield rel return rel', {});
+----
+
+Basically, we create a node `(:Article)`,
+then the second parameter of the xref::overview/apoc.periodic/apoc.periodic.iterate.adoc[apoc.periodic.iterate] open a new transaction and create a node `(:Category)`,
+and finally we try to create a relationship between `(:Article)` and `(:Category)` via xref::overview/apoc.create/apoc.create.relationship.adoc[apoc.create.relationship] (which uses under the hood the `org.neo4j.graphdb.Node.createRelationshipTo()` ).
+
+If we try to execute the second query the apoc.periodic.iterate will return an `errorMessage` similar to this:
+----
+Failed to invoke procedure `apoc.create.relationship`: Caused by: org.neo4j.graphdb.NotFoundException: Node[10] is deleted and cannot be used to create a relationship": 1
+----
+
+
+== How to solve
+
+To solve the previous `apoc.periodic,iterate`, 
+we can leverage return the internal id from the first statement and then match the node via id, 
+that means doing a `MATCH (n) WHERE id(n) = id(nodeId)`
+(this operation is called rebinding).
+
+That is:
+[source,cypher]
+----
+CALL apoc.periodic.iterate('MATCH (art:Article) RETURN id(art) as id',
+'CREATE (node:Category) WITH id, node MATCH (art) where id(art) = id 
+    WITH art, node call apoc.create.relationship(art, "CATEGORY", {b: 1}, node) yield rel return rel', {});
+----
+
+Alternatively, we can wrap with the `apoc.node.rebind` function the node that have to be rebound, like this:
+[source,cypher]
+----
+CALL apoc.periodic.iterate('MATCH (art:Article) RETURN art',
+'CREATE (node:Category) with art, node call apoc.create.relationship(art, "CATEGORY", {b: 1}, node) yield rel return rel', {});
+----
+
+Regarding relationships, we can use `apoc.rel.rebind`:
+[source,cypher]
+----
+// other operations...
+MATCH (:Start)-[rel:REL]->(:End) /*...*/ RETURN apoc.rel.rebind(rel)
+----
+
+We can also use the `apoc.any.rebind(ANY)` to rebind multiple entities placed in maps, lists, paths, or a combination of these three.
+This will return the same structure passed in the argument, but with rebound entities.
+For example:
+
+.Map of entities
+[source,cypher]
+----
+CREATE (a:Foo)-[r1:MY_REL]->(b:Bar)-[r2:ANOTHER_REL]->(c:Baz) WITH a,b,c,r1,r2
+RETURN apoc.any.rebind({first: a, second: b, third: c, rels: [r1, r2]}) as rebind
+----
+
+.List of paths
+[source,cypher]
+----
+CREATE p1=(a:Foo)-[r1:MY_REL]->(b:Bar), p2=(:Bar)-[r2:ANOTHER_REL]->(c:Baz)
+RETURN apoc.any.rebind([p1, p2]) as rebind
+----

extended/src/main/java/apoc/nodes/NodesExtended.java

@@ -0,0 +1,37 @@
+package apoc.nodes;
+
+import apoc.Extended;
+import apoc.util.EntityUtil;
+import apoc.util.Util;
+import org.neo4j.graphdb.Node;
+import org.neo4j.graphdb.Relationship;
+import org.neo4j.graphdb.Transaction;
+import org.neo4j.procedure.Context;
+import org.neo4j.procedure.Description;
+import org.neo4j.procedure.Name;
+import org.neo4j.procedure.UserFunction;
+
+@Extended
+public class NodesExtended {
+
+    @Context
+    public Transaction tx;
+
+    @UserFunction("apoc.node.rebind")
+    @Description("apoc.node.rebind(node - to rebind a node (i.e. executing a Transaction.getNodeById(node.getId())  ")
+    public Node nodeRebind(@Name("node") Node node) {
+        return Util.rebind(tx, node);
+    }
+
+    @UserFunction("apoc.rel.rebind")
+    @Description("apoc.rel.rebind(rel) - to rebind a rel (i.e. executing a Transaction.getRelationshipById(rel.getId())  ")
+    public Relationship relationshipRebind(@Name("rel") Relationship rel) {
+        return Util.rebind(tx, rel);
+    }
+
+    @UserFunction("apoc.any.rebind")
+    @Description("apoc.any.rebind(Object) - to rebind any rel, node, path, map, list or combination of them (i.e. executing a Transaction.getNodeById(node.getId()) / Transaction.getRelationshipById(rel.getId()))")
+    public Object anyRebind(@Name("any") Object any) {
+        return EntityUtil.anyRebind(tx, any);
+    }
+}

extended/src/main/java/apoc/util/EntityUtil.java

@@ -0,0 +1,37 @@
+package apoc.util;
+
+import org.neo4j.graphalgo.impl.util.PathImpl;
+import org.neo4j.graphdb.Entity;
+import org.neo4j.graphdb.Path;
+import org.neo4j.graphdb.Relationship;
+import org.neo4j.graphdb.Transaction;
+import org.neo4j.internal.helpers.collection.Iterables;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class EntityUtil {
+
+    public static <T> T anyRebind(Transaction tx, T any) {
+        if (any instanceof Map) {
+            return (T) ((Map<String, Object>) any).entrySet().stream()
+                    .collect(Collectors.toMap(e -> e.getKey(), e -> anyRebind(tx, e.getValue())));
+        }
+        if (any instanceof Path) {
+            final Path path = (Path) any;
+            PathImpl.Builder builder = new PathImpl.Builder(Util.rebind(tx, path.startNode()));
+            for (Relationship rel: path.relationships()) {
+                builder = builder.push(Util.rebind(tx, rel));
+            }
+            return (T) builder.build();
+        }
+        if (any instanceof Iterable) {
+            return (T) Iterables.stream((Iterable) any)
+                    .map(i -> anyRebind(tx, i)).collect(Collectors.toList());
+        }
+        if (any instanceof Entity) {
+            return (T) Util.rebind(tx, (Entity) any);
+        }
+        return any;
+    }
+}

extended/src/main/resources/extended.txt

@@ -154,8 +154,11 @@ apoc.uuid.remove
 apoc.uuid.removeAll
 apoc.uuid.setup
 apoc.uuid.show
+apoc.any.rebind
 apoc.coll.avgDuration
 apoc.data.email
+apoc.node.rebind
+apoc.rel.rebind
 apoc.static.get
 apoc.static.getAll
 apoc.trigger.nodesByLabel

extended/src/test/java/apoc/nodes/NodesExtendedTest.java

@@ -0,0 +1,69 @@
+package apoc.nodes;
+
+import apoc.create.Create;
+import apoc.util.TestUtil;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.neo4j.graphdb.Label;
+import org.neo4j.graphdb.Path;
+import org.neo4j.graphdb.Relationship;
+import org.neo4j.graphdb.RelationshipType;
+import org.neo4j.internal.helpers.collection.Iterables;
+import org.neo4j.test.rule.DbmsRule;
+import org.neo4j.test.rule.ImpermanentDbmsRule;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertEquals;
+
+
+public class NodesExtendedTest {
+
+    @Rule
+    public DbmsRule db = new ImpermanentDbmsRule();
+
+    @Before
+    public void setUp() throws Exception {
+        TestUtil.registerProcedure(db, NodesExtended.class, Create.class);
+    }
+
+    @Test
+    public void rebind() {
+        TestUtil.testCall(db, "CREATE (a:Foo)-[r1:MY_REL]->(b:Bar)-[r2:ANOTHER_REL]->(c:Baz) WITH a,b,c,r1,r2 \n" +
+                        "RETURN apoc.any.rebind({first: a, second: b, third: c, rels: [r1, r2]}) as rebind",
+                (row) -> {
+                    final Map<String, Object> rebind = (Map<String, Object>) row.get("rebind");
+                    final List<Relationship> rels = (List<Relationship>) rebind.get("rels");
+                    final Relationship firstRel = rels.get(0);
+                    final Relationship secondRel = rels.get(1);
+                    assertEquals(firstRel.getStartNode(), rebind.get("first"));
+                    assertEquals(firstRel.getEndNode(), rebind.get("second"));
+                    assertEquals(secondRel.getStartNode(), rebind.get("second"));
+                    assertEquals(secondRel.getEndNode(), rebind.get("third"));
+                });
+
+        TestUtil.testCall(db, "CREATE p1=(a:Foo)-[r1:MY_REL]->(b:Bar), p2=(:Bar)-[r2:ANOTHER_REL]->(c:Baz) \n" +
+                        "RETURN apoc.any.rebind([p1, p2]) as rebind",
+                (row) -> {
+                    final List<Path> rebindList = (List<Path>) row.get("rebind");
+                    assertEquals(2, rebindList.size());
+                    final Path firstPath = rebindList.get(0);
+                    assertPath(firstPath, List.of("Foo", "Bar"), List.of("MY_REL"));
+                    final Path secondPath = rebindList.get(1);
+                    assertPath(secondPath, List.of("Bar", "Baz"), List.of("ANOTHER_REL"));
+                });
+    }
+
+    private void assertPath(Path rebind, List<String> labels, List<String> relTypes) {
+        final List<String> actualLabels = Iterables.stream(rebind.nodes())
+                .map(i -> i.getLabels().iterator().next())
+                .map(Label::name).collect(Collectors.toList());
+        assertEquals(labels, actualLabels);
+        final List<String> actualRelTypes = Iterables.stream(rebind.relationships()).map(Relationship::getType)
+                .map(RelationshipType::name).collect(Collectors.toList());
+        assertEquals(relTypes, actualRelTypes);
+    }
+}

extended/src/test/java/apoc/periodic/PeriodicExtendedTest.java

@@ -1,5 +1,9 @@
 package apoc.periodic;
 
+import apoc.create.Create;
+import apoc.load.Jdbc;
+import apoc.nlp.gcp.GCPProcedures;
+import apoc.nodes.NodesExtended;
 import apoc.util.TestUtil;
 import org.junit.Before;
 import org.junit.Rule;
@@ -8,9 +12,12 @@
 import org.neo4j.test.rule.ImpermanentDbmsRule;
 
 import java.util.Collections;
+import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
 
 import static apoc.util.TestUtil.testCall;
+import static apoc.util.TestUtil.testCallCount;
 import static org.junit.Assert.assertEquals;
 import static org.neo4j.test.assertion.Assert.assertEventually;
 
@@ -21,7 +28,7 @@ public class PeriodicExtendedTest {
 
     @Before
     public void initDb() {
-        TestUtil.registerProcedure(db, PeriodicExtended.class, Periodic.class);
+        TestUtil.registerProcedure(db, Periodic.class, NodesExtended.class, GCPProcedures.class, Create.class, PeriodicExtended.class, Jdbc.class);
     }
 
     @Test
@@ -61,4 +68,43 @@ public void testSubmitSchemaWithWriteOperation() {
             assertEquals(true, row.get("done"));
         });
     }
+
+    @Test
+    public void testRebindWithNlpWriteProcedure() {
+        // use case: https://community.neo4j.com/t5/neo4j-graph-platform/use-of-apoc-periodic-iterate-with-apoc-nlp-gcp-classify-graph/m-p/56846#M33854
+        final String iterate = "MATCH (node:Article) RETURN node";
+        final String actionRebind = "CALL apoc.nlp.gcp.classify.graph(apoc.node.rebind(node), $nlpConf) YIELD graph RETURN null";
+        testRebindCommon(iterate, actionRebind, 2, this::assertNoErrors);
+
+        // "manual" rebind, i.e. "return id(node) as id" in iterate query, and "match .. where id(n)=id" in action query
+        final String iterateId = "MATCH (node:Article) RETURN id(node) AS id";
+        final String actionId = "MATCH (node) WHERE id(node) = id CALL apoc.nlp.gcp.classify.graph(node, $nlpConf) YIELD graph RETURN null";
+        testRebindCommon(iterateId, actionId, 2, this::assertNoErrors);
+    }
+
+    @Test
+    public void testRebindWithMapIterationAndCreateRelationshipProcedure() {
+        final String iterate = "MATCH (art:Article) RETURN {key: art, key2: 'another'} as map";
+        final String action = "CREATE (node:Category) with map.key as art, node call apoc.create.relationship(art, 'CATEGORY', {b: 1}, node) yield rel return rel";
+
+        final String actionRebind = "WITH apoc.any.rebind(map) AS map " + action;
+        testRebindCommon(iterate, actionRebind, 1, this::assertNoErrors);
+    }
+
+    private void assertNoErrors(Map<String, Object> r) {
+        assertEquals(Collections.emptyMap(), r.get("errorMessages"));
+    }
+
+    private void testRebindCommon(String iterate, String action, int expected, Consumer<Map<String, Object>> assertions) {
+        final Map<String, Object> nlpConf = Map.of("key", "myKey", "nodeProperty", "content", "write", true, "unsupportedDummyClient", true);
+        final Map<String, Object> config = Map.of("params", Map.of("nlpConf", nlpConf));
+
+        db.executeTransactionally("CREATE (:Article {content: 'contentBody'})");
+        testCall(db,"CALL apoc.periodic.iterate($iterate, $action, $config)",
+                Map.of( "iterate" , iterate, "action", action, "config", config),
+                assertions);
+
+        testCallCount(db, "MATCH p=(:Category)<-[:CATEGORY]-(:Article) RETURN p", expected);
+        db.executeTransactionally("MATCH (n) DETACH DELETE n");
+    }
 }