Polishing.

Rename generics from C extends PersistentProperty to P extends PersistentProperty.
Refine conversion setup. Make PropertyValueConversions.getValueConverter(…) to return non-null. Return PropertyValueConversions from CustomConversions.

Resolves #2577
Closes #2592

Author: mp911de

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

@@ -33,6 +33,7 @@
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+
 import org.springframework.core.GenericTypeResolver;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.core.convert.converter.Converter;
@@ -42,7 +43,6 @@
 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.PersistentProperty;
 import org.springframework.data.mapping.model.SimpleTypeHolder;
 import org.springframework.data.util.Predicates;
 import org.springframework.data.util.Streamable;
@@ -185,35 +185,9 @@ public void registerConvertersIn(ConverterRegistry conversionService) {
 		VavrCollectionConverters.getConvertersToRegister().forEach(it -> registerConverterIn(it, conversionService));
 	}
 
-	/**
-	 * Delegate check if a {@link PropertyValueConverter} for the given {@literal property} is present via
-	 * {@link PropertyValueConversions}.
-	 *
-	 * @param property must not be {@literal null}.
-	 * @return {@literal true} if a specific {@link PropertyValueConverter} is available.
-	 * @see PropertyValueConversions#hasValueConverter(PersistentProperty)
-	 * @since 2.7
-	 */
-	public boolean hasPropertyValueConverter(PersistentProperty<?> property) {
-		return propertyValueConversions != null && propertyValueConversions.hasValueConverter(property);
-	}
-
-	/**
-	 * Delegate to obtain the {@link PropertyValueConverter} for the given {@literal property} from
-	 * {@link PropertyValueConversions}.
-	 *
-	 * @param property must not be {@literal null}.
-	 * @param <DV> domain-specific type
-	 * @param <SV> store-native type
-	 * @param <C> conversion context type
-	 * @return the suitable {@link PropertyValueConverter} or {@literal null} if none available.
-	 * @see PropertyValueConversions#getValueConverter(PersistentProperty)
-	 * @since 2.7
-	 */
 	@Nullable
