GH-2345 - Add missing relationships during mapping.

If it is possible (the structure is mutable),
SDN should try to add relationships to objects if those information
come in subsequent records.

closes #2345

Author: meistermeier

src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

@@ -102,6 +102,8 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter {
 	public <R> R read(Class<R> targetType, MapAccessor mapAccessor) {
 
 		Neo4jPersistentEntity<R> rootNodeDescription = (Neo4jPersistentEntity) nodeDescriptionStore.getNodeDescription(targetType);
+		knownObjects.nextRecord();
+
 		MapAccessor queryRoot = determineQueryRoot(mapAccessor, rootNodeDescription);
 		if (queryRoot == null) {
 			throw new NoRootNodeMappingException(String.format("Could not find mappable nodes or relationships inside %s for %s", mapAccessor, rootNodeDescription));
@@ -272,45 +274,77 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
 			Neo4jPersistentEntity<ET> concreteNodeDescription = (Neo4jPersistentEntity<ET>) nodeDescriptionAndLabels
 					.getNodeDescription();
 
-			boolean isKotlinType = KotlinDetector.isKotlinType(concreteNodeDescription.getType());
 			ET instance = instantiate(concreteNodeDescription, queryResult,
 					nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult);
 
 			knownObjects.removeFromInCreation(internalId);
-			PersistentPropertyAccessor<ET> propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance);
 
-			if (concreteNodeDescription.requiresPropertyPopulation()) {
-
-				// Fill simple properties
-				Predicate<Neo4jPersistentProperty> isConstructorParameter = concreteNodeDescription
-						.getPersistenceConstructor()::isConstructorParameter;
-				PropertyHandler<Neo4jPersistentProperty> handler = populateFrom(queryResult, propertyAccessor,
-						isConstructorParameter, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, isKotlinType);
-				concreteNodeDescription.doWithProperties(handler);
-
-				// in a cyclic graph / with bidirectional relationships, we could end up in a state in which we
-				// reference the start again. Because it is getting still constructed, it won't be in the knownObjects
-				// store unless we temporarily put it there.
-				knownObjects.storeObject(internalId, instance);
-				// Fill associations
-				concreteNodeDescription.doWithAssociations(
-						populateFrom(queryResult, propertyAccessor, isConstructorParameter, relationshipsFromResult, nodesFromResult));
-			}
+			populateProperties(queryResult, nodeDescription, internalId, instance, lastMappedEntity, relationshipsFromResult, nodesFromResult, false);
+
+			PersistentPropertyAccessor<ET> propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance);
 			ET bean = propertyAccessor.getBean();
 
 			// save final state of the bean
 			knownObjects.storeObject(internalId, bean);
 			return bean;
 		};
 
-		Object mappedObject = knownObjects.getObject(internalId);
+		ET mappedObject = (ET) knownObjects.getObject(internalId);
 		if (mappedObject == null) {
-			mappedObject = mappedObjectSupplier.get();
+			mappedObject = (ET) mappedObjectSupplier.get();
 			knownObjects.storeObject(internalId, mappedObject);
+		} else if (knownObjects.alreadyMappedInPreviousRecord(internalId)) {
+			// If the object were created in a run before, it _could_ have missing relationships
+			// (e.g. due to incomplete fetching by a custom query)
+			// in such cases we will add the additional data from the next record.
+			// This can and should only work for
+			// 1. mutable owning types
+			// AND (!!!)
+			// 2. mutable target types
+			// because we cannot just create new instances
+			populateProperties(queryResult, nodeDescription, internalId, mappedObject, lastMappedEntity, relationshipsFromResult, nodesFromResult, true);
 		}
 		return (ET) mappedObject;
 	}
 
