Deleting nodes when having an apoc.trigger registered returns Neo.Dat…

…abaseError.Transaction.TransactionCommitFailed (#2596)

Fixes #1152 and #2247

core/src/main/java/apoc/result/VirtualRelationship.java

@@ -28,6 +28,11 @@ public class VirtualRelationship implements Relationship {
     private final long id;
     private final Map<String, Object> props = new HashMap<>();
 
+    public VirtualRelationship(Node startNode, Node endNode, RelationshipType type, Map<String, Object> props) {
+        this(startNode, endNode, type);
+        this.props.putAll(props);
+    }
+
     public VirtualRelationship(Node startNode, Node endNode, RelationshipType type) {
         validateNodes(startNode, endNode);
         this.id = MIN_ID.getAndDecrement();

core/src/main/java/apoc/trigger/TriggerMetadata.java

@@ -77,34 +77,62 @@ public static TriggerMetadata from(TransactionData txData, boolean rebindDeleted
         }
         List<Node> createdNodes = Convert.convertToList(txData.createdNodes());
         List<Relationship> createdRelationships = Convert.convertToList(txData.createdRelationships());
-        List<Node> deletedNodes = rebindDeleted ? rebindDeleted(Convert.convertToList(txData.deletedNodes())) : Convert.convertToList(txData.deletedNodes());
-        List<Relationship> deletedRelationships = rebindDeleted ? rebindDeleted(Convert.convertToList(txData.deletedRelationships())) : Convert.convertToList(txData.deletedRelationships());
+        List<Node> deletedNodes = rebindDeleted ? rebindDeleted(Convert.convertToList(txData.deletedNodes()), txData) : Convert.convertToList(txData.deletedNodes());
+        List<Relationship> deletedRelationships = rebindDeleted ? rebindDeleted(Convert.convertToList(txData.deletedRelationships()), txData) : Convert.convertToList(txData.deletedRelationships());
         Map<String, List<Node>> removedLabels = aggregateLabels(txData.removedLabels());
         Map<String, List<Node>> assignedLabels = aggregateLabels(txData.assignedLabels());
-        final Map<String, List<PropertyEntryContainer<Node>>> removedNodeProperties = aggregatePropertyKeys(txData.removedNodeProperties(), true);
-        final Map<String, List<PropertyEntryContainer<Relationship>>> removedRelationshipProperties = aggregatePropertyKeys(txData.removedRelationshipProperties(), true);
+        Map<String, List<PropertyEntryContainer<Node>>> removedNodeProperties = aggregatePropertyKeys(txData.removedNodeProperties(), true);
+        Map<String, List<PropertyEntryContainer<Relationship>>> removedRelationshipProperties = aggregatePropertyKeys(txData.removedRelationshipProperties(), true);
         final Map<String, List<PropertyEntryContainer<Node>>> assignedNodeProperties = aggregatePropertyKeys(txData.assignedNodeProperties(), false);
         final Map<String, List<PropertyEntryContainer<Relationship>>> assignedRelationshipProperties = aggregatePropertyKeys(txData.assignedRelationshipProperties(), false);
+        if (rebindDeleted) {
+            removedLabels = removedLabels.entrySet()
+                    .stream()
+                    .collect(Collectors.toMap(Map.Entry::getKey, e -> rebindDeleted(e.getValue(), txData)));
+            removedNodeProperties = rebindPropsEntries(txData, removedNodeProperties);
+            removedRelationshipProperties = rebindPropsEntries(txData, removedRelationshipProperties);
+        }
         return new TriggerMetadata(txId, commitTime, createdNodes, createdRelationships, deletedNodes, deletedRelationships,
                 removedLabels,removedNodeProperties, removedRelationshipProperties, assignedLabels, assignedNodeProperties,
                 assignedRelationshipProperties, txData.metaData());
     }
 
-    private static <T extends Entity> List<T> rebindDeleted(List<T> entities) {
-        return (List<T>) entities.stream()
-                .map(e -> {
-                    if (e instanceof Node) {
-                        Node node = (Node) e;
-                        Label[] labels = Iterables.asArray(Label.class, node.getLabels());
-                        return new VirtualNode(labels, e.getAllProperties());
-                    } else {
-                        Relationship rel = (Relationship) e;
-                        return new VirtualRelationship(rel.getStartNode(), rel.getEndNode(), rel.getType());
-                    }
-                })
+    private static <T extends Entity> Map<String, List<PropertyEntryContainer<T>>> rebindPropsEntries(TransactionData txData, Map<String, List<PropertyEntryContainer<T>>> removedNodeProperties) {
+        return removedNodeProperties.entrySet()
+                .stream()
+                .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().stream()
+                        .map(entry ->  entry.copy((T) toVirtualEntity(txData, entry.entity)))
+                        .collect(Collectors.toList())));
+    }
+
+    private static <T extends Entity> List<T> rebindDeleted(List<T> entities, TransactionData txData) {
+        return entities.stream()
+                .map(e -> (T) toVirtualEntity(txData, e))
                 .collect(Collectors.toList());
     }
 
+    private static <T extends Entity> Entity toVirtualEntity(TransactionData txData, T e) {
+        if (e instanceof Node) {
+            Node node = (Node) e;
+            final Label[] labels = Iterables.stream(txData.removedLabels())
+                    .filter(label -> label.node().equals(node))
+                    .map(LabelEntry::label)
+                    .toArray(Label[]::new);
+            final Map<String, Object> props = getProps(txData.removedNodeProperties(), node);
+            return new VirtualNode(labels, props);
+        } else {
+            Relationship rel = (Relationship) e;
+            final Map<String, Object> props = getProps(txData.removedRelationshipProperties(), rel);
+            return new VirtualRelationship(rel.getStartNode(), rel.getEndNode(), rel.getType(), props);
+        }
+    }
+
+    private static <T extends Entity> Map<String, Object> getProps(Iterable<PropertyEntry<T>> propertyEntries, T entity) {
+        return Iterables.stream(propertyEntries)
+                .filter(label -> label.entity().equals(entity))
+                .collect(Collectors.toMap(PropertyEntry::key, PropertyEntry::previouslyCommittedValue));
+    }
+
     public TriggerMetadata rebind(Transaction tx) {
         final List<Node> createdNodes = Util.rebind(this.createdNodes, tx);
         final List<Relationship> createdRelationships = Util.rebind(this.createdRelationships, tx);
@@ -189,6 +217,10 @@ PropertyEntryContainer<T> rebind(Transaction tx) {
             return new PropertyEntryContainer<T>(key, Util.rebind(tx, entity), oldVal, newVal);
         }
 
+        PropertyEntryContainer<T> copy(T entity) {
+            return new PropertyEntryContainer<T>(key, entity, oldVal, newVal);
+        }
+
         Map<String, Object> toMap() {
             final Map<String, Object> map = map("key", key, entity instanceof Node ? "node" : "relationship", entity, "old", oldVal);
             if (newVal != null) {

core/src/test/java/apoc/trigger/TriggerTest.java

@@ -1,25 +1,29 @@
 package apoc.trigger;
 
+import apoc.nodes.Nodes;
 import apoc.util.TestUtil;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.neo4j.graphdb.Node;
 import org.neo4j.graphdb.QueryExecutionException;
 import org.neo4j.graphdb.Relationship;
+import org.neo4j.graphdb.ResourceIterator;
 import org.neo4j.graphdb.Transaction;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.impl.coreapi.TransactionImpl;
 import org.neo4j.test.rule.DbmsRule;
 import org.neo4j.test.rule.ImpermanentDbmsRule;
 
 import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
 import static apoc.ApocSettings.apoc_trigger_enabled;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.neo4j.configuration.GraphDatabaseSettings.procedure_unrestricted;
 import static org.neo4j.internal.helpers.collection.MapUtil.map;
 
 /**
@@ -30,14 +34,15 @@ public class TriggerTest {
 
     @Rule
     public DbmsRule db = new ImpermanentDbmsRule()
+            .withSetting(procedure_unrestricted, List.of("apoc*"))
             .withSetting(apoc_trigger_enabled, true);  // need to use settings here, apocConfig().setProperty in `setUp` is too late
 
     private long start;
 
     @Before
     public void setUp() throws Exception {
         start = System.currentTimeMillis();
-        TestUtil.registerProcedure(db, Trigger.class);
+        TestUtil.registerProcedure(db, Trigger.class, Nodes.class);
     }
 
     @Test
@@ -65,6 +70,16 @@ public void testRemoveNode() throws Exception {
         });
     }
 
+    @Test
+    public void testIssue2247() {
+        db.executeTransactionally("CREATE (n:ToBeDeleted)");
+        db.executeTransactionally("CALL apoc.trigger.add('myTrig', 'RETURN 1', {phase: 'afterAsync'})");
+
+        db.executeTransactionally("MATCH (n:ToBeDeleted) DELETE n");
+
+        db.executeTransactionally("CALL apoc.trigger.remove('myTrig')");
+    }
+
     @Test
     public void testRemoveRelationship() throws Exception {
         db.executeTransactionally("CREATE (:Counter {count:0})");
@@ -242,20 +257,73 @@ public void testCreatedRelationshipsAsync() throws Exception {
     }
 
     @Test
-    public void testDeleteRelationshipsAsync() throws Exception {
-        db.executeTransactionally("CREATE (a:A {name: \"A\"})-[:R1]->(z:Z {name: \"Z\"}), (a)-[:R2]->(z)");
-        db.executeTransactionally("CALL apoc.trigger.add('trigger-after-async', 'UNWIND $deletedRelationships AS r\n" +
+    public void testDeleteRelationshipsAsync() {
+        db.executeTransactionally("CREATE (a:A {name: \"A\"})-[:R1 {omega: 3}]->(z:Z {name: \"Z\"}), (a)-[:R2 {alpha: 1}]->(z)");
+        final String query = "UNWIND $deletedRelationships AS r\n" +
                 "MATCH (a)-[r1:R1]->(z)\n" +
-                "SET r1.triggerAfterAsync = size($deletedRelationships) > 0, r1.size = size($deletedRelationships), r1.deleted = type(r) RETURN *', {phase: 'afterAsync'})");
-        db.executeTransactionally("MATCH (a:A {name: \"A\"})-[r:R2]->(z:Z {name: \"Z\"})\n" +
-                "DELETE r");
+                "SET a.alpha = apoc.any.property(r, \"alpha\"), r1.triggerAfterAsync = size($deletedRelationships) > 0, r1.size = size($deletedRelationships), r1.deleted = type(r) RETURN *";
+        db.executeTransactionally("CALL apoc.trigger.add('trigger-after-async-1', $query, {phase: 'afterAsync'})",
+                map("query", query));
+
+        // delete rel
+        commonDeleteAfterAsync("MATCH (a:A {name: 'A'})-[r:R2]->(z:Z {name: 'Z'}) DELETE r");
+    }
+
+    @Test
+    public void testDeleteRelationshipsAsyncWithCreationInQuery() {
+        db.executeTransactionally("CREATE (a:A {name: \"A\"})-[:R1 {omega: 3}]->(z:Z {name: \"Z\"}), (a)-[:R2 {alpha: 1}]->(z)");
+        final String query = "UNWIND $deletedRelationships AS r\n" +
+                "CREATE (a:A)-[r1:R1 {omega: 3}]->(z)\n" +
+                "SET a.alpha = apoc.any.property(r, \"alpha\"), r1.triggerAfterAsync = size($deletedRelationships) > 0, r1.size = size($deletedRelationships), r1.deleted = type(r) RETURN *";
+        db.executeTransactionally("CALL apoc.trigger.add('trigger-after-async-2', $query, {phase: 'afterAsync'})",
+                map("query", query));
+
+        // delete rel
+        commonDeleteAfterAsync("MATCH (a:A {name: 'A'})-[r:R2]->(z:Z {name: 'Z'}) DELETE r");
+    }
+
+    @Test
+    public void testDeleteNodesAsync() {
+        db.executeTransactionally("CREATE (a:A {name: 'A'})-[:R1 {omega: 3}]->(z:Z {name: 'Z'}), (:R2:Other {alpha: 1})");
+        final String query = "UNWIND $deletedNodes AS n\n" +
+                "MATCH (a)-[r1:R1]->(z)\n" +
+                "SET a.alpha = apoc.any.property(n, \"alpha\"), r1.triggerAfterAsync = size($deletedNodes) > 0, r1.size = size($deletedNodes), r1.deleted = apoc.node.labels(n)[0] RETURN *";
+
+        db.executeTransactionally("CALL apoc.trigger.add('trigger-after-async-3', $query, {phase: 'afterAsync'})", 
+                map("query", query));
+
+        // delete node
+        commonDeleteAfterAsync("MATCH (n:R2) DELETE n");
+    }
+
+    @Test
+    public void testDeleteNodesAsyncWithCreationQuery() {
+        db.executeTransactionally("CREATE (:R2:Other {alpha: 1})");
+        final String query = "UNWIND $deletedNodes AS n\n" +
+                "CREATE (a:A)-[r1:R1 {omega: 3}]->(z:Z)\n" +
+                "SET a.alpha = apoc.any.property(n, \"alpha\"), r1.triggerAfterAsync = size($deletedNodes) > 0, r1.size = size($deletedNodes), r1.deleted = apoc.node.labels(n)[0] RETURN *";
+
+        db.executeTransactionally("CALL apoc.trigger.add('trigger-after-async-4', $query, {phase: 'afterAsync'})", 
+                map("query", query));
+
+        // delete node
+        commonDeleteAfterAsync("MATCH (n:R2) DELETE n");
+    }
+
+    private void commonDeleteAfterAsync(String deleteQuery) {
+        db.executeTransactionally(deleteQuery);
+
+        final Map<String, Object> expectedProps = Map.of("deleted", "R2",
+                "triggerAfterAsync", true,
+                "size", 1L,
+                "omega", 3L);
 
         org.neo4j.test.assertion.Assert.assertEventually(() ->
-                        db.executeTransactionally("MATCH ()-[r:R1]->() RETURN r", Map.of(),
+                        db.executeTransactionally("MATCH (a:A {alpha: 1})-[r:R1]->() RETURN r", Map.of(),
                                 result -> {
-                                    final Relationship r = result.<Relationship>columnAs("r").next();
-                                    return (boolean) r.getProperty("triggerAfterAsync", false)
-                                            && r.getProperty("deleted", "").equals("R2");
+                                    final ResourceIterator<Relationship> relIterator = result.columnAs("r");
+                                    return relIterator.hasNext()
+                                            && relIterator.next().getAllProperties().equals(expectedProps);
                                 })
                 , (value) -> value, 30L, TimeUnit.SECONDS);
     }

docs/asciidoc/modules/ROOT/pages/background-operations/triggers.adoc

@@ -61,6 +61,8 @@ You can use these helper functions to extract nodes or relationships by label/re
 |===
 | apoc.trigger.nodesByLabel($assignedLabels/$assignedNodeProperties,'Label') | function to filter entries by label, to be used within a trigger statement with `$assignedLabels` and `$removedLabels`
 | apoc.trigger.propertiesByKey($assignedNodeProperties,'key') | function to filter propertyEntries by property-key, to be used within a trigger statement with $assignedNode/RelationshipProperties and $removedNode/RelationshipProperties. Returns [{old,[new],key,node,relationship}]
+| apoc.trigger.toNode(node, $removedLabels, $removedNodeProperties) | function to rebuild a node as a virtual, to be used in triggers with a not 'afterAsync' phase
+| apoc.trigger.toRelationship(rel, $removedRelationshipProperties) | function to rebuild a relationship as a virtual, to be used in triggers with a not 'afterAsync' phase
 |===
 
 
@@ -316,6 +318,47 @@ We can pass as a 4th parameter, a `{params: {parameterMaps}}` to insert addition
 CALL apoc.trigger.add('timeParams','UNWIND $createdNodes AS n SET n.time = $time', {}, {params: {time: timestamp()}});
 ----
 
+.Handle deleted entities
+
+If we to create a 'before' or 'after' trigger query, with `$deletedRelationships` or `$deletedNodes`,
+and then we want to retrieve entities information like labels and/or properties,
+we cannot use the 'classic' cypher functions `labels()` and `properties()`,
+but we can leverage on xref::virtual/virtual-nodes-rels.adoc[virtual nodes and relationships],
+via the functions `apoc.trigger.toNode(node, $removedLabels, $removedNodeProperties)` and `apoc.trigger.toRelationship(rel, $removedRelationshipProperties)`.
+
+So that, we can retrieve information about nodes and relations, 
+using the `apoc.any.properties`, and the `apoc.node.labels` functions.
+
+For example, if we want to create a new node with the same properties (plus the id) and with an additional label retrieved for each deleted node, we can execute:
+
+[source,cypher]
+----
+CALL apoc.trigger.add('myTrigger', 
+"UNWIND $deletedNodes as deletedNode 
+WITH apoc.trigger.toNode(deletedNode, $removedLabels, $removedNodeProperties) AS deletedNode 
+CREATE (r:Report {id: id(deletedNode)}) WITH r, deletedNode 
+CALL apoc.create.addLabels(r, apoc.node.labels(deletedNode)) yield node with node, deletedNode 
+set node+=apoc.any.properties(deletedNode)" ,
+{phase:'before'})
+----
+
+Or also, if we want to  create a node `Report` with the same properties (plus the id and rel-type as additional properties) for each deleted relationship, we can execute:
+
+[source,cypher]
+----
+CALL apoc.trigger.add('myTrigger', 
+"UNWIND $deletedRelationships as deletedRel 
+WITH apoc.trigger.toRelationship(deletedRel, $removedRelationshipProperties) AS deletedRel 
+CREATE (r:Report {id: id(deletedRel), type: apoc.rel.type(deletedRel)}) 
+WITH r, deletedRelset r+=apoc.any.properties(deletedRel)" ,
+{phase:'before'})
+----
+
+[NOTE]
+====
+By using phase 'afterAsync', we don't need to execute `apoc.trigger.toNode` and `apoc.trigger.toRelationship`,
+because using this one, the rebuild of entities is executed automatically under the hood.
+====
 
 .Other examples
 [source,cypher]

full/src/main/java/apoc/trigger/TriggerExtended.java

@@ -3,12 +3,16 @@
 import apoc.Description;
 import apoc.Extended;
 import apoc.coll.SetBackedList;
+import apoc.result.VirtualNode;
+import apoc.result.VirtualRelationship;
 import org.neo4j.graphdb.GraphDatabaseService;
 import org.neo4j.graphdb.Label;
 import org.neo4j.graphdb.Node;
+import org.neo4j.graphdb.Relationship;
 import org.neo4j.procedure.*;
 
 import java.util.*;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /**
@@ -102,4 +106,41 @@ public TriggerInfo toTriggerInfo(Map.Entry<String, Object> e) {
         }
         return new TriggerInfo(name, null, null, false, false);
     }
+
+    @UserFunction
+    @Description("apoc.trigger.toNode(node, $removedLabels, $removedNodeProperties) | function to rebuild a node as a virtual, to be used in triggers with a not 'afterAsync' phase")
+    public Node toNode(@Name("id") Node node, @Name("removedLabels") Map<String, List<Node>> removedLabels, @Name("removedNodeProperties") Map<String, List<Map>> removedNodeProperties) {
+
+        final long id = node.getId();
+        final Label[] labels = removedLabels.entrySet().stream()
+                .filter(i -> i.getValue().stream().anyMatch(l -> l.getId() == id))
+                .map(e -> Label.label(e.getKey()))
+                .toArray(Label[]::new);
+
+        final Map<String, Object> props = removedNodeProperties.entrySet().stream()
+                .map(i -> i.getValue().stream()
+                        .filter(l -> ((Node) l.get("node")).getId() == id)
+                        .findAny()
+                        .map(v -> new AbstractMap.SimpleEntry<>(i.getKey(), v.get("old"))))
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .collect(Collectors.toMap(AbstractMap.SimpleEntry::getKey, AbstractMap.SimpleEntry::getValue));
+
+        return new VirtualNode(labels, props);
+    }
+
+    @UserFunction
+    @Description("apoc.trigger.toRelationship(rel, $removedRelationshipProperties) | function to rebuild a relationship as a virtual, to be used in triggers with a not 'afterAsync' phase")
+    public Relationship toRelationship(@Name("id") Relationship rel, @Name("removedRelationshipProperties") Map<String, List<Map>> removedRelationshipProperties) {
+        final Map<String, Object> props = removedRelationshipProperties.entrySet().stream()
+                .map(i -> i.getValue().stream()
+                        .filter(l -> ((Relationship) l.get("relationship")).getId() == rel.getId())
+                        .findAny()
+                        .map(v -> new AbstractMap.SimpleEntry<>(i.getKey(), v.get("old"))))
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .collect(Collectors.toMap(AbstractMap.SimpleEntry::getKey, AbstractMap.SimpleEntry::getValue));
+
+        return new VirtualRelationship(rel.getStartNode(), rel.getEndNode(), rel.getType(), props);
+    }
 }

full/src/main/resources/extended.txt

@@ -163,4 +163,6 @@ apoc.static.get
 apoc.static.getAll
 apoc.trigger.nodesByLabel
 apoc.trigger.propertiesByKey
+apoc.trigger.toNode
+apoc.trigger.toRelationship
 apoc.ttl.config