GH-2572 - Filter out records that could not be mapped to allow Cypher OPTIONAL returns.

This fixes #2572 and is a tough call: The query presented there is totally valid but returns not an empty result but a record containing only one `null` element due to the way the Cyper `OPTIONAL` keyword works. We could try parsing the query, but this will be an endless rabbit whole in the future to come.

The approach here is to check now whether a result contains exactly one record with one `NULL` value. If that is the case, no exception is thrown but  `null` returned, which is later filtered on the clients.

That however drops the checking for non-null values there, so that methods don’t throw in those cases but returns empty optionals or empty / smaller lists.

Author: michael-simons

src/main/java/org/springframework/data/neo4j/core/DefaultNeo4jClient.java

@@ -19,6 +19,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
@@ -442,7 +443,7 @@ class DefaultRecordFetchSpec<T> implements RecordFetchSpec<T>, MappingSpec<T> {
 		public RecordFetchSpec<T> mappedBy(
 				@SuppressWarnings("HiddenField") BiFunction<TypeSystem, Record, T> mappingFunction) {
 
-			this.mappingFunction = new DelegatingMappingFunctionWithNullCheck<>(mappingFunction);
+			this.mappingFunction = mappingFunction;
 			return this;
 		}
 
@@ -468,7 +469,7 @@ public Optional<T> first() {
 
 			try (QueryRunner statementRunner = getQueryRunner(this.databaseSelection, this.impersonatedUser)) {
 				Result result = runnableStatement.runWith(statementRunner);
-				Optional<T> optionalValue = result.stream().map(partialMappingFunction(typeSystem)).findFirst();
+				Optional<T> optionalValue = result.stream().map(partialMappingFunction(typeSystem)).filter(Objects::nonNull).findFirst();
 				ResultSummaries.process(result.consume());
 				return optionalValue;
 			} catch (RuntimeException e) {
@@ -483,7 +484,7 @@ public Collection<T> all() {
 
 			try (QueryRunner statementRunner = getQueryRunner(this.databaseSelection, this.impersonatedUser)) {
 				Result result = runnableStatement.runWith(statementRunner);
-				Collection<T> values = result.stream().map(partialMappingFunction(typeSystem)).collect(Collectors.toList());
+				Collection<T> values = result.stream().map(partialMappingFunction(typeSystem)).filter(Objects::nonNull).collect(Collectors.toList());
 				ResultSummaries.process(result.consume());
 				return values;
 			} catch (RuntimeException e) {

src/main/java/org/springframework/data/neo4j/core/DefaultReactiveNeo4jClient.java

@@ -398,7 +398,7 @@ class DefaultRecordFetchSpec<T> implements RecordFetchSpec<T>, MappingSpec<T> {
 		@Override
 		public RecordFetchSpec<T> mappedBy(@SuppressWarnings("HiddenField") BiFunction<TypeSystem, Record, T> mappingFunction) {
 
-			this.mappingFunction = new DelegatingMappingFunctionWithNullCheck<>(mappingFunction);
+			this.mappingFunction = mappingFunction;
 			return this;
 		}
 

src/main/java/org/springframework/data/neo4j/core/DelegatingMappingFunctionWithNullCheck.java

@@ -43,6 +43,7 @@
 public interface Neo4jClient {
 
 	LogAccessor cypherLog = new LogAccessor(LogFactory.getLog("org.springframework.data.neo4j.cypher"));
+	LogAccessor log = new LogAccessor(LogFactory.getLog(Neo4jClient.class));
 
 	static Neo4jClient create(Driver driver) {
 

src/main/java/org/springframework/data/neo4j/core/Neo4jClient.java

@@ -46,6 +46,7 @@
 public interface ReactiveNeo4jClient {
 
 	LogAccessor cypherLog = new LogAccessor(LogFactory.getLog("org.springframework.data.neo4j.cypher"));
+	LogAccessor log = new LogAccessor(LogFactory.getLog(ReactiveNeo4jClient.class));
 
 	static ReactiveNeo4jClient create(Driver driver) {
 

src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jClient.java

@@ -104,6 +104,7 @@ final class DefaultNeo4jEntityConverter implements Neo4jEntityConverter {
 	}
 
 	@Override
+	@Nullable
 	public <R> R read(Class<R> targetType, MapAccessor mapAccessor) {
 
 		knownObjects.nextRecord();
@@ -112,12 +113,9 @@ public <R> R read(Class<R> targetType, MapAccessor mapAccessor) {
 		@SuppressWarnings("unchecked") // ¯\_(ツ)_/¯
 		Neo4jPersistentEntity<R> rootNodeDescription = (Neo4jPersistentEntity<R>) nodeDescriptionStore.getNodeDescription(targetType);
 		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));
-		}
 
 		try {
-			return map(queryRoot, queryRoot, rootNodeDescription);
+			return queryRoot == null ? null : map(queryRoot, queryRoot, rootNodeDescription);
 		} catch (Exception e) {
 			throw new MappingException("Error mapping " + mapAccessor, e);
 		}
@@ -157,26 +155,35 @@ private <R> MapAccessor determineQueryRoot(MapAccessor mapAccessor, @Nullable Ne
 
 		// Prefer the candidates over candidates previously seen
 		List<Node> finalCandidates = matchingNodes.isEmpty() ? seenMatchingNodes : matchingNodes;
-		MapAccessor queryRoot = null;
 
 		if (finalCandidates.size() > 1) {
 			throw new MappingException("More than one matching node in the record");
 		} else if (!finalCandidates.isEmpty()) {
 			if (mapAccessor.size() > 1) {
-				queryRoot = mergeRootNodeWithRecord(finalCandidates.get(0), mapAccessor);
+				return mergeRootNodeWithRecord(finalCandidates.get(0), mapAccessor);
 			} else {
-				queryRoot = finalCandidates.get(0);
+				return finalCandidates.get(0);
 			}
 		} else {
+			int cnt = 0;
+			Value firstValue = Values.NULL;
 			for (Value value : recordValues) {
-				if (value.hasType(mapType) && !(value.hasType(nodeType) || value.hasType(
-						relationshipType))) {
-					queryRoot = value;
-					break;
+				if (cnt == 0) {
+					firstValue = value;
 				}
+				if (value.hasType(mapType) && !(value.hasType(nodeType) || value.hasType(relationshipType))) {
+					return value;
+				}
+				++cnt;
+			}
+
+			// Cater for results that have one single, null column. This is the case for MATCH (x) OPTIONAL MATCH (something) RETURN something
+			if (cnt == 1 && firstValue.isNull()) {
+				return null;
 			}
 		}
-		return queryRoot;
+
+		throw new NoRootNodeMappingException(mapAccessor, rootNodeDescription);
 	}
 
 	private Collection<String> createDynamicLabelsProperty(TypeInformation<?> type, Collection<String> dynamicLabels) {

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

@@ -15,7 +15,12 @@
  */
 package org.springframework.data.neo4j.core.mapping;
 
+import java.util.Formattable;
+import java.util.Formatter;
+import java.util.Locale;
+
 import org.apiguardian.api.API;
+import org.neo4j.driver.types.MapAccessor;
 import org.springframework.data.mapping.MappingException;
 
 /**
@@ -29,9 +34,22 @@
  * @since 6.0.2
  */
 @API(status = API.Status.INTERNAL, since = "6.0.2")
-public final class NoRootNodeMappingException extends MappingException {
+public final class NoRootNodeMappingException extends MappingException implements Formattable {
+
+	private MapAccessor mapAccessor;
+	private Neo4jPersistentEntity<?> entity;
+
+	public NoRootNodeMappingException(MapAccessor mapAccessor, Neo4jPersistentEntity<?> entity) {
+		super(String.format("Could not find mappable nodes or relationships inside %s for %s", mapAccessor, entity));
+		this.mapAccessor = mapAccessor;
+		this.entity = entity;
+	}
 
-	public NoRootNodeMappingException(String s) {
-		super(s);
+	@Override
+	public void formatTo(Formatter formatter, int flags, int width, int precision) {
+		String className = entity.getUnderlyingClass().getSimpleName();
+		formatter.format("Could not find mappable nodes or relationships inside %s for %s:%s", mapAccessor,
+				className.substring(0, 1).toLowerCase(
+						Locale.ROOT), String.join(":", entity.getStaticLabels()));
 	}
 }

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

@@ -17,7 +17,6 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
-import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.assertj.core.api.Assumptions.assumeThat;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyMap;
@@ -438,23 +437,27 @@ void shouldApplyNullChecksDuringReading() {
 
 			when(session.run(anyString(), anyMap())).thenReturn(result);
 			when(result.stream()).thenReturn(Stream.of(record1, record2));
+			when(result.consume()).thenReturn(resultSummary);
 			when(record1.get("name")).thenReturn(Values.value("michael"));
 
 			Neo4jClient client = Neo4jClient.create(driver);
 
-			assertThatIllegalStateException()
-					.isThrownBy(() -> client.query("MATCH (n) RETURN n").fetchAs(BikeOwner.class).mappedBy((t, r) -> {
+			Collection<BikeOwner> owners = client.query("MATCH (n) RETURN n").fetchAs(BikeOwner.class)
+					.mappedBy((t, r) -> {
 						if (r == record1) {
 							return new BikeOwner(r.get("name").asString(), Collections.emptyList());
 						} else {
 							return null;
 						}
-					}).all());
-
+					}).all();
+			assertThat(owners).hasSize(1);
 			verifyDatabaseSelection(null);
 
 			verify(session).run(eq("MATCH (n) RETURN n"), MockitoHamcrest.argThat(new MapAssertionMatcher(Collections.emptyMap())));
 			verify(result).stream();
+			verify(result).consume();
+			verify(resultSummary).notifications();
+			verify(resultSummary).hasPlan();
 			verify(record1).get("name");
 			verify(session).close();
 		}

src/test/java/org/springframework/data/neo4j/core/DelegatingMappingFunctionWithNullCheckTest.java

@@ -453,7 +453,7 @@ void shouldApplyNullChecksDuringReading() {
 						}
 					}).all();
 
-			StepVerifier.create(bikeOwners).expectNextCount(1).verifyError();
+			StepVerifier.create(bikeOwners).expectNextCount(1).verifyComplete();
 
 			verifyDatabaseSelection(null);
 

src/test/java/org/springframework/data/neo4j/core/Neo4jClientTest.java

@@ -113,6 +113,8 @@
 import org.springframework.data.neo4j.integration.issues.gh2533.GH2533Repository;
 import org.springframework.data.neo4j.integration.issues.gh2542.TestNode;
 import org.springframework.data.neo4j.integration.issues.gh2542.TestNodeRepository;
+import org.springframework.data.neo4j.integration.issues.gh2572.GH2572Repository;
+import org.springframework.data.neo4j.integration.issues.gh2572.GH2572Child;
 import org.springframework.data.neo4j.integration.misc.ConcreteImplementationTwo;
 import org.springframework.data.neo4j.repository.config.EnableNeo4jRepositories;
 import org.springframework.data.neo4j.repository.query.QueryFragmentsAndParameters;
@@ -158,6 +160,7 @@ protected static void setupData(@Autowired BookmarkCapture bookmarkCapture) {
 				setupGH2323(transaction);
 				setupGH2328(transaction);
 				setupGH2459(transaction);
+				setupGH2572(transaction);
 
 				transaction.commit();
 			}
@@ -789,6 +792,48 @@ void shouldThrowDataIntegrityViolationException(@Autowired TestNodeRepository re
 				.isThrownBy(() -> repository.save(secondNode));
 	}
 
+	@Test
+	@Tag("GH-2572")
+	void allShouldFetchCorrectNumberOfChildNodes(@Autowired GH2572Repository GH2572Repository) {
+		List<GH2572Child> dogsForPerson = GH2572Repository.getDogsForPerson("GH2572Parent-2");
+		assertThat(dogsForPerson).hasSize(2);
+	}
+
+	@Test
+	@Tag("GH-2572")
+	void allShouldNotFailWithoutMatchingRootNodes(@Autowired GH2572Repository GH2572Repository) {
+		List<GH2572Child> dogsForPerson = GH2572Repository.getDogsForPerson("GH2572Parent-1");
+		assertThat(dogsForPerson).isEmpty();
+	}
+
+	@Test
+	@Tag("GH-2572")
+	void oneShouldFetchCorrectNumberOfChildNodes(@Autowired GH2572Repository GH2572Repository) {
+		Optional<GH2572Child> optionalChild = GH2572Repository.findOneDogForPerson("GH2572Parent-2");
+		assertThat(optionalChild).map(GH2572Child::getName).hasValue("a-pet");
+	}
+
+	@Test
+	@Tag("GH-2572")
+	void oneShouldNotFailWithoutMatchingRootNodes(@Autowired GH2572Repository GH2572Repository) {
+		Optional<GH2572Child> optionalChild = GH2572Repository.findOneDogForPerson("GH2572Parent-1");
+		assertThat(optionalChild).isEmpty();
+	}
+
+	@Test
+	@Tag("GH-2572")
+	void getOneShouldFetchCorrectNumberOfChildNodes(@Autowired GH2572Repository GH2572Repository) {
+		GH2572Child gh2572Child = GH2572Repository.getOneDogForPerson("GH2572Parent-2");
+		assertThat(gh2572Child.getName()).isEqualTo("a-pet");
+	}
+
+	@Test
+	@Tag("GH-2572")
+	void getOneShouldNotFailWithoutMatchingRootNodes(@Autowired GH2572Repository GH2572Repository) {
+		GH2572Child gh2572Child = GH2572Repository.getOneDogForPerson("GH2572Parent-1");
+		assertThat(gh2572Child).isNull();
+	}
+
 	@Configuration
 	@EnableTransactionManagement
 	@EnableNeo4jRepositories(namedQueriesLocation = "more-custom-queries.properties")