+
+	private <ET> void populateProperties(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescription, Long internalId,
+										 ET mappedObject, @Nullable Object lastMappedEntity,
+										 Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult, boolean objectAlreadyMapped) {
+
+		List<String> allLabels = getLabels(queryResult, nodeDescription);
+		NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore
+				.deriveConcreteNodeDescription(nodeDescription, allLabels);
+
+		@SuppressWarnings("unchecked")
+		Neo4jPersistentEntity<ET> concreteNodeDescription = (Neo4jPersistentEntity<ET>) nodeDescriptionAndLabels
+				.getNodeDescription();
+
+		if (!concreteNodeDescription.requiresPropertyPopulation()) {
+			return;
+		}
+
+		PersistentPropertyAccessor<ET> propertyAccessor = concreteNodeDescription.getPropertyAccessor(mappedObject);
+		Predicate<Neo4jPersistentProperty> isConstructorParameter = concreteNodeDescription
+				.getPersistenceConstructor()::isConstructorParameter;
+
+		// if the object were mapped before, we assume that at least all properties are populated
+		if (!objectAlreadyMapped) {
+			boolean isKotlinType = KotlinDetector.isKotlinType(concreteNodeDescription.getType());
+			// Fill simple properties
+			PropertyHandler<Neo4jPersistentProperty> handler = populateFrom(queryResult, propertyAccessor,
+					isConstructorParameter, nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, isKotlinType);
+			concreteNodeDescription.doWithProperties(handler);
+		}
+		// in a cyclic graph / with bidirectional relationships, we could end up in a state in which we
+		// reference the start again. Because it is getting still constructed, it won't be in the knownObjects
+		// store unless we temporarily put it there.
+		knownObjects.storeObject(internalId, mappedObject);
+		// Fill associations
+		concreteNodeDescription.doWithAssociations(
+				populateFrom(queryResult, propertyAccessor, isConstructorParameter, objectAlreadyMapped, relationshipsFromResult, nodesFromResult));
+	}
+
 	@Nullable
 	private Long getInternalId(@NonNull MapAccessor queryResult) {
 		return queryResult instanceof Node
@@ -398,8 +432,9 @@ public Object getParameterValue(PreferredConstructor.Parameter parameter) {
 	}
 
 	private PropertyHandler<Neo4jPersistentProperty> populateFrom(MapAccessor queryResult,
-			PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter,
-			Collection<String> surplusLabels, Object targetNode, boolean ownerIsKotlinType) {
+			  PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter,
+			  Collection<String> surplusLabels, @Nullable Object targetNode, boolean ownerIsKotlinType) {
+
 		return property -> {
 			if (isConstructorParameter.test(property)) {
 				return;
@@ -421,20 +456,55 @@ private PropertyHandler<Neo4jPersistentProperty> populateFrom(MapAccessor queryR
 		};
 	}
 
+	@Nullable
 	private static Object getValueOrDefault(boolean ownerIsKotlinType, Class<?> rawType, @Nullable Object value) {
 
 		return value == null && !ownerIsKotlinType && rawType.isPrimitive() ? ReflectionUtils.getPrimitiveDefault(rawType) : value;
 	}
 
 	private AssociationHandler<Neo4jPersistentProperty> populateFrom(MapAccessor queryResult,
-			PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter, Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult) {
+			PersistentPropertyAccessor<?> propertyAccessor, Predicate<Neo4jPersistentProperty> isConstructorParameter,
+		    boolean objectAlreadyMapped, Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult) {
+
 		return association -> {
 
 			Neo4jPersistentProperty persistentProperty = association.getInverse();
+
 			if (isConstructorParameter.test(persistentProperty)) {
 				return;
 			}
 
+			if (objectAlreadyMapped) {
+
+				// avoid multiple instances of the "same" object
+				boolean willCreateNewInstance = persistentProperty.getWither() != null;
+				if (willCreateNewInstance) {
+					throw new MappingException("Cannot create a new instance of an already existing object.");
+				}
+
+				Object propertyValue = propertyAccessor.getProperty(persistentProperty);
+
+				boolean propertyValueNotNull = propertyValue != null;
+
+				boolean populatedCollection = persistentProperty.isCollectionLike()
+						&& propertyValueNotNull
+						&& !((Collection<?>) propertyValue).isEmpty();
+
+				boolean populatedMap = persistentProperty.isMap()
+						&& propertyValueNotNull
+						&& !((Map<?, ?>) propertyValue).isEmpty();
+
+				boolean populatedScalarValue = !persistentProperty.isCollectionLike()
+						&& propertyValueNotNull;
+
+				boolean propertyAlreadyPopulated = populatedCollection || populatedMap || populatedScalarValue;
+
+				// avoid unnecessary re-assignment of values
+				if (propertyAlreadyPopulated) {
+					return;
+				}
+			}
+
 			createInstanceOfRelationships(persistentProperty, queryResult, (RelationshipDescription) association, relationshipsFromResult, nodesFromResult)
 					.ifPresent(value -> propertyAccessor.setProperty(persistentProperty, value));
 		};
@@ -654,6 +724,7 @@ static class KnownObjects {
 		private final Lock write = lock.writeLock();
 
 		private final Map<Long, Object> internalIdStore = new HashMap<>();
+		private final Map<Long, Boolean> internalNextRecord = new HashMap<>();
 		private final Set<Long> idsInCreation = new HashSet<>();
 
 		private void storeObject(@Nullable Long internalId, Object object) {
@@ -664,6 +735,7 @@ private void storeObject(@Nullable Long internalId, Object object) {
 				write.lock();
 				idsInCreation.remove(internalId);
 				internalIdStore.put(internalId, object);
+				internalNextRecord.put(internalId, false);
 			} finally {
 				write.unlock();
 			}
@@ -725,5 +797,32 @@ private void removeFromInCreation(@Nullable Long internalId) {
 				write.unlock();
 			}
 		}
+
+		private boolean alreadyMappedInPreviousRecord(@Nullable Long internalId) {
+			if (internalId == null) {
+				return false;
+			}
+			try {
+
+				read.lock();
+
+				Boolean nextRecord = internalNextRecord.get(internalId);
+
+				if (nextRecord != null) {
+					return nextRecord;
+				}
+
+			} finally {
+				read.unlock();
+			}
+			return false;
+		}
+
+		/**
+		 * Mark all currently existing objects as mapped.
+		 */
+		private void nextRecord() {
+			internalNextRecord.replaceAll((x, y) -> true);
+		}
 	}
 }

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java

@@ -116,6 +116,7 @@
 import org.springframework.data.neo4j.integration.shared.common.FriendshipRelationship;
 import org.springframework.data.neo4j.integration.shared.common.Hobby;
 import org.springframework.data.neo4j.integration.shared.common.ImmutablePerson;
+import org.springframework.data.neo4j.integration.shared.common.ImmutablePet;
 import org.springframework.data.neo4j.integration.shared.common.Inheritance;
 import org.springframework.data.neo4j.integration.shared.common.KotlinPerson;
 import org.springframework.data.neo4j.integration.shared.common.LikesHobbyRelationship;
@@ -709,6 +710,27 @@ void customFindMapsDeepRelationships(@Autowired PetRepository repository) {
 			assertThat(pet2.getFriends()).containsExactly(comparisonPet3);
 		}
 
+		@Test // GH-2345
+		void customFindHydratesIncompleteCustomQueryObjectsCorrect(@Autowired PetRepository repository) {
+			doWithSession(session ->
+				session.run("CREATE (:Pet{name: 'Luna'})-[:Has]->(:Pet{name:'Luna'})-[:Has]->(:Pet{name:'Daphne'})").consume()
+			);
+
+			List<Pet> pets = repository.findLunas();
+			assertThat(pets).hasSize(2);
+
+			assertThat(pets).allMatch(pet -> !pet.getFriends().isEmpty());
+		}
+
+		@Test // GH-2345
+		void customFindFailsOnHydrationOfCustomQueryObjectsIfImmutable(@Autowired ImmutablePetRepository repository) {
+			doWithSession(session ->
+				session.run("CREATE (:ImmutablePet{name: 'Luna'})-[:Has]->(:ImmutablePet{name:'Luna'})-[:Has]->(:ImmutablePet{name:'Daphne'})").consume()
+			);
+
+			assertThatExceptionOfType(MappingException.class).isThrownBy(repository::findLunas);
+		}
+
 		@Test // DATAGRAPH-1409
 		void findPageWithCustomQuery(@Autowired PetRepository repository) {
 
@@ -4128,6 +4150,16 @@ interface PetRepository extends Neo4jRepository<Pet, Long> {
 		long countByFriendsNameAndFriendsFriendsName(String friendName, String friendFriendName);
 
 		boolean existsByName(String name);
+
+		@Query("MATCH (n:Pet) where n.name='Luna' OPTIONAL MATCH (n)-[r:Has]->(m:Pet) return n, collect(r), collect(m)")
+		List<Pet> findLunas();
+	}
+
+	interface ImmutablePetRepository extends Neo4jRepository<ImmutablePet, Long> {
+
+		@Query("MATCH (n:ImmutablePet) where n.name='Luna' OPTIONAL MATCH (n)-[r:Has]->(m:ImmutablePet) return n, collect(r), collect(m)")
+		List<ImmutablePet> findLunas();
+
 	}
 
 	interface OneToOneRepository extends Neo4jRepository<OneToOneSource, String> {

src/test/java/org/springframework/data/neo4j/integration/shared/common/ImmutablePet.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2011-2021 the original author or authors.
+ *
+ * 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
+ *
+ *      https://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.springframework.data.neo4j.integration.shared.common;
+
+import org.springframework.data.annotation.PersistenceConstructor;
+import org.springframework.data.neo4j.core.schema.GeneratedValue;
+import org.springframework.data.neo4j.core.schema.Id;
+import org.springframework.data.neo4j.core.schema.Node;
+import org.springframework.data.neo4j.core.schema.Relationship;
+
+import java.util.Set;
+
+/**
+ * @author Gerrit Meier
+ */
+@Node
+public class ImmutablePet {
+
+	@Id
+	@GeneratedValue
+	public final Long id;
+
+	public final String name;
+
+	@Relationship("Has")
+	public final Set<ImmutablePet> friends;
+
+	@PersistenceConstructor
+	public ImmutablePet(Long id, String name) {
+		this.id = id;
+		this.name = name;
+		this.friends = null;
+	}
+
+	public ImmutablePet(Long id, String name, Set<ImmutablePet> friends) {
+		this.id = id;
+		this.name = name;
+		this.friends = friends;
+	}
+
+	public ImmutablePet withFriends(Set<ImmutablePet> newFriends) {
+		return new ImmutablePet(id, name, newFriends);
+	}
+}