GH-2340 - Clear exception if mapping recursive immutable objects.

Also improving the documentation around this topic.

Closes #2340

Author: meistermeier

src/main/asciidoc/object-mapping/sdc-object-mapping.adoc

@@ -222,6 +222,16 @@ It's an established pattern to rather use static factory methods to expose these
 * _Use Lombok to avoid boilerplate code_ --
 As persistence operations usually require a constructor taking all arguments, their declaration becomes a tedious repetition of boilerplate parameter to field assignments that can best be avoided by using Lombok's `@AllArgsConstructor`.
 
+[[mapping.fundamentals.recommendations.note-immutable]]
+=== A note on immutable mapping
+
+Although we recommend to use immutable mapping and constructs wherever possible, there are some limitations when it comes to mapping.
+Given a bidirectional relationship where `A` has a constructor reference to `B` and `B` has a reference to `A`, or a more complex scenario.
+This hen/egg situation is not solvable for Spring Data Neo4j.
+During the instantiation of `A` it eagerly needs to have a fully instantiated `B`, which on the other hand requires an instance (to be precise, the _same_ instance) of `A`.
+SDN allows such models in general, but will throw a `MappingException` at runtime if the data that gets returned from the database contains such constellation as described above.
+In such cases or scenarios, where you cannot foresee what the data that gets returned looks like, you are better suited with a mutable field for the relationships.
+
 [[mapping.fundamentals.kotlin]]
 == Kotlin support
 

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

@@ -254,6 +254,15 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
 		Long internalId = getInternalId(queryResult);
 
 		Supplier<Object> mappedObjectSupplier = () -> {
+			if (knownObjects.isInCreation(internalId)) {
+				throw new MappingException(
+						String.format(
+								"The node with id %s has a logical cyclic mapping dependency. " +
+								"Its creation caused the creation of another node that has a reference to this.",
+								internalId)
+				);
+			}
+			knownObjects.setInCreation(internalId);
 
 			List<String> allLabels = getLabels(queryResult, nodeDescription);
 			NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore
@@ -264,6 +273,7 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
 			ET instance = instantiate(concreteNodeDescription, queryResult,
 					nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult);
 
+			knownObjects.removeFromInCreation(internalId);
 			PersistentPropertyAccessor<ET> propertyAccessor = concreteNodeDescription.getPropertyAccessor(instance);
 
 			if (concreteNodeDescription.requiresPropertyPopulation()) {
@@ -634,19 +644,45 @@ static class KnownObjects {
 		private final Lock write = lock.writeLock();
 
 		private final Map<Long, Object> internalIdStore = new HashMap<>();
+		private final Set<Long> idsInCreation = new HashSet<>();
 
 		private void storeObject(@Nullable Long internalId, Object object) {
 			if (internalId == null) {
 				return;
 			}
 			try {
 				write.lock();
+				idsInCreation.remove(internalId);
 				internalIdStore.put(internalId, object);
 			} finally {
 				write.unlock();
 			}
 		}
 
+		private void setInCreation(@Nullable Long internalId) {
+			if (internalId == null) {
+				return;
+			}
+			try {
+				write.lock();
+				idsInCreation.add(internalId);
+			} finally {
+				write.unlock();
+			}
+		}
+
+		private boolean isInCreation(@Nullable Long internalId) {
+			if (internalId == null) {
+				return false;
+			}
+			try {
+				read.lock();
+				return idsInCreation.contains(internalId);
+			} finally {
+				read.unlock();
+			}
+		}
+
 		@Nullable
 		private Object getObject(@Nullable Long internalId) {
 			if (internalId == null) {
@@ -667,5 +703,17 @@ private Object getObject(@Nullable Long internalId) {
 			}
 			return null;
 		}
+
+		private void removeFromInCreation(@Nullable Long internalId) {
+			if (internalId == null) {
+				return;
+			}
+			try {
+				write.lock();
+				idsInCreation.remove(internalId);
+			} finally {
+				write.unlock();
+			}
+		}
 	}
 }

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

@@ -18,6 +18,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.tuple;
 
 import java.time.Instant;
@@ -78,6 +79,7 @@
 import org.springframework.data.geo.Distance;
 import org.springframework.data.geo.Metrics;
 import org.springframework.data.geo.Polygon;
+import org.springframework.data.mapping.MappingException;
 import org.springframework.data.neo4j.config.AbstractNeo4jConfig;
 import org.springframework.data.neo4j.core.DatabaseSelection;
 import org.springframework.data.neo4j.core.DatabaseSelectionProvider;
@@ -977,21 +979,17 @@ void findEntityWithRelationshipToTheSameNode(@Autowired RelationshipRepository r
 		}
 
 		@Test
-		void findEntityWithBidirectionalRelationship(@Autowired BidirectionalStartRepository repository) {
+		void findEntityWithBidirectionalRelationshipInConstructorThrowsException(@Autowired BidirectionalStartRepository repository) {
 
 			long startId = doWithSession(session ->  session
 						.run("CREATE (n:BidirectionalStart{name:'Ernie'})-[:CONNECTED]->(e:BidirectionalEnd{name:'Bert'}), "
 							 + "(e)<-[:ANOTHER_CONNECTION]-(anotherStart:BidirectionalStart{name:'Elmo'})" + "RETURN n")
 						.single().get("n").asNode().id());
 
-			Optional<BidirectionalStart> entityOptional = repository.findById(startId);
-			assertThat(entityOptional).isPresent();
-			BidirectionalStart entity = entityOptional.get();
-			assertThat(entity.getEnds()).hasSize(1);
-
-			BidirectionalEnd end = entity.getEnds().iterator().next();
-			assertThat(end.getAnotherStart()).isNotNull();
-			assertThat(end.getAnotherStart().getName()).isEqualTo("Elmo");
+			assertThatThrownBy(() -> repository.findById(startId))
+					.hasRootCauseMessage("The node with id " + startId + " has a logical cyclic mapping dependency. " +
+							"Its creation caused the creation of another node that has a reference to this.")
+					.hasRootCauseInstanceOf(MappingException.class);
 
 		}
 

src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java

@@ -19,6 +19,7 @@
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.tuple;
 
+import org.springframework.data.mapping.MappingException;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 import reactor.test.StepVerifier;
@@ -829,9 +830,14 @@ void loadEntityWithBidirectionalRelationship(@Autowired BidirectionalStartReposi
 				return startNode.id();
 			});
 
-			StepVerifier.create(repository.findById(startId)).assertNext(entity -> {
-				assertThat(entity.getEnds()).hasSize(1);
-			}).verifyComplete();
+			StepVerifier.create(repository.findById(startId))
+					.verifyErrorMatches(error -> {
+						Throwable cause = error.getCause();
+						return cause instanceof MappingException && cause.getMessage().equals(
+								"The node with id " + startId + " has a logical cyclic mapping dependency. " +
+							"Its creation caused the creation of another node that has a reference to this.");
+					});
+
 		}
 
 		@Test