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

CustomConversions now registers all given user defined converters and selects only those converters from the StoreConversions that convert to or from a store supported simple type.
By doing so we make sure to only register supported converters which removes warning messages from the log and reduces the number of converters within the ConversionService.

Author: christophstrobl

Diff for: 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) {

Diff for: 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();