DAATCMNS-1034 - Improvements to new ConverterBuilder API.

Made nested API interfaces public so that they can actually be used. CustomConversions now considers ConverterAware and treats them appropriately for ConvertiblePair registration as well as during the registration in the ConversionService. Tweaked parameter types in CustomConversions to rather accept a Collection<?> for the converters (previously List<?>). Also, registerConverterIn(…) now takes a ConverterRegistry over a GenericConversionService. Polished Javadoc and non-null assertions.

Author: odrotbohm

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

@@ -87,7 +87,7 @@ static <S, T> WritingConverterBuilder<S, T> writing(Class<S> source, Class<T> ta
 	 * @author Oliver Gierke
 	 * @since 2.0
 	 */
-	interface WritingConverterAware {
+	public interface WritingConverterAware {
 
 		/**
 		 * Returns the writing converter already created.
@@ -103,7 +103,7 @@ interface WritingConverterAware {
 	 * @author Oliver Gierke
 	 * @since 2.0
 	 */
-	interface ReadingConverterAware {
+	public interface ReadingConverterAware {
 
 		/**
 		 * Returns the reading converter already created.
@@ -119,7 +119,7 @@ interface ReadingConverterAware {
 	 * @author Oliver Gierke
 	 * @since 2.0
 	 */
-	interface ReadingConverterBuilder<T, S> extends ConverterBuilder, ReadingConverterAware {
+	public interface ReadingConverterBuilder<T, S> extends ConverterBuilder, ReadingConverterAware {
 
 		/**
 		 * Creates a new {@link ConverterAware} by registering the given {@link Function} to add a write converter.
@@ -136,7 +136,7 @@ interface ReadingConverterBuilder<T, S> extends ConverterBuilder, ReadingConvert
 	 * @author Oliver Gierke
 	 * @since 2.0
 	 */
-	interface WritingConverterBuilder<S, T> extends ConverterBuilder, WritingConverterAware {
+	public interface WritingConverterBuilder<S, T> extends ConverterBuilder, WritingConverterAware {
 
 		/**
 		 * Creates a new {@link ConverterAware} by registering the given {@link Function} to add a write converter.
@@ -153,5 +153,5 @@ interface WritingConverterBuilder<S, T> extends ConverterBuilder, WritingConvert
 	 * @author Oliver Gierke
 	 * @since 2.0
 	 */
-	interface ConverterAware extends ConverterBuilder, ReadingConverterAware, WritingConverterAware {}
+	public interface ConverterAware extends ConverterBuilder, ReadingConverterAware, WritingConverterAware {}
 }

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

@@ -37,9 +37,11 @@
 import org.springframework.core.GenericTypeResolver;
 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.converter.GenericConverter;
 import org.springframework.core.convert.converter.GenericConverter.ConvertiblePair;
 import org.springframework.core.convert.support.GenericConversionService;
+import org.springframework.data.convert.ConverterBuilder.ConverterAware;
 import org.springframework.data.mapping.model.SimpleTypeHolder;
 import org.springframework.data.util.Optionals;
 import org.springframework.data.util.Streamable;
@@ -90,10 +92,12 @@ public class CustomConversions {
 	/**
 	 * Creates a new {@link CustomConversions} instance registering the given converters.
 	 * 
-	 * @param converters
+	 * @param storeConversions must not be {@literal null}.
+	 * @param converters must not be {@literal null}.
 	 */
-	public CustomConversions(StoreConversions storeConversions, List<?> converters) {
+	public CustomConversions(StoreConversions storeConversions, Collection<?> converters) {
 
+		Assert.notNull(storeConversions, "StoreConversions must not be null!");
 		Assert.notNull(converters, "List of converters must not be null!");
 
 		this.readingPairs = new LinkedHashSet<>();
@@ -149,33 +153,46 @@ public boolean isSimpleType(Class<?> type) {
 	 * 
 	 * @param conversionService
 	 */
-	public void registerConvertersIn(GenericConversionService conversionService) {
+	public void registerConvertersIn(ConverterRegistry conversionService) {
 
 		Assert.notNull(conversionService, "ConversionService must not be null!");
 
-		converters.forEach(it -> {
+		converters.forEach(it -> registerConverterIn(it, conversionService));
+	}
 
-			boolean added = false;
+	/**
+	 * Registers the given converter in the given {@link GenericConversionService}.
+	 * 
+	 * @param candidate must not be {@literal null}.
+	 * @param conversionService must not be {@literal null}.
+	 */
+	private void registerConverterIn(Object candidate, ConverterRegistry conversionService) {
 
-			if (it instanceof Converter) {
-				conversionService.addConverter(Converter.class.cast(it));
-				added = true;
-			}
+		boolean added = false;
 
-			if (it instanceof ConverterFactory) {
-				conversionService.addConverterFactory(ConverterFactory.class.cast(it));
-				added = true;
-			}
+		if (candidate instanceof Converter) {
+			conversionService.addConverter(Converter.class.cast(candidate));
+			added = true;
+		}
 
-			if (it instanceof GenericConverter) {
-				conversionService.addConverter(GenericConverter.class.cast(it));
-				added = true;
-			}
+		if (candidate instanceof ConverterFactory) {
+			conversionService.addConverterFactory(ConverterFactory.class.cast(candidate));
+			added = true;
+		}
 
-			if (!added) {
-				throw new IllegalArgumentException(String.format(NOT_A_CONVERTER, it));
-			}
-		});
+		if (candidate instanceof GenericConverter) {
+			conversionService.addConverter(GenericConverter.class.cast(candidate));
+			added = true;
+		}
+
+		if (candidate instanceof ConverterAware) {
+			ConverterAware.class.cast(candidate).getConverters().forEach(it -> registerConverterIn(it, conversionService));
+			added = true;
+		}
+
+		if (!added) {
+			throw new IllegalArgumentException(String.format(NOT_A_CONVERTER, candidate));
+		}
 	}
 
 	/**
@@ -460,10 +477,15 @@ public Streamable<ConverterRegistration> getRegistrationsFor(Object converter) {
 			boolean isWriting = type.isAnnotationPresent(WritingConverter.class);
 			boolean isReading = type.isAnnotationPresent(ReadingConverter.class);
 
-			if (converter instanceof GenericConverter) {
+			if (converter instanceof ConverterAware) {
+
+				return Streamable.of(() -> ConverterAware.class.cast(converter).getConverters().stream()//
+						.flatMap(it -> getRegistrationsFor(it).stream()));
+
+			} else if (converter instanceof GenericConverter) {
 
-				GenericConverter genericConverter = (GenericConverter) converter;
-				return Streamable.of(genericConverter.getConvertibleTypes()).map(it -> register(it, isReading, isWriting));
+				return Streamable.of(GenericConverter.class.cast(converter).getConvertibleTypes())//
+						.map(it -> register(it, isReading, isWriting));
 
 			} else if (converter instanceof ConverterFactory) {
 
@@ -474,7 +496,7 @@ public Streamable<ConverterRegistration> getRegistrationsFor(Object converter) {
 				return getRegistrationFor(converter, Converter.class, isReading, isWriting);
 
 			} else {
-				throw new IllegalArgumentException("Unsupported converter type!");
+				throw new IllegalArgumentException(String.format("Unsupported converter type %s!", converter));
 			}
 		}
 

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

@@ -32,8 +32,10 @@
 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.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.threeten.bp.LocalDateTime;
 
@@ -158,6 +160,24 @@ public void registersConverterFactoryCorrectly() {
 		assertThat(customConversions.getCustomWriteTarget(String.class, SimpleDateFormat.class)).isPresent();
 	}
 
+	@Test // DATACMNS-1034
+	public void registersConverterFromConverterAware() {
+
+		ConverterAware converters = ConverterBuilder.reading(Left.class, Right.class, left -> new Right())
+				.andWriting(right -> new Left());
+
+		CustomConversions conversions = new CustomConversions(StoreConversions.NONE, Collections.singletonList(converters));
+
+		assertThat(conversions.hasCustomWriteTarget(Right.class)).isTrue();
+		assertThat(conversions.hasCustomReadTarget(Left.class, Right.class)).isTrue();
+
+		ConfigurableConversionService conversionService = new GenericConversionService();
+		conversions.registerConvertersIn(conversionService);
+
+		assertThat(conversionService.canConvert(Left.class, Right.class)).isTrue();
+		assertThat(conversionService.canConvert(Right.class, Left.class)).isTrue();
+	}
+
 	private static Class<?> createProxyTypeFor(Class<?> type) {
 
 		ProxyFactory factory = new ProxyFactory();
@@ -274,4 +294,8 @@ public T convert(String source) {
 			}
 		}
 	}
+
+	static class Left {}
+
+	static class Right {}
 }