Additional refinements.

Resolves #2577
Closes #2592

Author: jxblum

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

@@ -16,36 +16,50 @@
 package org.springframework.data.convert;
 
 import org.springframework.data.mapping.PersistentProperty;
+import org.springframework.lang.NonNull;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 
 /**
- * Conversion service based on {@link CustomConversions} to convert domain and store values using
+ * Conversion service based on {@link CustomConversions} used to convert domain and store values using
  * {@link PropertyValueConverter property-specific converters}.
  *
  * @author Mark Paluch
+ * @see PropertyValueConverter
  * @since 2.7
  */
 public class PropertyValueConversionService {
 
 	private final PropertyValueConversions conversions;
 
-	public PropertyValueConversionService(CustomConversions conversions) {
+	/**
+	 * Constructs a new instance of the {@link PropertyValueConversionService} initialized with the given,
+	 * required {@link CustomConversions} for resolving the {@link PropertyValueConversions} used to
+	 * convert {@link PersistentProperty} values during data access operations.
+	 *
+	 * @param conversions {@link CustomConversions} used to handle domain and store type conversions;
+	 * must not be {@literal null}.
+	 * @throws IllegalArgumentException if {@link CustomConversions} is {@literal null}.
+	 * @see CustomConversions
+	 */
+	public PropertyValueConversionService(@NonNull CustomConversions conversions) {
 
 		Assert.notNull(conversions, "CustomConversions must not be null");
 
-		PropertyValueConversions pvc = conversions.getPropertyValueConversions();
-		this.conversions = pvc == null ? NoOpPropertyValueConversions.INSTANCE : pvc;
+		PropertyValueConversions valueConversions = conversions.getPropertyValueConversions();
+
+		this.conversions = valueConversions != null ? valueConversions : NoOpPropertyValueConversions.INSTANCE;
 	}
 
 	/**
-	 * Return {@literal true} there is a converter registered for {@link PersistentProperty}.
+	 * Return {@literal true} if a {@link PropertyValueConverter} is registered for the {@link PersistentProperty}.
 	 * <p>
 	 * If this method returns {@literal true}, it means {@link #read(Object, PersistentProperty, ValueConversionContext)}
-	 * and {@link #write(Object, PersistentProperty, ValueConversionContext)} are capable to invoke conversion.
+	 * and {@link #write(Object, PersistentProperty, ValueConversionContext)} are capable of handling conversions.
 	 *
-	 * @param property the underlying property.
-	 * @return {@literal true} there is a converter registered for {@link PersistentProperty}.
+	 * @param property {@link PersistentProperty property} to evaluate for registration.
+	 * @return {@literal true} if a {@link PropertyValueConverter} is registered for the {@link PersistentProperty}.
+	 * @see PersistentProperty
 	 */
 	public boolean hasConverter(PersistentProperty<?> property) {
 		return conversions.hasValueConverter(property);
@@ -68,11 +82,8 @@ public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>>
 		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = conversions
 				.getValueConverter(property);
 
-		if (value == null) {
-			return converter.readNull(context);
-		}
-
-		return converter.read(value, context);
+		return value != null ? converter.read(value, context)
+				: converter.readNull(context);
 	}
 
 	/**
@@ -92,11 +103,8 @@ public <P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>>
 		PropertyValueConverter<Object, Object, ValueConversionContext<P>> converter = conversions
 				.getValueConverter(property);
 
-		if (value == null) {
-			return converter.writeNull(context);
-		}
-
-		return converter.write(value, context);
+		return value != null ? converter.write(value, context)
+				: converter.writeNull(context);
 	}
 
 	enum NoOpPropertyValueConversions implements PropertyValueConversions {
@@ -111,7 +119,7 @@ public boolean hasValueConverter(PersistentProperty<?> property) {
 		@Override
 		public <DV, SV, P extends PersistentProperty<P>, VCC extends ValueConversionContext<P>> PropertyValueConverter<DV, SV, VCC> getValueConverter(
 				P property) {
-			throw new UnsupportedOperationException();
+			throw new UnsupportedOperationException("No PropertyValueConversions was configured");
 		}
 	}
 }

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

@@ -21,9 +21,10 @@
 
 /**
  * {@link PropertyValueConversions} provides access to {@link PropertyValueConverter converters} that may only be
- * applied to a specific property. Other than {@link org.springframework.core.convert.converter.Converter converters}
- * registered in {@link CustomConversions}, the property based variants accept and allow returning {@literal null}
- * values and provide access to a store specific {@link ValueConversionContext conversion context}.
+ * applied to a specific {@link PersistentProperty property}. Other than
+ * {@link org.springframework.core.convert.converter.Converter converters} registered in {@link CustomConversions},
+ * the {@link PersistentProperty property} based variants accept and allow returning {@literal null} values
+ * and provide access to a store-specific {@link ValueConversionContext conversion context}.
  *
  * @author Christoph Strobl
  * @since 2.7
@@ -32,30 +33,41 @@
 public interface PropertyValueConversions {
 
 	/**
-	 * Check if a {@link PropertyValueConverter} is present for the given {@literal property}.
+	 * Check if a {@link PropertyValueConverter converter} is registered for
+	 * the given {@link PersistentProperty property}.
 	 *
-	 * @param property must not be {@literal null}.
-	 * @return {@literal true} if a specific {@link PropertyValueConverter} is available.
+	 * @param property {@link PersistentProperty} to evaluate; must not be {@literal null}.
+	 * @return {@literal true} if a specific {@link PropertyValueConverter converter} is registered for
+	 * the given {@link PersistentProperty}.
+	 * @see PersistentProperty
 	 */
 	boolean hasValueConverter(PersistentProperty<?> property);
 
 	/**
-	 * Get the {@link PropertyValueConverter} for the given {@literal property}.
+	 * Get the {@link PropertyValueConverter converter} registered for the given {@link PersistentProperty property}.
 	 *
-	 * @param property must not be {@literal null}.
+	 * @param property {@link PersistentProperty} used to look up the registered {@link PropertyValueConverter};
+	 * must not be {@literal null}.
 	 * @param <DV> domain-specific type
 	 * @param <SV> store-native type
 	 * @param <P> conversion context type
-	 * @return the suitable {@link PropertyValueConverter}.
-	 * @throws IllegalArgumentException if there is no converter available for {@code property}.
+	 * @return the {@link PropertyValueConverter} registered for the given {@link PersistentProperty};
+	 * never {@literal null}.
+	 * @throws IllegalArgumentException if no {@link PropertyValueConverter} was registered for
+	 * the given {@link PersistentProperty}.
 	 * @see #hasValueConverter(PersistentProperty)
+	 * @see PropertyValueConverter
+	 * @see PersistentProperty
 	 */
 	<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
-	 * {@link PropertyValueConverter converters} provided via the given callback.
+	 * Helper method used to create a {@link PropertyValueConversions} instance with the configured
+	 * {@link PropertyValueConverter converters} provided by the {@link Consumer callback}.
+	 *
+	 * @see PropertyValueConverterRegistrar
+	 * @see PropertyValueConversions
 	 */
 	@SuppressWarnings({ "rawtypes", "unchecked" })
 	static <P extends PersistentProperty<P>> PropertyValueConversions simple(

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

@@ -18,6 +18,7 @@
 import java.util.function.BiFunction;
 
 import org.springframework.data.mapping.PersistentProperty;
+import org.springframework.lang.NonNull;
 import org.springframework.lang.Nullable;
 
 /**
@@ -36,6 +37,8 @@
  * @param <DV> domain-specific type.
  * @param <SV> store-native type.
  * @param <C> the store specific {@link ValueConversionContext conversion context}.
+ * @see org.springframework.data.convert.ValueConversionContext
+ * @see org.springframework.data.mapping.PersistentProperty
  * @since 2.7
  */
 public interface PropertyValueConverter<DV, SV, C extends ValueConversionContext<? extends PersistentProperty<?>>> {
@@ -44,18 +47,20 @@ public interface PropertyValueConverter<DV, SV, C extends ValueConversionContext
 	 * Convert the given store specific value into it's domain value representation. Typically, a {@literal read}
 	 * operation.
 	 *
-	 * @param value the value to read.
-	 * @param context never {@literal null}.
+	 * @param value value to read.
+	 * @param context {@link ValueConversionContext} containing store-specific metadata
+	 * used in the value conversion; never {@literal null}.
 	 * @return the converted value. Can be {@literal null}.
 	 */
 	@Nullable
 	DV read(SV value, C context);
 
 	/**
-	 * Convert the given {@code null} value from the store into it's domain value representation. Typically, a
+	 * Convert the given {@code null} value from the store into its domain value representation. Typically, a
 	 * {@literal read} operation. Returns {@code null} by default.
 	 *
-	 * @param context never {@literal null}.
+	 * @param context {@link ValueConversionContext} containing store-specific metadata
+	 * used in the value conversion; never {@literal null}.
 	 * @return the converted value. Can be {@literal null}.
 	 */
 	@Nullable
@@ -67,8 +72,9 @@ default DV readNull(C context) {
 	 * Convert the given domain-specific value into it's native store representation. Typically, a {@literal write}
 	 * operation.
 	 *
-	 * @param value the value to write.
-	 * @param context never {@literal null}.
+	 * @param value value to write; can be {@literal null}.
+	 * @param context {@link ValueConversionContext} containing store-specific metadata
+	 * used in the value conversion; never {@literal null}.
 	 * @return the converted value. Can be {@literal null}.
 	 */
 	@Nullable
@@ -78,7 +84,8 @@ default DV readNull(C context) {
 	 * Convert the given {@code null} value from the domain model into it's native store representation. Typically, a
 	 * {@literal write} operation. Returns {@code null} by default.
 	 *
-	 * @param context never {@literal null}.
+	 * @param context {@link ValueConversionContext} containing store-specific metadata
+	 * used in the value conversion; never {@literal null}.
 	 * @return the converted value. Can be {@literal null}.
 	 */
 	@Nullable
@@ -120,32 +127,32 @@ class FunctionPropertyValueConverter<DV, SV, P extends PersistentProperty<P>>
 		private final BiFunction<DV, ValueConversionContext<P>, SV> writer;
 		private final BiFunction<SV, ValueConversionContext<P>, DV> reader;
 
-		public FunctionPropertyValueConverter(BiFunction<DV, ValueConversionContext<P>, SV> writer,
-				BiFunction<SV, ValueConversionContext<P>, DV> reader) {
+		public FunctionPropertyValueConverter(@NonNull BiFunction<DV, ValueConversionContext<P>, SV> writer,
+				@NonNull BiFunction<SV, ValueConversionContext<P>, DV> reader) {
 
 			this.writer = writer;
 			this.reader = reader;
 		}
 
 		@Nullable
 		@Override
-		public SV write(DV value, ValueConversionContext<P> context) {
+		public SV write(@Nullable DV value, @NonNull ValueConversionContext<P> context) {
 			return writer.apply(value, context);
 		}
 
 		@Override
-		public SV writeNull(ValueConversionContext<P> context) {
+		public SV writeNull(@NonNull ValueConversionContext<P> context) {
 			return writer.apply(null, context);
 		}
 
 		@Nullable
 		@Override
-		public DV read(@Nullable SV value, ValueConversionContext<P> context) {
+		public DV read(@Nullable SV value, @NonNull ValueConversionContext<P> context) {
 			return reader.apply(value, context);
 		}
 
 		@Override
-		public DV readNull(ValueConversionContext<P> context) {
+		public DV readNull(@NonNull ValueConversionContext<P> context) {
 			return reader.apply(null, context);
 		}
 	}

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

@@ -50,23 +50,26 @@ final class PropertyValueConverterFactories {
 	 */
 	static class ChainedPropertyValueConverterFactory implements PropertyValueConverterFactory {
 
-		private List<PropertyValueConverterFactory> delegates;
+		private final List<PropertyValueConverterFactory> delegates;
 
 		ChainedPropertyValueConverterFactory(List<PropertyValueConverterFactory> delegates) {
 			this.delegates = Collections.unmodifiableList(delegates);
 		}
 
 		@Nullable
 		@Override
+		@SuppressWarnings("unchecked")
 		public <A, B, C extends ValueConversionContext<?>> PropertyValueConverter<A, B, C> getConverter(
 				PersistentProperty<?> property) {
+
 			return delegates.stream().map(it -> (PropertyValueConverter<A, B, C>) it.getConverter(property))
 					.filter(Objects::nonNull).findFirst().orElse(null);
 		}
 
 		@Override
 		public <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T, C> getConverter(
 				Class<? extends PropertyValueConverter<S, T, C>> converterType) {
+
 			return delegates.stream().filter(it -> it.getConverter(converterType) != null).findFirst()
 					.map(it -> it.getConverter(converterType)).orElse(null);
 		}
@@ -82,6 +85,7 @@ public <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T,
 	static class SimplePropertyConverterFactory implements PropertyValueConverterFactory {
 
 		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T, C> getConverter(
 				Class<? extends PropertyValueConverter<S, T, C>> converterType) {
 
@@ -90,6 +94,7 @@ public <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T,
 			if (converterType.isEnum()) {
 				return (PropertyValueConverter<S, T, C>) EnumSet.allOf((Class) converterType).iterator().next();
 			}
+
 			return BeanUtils.instantiateClass(converterType);
 		}
 	}
@@ -110,10 +115,11 @@ static class BeanFactoryAwarePropertyValueConverterFactory implements PropertyVa
 		}
 
 		@Override
+		@SuppressWarnings("unchecked")
 		public <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, C> getConverter(
 				Class<? extends PropertyValueConverter<DV, SV, C>> converterType) {
 
-			Assert.notNull(converterType, "ConverterType must not be null!");
+			Assert.notNull(converterType, "ConverterType must not be null");
 
 			PropertyValueConverter<DV, SV, C> converter = beanFactory.getBeanProvider(converterType).getIfAvailable();
 
@@ -139,21 +145,25 @@ static class ConfiguredInstanceServingValueConverterFactory implements PropertyV
 
 		ConfiguredInstanceServingValueConverterFactory(ValueConverterRegistry<?> converterRegistry) {
 
-			Assert.notNull(converterRegistry, "ConversionsRegistrar must not be null!");
+			Assert.notNull(converterRegistry, "ConversionsRegistrar must not be null");
+
 			this.converterRegistry = converterRegistry;
 		}
 
 		@Nullable
 		@Override
+		@SuppressWarnings("unchecked")
 		public <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, C> getConverter(
 				PersistentProperty<?> property) {
+
 			return (PropertyValueConverter<DV, SV, C>) converterRegistry.getConverter(property.getOwner().getType(),
 					property.getName());
 		}
 
 		@Override
 		public <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T, C> getConverter(
 				Class<? extends PropertyValueConverter<S, T, C>> converterType) {
+
 			return null;
 		}
 	}
@@ -172,12 +182,14 @@ static class CachingPropertyValueConverterFactory implements PropertyValueConver
 
 		CachingPropertyValueConverterFactory(PropertyValueConverterFactory delegate) {
 
-			Assert.notNull(delegate, "Delegate must not be null!");
+			Assert.notNull(delegate, "Delegate must not be null");
+
 			this.delegate = delegate;
 		}
 
 		@Nullable
 		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, C> getConverter(
 				PersistentProperty<?> property) {
 
@@ -188,6 +200,7 @@ public <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV,
 		}
 
 		@Override
+		@SuppressWarnings({ "rawtypes", "unchecked" })
 		public <DV, SV, C extends ValueConversionContext<?>> PropertyValueConverter<DV, SV, C> getConverter(
 				Class<? extends PropertyValueConverter<DV, SV, C>> converterType) {
 
@@ -218,15 +231,19 @@ <S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T, C> cach
 				AnnotatedPropertyValueConverterAccessor accessor = new AnnotatedPropertyValueConverterAccessor(property);
 				Class<? extends PropertyValueConverter<?, ?, ? extends ValueConversionContext<? extends PersistentProperty<?>>>> valueConverterType = accessor
 						.getValueConverterType();
+
 				if (valueConverterType != null) {
 					cache(valueConverterType, converter);
 				}
+
 				return converter;
 			}
 
 			<S, T, C extends ValueConversionContext<?>> PropertyValueConverter<S, T, C> cache(Class<?> type,
 					@Nullable PropertyValueConverter<S, T, C> converter) {
+
 				typeCache.putIfAbsent(type, Optional.ofNullable(converter));
+
 				return converter;
 			}
 		}