GH-903 - Increment only version properties of changed relationship entities.

The fix is to use an increment of 0 in the statements handling existing relationship entities. While I tried at first to exclude to corresponding builders in `org.neo4j.ogm.context.EntityGraphMapper#getRelationshipBuilder` (and with them the edges and eventually the statements) from the list of statements to execute, we didn’t succeed with that. The `PizzaIntegrationTest`, more specifically, the `shouldBeAbleToSaveAndRetrieveFullyLoadedPizza` became flaky with that.

I have no clue what the actual tests meaning is, but I assume the underlying cause is the fact the the context won’t get updated with all the affected entities (the test uses a couple of different sessions). That means: A fully hydrated and changed object is updated in one session, which may or may not have the information about the content of the local graph. When the statements of unchanged entities are excluded, they won’t never be returned from the datatbase, hence not make their way into the session cache and therefore will never update the update in question.

This fixes #903.

Author: michael-simons

core/src/main/java/org/neo4j/ogm/context/EntityGraphMapper.java

@@ -152,8 +152,7 @@ public CompileContext map(Object entity, int horizon) {
                 DirectedRelationship directedRelationship = new DirectedRelationship(relationshipType,
                     Direction.OUTGOING);
 
-                RelationshipBuilder relationshipBuilder = getRelationshipBuilder(compiler, entity, directedRelationship,
-                    false);
+                RelationshipBuilder relationshipBuilder = getRelationshipBuilder(compiler, entity, directedRelationship, mappingContext.isDirty(entity));
 
                 // 2. create or update the actual relationship (edge) in the graph
                 updateRelationshipEntity(compiler.context(), entity, relationshipBuilder, reInfo);
@@ -573,7 +572,7 @@ private RelationshipBuilder getRelationshipBuilder(Compiler cypherBuilder, Objec
                     EntityUtils.setIdentity(entity, null, metaData);
                 }
             } else {
-                relationshipBuilder = cypherBuilder.existingRelationship(relId, directedRelationship.direction(), directedRelationship.type());
+                relationshipBuilder = cypherBuilder.existingRelationship(relId, directedRelationship.direction(), directedRelationship.type(), mappingContext.isDirty(entity));
 
                 this.mappingContext.getSnapshotOf(entity).ifPresent(snapshot ->
                     relationshipBuilder
@@ -753,7 +752,7 @@ private <T> void updateVersionField(Object entity, PropertyContainerBuilder<T> b
 
         if (version == null) {
             version = 0L;
-        } else {
+        } else if (mappingContext.isDirty(entity)) {
             version = version + 1;
         }
         fieldInfo.writeDirect(entity, version);

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

@@ -71,7 +71,7 @@ public interface Compiler {
      * @param direction              The direction of the existing relationship
      * @return A new {@link RelationshipBuilder} bound to the identified relationship entity
      */
-    RelationshipBuilder existingRelationship(Long existingRelationshipId, Relationship.Direction direction, String type);
+    RelationshipBuilder existingRelationship(Long existingRelationshipId, Relationship.Direction direction, String type, boolean wasDirty);
 
     /**
      * Defines a relationship deletion between the specified start node to end node with the given relationship type and direction.

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

@@ -93,9 +93,11 @@ public NodeBuilder existingNode(Long existingNodeId) {
     }
 
     @Override
-    public RelationshipBuilder existingRelationship(Long existingRelationshipId, Relationship.Direction direction, String type) {
+    public RelationshipBuilder existingRelationship(Long existingRelationshipId, Relationship.Direction direction, String type, boolean wasDirty) {
         String key = existingRelationshipId + ";" + direction.name();
-        return existingRelationshipBuilders.computeIfAbsent(key, k -> new DefaultRelationshipBuilder(type, existingRelationshipId));
+        RelationshipBuilder relationshipBuilder = existingRelationshipBuilders.computeIfAbsent(key, k -> new DefaultRelationshipBuilder(type, existingRelationshipId));
+        relationshipBuilder.setDirty(wasDirty);
+        return relationshipBuilder;
     }
 
     @Override
@@ -199,15 +201,21 @@ public List<Statement> updateRelationshipStatements() {
             return Collections.emptyList();
         }
 
-        Set<Edge> relationships = new HashSet<>(existingRelationshipBuilders.size());
-        List<Statement> statements = new ArrayList<>(existingRelationshipBuilders.size());
-        for (RelationshipBuilder relBuilder : existingRelationshipBuilders.values()) {
-            relationships.add(relBuilder.edge());
+        Map<Boolean, Set<Edge>> collect = existingRelationshipBuilders.values().stream()
+            .collect(partitioningBy(RelationshipBuilder::isDirty, Collectors.mapping(RelationshipBuilder::edge, Collectors.toSet())));
+
+        List<Statement> result = new ArrayList<>();
+        if (!collect.get(true).isEmpty()) {
+            ExistingRelationshipStatementBuilder builder = new ExistingRelationshipStatementBuilder(collect.get(true), statementFactory, true);
+            result.add(builder.build());
         }
-        ExistingRelationshipStatementBuilder existingRelationshipBuilder = new ExistingRelationshipStatementBuilder(
-            relationships, statementFactory);
-        statements.add(existingRelationshipBuilder.build());
-        return statements;
+
+        if (!collect.get(false).isEmpty()) {
+            ExistingRelationshipStatementBuilder builder = new ExistingRelationshipStatementBuilder(collect.get(false), statementFactory, false);
+            result.add(builder.build());
+        }
+
+        return result;
     }
 
     @Override

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

@@ -53,6 +53,9 @@ public interface RelationshipBuilder extends PropertyContainerBuilder<Relationsh
     void setRelationshipEntity(boolean relationshipEntity);
 
     boolean isNew();
+    boolean isDirty();
+
+    void setDirty(boolean dirty);
 
     Edge edge();
 

core/src/main/java/org/neo4j/ogm/cypher/compiler/builders/node/DefaultRelationshipBuilder.java

@@ -38,6 +38,7 @@ public class DefaultRelationshipBuilder extends AbstractPropertyContainerBuilder
     private boolean singleton = true; // will be false if the relationship can be mapped multiple times between two instances
     private boolean bidirectional = false;
     private boolean relationshipEntity = false;
+    private boolean dirty = true;
 
     public DefaultRelationshipBuilder(String type, boolean bidirectional) {
         this(type, null);
@@ -138,4 +139,14 @@ public RelationshipBuilder setVersionProperty(String name, Long version) {
         super.targetContainer.setVersion(new PropertyModel<>(name, version));
         return this;
     }
+
+    @Override
+    public boolean isDirty() {
+        return dirty;
+    }
+
+    @Override
+    public void setDirty(boolean dirty) {
+        this.dirty = dirty;
+    }
 }

core/src/main/java/org/neo4j/ogm/cypher/compiler/builders/statement/ExistingRelationshipStatementBuilder.java

@@ -41,9 +41,15 @@ public class ExistingRelationshipStatementBuilder implements CypherStatementBuil
 
     private final Set<Edge> edges;
 
-    public ExistingRelationshipStatementBuilder(Set<Edge> edges, StatementFactory statementFactory) {
+    /**
+     * Set to {@literal true} if all the edges above are dirty (changed) edges.
+     */
+    private final boolean dirtyEdges;
+
+    public ExistingRelationshipStatementBuilder(Set<Edge> edges, StatementFactory statementFactory, boolean dirtyOnes) {
         this.edges = edges;
         this.statementFactory = statementFactory;
+        this.dirtyEdges = dirtyOnes;
     }
 
     @Override
@@ -56,7 +62,7 @@ public Statement build() {
             queryBuilder.append("UNWIND $rows AS row MATCH ()-[r]->() WHERE ID(r) = row.relId ");
 
             if (firstEdge.hasVersionProperty()) {
-                queryBuilder.append(OptimisticLockingUtils.getFragmentForExistingNodesAndRelationships(firstEdge, "r"));
+                queryBuilder.append(OptimisticLockingUtils.getFragmentForExistingNodesAndRelationships(firstEdge, "r", dirtyEdges ? 1 : 0));
             }
 
             queryBuilder.append(firstEdge.createPropertyRemovalFragment("r"));

core/src/main/java/org/neo4j/ogm/cypher/compiler/builders/statement/OptimisticLockingUtils.java

@@ -38,9 +38,9 @@ final class OptimisticLockingUtils {
 
     private final static String VERSION_PROPERTY_CHECK_FOR_EXISTING_NODES_AND_RELATIONSHIPS = ""
         + "AND %1$s.`%2$s` = row.`%2$s` "
-        + "SET %1$s.`%2$s` = %1$s.`%2$s` + 1 "
+        + "SET %1$s.`%2$s` = %1$s.`%2$s` + %3$d "
         + "WITH %1$s, row "
-        + "WHERE %1$s.`%2$s` = row.`%2$s` + 1 ";
+        + "WHERE %1$s.`%2$s` = row.`%2$s` + %3$d ";
 
     /**
      * In case an entity with an externally assigned ID has also a @Version field, the version check is
@@ -56,14 +56,25 @@ final class OptimisticLockingUtils {
         + "WITH %1$s, row "
         + "WHERE (row.`%2$s` IS NULL OR %1$s.`%2$s` = row.`%2$s` + 1) ";
 
+
     /**
      * @param container node / relationship to check
      * @param variable  The variable representing the node or relationship to upgrade
-     * @return
+     * @return A fragment to be included in a statement to increment the version property
      */
     static String getFragmentForExistingNodesAndRelationships(PropertyContainer container, String variable) {
+        return getFragmentForExistingNodesAndRelationships(container, variable, 1);
+    }
+
+    /**
+     * @param container node / relationship to check
+     * @param variable  The variable representing the node or relationship to upgrade
+     * @param increment use {@literal 0} to avoid any increment (for non-dirty relationships)
+     * @return A fragment to be included in a statement to increment the version property
+     */
+    static String getFragmentForExistingNodesAndRelationships(PropertyContainer container, String variable, int increment) {
         String key = container.getVersion().getKey();
-        return String.format(VERSION_PROPERTY_CHECK_FOR_EXISTING_NODES_AND_RELATIONSHIPS, variable, key);
+        return String.format(VERSION_PROPERTY_CHECK_FOR_EXISTING_NODES_AND_RELATIONSHIPS, variable, key, increment);
     }
 
     /**

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/examples/locking/NodeOptimisticLockingTest.java

@@ -27,6 +27,7 @@
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.neo4j.ogm.domain.locking.FriendOf;
 import org.neo4j.ogm.domain.locking.Location;
 import org.neo4j.ogm.domain.locking.PowerUser;
 import org.neo4j.ogm.domain.locking.User;
@@ -232,7 +233,7 @@ public void assertWorkingLocksOnReReadWithChangedRelationships() {
         // Re-Read on purpose, see ticket
         bert = session2.load(User.class, bertId);
         bert.setName("Bert");
-        bert.addFriend(ernie);
+        FriendOf friendOf = bert.addFriend(ernie);
         session2.save(bert);
         assertThat(bert.getVersion()).isEqualTo(2L);
 
@@ -256,6 +257,19 @@ public void assertWorkingLocksOnReReadWithChangedRelationships() {
             .queryResults().iterator().next();
         session1.clear();
         assertThat(versions).containsEntry("userVersion", 3L);
+        assertThat(versions).containsEntry("relVersion", 0L);
+
+        friendOf.setDescription("an updated description");
+
+        session2.save(bert);
+        assertThat(bert.getVersion()).isEqualTo(3L);
+
+        versions = session1.query("MATCH (a:User {name: $name}) -[f:FRIEND_OF] -> (:User) RETURN"
+                + " a.version as userVersion, f.version as relVersion, f.description as description", Collections.singletonMap("name", bert.getName()))
+            .queryResults().iterator().next();
+        session1.clear();
+        assertThat(versions).containsEntry("userVersion", 3L);
         assertThat(versions).containsEntry("relVersion", 1L);
+        assertThat(versions).containsEntry("description", "an updated description");
     }
 }

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/examples/versioned_rel_entity/A.java

@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2002-2022 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.persistence.examples.versioned_rel_entity;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.NodeEntity;
+import org.neo4j.ogm.annotation.Relationship;
+
+/**
+ * @author Michael J. Simons
+ */
+@NodeEntity
+public class A {
+
+    @Id
+    private String id;
+
+    @Relationship(value = "R", direction = Relationship.Direction.INCOMING)
+    private List<R> bs = new ArrayList<>();
+
+    public A() {
+    }
+
+    public A(String id) {
+        this.id = id;
+    }
+
+    public String getId() {
+        return id;
+    }
+
+    public R add(B b) {
+        R r = new R(this, b);
+        this.bs.add(r);
+        return r;
+    }
+
+    public List<R> getBs() {
+        return bs;
+    }
+
+    @Override
+    public String toString() {
+        return "A{" +
+            "id='" + id + '\'' +
+            '}';
+    }
+}

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/examples/versioned_rel_entity/B.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2002-2022 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.persistence.examples.versioned_rel_entity;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.NodeEntity;
+
+/**
+ * @author Michael J. Simons
+ */
+@NodeEntity
+public class B {
+
+    @Id @GeneratedValue
+    private Long id;
+
+    private String name;
+
+    public B() {
+    }
+
+    public B(String name) {
+        this.name = name;
+    }
+
+    public Long getId() {
+        return id;
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    @Override public String toString() {
+        return "B{" +
+            "id=" + id +
+            ", name='" + name + '\'' +
+            '}';
+    }
+}