From 55419890581a80121cbe5037f28f9a9503797fe6 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Tue, 20 May 2014 08:46:08 +0200 Subject: [PATCH] DATACMNS-509 - Fixed non-NullableWrapper conversion. In case the repository method execution returns a collection that needs to be converted into a different collection (e.g. List -> Set) we erroneously ran into a NullableWrapper conversion. This was due to the fact that we only used the raw method return type, which caused the ObjectToCollectionConverter to kick in (as the target type's component type falls back to Object if we use raw types). We're now using Spring's MethodParameter abstraction to create a TypeDescriptor for the method's return type which fully supports generics, so that the conversion check for NullableWrapper -> target type does not match the ObjectToCollectionWrapper. Prepared the QueryExecutionConverters to be able to support recursive type conversion once we move to Spring 4 as TypeDescriptor exposes the ResolvedType as of this version so that we can inspect the type parameter of the wrapper type to invoke a nested conversion (e.g. Integer -> Optional requires NullableWrapperTo(Guava|Jdk)OptionalConverter being invoked and in turn recursively convert the Integer to String). --- .../support/RepositoryFactorySupport.java | 13 +- .../util/QueryExecutionConverters.java | 181 ++++++++++++++---- .../core/support/DummyRepositoryFactory.java | 21 +- .../RepositoryFactorySupportUnitTests.java | 59 +++++- 4 files changed, 226 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java b/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java index 7abf45ed6..157da489b 100644 --- a/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java +++ b/src/main/java/org/springframework/data/repository/core/support/RepositoryFactorySupport.java @@ -30,6 +30,7 @@ import org.aopalliance.intercept.MethodInvocation; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.BeanClassLoaderAware; import org.springframework.core.GenericTypeResolver; +import org.springframework.core.MethodParameter; import org.springframework.core.convert.TypeDescriptor; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.core.convert.support.GenericConversionService; @@ -341,13 +342,21 @@ public abstract class RepositoryFactorySupport implements BeanClassLoaderAware { public Object invoke(MethodInvocation invocation) throws Throwable { Object result = doInvoke(invocation); - Class expectedReturnType = invocation.getMethod().getReturnType(); + + Method method = invocation.getMethod(); + + // Looking up the TypeDescriptor for the return type - yes, this way o.O + MethodParameter parameter = new MethodParameter(method, -1); + TypeDescriptor methodReturnTypeDescriptor = TypeDescriptor.nested(parameter, 0); + + Class expectedReturnType = method.getReturnType(); if (result != null && expectedReturnType.isInstance(result)) { return result; } - if (conversionService.canConvert(NullableWrapper.class, expectedReturnType) + if (QueryExecutionConverters.supports(expectedReturnType) + && conversionService.canConvert(WRAPPER_TYPE, methodReturnTypeDescriptor) && !conversionService.canBypassConvert(WRAPPER_TYPE, TypeDescriptor.valueOf(expectedReturnType))) { return conversionService.convert(new NullableWrapper(result), expectedReturnType); } 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 4c628e566..4b21eee09 100644 --- a/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java +++ b/src/main/java/org/springframework/data/repository/util/QueryExecutionConverters.java @@ -20,7 +20,10 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.Future; +import org.springframework.core.convert.ConversionService; +import org.springframework.core.convert.TypeDescriptor; import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.GenericConverter; import org.springframework.core.convert.support.ConfigurableConversionService; import org.springframework.scheduling.annotation.AsyncResult; import org.springframework.util.Assert; @@ -42,29 +45,19 @@ public class QueryExecutionConverters { private static final boolean JDK_PRESENT = ClassUtils.isPresent("java.util.Optional", QueryExecutionConverters.class.getClassLoader()); - private static final Set> WRAPPER_TYPES; - private static final Set> CONVERTERS; + private static Set> WRAPPER_TYPES = new HashSet>(); static { - Set> wrapperTypes = new HashSet>(); - Set> converters = new HashSet>(); - - wrapperTypes.add(Future.class); - converters.add(ObjectToFutureConverter.INSTANCE); + WRAPPER_TYPES.add(Future.class); if (GUAVA_PRESENT) { - wrapperTypes.add(ObjectToGuavaOptionalConverter.INSTANCE.getWrapperType()); - converters.add(ObjectToGuavaOptionalConverter.INSTANCE); + WRAPPER_TYPES.add(NullableWrapperToGuavaOptionalConverter.getWrapperType()); } if (JDK_PRESENT) { - wrapperTypes.add(ObjectToJdk8OptionalConverter.INSTANCE.getWrapperType()); - converters.add(ObjectToJdk8OptionalConverter.INSTANCE); + WRAPPER_TYPES.add(NullableWrapperToJdk8OptionalConverter.getWrapperType()); } - - WRAPPER_TYPES = Collections.unmodifiableSet(wrapperTypes); - CONVERTERS = Collections.unmodifiableSet(converters); } /** @@ -88,9 +81,81 @@ public class QueryExecutionConverters { Assert.notNull(conversionService, "ConversionService must not be null!"); - for (Converter converter : CONVERTERS) { - conversionService.addConverter(converter); + if (GUAVA_PRESENT) { + conversionService.addConverter(new NullableWrapperToGuavaOptionalConverter(conversionService)); } + + if (JDK_PRESENT) { + conversionService.addConverter(new NullableWrapperToJdk8OptionalConverter(conversionService)); + } + + conversionService.addConverter(new NullableWrapperToFutureConverter(conversionService)); + } + + /** + * Base class for converters that create instances of wrapper types such as Google Guava's and JDK 8's + * {@code Optional} types. + * + * @author Oliver Gierke + */ + private static abstract class AbstractWrapperTypeConverter implements GenericConverter { + + @SuppressWarnings("unused")// + private final ConversionService conversionService; + private final Class wrapperType; + + /** + * Creates a new {@link AbstractWrapperTypeConverter} using the given {@link ConversionService} and wrapper type. + * + * @param conversionService must not be {@literal null}. + * @param wrapperType must not be {@literal null}. + */ + protected AbstractWrapperTypeConverter(ConversionService conversionService, Class wrapperType) { + + Assert.notNull(conversionService, "ConversionService must not be null!"); + Assert.notNull(wrapperType, "Wrapper type must not be null!"); + + this.conversionService = conversionService; + this.wrapperType = wrapperType; + } + + /* + * (non-Javadoc) + * @see org.springframework.core.convert.converter.GenericConverter#getConvertibleTypes() + */ + @Override + public Set getConvertibleTypes() { + return Collections.singleton(new ConvertiblePair(NullableWrapper.class, wrapperType)); + } + + /* + * (non-Javadoc) + * @see org.springframework.core.convert.converter.GenericConverter#convert(java.lang.Object, org.springframework.core.convert.TypeDescriptor, org.springframework.core.convert.TypeDescriptor) + */ + @Override + public final Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) { + + NullableWrapper wrapper = (NullableWrapper) source; + Object value = wrapper.getValue(); + + // TODO: Add Recursive conversion once we move to Spring 4 + return value == null ? getNullValue() : wrap(value); + } + + /** + * Return the object that shall be used as a replacement for {@literal null}. + * + * @return must not be {@literal null}. + */ + protected abstract Object getNullValue(); + + /** + * Wrap the given, non-{@literal null} value into the wrapper type. + * + * @param source will never be {@literal null}. + * @return must not be {@literal null}. + */ + protected abstract Object wrap(Object source); } /** @@ -98,20 +163,36 @@ public class QueryExecutionConverters { * * @author Oliver Gierke */ - private static enum ObjectToGuavaOptionalConverter implements Converter> { + private static class NullableWrapperToGuavaOptionalConverter extends AbstractWrapperTypeConverter { - INSTANCE; + /** + * Creates a new {@link NullableWrapperToGuavaOptionalConverter} using the given {@link ConversionService}. + * + * @param conversionService must not be {@literal null}. + */ + public NullableWrapperToGuavaOptionalConverter(ConversionService conversionService) { + super(conversionService, Optional.class); + } /* * (non-Javadoc) - * @see org.springframework.core.convert.converter.Converter#convert(java.lang.Object) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#getNullValue() */ @Override - public Optional convert(NullableWrapper source) { - return Optional.fromNullable(source.getValue()); + protected Object getNullValue() { + return Optional.absent(); } - public Class getWrapperType() { + /* + * (non-Javadoc) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#wrap(java.lang.Object) + */ + @Override + protected Object wrap(Object source) { + return Optional.of(source); + } + + public static Class getWrapperType() { return Optional.class; } } @@ -121,20 +202,36 @@ public class QueryExecutionConverters { * * @author Oliver Gierke */ - private static enum ObjectToJdk8OptionalConverter implements Converter> { + private static class NullableWrapperToJdk8OptionalConverter extends AbstractWrapperTypeConverter { - INSTANCE; + /** + * Creates a new {@link NullableWrapperToJdk8OptionalConverter} using the given {@link ConversionService}. + * + * @param conversionService must not be {@literal null}. + */ + public NullableWrapperToJdk8OptionalConverter(ConversionService conversionService) { + super(conversionService, java.util.Optional.class); + } /* * (non-Javadoc) - * @see org.springframework.core.convert.converter.Converter#convert(java.lang.Object) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#getNullValue() */ @Override - public java.util.Optional convert(NullableWrapper source) { - return java.util.Optional.ofNullable(source.getValue()); + protected Object getNullValue() { + return java.util.Optional.empty(); } - public Class getWrapperType() { + /* + * (non-Javadoc) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#wrap(java.lang.Object) + */ + @Override + protected Object wrap(Object source) { + return java.util.Optional.of(source); + } + + public static Class getWrapperType() { return java.util.Optional.class; } } @@ -144,17 +241,35 @@ public class QueryExecutionConverters { * * @author Oliver Gierke */ - private static enum ObjectToFutureConverter implements Converter> { + private static class NullableWrapperToFutureConverter extends AbstractWrapperTypeConverter { - INSTANCE; + private final AsyncResult NULL_OBJECT = new AsyncResult(null); + + /** + * Creates a new {@link NullableWrapperToFutureConverter} using the given {@link ConversionService}. + * + * @param conversionService must not be {@literal null}. + */ + public NullableWrapperToFutureConverter(ConversionService conversionService) { + super(conversionService, Future.class); + } /* * (non-Javadoc) - * @see org.springframework.core.convert.converter.Converter#convert(java.lang.Object) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#getNullValue() */ @Override - public Future convert(NullableWrapper source) { - return new AsyncResult(source.getValue()); + protected Object getNullValue() { + return NULL_OBJECT; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.repository.util.QueryExecutionConverters.AbstractWrapperTypeConverter#wrap(java.lang.Object) + */ + @Override + protected Object wrap(Object source) { + return new AsyncResult(source); } } } diff --git a/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java b/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java index d107c62ce..ee67e46cd 100644 --- a/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java +++ b/src/test/java/org/springframework/data/repository/core/support/DummyRepositoryFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2012 the original author or authors. + * Copyright 2012-2014 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. @@ -29,15 +29,27 @@ import org.springframework.data.repository.query.QueryLookupStrategy; import org.springframework.data.repository.query.QueryLookupStrategy.Key; import org.springframework.data.repository.query.RepositoryQuery; +/** + * Dummy implementation for {@link RepositoryFactorySupport} that is equipped with mocks to simulate behavior for test + * cases. + * + * @author Oliver Gierke + */ public class DummyRepositoryFactory extends RepositoryFactorySupport { public final MyRepositoryQuery queryOne = mock(MyRepositoryQuery.class); public final RepositoryQuery queryTwo = mock(RepositoryQuery.class); + public final QueryLookupStrategy strategy = mock(QueryLookupStrategy.class); private final Object repository; public DummyRepositoryFactory(Object repository) { + this.repository = repository; + + when( + strategy.resolveQuery(Mockito.any(Method.class), Mockito.any(RepositoryMetadata.class), + Mockito.any(NamedQueries.class))).thenReturn(queryOne); } /* @@ -74,13 +86,6 @@ public class DummyRepositoryFactory extends RepositoryFactorySupport { */ @Override protected QueryLookupStrategy getQueryLookupStrategy(Key key) { - - QueryLookupStrategy strategy = mock(QueryLookupStrategy.class); - - when( - strategy.resolveQuery(Mockito.any(Method.class), Mockito.any(RepositoryMetadata.class), - Mockito.any(NamedQueries.class))).thenReturn(queryOne, queryTwo); - return strategy; } } diff --git a/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java b/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java index c2c4e48ff..b2d20c168 100644 --- a/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java +++ b/src/test/java/org/springframework/data/repository/core/support/RepositoryFactorySupportUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011 the original author or authors. + * Copyright 2011-2014 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. @@ -15,12 +15,15 @@ */ package org.springframework.data.repository.core.support; -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.mockito.Mockito.*; import java.io.Serializable; +import java.lang.reflect.Method; +import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.concurrent.Future; import org.junit.Assume; @@ -41,6 +44,8 @@ import org.springframework.data.domain.Sort; import org.springframework.data.repository.PagingAndSortingRepository; import org.springframework.data.repository.Repository; import org.springframework.data.repository.RepositoryDefinition; +import org.springframework.data.repository.core.NamedQueries; +import org.springframework.data.repository.core.RepositoryMetadata; import org.springframework.data.repository.query.RepositoryQuery; import org.springframework.scheduling.annotation.Async; import org.springframework.scheduling.annotation.AsyncAnnotationBeanPostProcessor; @@ -71,6 +76,12 @@ public class RepositoryFactorySupportUnitTests { @Test public void invokesCustomQueryCreationListenerForSpecialRepositoryQueryOnly() throws Exception { + Mockito.reset(factory.strategy); + + when( + factory.strategy.resolveQuery(Mockito.any(Method.class), Mockito.any(RepositoryMetadata.class), + Mockito.any(NamedQueries.class))).thenReturn(factory.queryOne, factory.queryTwo); + factory.addQueryCreationListener(listener); factory.addQueryCreationListener(otherListener); @@ -148,11 +159,11 @@ public class RepositoryFactorySupportUnitTests { } }); - AsyncRepository repository = factory.getRepository(AsyncRepository.class); + ConvertingRepository repository = factory.getRepository(ConvertingRepository.class); AsyncAnnotationBeanPostProcessor processor = new AsyncAnnotationBeanPostProcessor(); processor.setBeanFactory(new DefaultListableBeanFactory()); - repository = (AsyncRepository) processor.postProcessAfterInitialization(repository, null); + repository = (ConvertingRepository) processor.postProcessAfterInitialization(repository, null); Future future = repository.findByFirstname("Foo"); @@ -167,6 +178,40 @@ public class RepositoryFactorySupportUnitTests { verify(factory.queryOne, times(1)).execute(Mockito.any(Object[].class)); } + /** + * @see DATACMNS-509 + */ + @Test + public void convertsWithSameElementType() { + + List names = Collections.singletonList("Dave"); + + when(factory.queryOne.execute(Mockito.any(Object[].class))).thenReturn(names); + + ConvertingRepository repository = factory.getRepository(ConvertingRepository.class); + Set result = repository.convertListToStringSet(); + + assertThat(result, hasSize(1)); + assertThat(result.iterator().next(), is("Dave")); + } + + /** + * @see DATACMNS-509 + */ + @Test + public void convertsCollectionToOtherCollectionWithElementSuperType() { + + List names = Collections.singletonList("Dave"); + + when(factory.queryOne.execute(Mockito.any(Object[].class))).thenReturn(names); + + ConvertingRepository repository = factory.getRepository(ConvertingRepository.class); + Set result = repository.convertListToObjectSet(); + + assertThat(result, hasSize(1)); + assertThat(result.iterator().next(), is((Object) "Dave")); + } + interface ObjectRepository extends Repository, ObjectRepositoryCustom { Object findByClass(Class clazz); @@ -217,7 +262,11 @@ public class RepositoryFactorySupportUnitTests { } - interface AsyncRepository extends Repository { + interface ConvertingRepository extends Repository { + + Set convertListToStringSet(); + + Set convertListToObjectSet(); @Async Future findByFirstname(String firstname);