From 902cb44e86fb183be234da08aed5f8ff96838028 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Tue, 13 Dec 2016 15:31:54 +0100 Subject: [PATCH] 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>). --- .../data/repository/query/QueryMethod.java | 18 ++++- .../repository/util/JavaslangCollections.java | 5 +- .../util/QueryExecutionConverters.java | 66 ++++++++++++++----- .../query/QueryMethodUnitTests.java | 47 ++++++++++++- 4 files changed, 113 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/springframework/data/repository/query/QueryMethod.java b/src/main/java/org/springframework/data/repository/query/QueryMethod.java index 3e876764d..65784283c 100644 --- a/src/main/java/org/springframework/data/repository/query/QueryMethod.java +++ b/src/main/java/org/springframework/data/repository/query/QueryMethod.java @@ -173,8 +173,22 @@ public class QueryMethod { */ 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(); } diff --git a/src/main/java/org/springframework/data/repository/util/JavaslangCollections.java b/src/main/java/org/springframework/data/repository/util/JavaslangCollections.java index 3d1fbf847..6ee4b4221 100644 --- a/src/main/java/org/springframework/data/repository/util/JavaslangCollections.java +++ b/src/main/java/org/springframework/data/repository/util/JavaslangCollections.java @@ -28,6 +28,7 @@ import java.util.Set; 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 @@ class JavaslangCollections { INSTANCE; - public Class getWrapperType() { - return javaslang.collection.Traversable.class; + public WrapperType getWrapperType() { + return WrapperType.multiValue(javaslang.collection.Traversable.class); } /* diff --git a/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java b/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java index 65030ecf4..c9a19d123 100644 --- a/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java +++ b/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java @@ -16,6 +16,9 @@ package org.springframework.data.repository.util; import javaslang.collection.Traversable; +import lombok.AccessLevel; +import lombok.RequiredArgsConstructor; +import lombok.Value; import scala.Function0; import scala.Option; import scala.runtime.AbstractFunction0; @@ -70,13 +73,13 @@ public abstract class QueryExecutionConverters { private static final boolean JAVASLANG_PRESENT = ClassUtils.isPresent("javaslang.control.Option", QueryExecutionConverters.class.getClassLoader()); - private static final Set> WRAPPER_TYPES = new HashSet>(); + private static final Set WRAPPER_TYPES = new HashSet(); private static final Set> UNWRAPPERS = new HashSet>(); static { - WRAPPER_TYPES.add(Future.class); - WRAPPER_TYPES.add(ListenableFuture.class); + WRAPPER_TYPES.add(WrapperType.singleValue(Future.class)); + WRAPPER_TYPES.add(WrapperType.singleValue(ListenableFuture.class)); if (GUAVA_PRESENT) { WRAPPER_TYPES.add(NullableWrapperToGuavaOptionalConverter.getWrapperType()); @@ -118,8 +121,8 @@ public abstract class QueryExecutionConverters { 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; } } @@ -127,6 +130,17 @@ public abstract class QueryExecutionConverters { 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. * @@ -277,8 +291,8 @@ public abstract class QueryExecutionConverters { return Optional.of(source); } - public static Class getWrapperType() { - return Optional.class; + public static WrapperType getWrapperType() { + return WrapperType.singleValue(Optional.class); } } @@ -307,8 +321,8 @@ public abstract class QueryExecutionConverters { 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); } } @@ -363,8 +377,8 @@ public abstract class QueryExecutionConverters { return source instanceof CompletableFuture ? source : CompletableFuture.completedFuture(source); } - public static Class getWrapperType() { - return CompletableFuture.class; + public static WrapperType getWrapperType() { + return WrapperType.singleValue(CompletableFuture.class); } } @@ -389,8 +403,8 @@ public abstract class QueryExecutionConverters { return Option.apply(source); } - public static Class getWrapperType() { - return Option.class; + public static WrapperType getWrapperType() { + return WrapperType.singleValue(Option.class); } } @@ -406,8 +420,8 @@ public abstract class QueryExecutionConverters { 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"); } /** @@ -416,11 +430,11 @@ public abstract class QueryExecutionConverters { * @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); } /* @@ -552,4 +566,20 @@ public abstract class QueryExecutionConverters { return source; } } + + @Value + @RequiredArgsConstructor(access = AccessLevel.PRIVATE) + public static class WrapperType { + + Class type; + boolean singleValue; + + public static WrapperType singleValue(Class type) { + return new WrapperType(type, true); + } + + public static WrapperType multiValue(Class type) { + return new WrapperType(type, false); + } + } } diff --git a/src/test/java/org/springframework/data/repository/query/QueryMethodUnitTests.java b/src/test/java/org/springframework/data/repository/query/QueryMethodUnitTests.java index 5a8a0f1fc..402eca590 100644 --- a/src/test/java/org/springframework/data/repository/query/QueryMethodUnitTests.java +++ b/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.hamcrest.CoreMatchers.*; 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; @@ -226,6 +229,42 @@ public class QueryMethodUnitTests { 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 { String pagingMethodWithInvalidReturnType(Pageable pageable); @@ -269,6 +308,12 @@ public class QueryMethodUnitTests { * @see DATACMNS-716 */ Future> returnsFutureForEntityCollection(); + + Seq returnsSeq(); + + Future> returnsFutureOfSeq(); + + Future> returnsFutureOfOption(); } class User {