DATACMNS-940 - Fix query method query result type detection for custom collections.

The nullable wrapper types now expose whether they're a single value type or a multi value type. This is necessary as Javaslang's Option implements Iterable (something we previously checked for to detect a collection query) and the custom collections potentially being usable within other wrapper types (e.g. Future<Seq<User>>).

Author: odrotbohm

src/main/java/org/springframework/data/repository/query/QueryMethod.java

@@ -173,8 +173,22 @@ public Class<?> getReturnedObjectType() {
 	 */
 	public boolean isCollectionQuery() {
 
-		return !(isPageQuery() || isSliceQuery())
-				&& org.springframework.util.ClassUtils.isAssignable(Iterable.class, unwrappedReturnType)
+		if (isPageQuery() || isSliceQuery()) {
+			return false;
+		}
+
+		Class<?> returnType = method.getReturnType();
+
+		if (QueryExecutionConverters.supports(returnType) && !QueryExecutionConverters.isSingleValue(returnType)) {
+			return true;
+		}
+
+		if (QueryExecutionConverters.supports(unwrappedReturnType)
+				&& QueryExecutionConverters.isSingleValue(unwrappedReturnType)) {
+			return false;
+		}
+
+		return org.springframework.util.ClassUtils.isAssignable(Iterable.class, unwrappedReturnType)
 				|| unwrappedReturnType.isArray();
 	}
 

src/main/java/org/springframework/data/repository/util/JavaslangCollections.java

@@ -28,6 +28,7 @@
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.ConditionalGenericConverter;
 import org.springframework.core.convert.converter.Converter;
+import org.springframework.data.repository.util.QueryExecutionConverters.WrapperType;
 import org.springframework.util.ReflectionUtils;
 
 /**
@@ -42,8 +43,8 @@ public enum ToJavaConverter implements Converter<Object, Object> {
 
 		INSTANCE;
 
-		public Class<?> getWrapperType() {
-			return javaslang.collection.Traversable.class;
+		public WrapperType getWrapperType() {
+			return WrapperType.multiValue(javaslang.collection.Traversable.class);
 		}
 
 		/* 

src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java

@@ -16,6 +16,10 @@
 package org.springframework.data.repository.util;
 
 import javaslang.collection.Traversable;
+import lombok.AccessLevel;
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Value;
 import scala.Function0;
 import scala.Option;
 import scala.runtime.AbstractFunction0;
@@ -28,6 +32,7 @@
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Future;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import org.springframework.core.convert.ConversionService;
 import org.springframework.core.convert.TypeDescriptor;
@@ -72,16 +77,16 @@ public abstract class QueryExecutionConverters {
 	private static final boolean JAVASLANG_PRESENT = ClassUtils.isPresent("javaslang.control.Option",
 			QueryExecutionConverters.class.getClassLoader());
 
-	private static final Set<Class<?>> WRAPPER_TYPES = new HashSet<Class<?>>();
-	private static final Set<Class<?>> UNWRAPPER_TYPES = new HashSet<Class<?>>();
+	private static final Set<WrapperType> WRAPPER_TYPES = new HashSet<WrapperType>();
+	private static final Set<WrapperType> UNWRAPPER_TYPES = new HashSet<WrapperType>();
 	private static final Set<Converter<Object, Object>> UNWRAPPERS = new HashSet<Converter<Object, Object>>();
 
 	static {
 
-		WRAPPER_TYPES.add(Future.class);
-		UNWRAPPER_TYPES.add(Future.class);
-		WRAPPER_TYPES.add(ListenableFuture.class);
-		UNWRAPPER_TYPES.add(ListenableFuture.class);
+		WRAPPER_TYPES.add(WrapperType.singleValue(Future.class));
+		UNWRAPPER_TYPES.add(WrapperType.singleValue(Future.class));
+		WRAPPER_TYPES.add(WrapperType.singleValue(ListenableFuture.class));
+		UNWRAPPER_TYPES.add(WrapperType.singleValue(ListenableFuture.class));
 
 		if (GUAVA_PRESENT) {
 			WRAPPER_TYPES.add(NullableWrapperToGuavaOptionalConverter.getWrapperType());
@@ -115,9 +120,12 @@ public abstract class QueryExecutionConverters {
 		}
 
 		if (ReactiveWrappers.isAvailable()) {
-			WRAPPER_TYPES.addAll(ReactiveWrappers.getNoValueTypes());
-			WRAPPER_TYPES.addAll(ReactiveWrappers.getSingleValueTypes());
-			WRAPPER_TYPES.addAll(ReactiveWrappers.getMultiValueTypes());
+			WRAPPER_TYPES
+					.addAll(ReactiveWrappers.getNoValueTypes().stream().map(WrapperType::noValue).collect(Collectors.toList()));
+			WRAPPER_TYPES.addAll(
+					ReactiveWrappers.getSingleValueTypes().stream().map(WrapperType::singleValue).collect(Collectors.toList()));
+			WRAPPER_TYPES.addAll(
+					ReactiveWrappers.getMultiValueTypes().stream().map(WrapperType::multiValue).collect(Collectors.toList()));
 		}
 	}
 
@@ -133,8 +141,8 @@ public static boolean supports(Class<?> type) {
 
 		Assert.notNull(type, "Type must not be null!");
 
-		for (Class<?> candidate : WRAPPER_TYPES) {
-			if (candidate.isAssignableFrom(type)) {
+		for (WrapperType candidate : WRAPPER_TYPES) {
+			if (candidate.getType().isAssignableFrom(type)) {
 				return true;
 			}
 		}
@@ -152,15 +160,26 @@ public static boolean supportsUnwrapping(Class<?> type) {
 
 		Assert.notNull(type, "Type must not be null!");
 
-		for (Class<?> candidate : UNWRAPPER_TYPES) {
-			if (candidate.isAssignableFrom(type)) {
+		for (WrapperType candidate : UNWRAPPER_TYPES) {
+			if (candidate.getType().isAssignableFrom(type)) {
 				return true;
 			}
 		}
 
 		return false;
 	}
 
+	public static boolean isSingleValue(Class<?> type) {
+
+		for (WrapperType candidate : WRAPPER_TYPES) {
+			if (candidate.getType().isAssignableFrom(type)) {
+				return candidate.isSingleValue();
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Registers converters for wrapper types found on the classpath.
 	 * 
@@ -311,8 +330,8 @@ protected Object wrap(Object source) {
 			return Optional.of(source);
 		}
 
-		public static Class<?> getWrapperType() {
-			return Optional.class;
+		public static WrapperType getWrapperType() {
+			return WrapperType.singleValue(Optional.class);
 		}
 	}
 
@@ -341,8 +360,8 @@ protected Object wrap(Object source) {
 			return java.util.Optional.of(source);
 		}
 
-		public static Class<?> getWrapperType() {
-			return java.util.Optional.class;
+		public static WrapperType getWrapperType() {
+			return WrapperType.singleValue(java.util.Optional.class);
 		}
 	}
 
@@ -397,8 +416,8 @@ protected Object wrap(Object source) {
 			return source instanceof CompletableFuture ? source : CompletableFuture.completedFuture(source);
 		}
 
-		public static Class<?> getWrapperType() {
-			return CompletableFuture.class;
+		public static WrapperType getWrapperType() {
+			return WrapperType.singleValue(CompletableFuture.class);
 		}
 	}
 
@@ -423,8 +442,8 @@ protected Object wrap(Object source) {
 			return Option.apply(source);
 		}
 
-		public static Class<?> getWrapperType() {
-			return Option.class;
+		public static WrapperType getWrapperType() {
+			return WrapperType.singleValue(Option.class);
 		}
 	}
 
@@ -440,8 +459,8 @@ private static class NullableWrapperToJavaslangOptionConverter extends AbstractW
 		private static final Method NONE_METHOD;
 
 		static {
-			OF_METHOD = ReflectionUtils.findMethod(getWrapperType(), "of", Object.class);
-			NONE_METHOD = ReflectionUtils.findMethod(getWrapperType(), "none");
+			OF_METHOD = ReflectionUtils.findMethod(javaslang.control.Option.class, "of", Object.class);
+			NONE_METHOD = ReflectionUtils.findMethod(javaslang.control.Option.class, "none");
 		}
 
 		/**
@@ -450,11 +469,11 @@ private static class NullableWrapperToJavaslangOptionConverter extends AbstractW
 		 * @param conversionService must not be {@literal null}.
 		 */
 		public NullableWrapperToJavaslangOptionConverter(ConversionService conversionService) {
-			super(conversionService, createEmptyOption(), getWrapperType());
+			super(conversionService, createEmptyOption(), javaslang.control.Option.class);
 		}
 
-		public static Class<?> getWrapperType() {
-			return javaslang.control.Option.class;
+		public static WrapperType getWrapperType() {
+			return WrapperType.singleValue(javaslang.control.Option.class);
 		}
 
 		/* 
@@ -586,4 +605,32 @@ public Object convert(Object source) {
 			return source;
 		}
 	}
+
+	@Value
+	@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
+	public static class WrapperType {
+
+		enum Cardinality {
+			NONE, SINGLE, MULTI;
+		}
+
+		Class<?> type;
+		@Getter(AccessLevel.NONE) Cardinality cardinality;
+
+		public static WrapperType singleValue(Class<?> type) {
+			return new WrapperType(type, Cardinality.SINGLE);
+		}
+
+		public static WrapperType multiValue(Class<?> type) {
+			return new WrapperType(type, Cardinality.MULTI);
+		}
+
+		public static WrapperType noValue(Class<?> type) {
+			return new WrapperType(type, Cardinality.NONE);
+		}
+
+		boolean isSingleValue() {
+			return cardinality.equals(Cardinality.SINGLE);
+		}
+	}
 }

src/test/java/org/springframework/data/repository/query/QueryMethodUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2015 the original author or authors.
+ * Copyright 2008-2016 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.
@@ -19,6 +19,9 @@
 import static org.junit.Assert.*;
 import static org.junit.Assume.*;
 
+import javaslang.collection.Seq;
+import javaslang.control.Option;
+
 import java.io.Serializable;
 import java.lang.reflect.Method;
 import java.util.List;
@@ -181,6 +184,42 @@ public void doesNotRejectFutureQueryForEntityCollection() throws Exception {
 		assertThat(new QueryMethod(method, repositoryMetadata, factory).isCollectionQuery(), is(true));
 	}
 
+	/**
+	 * @see DATACMNS-940
+	 */
+	@Test
+	public void detectsCustomCollectionReturnType() throws Exception {
+
+		RepositoryMetadata repositoryMetadata = new DefaultRepositoryMetadata(SampleRepository.class);
+		Method method = SampleRepository.class.getMethod("returnsSeq");
+
+		assertThat(new QueryMethod(method, repositoryMetadata, factory).isCollectionQuery(), is(true));
+	}
+
+	/**
+	 * @see DATACMNS-940
+	 */
+	@Test
+	public void detectsWrapperWithinWrapper() throws Exception {
+
+		RepositoryMetadata repositoryMetadata = new DefaultRepositoryMetadata(SampleRepository.class);
+		Method method = SampleRepository.class.getMethod("returnsFutureOfSeq");
+
+		assertThat(new QueryMethod(method, repositoryMetadata, factory).isCollectionQuery(), is(true));
+	}
+
+	/**
+	 * @see DATACMNS-940
+	 */
+	@Test
+	public void detectsSinglValueWrapperWithinWrapper() throws Exception {
+
+		RepositoryMetadata repositoryMetadata = new DefaultRepositoryMetadata(SampleRepository.class);
+		Method method = SampleRepository.class.getMethod("returnsFutureOfOption");
+
+		assertThat(new QueryMethod(method, repositoryMetadata, factory).isCollectionQuery(), is(false));
+	}
+
 	interface SampleRepository extends Repository<User, Serializable> {
 
 		String pagingMethodWithInvalidReturnType(Pageable pageable);
@@ -216,6 +255,12 @@ interface SampleRepository extends Repository<User, Serializable> {
 
 		// DATACMNS-716
 		Future<List<User>> returnsFutureForEntityCollection();
+
+		Seq<User> returnsSeq();
+
+		Future<Seq<User>> returnsFutureOfSeq();
+
+		Future<Option<User>> returnsFutureOfOption();
 	}
 
 	class User {