GH-576 - Fix deletion of relationship with the same type between the …

…same nodes.

Co-authored-by: Michael Simons <michael.simons@neo4j.com>

core/src/main/java/org/neo4j/ogm/cypher/compiler/CypherContext.java

@@ -24,6 +24,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 import org.neo4j.ogm.compiler.SrcTargetKey;
 import org.neo4j.ogm.context.Mappable;
@@ -34,6 +36,8 @@
  * @author Mark Angrish
  * @author Vince Bickers
  * @author Luanne Misquitta
+ * @author Andreas Berger
+ * @author Michael J. Simons
  */
 public class CypherContext implements CompileContext {
 
@@ -125,36 +129,22 @@ public Collection<Object> registry() {
      * @return true if the relationship was deleted or doesn't exist in the graph, false otherwise
      */
     public boolean deregisterOutgoingRelationships(Long src, String relationshipType, Class endNodeType) {
+
         Iterator<Mappable> iterator = registeredRelationships.iterator();
-        boolean nothingToDelete = true;
-        List<Mappable> cleared = new ArrayList<>();
+        List<Mappable> candidatesForDeletion = new ArrayList<>();
         while (iterator.hasNext()) {
             Mappable mappedRelationship = iterator.next();
             if (mappedRelationship.getStartNodeId() == src &&
                 mappedRelationship.getRelationshipType().equals(relationshipType) &&
                 endNodeType.equals(mappedRelationship.getEndNodeType())) {
 
-                cleared.add(mappedRelationship);
+                candidatesForDeletion.add(mappedRelationship);
                 iterator.remove();
-                nothingToDelete = false;
             }
         }
-        if (nothingToDelete) {
-            return true; //relationships not in the graph, okay, we can return
-        }
 
-        //Check to see if the relationships were previously deleted, if so, restore them
-        iterator = cleared.iterator();
-        while (iterator.hasNext()) {
-            Mappable mappedRelationship = iterator.next();
-            if (isMappableAlreadyDeleted(mappedRelationship)) {
-                registerRelationship(mappedRelationship);
-                iterator.remove();
-            } else {
-                deletedRelationships.add(mappedRelationship);
-            }
-        }
-        return cleared.size() > 0;
+        boolean notInGraphAtAll = candidatesForDeletion.size() == 0;
+        return notInGraphAtAll || handleDeletionCandidates(candidatesForDeletion);
     }
 
     /**
@@ -175,38 +165,43 @@ public boolean deregisterOutgoingRelationships(Long src, String relationshipType
      */
     public boolean deregisterIncomingRelationships(Long tgt, String relationshipType, Class endNodeType,
         boolean relationshipEntity) {
+
         Iterator<Mappable> iterator = registeredRelationships.iterator();
-        List<Mappable> cleared = new ArrayList<>();
-        boolean nothingToDelete = true;
+        List<Mappable> candidatesForDeletion = new ArrayList<>();
         while (iterator.hasNext()) {
             Mappable mappedRelationship = iterator.next();
             if (mappedRelationship.getEndNodeId() == tgt &&
                 mappedRelationship.getRelationshipType().equals(relationshipType) &&
                 endNodeType.equals(
                     relationshipEntity ? mappedRelationship.getEndNodeType() : mappedRelationship.getStartNodeType())) {
 
-                cleared.add(mappedRelationship);
+                candidatesForDeletion.add(mappedRelationship);
                 iterator.remove();
-                nothingToDelete = false;
             }
         }
 
-        if (nothingToDelete) {
-            return true; //relationships not in the graph, okay, we can return
-        }
+        boolean notInGraphAtAll = candidatesForDeletion.size() == 0;
+        return notInGraphAtAll || handleDeletionCandidates(candidatesForDeletion);
+    }
 
-        //Check to see if the relationships were previously deleted, if so, restore them
-        iterator = cleared.iterator();
-        while (iterator.hasNext()) {
-            Mappable mappedRelationship = iterator.next();
-            if (isMappableAlreadyDeleted(mappedRelationship)) {
-                registerRelationship(mappedRelationship);
-                iterator.remove();
-            } else {
-                deletedRelationships.add(mappedRelationship);
-            }
-        }
-        return cleared.size() > 0;
+    /**
+     * Partitions the list of candidates for deletion by relationships that have already been deleted and those that
+     * need to be deleted and than re-register those that already have been marked as deleted or marks all others as deleted.
+     * @param candidatesForDeletion A list of candidates for deletion
+     * @return True, if any candidate needs to be deleted
+     */
+    private boolean handleDeletionCandidates(List<Mappable> candidatesForDeletion) {
+
+        Map<Boolean, List<Mappable>> help = candidatesForDeletion.stream()
+            .collect(Collectors.partitioningBy(this::isAlreadyDeleted));
+
+        List<Mappable> alreadyDeleted = help.get(true);
+        registeredRelationships.addAll(alreadyDeleted);
+
+        List<Mappable> boundForDeletion = help.get(false);
+        deletedRelationships.addAll(boundForDeletion);
+
+        return boundForDeletion.size() > 0;
     }
 
     public void visitRelationshipEntity(Long relationshipEntity) {
@@ -258,15 +253,20 @@ public Collection<Object> getTransientRelationships(SrcTargetKey srcTargetKey) {
         }
     }
 
-    private boolean isMappableAlreadyDeleted(Mappable mappedRelationship) {
-        for (Mappable deletedRelationship : deletedRelationships) {
-            if (deletedRelationship.getEndNodeId() == mappedRelationship.getEndNodeId() &&
-                deletedRelationship.getStartNodeId() == mappedRelationship.getStartNodeId() &&
-                deletedRelationship.getRelationshipType().equals(mappedRelationship.getRelationshipType())) {
-                return true;
-            }
-        }
-        return false;
+    /**
+     * Checks whether a mapped relationship is already marked as deleted. This is the case when one of the relationships
+     * marked as deleted contains a relationship starting and ending at the same nodes having the same relationship type.
+     *
+     * @param mappedRelationship The relationship that should be checked for being already marked as deleted or not
+     * @return True if {@code mappedRelationship} was already marked as deleted
+     */
+    private boolean isAlreadyDeleted(Mappable mappedRelationship) {
+
+        Predicate<Mappable> describesSameRelationship = r -> r.getStartNodeId() == mappedRelationship.getStartNodeId()
+            && r.getEndNodeId() == mappedRelationship.getEndNodeId()
+            && r.getRelationshipType().equals(mappedRelationship.getRelationshipType());
+
+        return deletedRelationships.stream().anyMatch(describesSameRelationship);
     }
 
     private static class NodeBuilderHorizonPair {

test/src/test/java/org/neo4j/ogm/cypher/compiler/CypherContextTest.java

@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2002-2019 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This product is licensed to you under the Apache License, Version 2.0 (the "License").
+ * You may not use this product except in compliance with the License.
+ *
+ * This product may include a number of subcomponents with
+ * separate copyright notices and license terms. Your use of the source
+ * code for these subcomponents is subject to the terms and
+ *  conditions of the subcomponent's license, as noted in the LICENSE file.
+ */
+package org.neo4j.ogm.cypher.compiler;
+
+import static org.assertj.core.api.Assertions.*;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.function.Predicate;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.neo4j.ogm.cypher.ComparisonOperator;
+import org.neo4j.ogm.cypher.Filter;
+import org.neo4j.ogm.domain.gh576.DataItem;
+import org.neo4j.ogm.domain.gh576.FormulaItem;
+import org.neo4j.ogm.domain.gh576.Variable;
+import org.neo4j.ogm.session.Session;
+import org.neo4j.ogm.session.SessionFactory;
+import org.neo4j.ogm.testutil.MultiDriverTestClass;
+import org.neo4j.ogm.testutil.TestUtils;
+import org.neo4j.test.rule.RepeatRule;
+
+/**
+ * @author Andreas Berger
+ * @author Michael J. Simons
+ */
+public class CypherContextTest extends MultiDriverTestClass {
+
+    @Rule
+    public RepeatRule repeatRule = new RepeatRule(false, 10);
+
+    private static SessionFactory sessionFactory;
+
+    private static String createTestDataStatement = TestUtils.readCQLFile("org/neo4j/ogm/cql/nodes.cql").toString();
+
+    private Session session;
+
+    @BeforeClass
+    public static void initSesssionFactory() {
+        sessionFactory = new SessionFactory(driver, "org.neo4j.ogm.domain.gh576");
+    }
+
+    @Before
+    public void init() throws IOException {
+        session = sessionFactory.openSession();
+        session.purgeDatabase();
+        session.clear();
+
+        session.query(createTestDataStatement, Collections.emptyMap());
+    }
+
+    @Test // GH-576
+    public void shouldDeregisterRelationshipEntities() {
+        Collection<DataItem> dataItems;
+        FormulaItem formulaItem;
+
+        Filter filter = new Filter("nodeId", ComparisonOperator.EQUALS, "m1");
+
+        dataItems = session.loadAll(DataItem.class, filter);
+        assertThat(dataItems).hasSize(1);
+
+        formulaItem = (FormulaItem) dataItems.iterator().next();
+        assertThat(formulaItem.getVariables()).hasSize(3);
+
+        Predicate<Variable> isVariableAWithDataItemM2 = v -> v.getVariable().equals("A") && v.getDataItem().getNodeId()
+            .equals("m2");
+        formulaItem.getVariables().removeIf(isVariableAWithDataItemM2);
+        assertThat(formulaItem.getVariables()).hasSize(2);
+
+        session.save(formulaItem);
+
+        dataItems = session.loadAll(DataItem.class, filter);
+        assertThat(dataItems).hasSize(1);
+
+        formulaItem = (FormulaItem) dataItems.iterator().next();
+        assertThat(formulaItem.getVariables()).hasSize(2);
+    }
+
+    @After
+    public void tearDown() {
+        session.purgeDatabase();
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh576/BaseEntity.java

@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2002-2019 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This product is licensed to you under the Apache License, Version 2.0 (the "License").
+ * You may not use this product except in compliance with the License.
+ *
+ * This product may include a number of subcomponents with
+ * separate copyright notices and license terms. Your use of the source
+ * code for these subcomponents is subject to the terms and
+ *  conditions of the subcomponent's license, as noted in the LICENSE file.
+ */
+package org.neo4j.ogm.domain.gh576;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+
+/**
+ * @author Andreas Berger
+ */
+abstract class BaseEntity {
+
+    @Id
+    @GeneratedValue
+    private Long id;
+
+    public Long getId() {
+        return id;
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh576/DataItem.java

@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2002-2019 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This product is licensed to you under the Apache License, Version 2.0 (the "License").
+ * You may not use this product except in compliance with the License.
+ *
+ * This product may include a number of subcomponents with
+ * separate copyright notices and license terms. Your use of the source
+ * code for these subcomponents is subject to the terms and
+ *  conditions of the subcomponent's license, as noted in the LICENSE file.
+ */
+package org.neo4j.ogm.domain.gh576;
+
+import java.util.List;
+
+import org.neo4j.ogm.annotation.NodeEntity;
+import org.neo4j.ogm.annotation.Relationship;
+
+/**
+ * @author Andreas Berger
+ */
+@NodeEntity
+public class DataItem extends BaseEntity {
+
+    private String nodeId;
+
+    /*
+     * This field is here, b/c the neo4j ogm driver optimizes queries against the class given to the query.
+     * If we want the returned child entities to have its relations mapped as well, we need to tell OGM all
+     * the fields by adding them here
+     */
+    @Relationship(type = "USES")
+    protected List<Variable> variables;
+
+    public String getNodeId() {
+        return nodeId;
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh576/FormulaItem.java

@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2002-2019 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This product is licensed to you under the Apache License, Version 2.0 (the "License").
+ * You may not use this product except in compliance with the License.
+ *
+ * This product may include a number of subcomponents with
+ * separate copyright notices and license terms. Your use of the source
+ * code for these subcomponents is subject to the terms and
+ *  conditions of the subcomponent's license, as noted in the LICENSE file.
+ */
+package org.neo4j.ogm.domain.gh576;
+
+import java.util.List;
+
+import org.neo4j.ogm.annotation.NodeEntity;
+
+/**
+ * @author Andreas Berger
+ */
+@NodeEntity
+public class FormulaItem extends DataItem {
+
+    public List<Variable> getVariables() {
+        return variables;
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh576/Variable.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2002-2019 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This product is licensed to you under the Apache License, Version 2.0 (the "License").
+ * You may not use this product except in compliance with the License.
+ *
+ * This product may include a number of subcomponents with
+ * separate copyright notices and license terms. Your use of the source
+ * code for these subcomponents is subject to the terms and
+ *  conditions of the subcomponent's license, as noted in the LICENSE file.
+ */
+package org.neo4j.ogm.domain.gh576;
+
+import org.neo4j.ogm.annotation.EndNode;
+import org.neo4j.ogm.annotation.RelationshipEntity;
+import org.neo4j.ogm.annotation.StartNode;
+
+/**
+ * @author Andreas Berger
+ */
+@RelationshipEntity(type = "USES")
+public class Variable extends BaseEntity {
+
+    @StartNode
+    private FormulaItem formulaItem;
+
+    @EndNode
+    private DataItem dataItem;
+
+    private String variable;
+
+    public FormulaItem getFormulaItem() {
+        return formulaItem;
+    }
+
+    public DataItem getDataItem() {
+        return dataItem;
+    }
+
+    public String getVariable() {
+        return variable;
+    }
+}