GH-666 - Check for contradicting annotations before mapping possible …

…related nodes.

The entity access manager tries several ways to find a field for mapping relationships. Usually, more than 3 tries are necessary for custom queries. The last approach just checks if there is any field of the same type as a currently mapped relationship and just dumps the relationships content into it.

This is bad in case when the field type matches but there’s actually an annotation contradicting it.

This commit makes the entity access manager check for contradicting annotations and if there is one, doesn’t fill in the object.

This is a backport of 0a9a921.

core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java

@@ -400,10 +400,6 @@ public Class<?> type() {
         return fieldType;
     }
 
-    public String relationshipName() {
-        return this.relationship();
-    }
-
     public boolean forScalar() {
         return !Iterable.class.isAssignableFrom(type()) && !type().isArray();
     }

core/src/main/java/org/neo4j/ogm/metadata/reflect/EntityAccessManager.java

@@ -303,11 +303,10 @@ public static FieldInfo getRelationalWriter(ClassInfo classInfo, String relation
     public static FieldInfo getRelationalWriter(ClassInfo classInfo, String relationshipType,
         String relationshipDirection, Class<?> objectType) {
 
-        final ClassInfo lookupClassInfo = classInfo;
         final DirectedRelationshipForType directedRelationship = new DirectedRelationshipForType(relationshipType,
             relationshipDirection, objectType);
         final Map<DirectedRelationshipForType, FieldInfo> typeFieldInfoMap = relationalWriterCache
-            .computeIfAbsent(lookupClassInfo, key -> new ConcurrentHashMap<>());
+            .computeIfAbsent(classInfo, key -> new ConcurrentHashMap<>());
 
         if (typeFieldInfoMap.containsKey(directedRelationship)) {
             return typeFieldInfoMap.get(directedRelationship);
@@ -361,11 +360,14 @@ public static FieldInfo getRelationalWriter(ClassInfo classInfo, String relation
                 // 4th, try to find a unique field that has the same type as the parameter
                 List<FieldInfo> fieldInfos = classInfo.findFields(objectType);
                 if (fieldInfos.size() == 1) {
-                    FieldInfo candidateFieldInfo = fieldInfos.iterator().next();
-                    if (!candidateFieldInfo.relationshipDirection(Relationship.UNDIRECTED)
-                        .equals(Relationship.INCOMING)) {
-                        typeFieldInfoMap.put(directedRelationship, candidateFieldInfo);
-                        return candidateFieldInfo;
+                    FieldInfo candidateField = fieldInfos.iterator().next();
+
+                    if (!candidateField.relationshipDirection(Relationship.UNDIRECTED).equals(Relationship.INCOMING)) {
+
+                        if (candidateField.relationshipTypeAnnotation() == null) {
+                            typeFieldInfoMap.put(directedRelationship, candidateField);
+                            return candidateField;
+                        }
                     }
                 }
             }
@@ -386,11 +388,10 @@ public static FieldInfo getRelationalWriter(ClassInfo classInfo, String relation
     public static FieldInfo getRelationalReader(ClassInfo classInfo, String relationshipType,
         String relationshipDirection) {
 
-        final ClassInfo lookupClassInfo = classInfo;
         final DirectedRelationship directedRelationship = new DirectedRelationship(relationshipType,
             relationshipDirection);
         final Map<DirectedRelationship, FieldInfo> relationshipFieldInfoMap = relationalReaderCache
-            .computeIfAbsent(lookupClassInfo, key -> new ConcurrentHashMap<>());
+            .computeIfAbsent(classInfo, key -> new ConcurrentHashMap<>());
 
         if (relationshipFieldInfoMap.containsKey(directedRelationship)) {
             return relationshipFieldInfoMap.get(directedRelationship);

test/src/test/java/org/neo4j/ogm/domain/gh666/MessedUpNode1.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2002-2019 "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.domain.gh666;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+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 MessedUpNode1 {
+
+    @Id
+    @GeneratedValue
+    private Long id;
+
+    @Relationship(type = "RELATION_A")
+    private MessedUpNode2 ref;
+
+    public Long getId() {
+        return id;
+    }
+
+    public MessedUpNode2 getRef() {
+        return ref;
+    }
+
+    public void setRef(MessedUpNode2 ref) {
+        this.ref = ref;
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh666/MessedUpNode2.java

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2002-2019 "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.domain.gh666;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+import org.neo4j.ogm.annotation.Id;
+import org.neo4j.ogm.annotation.Labels;
+import org.neo4j.ogm.annotation.NodeEntity;
+
+/**
+ * @author Michael J. Simons
+ */
+@NodeEntity
+public class MessedUpNode2 {
+
+    @Id
+    @GeneratedValue
+    private Long id;
+
+    @Labels
+    private List<String> types = new ArrayList<>();
+
+    public Long getId() {
+        return id;
+    }
+
+    public List<String> getTypes() {
+        return types;
+    }
+
+    public void setTypes(List<String> types) {
+        this.types = types;
+    }
+}

test/src/test/java/org/neo4j/ogm/domain/gh666/MessedUpNode3.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright (c) 2002-2019 "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.domain.gh666;
+
+import org.neo4j.ogm.annotation.GeneratedValue;
+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 MessedUpNode3 {
+
+    @Id
+    @GeneratedValue
+    private Long id;
+
+    private MessedUpNode2 relationA;
+
+    public Long getId() {
+        return id;
+    }
+
+    public MessedUpNode2 getRef() {
+        return relationA;
+    }
+
+    public void setRef(MessedUpNode2 ref) {
+        this.relationA = ref;
+    }
+}

test/src/test/java/org/neo4j/ogm/metadata/reflect/RelationshipWriterAnnotatedFieldsTest.java

@@ -33,6 +33,7 @@
 
 /**
  * @author Vince Bickers
+ * @author Michael J. Simons
  */
 public class RelationshipWriterAnnotatedFieldsTest {
 
@@ -47,7 +48,7 @@ public void shouldFindWriterForCollection() {
             .getRelationalWriter(classInfo, "LIST", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("LIST");
+        assertThat(objectAccess.relationship()).isEqualTo("LIST");
         assertThat(objectAccess.type()).isEqualTo(List.class);
     }
 
@@ -60,7 +61,7 @@ public void shouldFindWriterForScalar() {
             .getRelationalWriter(classInfo, "SCALAR", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("SCALAR");
+        assertThat(objectAccess.relationship()).isEqualTo("SCALAR");
         assertThat(objectAccess.type()).isEqualTo(T.class);
     }
 
@@ -73,7 +74,7 @@ public void shouldFindWriterForArray() {
             .getRelationalWriter(classInfo, "ARRAY", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("ARRAY");
+        assertThat(objectAccess.relationship()).isEqualTo("ARRAY");
         assertThat(objectAccess.type()).isEqualTo(T[].class);
     }
 

test/src/test/java/org/neo4j/ogm/metadata/reflect/RelationshipWriterPlainFieldsTest.java

@@ -30,6 +30,7 @@
 
 /**
  * @author Vince Bickers
+ * @author Michael J. Simons
  */
 public class RelationshipWriterPlainFieldsTest {
 
@@ -44,7 +45,7 @@ public void shouldFindWriterForCollection() {
             .getRelationalWriter(classInfo, "LIST", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("LIST");
+        assertThat(objectAccess.relationship()).isEqualTo("LIST");
     }
 
     @Test
@@ -56,7 +57,7 @@ public void shouldFindWriterForScalar() {
             .getRelationalWriter(classInfo, "SCALAR", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("SCALAR");
+        assertThat(objectAccess.relationship()).isEqualTo("SCALAR");
     }
 
     @Test
@@ -68,7 +69,7 @@ public void shouldFindWriterForArray() {
             .getRelationalWriter(classInfo, "ARRAY", Relationship.OUTGOING, new T());
         assertThat(objectAccess).as("The resultant object accessor shouldn't be null").isNotNull();
         assertThat(objectAccess instanceof FieldInfo).as("The access mechanism should be via the field").isTrue();
-        assertThat(objectAccess.relationshipName()).isEqualTo("ARRAY");
+        assertThat(objectAccess.relationship()).isEqualTo("ARRAY");
     }
 
     static class S {

test/src/test/java/org/neo4j/ogm/persistence/model/RelationshipMappingTest.java

@@ -25,6 +25,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.function.Function;
 
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -44,6 +45,7 @@
 import org.neo4j.ogm.domain.policy.Policy;
 import org.neo4j.ogm.domain.typed_relationships.SomeEntity;
 import org.neo4j.ogm.domain.typed_relationships.TypedEntity;
+import org.neo4j.ogm.metadata.ClassInfo;
 import org.neo4j.ogm.session.Session;
 import org.neo4j.ogm.session.SessionFactory;
 import org.neo4j.ogm.testutil.GraphTestUtils;
@@ -67,6 +69,7 @@ public static void oneTimeSetup() {
             "org.neo4j.ogm.domain.gh641",
             "org.neo4j.ogm.domain.typed_relationships",
             "org.neo4j.ogm.domain.gh656",
+            "org.neo4j.ogm.domain.gh666",
             "org.neo4j.ogm.domain.gh704");
     }
 
@@ -505,4 +508,63 @@ public void deletesOfEntitiesWithTheSameButNotEqualParentShouldWork3() {
         assertThat(actual).hasSize(1);
         assertThat(actual.iterator().next()).containsEntry("relTwo", true);
     }
+
+    @Test // GH-666
+    public void shouldNotMessUpNodes() {
+        assertMapping(org.neo4j.ogm.domain.gh666.MessedUpNode1.class, node -> node.getRef(), false);
+    }
+
+    @Test // GH-666
+    public void mayMessUpNodes() {
+        assertMapping(org.neo4j.ogm.domain.gh666.MessedUpNode3.class, node -> node.getRef(), true);
+    }
+
+    private <T> void assertMapping(Class<T> targetClass, Function<T, org.neo4j.ogm.domain.gh666.MessedUpNode2> refExtractor,
+        boolean wildCardMappingAllowed) {
+
+        ClassInfo targetclassInfo = sessionFactory.metaData().classInfo(targetClass);
+
+        Map<String, Object> ids = sessionFactory.openSession().query(""
+                + "MERGE (n3:MessedUpNode2:TypeY) <-[:RELATION_A] - (n1:" + targetclassInfo.neo4jName()
+                + ")-[:RELATION_NOT_MODELED]->(n2:MessedUpNode2:TypeX) "
+                + "RETURN id(n1) as id1, id(n2) as id2, id(n3) as id3",
+            Collections.emptyMap()
+        ).queryResults().iterator().next();
+
+        Session session = sessionFactory.openSession();
+        T n1 = session.load(targetClass, (long) ids.get("id1"));
+        org.neo4j.ogm.domain.gh666.MessedUpNode2 n2 = session.load(org.neo4j.ogm.domain.gh666.MessedUpNode2.class, (long) ids.get("id2"));
+        org.neo4j.ogm.domain.gh666.MessedUpNode2 n3 = session.load(org.neo4j.ogm.domain.gh666.MessedUpNode2.class, (long) ids.get("id3"));
+
+        assertThat(n1).isNotNull();
+        assertThat(n2).isNotNull();
+        assertThat(n3).isNotNull();
+
+        // Expected behaviour
+        assertThat(refExtractor.apply(n1)).isEqualTo(n3);
+
+        // Loaded from the session
+        String query = "MATCH (n1:" + targetclassInfo.neo4jName()
+            + ")-[rel:RELATION_NOT_MODELED]->(n2:`TypeX`) WHERE id(n1) = $id1 RETURN n1, rel, n2";
+        Iterable<Map<String, Object>> r = session
+            .query(query, Collections.singletonMap("id1", (long) ids.get("id1")), true).queryResults();
+        assertCorrectRelationShip(r, wildCardMappingAllowed ? n2 : n3);
+
+        // Loaded from the database
+        session = sessionFactory.openSession();
+        r = session.query(query, Collections.singletonMap("id1", (long) ids.get("id1")), true).queryResults();
+        assertCorrectRelationShip(r, wildCardMappingAllowed ? n2 : null);
+    }
+
+    private static void assertCorrectRelationShip(Iterable<Map<String, Object>> result, org.neo4j.ogm.domain.gh666.MessedUpNode2 expectedNode) {
+        assertThat(result).hasSize(1);
+        assertThat(result).allSatisfy(row -> {
+            assertThat(row).containsKeys("n1");
+            if (expectedNode != null) {
+                assertThat(row.get("n1")).extracting("ref").extracting("id").containsExactly(expectedNode.getId());
+            } else {
+                assertThat(row.get("n1")).extracting("ref").hasSize(1).containsNull();
+            }
+        });
+    }
 }