-	public <DV, SV, C extends PersistentProperty<C>, VCC extends ValueConversionContext<C>> PropertyValueConverter<DV, SV, VCC> getPropertyValueConverter(
-			C property) {
-		return propertyValueConversions != null ? propertyValueConversions.getValueConverter(property) : null;
+	public PropertyValueConversions getPropertyValueConversions() {
+		return propertyValueConversions;
 	}
 
 	/**

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

@@ -28,13 +28,14 @@
  */
 public class PropertyValueConversionService {
 
-	private final CustomConversions conversions;
+	private final PropertyValueConversions conversions;
 
 	public PropertyValueConversionService(CustomConversions conversions) {
 
 		Assert.notNull(conversions, "CustomConversions must not be null");
 
-		this.conversions = conversions;
+		PropertyValueConversions pvc = conversions.getPropertyValueConversions();
+		this.conversions = pvc == null ? NoOpPropertyValueConversions.INSTANCE : pvc;
 	}
 
 	/**
@@ -47,7 +48,7 @@ public PropertyValueConversionService(CustomConversions conversions) {
 	 * @return {@literal true} there is a converter registered for {@link PersistentProperty}.
 	 */
 	public boolean hasConverter(PersistentProperty<?> property) {
-		return conversions.hasPropertyValueConverter(property);
+		return conversions.hasValueConverter(property);
 	}
 
 	/**
@@ -64,7 +65,8 @@ public boolean hasConverter(PersistentProperty<?> property) {
 	public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>> Object read(@Nullable Object value,
 			P property, VCC context) {
 
-		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = getRequiredConverter(property);
+		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = conversions
+				.getValueConverter(property);
 
 		if (value == null) {
 			return converter.readNull(context);
@@ -87,7 +89,8 @@ public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>>
 	public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>> Object write(@Nullable Object value,
 			P property, VCC context) {
 
-		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = getRequiredConverter(property);
+		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = conversions
+				.getValueConverter(property);
 
 		if (value == null) {
 			return converter.writeNull(context);
@@ -96,16 +99,19 @@ public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>>
 		return converter.write(value, context);
 	}
 
-	private <P extends PersistentProperty<P>> PropertyValueConverter<Object, Object, ValueConversionContext<P>> getRequiredConverter(
-			P property) {
+	enum NoOpPropertyValueConversions implements PropertyValueConversions {
 
-		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = conversions
-				.getPropertyValueConverter(property);
+		INSTANCE;
 
-		if (converter == null) {
-			throw new IllegalArgumentException(String.format("No converter registered for property %s", property));
+		@Override
+		public boolean hasValueConverter(PersistentProperty<?> property) {
+			return false;
 		}
 
-		return converter;
+		@Override
+		public <DV, SV, P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>> PropertyValueConverter<DV, SV, VCC> getValueConverter(
+				P property) {
+			throw new UnsupportedOperationException();
+		}
 	}
 }

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

@@ -45,11 +45,13 @@ public interface PropertyValueConversions {
 	 * @param property must not be {@literal null}.
 	 * @param <DV> domain-specific type
 	 * @param <SV> store-native type
-	 * @param <C> conversion context type
+	 * @param <P> conversion context type
 	 * @return the suitable {@link PropertyValueConverter}.
+	 * @throws IllegalArgumentException if there is no converter available for {@code property}.
+	 * @see #hasValueConverter(PersistentProperty)
 	 */
-	<DV, SV, C extends PersistentProperty<C>, VCC extends ValueConversionContext<C>> PropertyValueConverter<DV, SV, VCC> getValueConverter(
-			C property);
+	<DV, SV, P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>> PropertyValueConverter<DV, SV, VCC> getValueConverter(
+			P property);
 
 	/**
 	 * Helper that allows to create {@link PropertyValueConversions} instance with the configured

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

@@ -46,12 +46,12 @@ public interface PropertyValueConverterFactory {
 	 * @param property must not be {@literal null}.
 	 * @param <DV> domain-specific type.
 	 * @param <SV> store-native type.
-	 * @param <C> value conversion context to use.
+	 * @param <P> value conversion context to use.
 	 * @return can be {@literal null}.
 	 */
 	@SuppressWarnings("unchecked")
 	@Nullable
-	default <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, C> getConverter(
+	default <DV, SV, P extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, P> getConverter(
 			PersistentProperty<?> property) {
 
 		AnnotatedPropertyValueConverterAccessor accessor = new AnnotatedPropertyValueConverterAccessor(property);
@@ -60,7 +60,7 @@ default <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV,
 			return null;
 		}
 
-		return getConverter((Class<PropertyValueConverter<DV, SV, C>>) accessor.getValueConverterType());
+		return getConverter((Class<PropertyValueConverter<DV, SV, P>>) accessor.getValueConverterType());
 	}
 
 	/**

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

@@ -105,11 +105,17 @@ public boolean hasValueConverter(PersistentProperty<?> property) {
 		return obtainConverterFactory().getConverter(property) != null;
 	}
 
-	@Nullable
 	@Override
-	public <DV, SV, C extends PersistentProperty<C>, D extends ValueConversionContext<C>> PropertyValueConverter<DV, SV, D> getValueConverter(
-			C property) {
-		return obtainConverterFactory().getConverter(property);
+	public <DV, SV, P extends PersistentProperty<P>, D extends ValueConversionContext<P>> PropertyValueConverter<DV, SV, D> getValueConverter(
+			P property) {
+
+		PropertyValueConverter<DV, SV, D> converter = obtainConverterFactory().getConverter(property);
+
+		if (converter == null) {
+			throw new IllegalArgumentException(String.format("No PropertyValueConverter registered for %s", property));
+		}
+
+		return converter;
 	}
 
 	/**

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

@@ -31,6 +31,7 @@
 import org.jmolecules.ddd.types.Identifier;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.Test;
+
 import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConverterFactory;
@@ -45,8 +46,8 @@
 import org.springframework.data.convert.Jsr310Converters.LocalDateTimeToDateConverter;
 import org.springframework.data.convert.ThreeTenBackPortConverters.LocalDateTimeToJavaTimeInstantConverter;
 import org.springframework.data.geo.Point;
-import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.mapping.model.SimpleTypeHolder;
+
 import org.threeten.bp.LocalDateTime;
 
 /**
@@ -297,30 +298,17 @@ void registersVavrConverters() {
 
 		ConfigurableConversionService conversionService = new DefaultConversionService();
 
-		new CustomConversions(StoreConversions.NONE, Collections.emptyList())
-				.registerConvertersIn(conversionService);
+		new CustomConversions(StoreConversions.NONE, Collections.emptyList()).registerConvertersIn(conversionService);
 
 		assertThat(conversionService.canConvert(io.vavr.collection.List.class, List.class)).isTrue();
 		assertThat(conversionService.canConvert(List.class, io.vavr.collection.List.class)).isTrue();
 	}
 
-	@Test // GH-1484
-	void allowsToRegisterPropertyConversions() {
-
-		PropertyValueConversions propertyValueConversions = mock(PropertyValueConversions.class);
-		when(propertyValueConversions.getValueConverter(any())).thenReturn(mock(PropertyValueConverter.class));
-
-		CustomConversions conversions = new CustomConversions(new ConverterConfiguration(StoreConversions.NONE,
-				Collections.emptyList(), (it) -> true, propertyValueConversions));
-		assertThat(conversions.getPropertyValueConverter(mock(PersistentProperty.class))).isNotNull();
-	}
-
 	@Test // GH-1484
 	void doesNotFailIfPropertiesConversionIsNull() {
 
-		CustomConversions conversions = new CustomConversions(new ConverterConfiguration(StoreConversions.NONE,
-				Collections.emptyList(), (it) -> true, null));
-		assertThat(conversions.getPropertyValueConverter(mock(PersistentProperty.class))).isNull();
+		new CustomConversions(
+				new ConverterConfiguration(StoreConversions.NONE, Collections.emptyList(), (it) -> true, null));
 	}
 
 	private static Class<?> createProxyTypeFor(Class<?> type) {