Revise DocumentCallback nullability constraints.

DocumentCallback is now generally non-nullable for both, the input Document and the returned result expecting EntityReader to always return a non-null object.

Also, use try-with-resources where applicable.

Closes #3648

Author: mp911de

Diff for: spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java

@@ -28,6 +28,7 @@
 import org.bson.conversions.Bson;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+
 import org.springframework.beans.BeansException;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
@@ -46,6 +47,7 @@
 import org.springframework.data.geo.GeoResult;
 import org.springframework.data.geo.GeoResults;
 import org.springframework.data.geo.Metric;
+import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.callback.EntityCallbacks;
 import org.springframework.data.mapping.context.MappingContext;
 import org.springframework.data.mongodb.MongoDatabaseFactory;
@@ -102,7 +104,6 @@
 import org.springframework.data.projection.SpelAwareProxyProjectionFactory;
 import org.springframework.data.util.CloseableIterator;
 import org.springframework.data.util.Optionals;
-import org.springframework.jca.cci.core.ConnectionCallback;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
@@ -972,7 +973,7 @@ public <T> GeoResults<T> geoNear(NearQuery near, Class<?> domainType, String col
 		for (Document element : results) {
 
 			GeoResult<T> geoResult = callback.doWith(element);
-			aggregate = aggregate.add(new BigDecimal(geoResult.getDistance().getValue()));
+			aggregate = aggregate.add(BigDecimal.valueOf(geoResult.getDistance().getValue()));
 			result.add(geoResult);
 		}
 
@@ -2751,25 +2752,24 @@ private MongoCollection<Document> getAndPrepareCollection(MongoDatabase db, Stri
 	 * Internal method using callbacks to do queries against the datastore that requires reading a single object from a
 	 * collection of objects. It will take the following steps
 	 * <ol>
-	 * <li>Execute the given {@link ConnectionCallback} for a {@link Document}.</li>
+	 * <li>Execute the given {@link CollectionCallback} for a {@link Document}.</li>
 	 * <li>Apply the given {@link DocumentCallback} to each of the {@link Document}s to obtain the result.</li>
 	 * <ol>
 	 *
 	 * @param <T>
 	 * @param collectionCallback the callback to retrieve the {@link Document} with
-	 * @param objectCallback the {@link DocumentCallback} to transform {@link Document}s into the actual domain type
+	 * @param documentCallback the {@link DocumentCallback} to transform {@link Document}s into the actual domain type
 	 * @param collectionName the collection to be queried
 	 * @return
 	 */
 	@Nullable
 	private <T> T executeFindOneInternal(CollectionCallback<Document> collectionCallback,
-			DocumentCallback<T> objectCallback, String collectionName) {
+			DocumentCallback<T> documentCallback, String collectionName) {
 
 		try {
 
-			T result = objectCallback
-					.doWith(collectionCallback.doInCollection(getAndPrepareCollection(doGetDatabase(), collectionName)));
-			return result;
+			Document document = collectionCallback.doInCollection(getAndPrepareCollection(doGetDatabase(), collectionName));
+			return document != null ? documentCallback.doWith(document) : null;
 		} catch (RuntimeException e) {
 			throw potentiallyConvertRuntimeException(e, exceptionTranslator);
 		}
@@ -2779,7 +2779,7 @@ private <T> T executeFindOneInternal(CollectionCallback<Document> collectionCall
 	 * Internal method using callback to do queries against the datastore that requires reading a collection of objects.
 	 * It will take the following steps
 	 * <ol>
-	 * <li>Execute the given {@link ConnectionCallback} for a {@link FindIterable}.</li>
+	 * <li>Execute the given {@link CollectionCallback} for a {@link FindIterable}.</li>
 	 * <li>Prepare that {@link FindIterable} with the given {@link CursorPreparer} (will be skipped if
 	 * {@link CursorPreparer} is {@literal null}</li>
 	 * <li>Iterate over the {@link FindIterable} and applies the given {@link DocumentCallback} to each of the
@@ -2789,36 +2789,27 @@ private <T> T executeFindOneInternal(CollectionCallback<Document> collectionCall
 	 * @param <T>
 	 * @param collectionCallback the callback to retrieve the {@link FindIterable} with
 	 * @param preparer the {@link CursorPreparer} to potentially modify the {@link FindIterable} before iterating over it
-	 * @param objectCallback the {@link DocumentCallback} to transform {@link Document}s into the actual domain type
+	 * @param documentCallback the {@link DocumentCallback} to transform {@link Document}s into the actual domain type
 	 * @param collectionName the collection to be queried
 	 * @return
 	 */
 	private <T> List<T> executeFindMultiInternal(CollectionCallback<FindIterable<Document>> collectionCallback,
-			CursorPreparer preparer, DocumentCallback<T> objectCallback, String collectionName) {
+			CursorPreparer preparer, DocumentCallback<T> documentCallback, String collectionName) {
 
 		try {
 
-			MongoCursor<Document> cursor = null;
-
-			try {
-
-				cursor = preparer
-						.initiateFind(getAndPrepareCollection(doGetDatabase(), collectionName), collectionCallback::doInCollection)
-						.iterator();
+			try (MongoCursor<Document> cursor = preparer
+					.initiateFind(getAndPrepareCollection(doGetDatabase(), collectionName), collectionCallback::doInCollection)
+					.iterator()) {
 
 				List<T> result = new ArrayList<>();
 
 				while (cursor.hasNext()) {
 					Document object = cursor.next();
-					result.add(objectCallback.doWith(object));
+					result.add(documentCallback.doWith(object));
 				}
 
 				return result;
-			} finally {
-
-				if (cursor != null) {
-					cursor.close();
-				}
 			}
 		} catch (RuntimeException e) {
 			throw potentiallyConvertRuntimeException(e, exceptionTranslator);
@@ -2828,24 +2819,13 @@ private <T> List<T> executeFindMultiInternal(CollectionCallback<FindIterable<Doc
 	private void executeQueryInternal(CollectionCallback<FindIterable<Document>> collectionCallback,
 			CursorPreparer preparer, DocumentCallbackHandler callbackHandler, String collectionName) {
 
-		try {
-
-			MongoCursor<Document> cursor = null;
-
-			try {
-
-				cursor = preparer
-						.initiateFind(getAndPrepareCollection(doGetDatabase(), collectionName), collectionCallback::doInCollection)
-						.iterator();
+		try (MongoCursor<Document> cursor = preparer
+				.initiateFind(getAndPrepareCollection(doGetDatabase(), collectionName), collectionCallback::doInCollection)
+				.iterator()) {
 
 				while (cursor.hasNext()) {
 					callbackHandler.processDocument(cursor.next());
 				}
-			} finally {
-				if (cursor != null) {
-					cursor.close();
-				}
-			}
 		} catch (RuntimeException e) {
 			throw potentiallyConvertRuntimeException(e, exceptionTranslator);
 		}
@@ -3143,8 +3123,7 @@ public Document doInCollection(MongoCollection<Document> collection) throws Mong
 
 	interface DocumentCallback<T> {
 
-		@Nullable
-		T doWith(@Nullable Document object);
+		T doWith(Document object);
 	}
 
 	/**
@@ -3168,22 +3147,19 @@ private class ReadDocumentCallback<T> implements DocumentCallback<T> {
 			this.collectionName = collectionName;
 		}
 
-		@Nullable
-		public T doWith(@Nullable Document document) {
-
-			T source = null;
+		public T doWith(Document document) {
 
-			if (document != null) {
 				maybeEmitEvent(new AfterLoadEvent<>(document, type, collectionName));
-				source = reader.read(type, document);
-			}
+				T entity = reader.read(type, document);
 
-			if (source != null) {
-				maybeEmitEvent(new AfterConvertEvent<>(document, source, collectionName));
-				source = maybeCallAfterConvert(source, document, collectionName);
-			}
+				if (entity == null) {
+					throw new MappingException(String.format("EntityReader %s returned null", reader));
+				}
 
-			return source;
+				maybeEmitEvent(new AfterConvertEvent<>(document, entity, collectionName));
+				entity = maybeCallAfterConvert(entity, document, collectionName);
+
+				return entity;
 		}
 	}
 
@@ -3216,8 +3192,7 @@ private class ProjectingReadCallback<S, T> implements DocumentCallback<T> {
 		 * @see org.springframework.data.mongodb.core.MongoTemplate.DocumentCallback#doWith(org.bson.Document)
 		 */
 		@SuppressWarnings("unchecked")
-		@Nullable
-		public T doWith(@Nullable Document document) {
+		public T doWith(Document document) {
 
 			if (document == null) {
 				return null;
@@ -3228,15 +3203,16 @@ public T doWith(@Nullable Document document) {
 
 			maybeEmitEvent(new AfterLoadEvent<>(document, targetType, collectionName));
 
-			Object source = reader.read(typeToRead, document);
-			Object result = targetType.isInterface() ? projectionFactory.createProjection(targetType, source) : source;
+			Object entity = reader.read(typeToRead, document);
 
-			if (result != null) {
-				maybeEmitEvent(new AfterConvertEvent<>(document, result, collectionName));
-				result = maybeCallAfterConvert(result, document, collectionName);
+			if (entity == null) {
+				throw new MappingException(String.format("EntityReader %s returned null", reader));
 			}
 
-			return (T) result;
+			Object result = targetType.isInterface() ? projectionFactory.createProjection(targetType, entity) : entity;
+
+				maybeEmitEvent(new AfterConvertEvent<>(document, result, collectionName));
+				return (T) maybeCallAfterConvert(result, document, collectionName);
 		}
 	}
 
@@ -3373,8 +3349,7 @@ static class GeoNearResultDocumentCallback<T> implements DocumentCallback<GeoRes
 			this.metric = metric;
 		}
 
-		@Nullable
-		public GeoResult<T> doWith(@Nullable Document object) {
+		public GeoResult<T> doWith(Document object) {
 
 			double distance = Double.NaN;
 			if (object.containsKey(distanceField)) {
@@ -3401,10 +3376,6 @@ static class CloseableIterableCursorAdapter<T> implements CloseableIterator<T> {
 
 		/**
 		 * Creates a new {@link CloseableIterableCursorAdapter} backed by the given {@link MongoCollection}.
-		 *
-		 * @param cursor
-		 * @param exceptionTranslator
-		 * @param objectReadCallback
 		 */
 		CloseableIterableCursorAdapter(MongoIterable<Document> cursor, PersistenceExceptionTranslator exceptionTranslator,
 				DocumentCallback<T> objectReadCallback) {
@@ -3448,8 +3419,7 @@ public T next() {
 
 			try {
 				Document item = cursor.next();
-				T converted = objectReadCallback.doWith(item);
-				return converted;
+				return objectReadCallback.doWith(item);
 			} catch (RuntimeException ex) {
 				throw potentiallyConvertRuntimeException(ex, exceptionTranslator);
 			}

Diff for: spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java

@@ -22,7 +22,15 @@
 import reactor.util.function.Tuple2;
 import reactor.util.function.Tuples;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -36,6 +44,7 @@
 import org.reactivestreams.Subscriber;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+
 import org.springframework.beans.BeansException;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
@@ -51,6 +60,7 @@
 import org.springframework.data.geo.Distance;
 import org.springframework.data.geo.GeoResult;
 import org.springframework.data.geo.Metric;
+import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.callback.ReactiveEntityCallbacks;
 import org.springframework.data.mapping.context.MappingContext;
@@ -3152,13 +3162,14 @@ public Mono<T> doWith(Document document) {
 
 			maybeEmitEvent(new AfterLoadEvent<>(document, type, collectionName));
 
-			T source = reader.read(type, document);
-			if (source != null) {
-				maybeEmitEvent(new AfterConvertEvent<>(document, source, collectionName));
-				return maybeCallAfterConvert(source, document, collectionName);
+			T entity = reader.read(type, document);
+
+			if (entity == null) {
+				throw new MappingException(String.format("EntityReader %s returned null", reader));
 			}
 
-			return Mono.empty();
+			maybeEmitEvent(new AfterConvertEvent<>(document, entity, collectionName));
+			return maybeCallAfterConvert(entity, document, collectionName);
 		}
 	}
 
@@ -3196,16 +3207,17 @@ public Mono<T> doWith(Document document) {
 
 			maybeEmitEvent(new AfterLoadEvent<>(document, typeToRead, collectionName));
 
-			Object source = reader.read(typeToRead, document);
-			Object result = targetType.isInterface() ? projectionFactory.createProjection(targetType, source) : source;
+			Object entity = reader.read(typeToRead, document);
 
-			T castEntity = (T) result;
-			if (castEntity != null) {
-				maybeEmitEvent(new AfterConvertEvent<>(document, castEntity, collectionName));
-				return maybeCallAfterConvert(castEntity, document, collectionName);
+			if (entity == null) {
+				throw new MappingException(String.format("EntityReader %s returned null", reader));
 			}
 
-			return Mono.empty();
+			Object result = targetType.isInterface() ? projectionFactory.createProjection(targetType, entity) : entity;
+
+			T castEntity = (T) result;
+			maybeEmitEvent(new AfterConvertEvent<>(document, castEntity, collectionName));
+			return maybeCallAfterConvert(castEntity, document, collectionName);
 		}
 	}
 

Diff for: spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java

@@ -270,7 +270,7 @@ public void setEntityCallbacks(EntityCallbacks entityCallbacks) {
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mongodb.core.core.MongoReader#read(java.lang.Class, com.mongodb.Document)
 	 */
-	public <S extends Object> S read(Class<S> clazz, final Bson bson) {
+	public <S extends Object> S read(Class<S> clazz, Bson bson) {
 		return read(ClassTypeInformation.from(clazz), bson);
 	}
 

Diff for: spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java

@@ -50,6 +50,7 @@
 import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoSettings;
 import org.mockito.quality.Strictness;
+
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationListener;
@@ -64,8 +65,10 @@
 import org.springframework.data.convert.CustomConversions;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.geo.Point;
+import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.callback.EntityCallbacks;
 import org.springframework.data.mapping.context.InvalidPersistentPropertyPath;
+import org.springframework.data.mapping.context.MappingContext;
 import org.springframework.data.mongodb.MongoDatabaseFactory;
 import org.springframework.data.mongodb.core.aggregation.*;
 import org.springframework.data.mongodb.core.aggregation.ComparisonOperators.Gte;
@@ -394,11 +397,24 @@ void findAllAndRemoveShouldRetrieveMatchingDocumentsPriorToRemoval() {
 		verify(collection, times(1)).find(Mockito.eq(query.getQueryObject()), any(Class.class));
 	}
 
+	@Test // GH-3648
+	void shouldThrowExceptionIfEntityReaderReturnsNull() {
+
+		when(cursor.hasNext()).thenReturn(true).thenReturn(true).thenReturn(false);
+		when(cursor.next()).thenReturn(new org.bson.Document("_id", Integer.valueOf(0)));
+		MappingMongoConverter converter = mock(MappingMongoConverter.class);
+		when(converter.getMappingContext()).thenReturn((MappingContext) mappingContext);
+		template = new MongoTemplate(factory, converter);
+
+		assertThatExceptionOfType(MappingException.class).isThrownBy(() -> template.findAll(Person.class))
+				.withMessageContaining("returned null");
+	}
+
 	@Test // DATAMONGO-566
 	void findAllAndRemoveShouldRemoveDocumentsReturedByFindQuery() {
 
-		Mockito.when(cursor.hasNext()).thenReturn(true).thenReturn(true).thenReturn(false);
-		Mockito.when(cursor.next()).thenReturn(new org.bson.Document("_id", Integer.valueOf(0)))
+		when(cursor.hasNext()).thenReturn(true).thenReturn(true).thenReturn(false);
+		when(cursor.next()).thenReturn(new org.bson.Document("_id", Integer.valueOf(0)))
 				.thenReturn(new org.bson.Document("_id", Integer.valueOf(1)));
 
 		ArgumentCaptor<org.bson.Document> queryCaptor = ArgumentCaptor.forClass(org.bson.Document.class);