DATACMNS-1615 - Allow fine grained store specific converter registration.

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.springframework.data</groupId>
 	<artifactId>spring-data-commons</artifactId>
-	<version>2.3.0.BUILD-SNAPSHOT</version>
+	<version>2.3.0.DATACMNS-1615-SNAPSHOT</version>
 
 	<name>Spring Data Core</name>
 

src/main/java/org/springframework/data/convert/CustomConversions.java

@@ -25,6 +25,7 @@
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import org.springframework.core.GenericTypeResolver;
 import org.springframework.core.convert.converter.Converter;
@@ -57,6 +58,10 @@ public class CustomConversions {
 	private static final String READ_CONVERTER_NOT_SIMPLE = "Registering converter from %s to %s as reading converter although it doesn't convert from a store-supported type! You might wanna check you annotation setup at the converter implementation.";
 	private static final String WRITE_CONVERTER_NOT_SIMPLE = "Registering converter from %s to %s as writing converter although it doesn't convert to a store-supported type! You might wanna check you annotation setup at the converter implementation.";
 	private static final String NOT_A_CONVERTER = "Converter %s is neither a Spring Converter, GenericConverter or ConverterFactory!";
+	private static final String CONVERTER_FILTER = "converter from %s to %s as %s converter.";
+	private static final String ADD_CONVERTER = "Adding %s" + CONVERTER_FILTER;
+	private static final String SKIP_CONVERTER = "Skipping " + CONVERTER_FILTER
+			+ "%s is not a store supported simple type!";
 	private static final List<Object> DEFAULT_CONVERTERS;
 
 	static {
@@ -89,7 +94,10 @@ public class CustomConversions {
 			convertiblePair.getSourceType(), null, writingPairs);
 
 	/**
-	 * Creates a new {@link CustomConversions} instance registering the given converters.
+	 * Creates a new {@link CustomConversions} instance registering all given user defined converters and selecting
+	 * {@link Converter converters} from {@link StoreConversions} depending on
+	 * {@link StoreConversions#getSimpleTypeHolder() store simple types} only considering those that either convert
+	 * to/from a store supported type.
 	 *
 	 * @param storeConversions must not be {@literal null}.
 	 * @param converters must not be {@literal null}.
@@ -99,21 +107,47 @@ public CustomConversions(StoreConversions storeConversions, Collection<?> conver
 		Assert.notNull(storeConversions, "StoreConversions must not be null!");
 		Assert.notNull(converters, "List of converters must not be null!");
 
-		List<Object> toRegister = new ArrayList<>();
+		List<Object> registeredConverters = collectPotentialConverterRegistrations(storeConversions, converters).stream() //
+				.filter(this::isSupportedConverter) //
+				.map(ConverterRegistrationIntent::getConverterRegistration) //
+				.map(this::register) //
+				.distinct() //
+				.collect(Collectors.toList());
 
-		// Add user provided converters to make sure they can override the defaults
-		toRegister.addAll(converters);
-		toRegister.addAll(storeConversions.getStoreConverters());
-		toRegister.addAll(DEFAULT_CONVERTERS);
+		Collections.reverse(registeredConverters);
 
-		toRegister.stream()//
-				.flatMap(it -> storeConversions.getRegistrationsFor(it).stream())//
-				.forEach(this::register);
+		this.converters = Collections.unmodifiableList(registeredConverters);
+		this.simpleTypeHolder = new SimpleTypeHolder(customSimpleTypes, storeConversions.getStoreTypeHolder());
+	}
 
-		Collections.reverse(toRegister);
+	/**
+	 * Validate a given {@link ConverterRegistration} in a specific setup. <br />
+	 * Non {@link ReadingConverter reading} and user defined {@link Converter converters} are only considered supported if
+	 * the {@link ConverterRegistrationIntent#isSimpleTargetType() target type} is considered to be a store simple type.
+	 * 
+	 * @param registration
+	 * @return
+	 * @since 2.3
+	 */
+	protected boolean isSupportedConverter(ConverterRegistrationIntent registration) {
 
-		this.converters = Collections.unmodifiableList(toRegister);
-		this.simpleTypeHolder = new SimpleTypeHolder(customSimpleTypes, storeConversions.getStoreTypeHolder());
+		boolean register = registration.isUserConverter() || (registration.isReading() && registration.isSimpleSourceType())
+				|| (registration.isWriting() && registration.isSimpleTargetType());
+
+		if (LOG.isDebugEnabled()) {
+
+			if (register) {
+				LOG.debug(String.format(ADD_CONVERTER, registration.isUserConverter() ? "user defined " : "",
+						registration.getSourceType(), registration.getTargetType(),
+						registration.isReading() ? "reading" : "writing"));
+			} else {
+				LOG.debug(String.format(SKIP_CONVERTER, registration.getSourceType(), registration.getTargetType(),
+						registration.isReading() ? "reading" : "writing",
+						registration.isReading() ? registration.getSourceType() : registration.getTargetType()));
+			}
+		}
+
+		return register;
 	}
 
 	/**
@@ -152,6 +186,36 @@ public void registerConvertersIn(ConverterRegistry conversionService) {
 		converters.forEach(it -> registerConverterIn(it, conversionService));
 	}
 
+	/**
+	 * Get all converters and add some origin information
+	 * 
+	 * @param storeConversions
+	 * @param converters
+	 * @return
+	 */
+	private List<ConverterRegistrationIntent> collectPotentialConverterRegistrations(StoreConversions storeConversions,
+			Collection<?> converters) {
+
+		List<ConverterRegistrationIntent> converterRegistrations = new ArrayList<>();
+
+		converterRegistrations.addAll(converters.stream() //
+				.flatMap(it -> storeConversions.getRegistrationsFor(it).stream()) //
+				.map(ConverterRegistrationIntent::userConverters) //
+				.collect(Collectors.toList()));
+
+		converterRegistrations.addAll(storeConversions.getStoreConverters().stream() //
+				.flatMap(it -> storeConversions.getRegistrationsFor(it).stream()) //
+				.map(ConverterRegistrationIntent::storeConverters) //
+				.collect(Collectors.toList()));
+
+		converterRegistrations.addAll(DEFAULT_CONVERTERS.stream() //
+				.flatMap(it -> storeConversions.getRegistrationsFor(it).stream()) //
+				.map(ConverterRegistrationIntent::defaultConverters) //
+				.collect(Collectors.toList()));
+
+		return converterRegistrations;
+	}
+
 	/**
 	 * Registers the given converter in the given {@link GenericConversionService}.
 	 *
@@ -193,7 +257,7 @@ private void registerConverterIn(Object candidate, ConverterRegistry conversionS
 	 *
 	 * @param converterRegistration
 	 */
-	private void register(ConverterRegistration converterRegistration) {
+	private Object register(ConverterRegistration converterRegistration) {
 
 		Assert.notNull(converterRegistration, "Converter registration must not be null!");
 
@@ -217,6 +281,8 @@ private void register(ConverterRegistration converterRegistration) {
 				LOG.warn(String.format(WRITE_CONVERTER_NOT_SIMPLE, pair.getSourceType(), pair.getTargetType()));
 			}
 		}
+
+		return converterRegistration.getConverter();
 	}
 
 	/**
@@ -442,15 +508,90 @@ public Class<?> computeIfAbsent(Class<?> targetType, Function<ConvertiblePair, C
 		}
 	}
 
+	/**
+	 * Value class tying together a {@link ConverterRegistration} and its {@link ConverterOrigin origin} to allow fine
+	 * grained registration based on store supported types.
+	 *
+	 * @since 2.3
+	 * @author Christoph Strobl
+	 */
+	protected static class ConverterRegistrationIntent {
+
+		private final ConverterRegistration delegate;
+		private final ConverterOrigin origin;
+
+		ConverterRegistrationIntent(ConverterRegistration delegate, ConverterOrigin origin) {
+			this.delegate = delegate;
+			this.origin = origin;
+		}
+
+		static ConverterRegistrationIntent userConverters(ConverterRegistration delegate) {
+			return new ConverterRegistrationIntent(delegate, ConverterOrigin.USER_DEFINED);
+		}
+
+		static ConverterRegistrationIntent storeConverters(ConverterRegistration delegate) {
+			return new ConverterRegistrationIntent(delegate, ConverterOrigin.STORE);
+		}
+
+		static ConverterRegistrationIntent defaultConverters(ConverterRegistration delegate) {
+			return new ConverterRegistrationIntent(delegate, ConverterOrigin.DEFAULT);
+		}
+
+		Class<?> getSourceType() {
+			return delegate.getConvertiblePair().getSourceType();
+		}
+
+		Class<?> getTargetType() {
+			return delegate.getConvertiblePair().getTargetType();
+		}
+
+		public boolean isWriting() {
+			return delegate.isWriting();
+		}
+
+		public boolean isReading() {
+			return delegate.isReading();
+		}
+
+		public boolean isSimpleSourceType() {
+			return delegate.isSimpleSourceType();
+		}
+
+		public boolean isSimpleTargetType() {
+			return delegate.isSimpleTargetType();
+		}
+
+		public boolean isUserConverter() {
+			return isConverterOfSource(ConverterOrigin.USER_DEFINED);
+		}
+
+		public boolean isDefaultConveter() {
+			return isConverterOfSource(ConverterOrigin.DEFAULT);
+		}
+
+		public ConverterRegistration getConverterRegistration() {
+			return delegate;
+		}
+
+		private boolean isConverterOfSource(ConverterOrigin source) {
+			return origin.equals(source);
+		}
+
+		protected enum ConverterOrigin {
+			DEFAULT, USER_DEFINED, STORE
+		}
+	}
+
 	/**
 	 * Conversion registration information.
 	 *
 	 * @author Oliver Gierke
 	 * @author Mark Paluch
 	 */
 	@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
-	static class ConverterRegistration {
+	private static class ConverterRegistration {
 
+		private final Object converter;
 		private final @NonNull ConvertiblePair convertiblePair;
 		private final @NonNull StoreConversions storeConversions;
 		private final boolean reading;
@@ -500,6 +641,13 @@ public boolean isSimpleSourceType() {
 		public boolean isSimpleTargetType() {
 			return storeConversions.isStoreSimpleType(convertiblePair.getTargetType());
 		}
+
+		/**
+		 * @return
+		 */
+		Object getConverter() {
+			return converter;
+		}
 	}
 
 	/**
@@ -575,7 +723,7 @@ public Streamable<ConverterRegistration> getRegistrationsFor(Object converter) {
 
 				return convertibleTypes == null //
 						? Streamable.empty() //
-						: Streamable.of(convertibleTypes).map(it -> register(it, isReading, isWriting));
+						: Streamable.of(convertibleTypes).map(it -> register(converter, it, isReading, isWriting));
 
 			} else if (converter instanceof ConverterFactory) {
 
@@ -600,15 +748,17 @@ private Streamable<ConverterRegistration> getRegistrationFor(Object converter, C
 				throw new IllegalStateException(String.format("Couldn't resolve type arguments for %s!", converterType));
 			}
 
-			return Streamable.of(register(arguments[0], arguments[1], isReading, isWriting));
+			return Streamable.of(register(converter, arguments[0], arguments[1], isReading, isWriting));
 		}
 
-		private ConverterRegistration register(Class<?> source, Class<?> target, boolean isReading, boolean isWriting) {
-			return register(new ConvertiblePair(source, target), isReading, isWriting);
+		private ConverterRegistration register(Object converter, Class<?> source, Class<?> target, boolean isReading,
+				boolean isWriting) {
+			return register(converter, new ConvertiblePair(source, target), isReading, isWriting);
 		}
 
-		private ConverterRegistration register(ConvertiblePair pair, boolean isReading, boolean isWriting) {
-			return new ConverterRegistration(pair, this, isReading, isWriting);
+		private ConverterRegistration register(Object converter, ConvertiblePair pair, boolean isReading,
+				boolean isWriting) {
+			return new ConverterRegistration(converter, pair, this, isReading, isWriting);
 		}
 
 		private boolean isStoreSimpleType(Class<?> type) {

src/test/java/org/springframework/data/convert/CustomConversionsUnitTests.java

@@ -16,6 +16,7 @@
 package org.springframework.data.convert;
 
 import static org.assertj.core.api.Assertions.*;
+import static org.mockito.Mockito.*;
 
 import java.text.DateFormat;
 import java.text.Format;
@@ -27,17 +28,17 @@
 
 import org.joda.time.DateTime;
 import org.junit.Test;
-
 import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConverterFactory;
+import org.springframework.core.convert.converter.ConverterRegistry;
 import org.springframework.core.convert.support.ConfigurableConversionService;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.core.convert.support.GenericConversionService;
 import org.springframework.data.convert.ConverterBuilder.ConverterAware;
 import org.springframework.data.convert.CustomConversions.StoreConversions;
+import org.springframework.data.convert.ThreeTenBackPortConverters.LocalDateTimeToJavaTimeInstantConverter;
 import org.springframework.data.mapping.model.SimpleTypeHolder;
-
 import org.threeten.bp.LocalDateTime;
 
 /**
@@ -194,6 +195,29 @@ public void registersConverterFromConverterAware() {
 		assertThat(conversionService.canConvert(Locale.class, CustomType.class)).isTrue();
 	}
 
+	@Test // DATACMNS-1615
+	public void skipsUnsupportedDefaultWritingConverter() {
+
+		ConverterRegistry registry = mock(ConverterRegistry.class);
+		StoreConversions storeConversions = StoreConversions
+				.of(new SimpleTypeHolder(Collections.singleton(Date.class), true) {
+
+					@Override
+					public boolean isSimpleType(Class<?> type) {
+
+						if (type.getName().startsWith("java.time")) {
+							return false;
+						}
+
+						return super.isSimpleType(type);
+					}
+				});
+
+		new CustomConversions(storeConversions, Collections.emptyList()).registerConvertersIn(registry);
+
+		verify(registry, never()).addConverter(any(LocalDateTimeToJavaTimeInstantConverter.class));
+	}
+
 	private static Class<?> createProxyTypeFor(Class<?> type) {
 
 		ProxyFactory factory = new ProxyFactory();