From dbcf9aefff1be9da4168bdd479473322133f0130 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 22 Nov 2019 09:25:43 +0100 Subject: [PATCH] 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. Original pull request: #421. --- .../data/convert/CustomConversions.java | 332 ++++++++++++++++-- .../convert/CustomConversionsUnitTests.java | 66 +++- 2 files changed, 357 insertions(+), 41 deletions(-) diff --git a/src/main/java/org/springframework/data/convert/CustomConversions.java b/src/main/java/org/springframework/data/convert/CustomConversions.java index f987bc3a0..2da52f414 100644 --- a/src/main/java/org/springframework/data/convert/CustomConversions.java +++ b/src/main/java/org/springframework/data/convert/CustomConversions.java @@ -22,9 +22,20 @@ import lombok.RequiredArgsConstructor; import lombok.Value; import lombok.extern.slf4j.Slf4j; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; +import java.util.function.Supplier; +import java.util.stream.Collectors; import org.springframework.core.GenericTypeResolver; import org.springframework.core.convert.converter.Converter; @@ -57,6 +68,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 want to check your 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 want to check your 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 DEFAULT_CONVERTERS; static { @@ -79,6 +94,8 @@ public class CustomConversions { private final ConversionTargetsCache customReadTargetTypes = new ConversionTargetsCache(); private final ConversionTargetsCache customWriteTargetTypes = new ConversionTargetsCache(); + private final ConverterConfiguration converterConfiguration; + private final Function> getReadTarget = convertiblePair -> getCustomTarget( convertiblePair.getSourceType(), convertiblePair.getTargetType(), readingPairs); @@ -89,31 +106,51 @@ public class CustomConversions { convertiblePair.getSourceType(), null, writingPairs); /** - * Creates a new {@link CustomConversions} instance registering the given converters. + * Deferred initialization allowing store implementations use a callback/consumer driven configuration of the actual + * {@link CustomConversions}. + * + * @param converterConfiguration the {@link Supplier} providing a {@link ConverterConfiguration}. + * @since 2.3 + */ + protected CustomConversions(Supplier converterConfiguration) { + this(converterConfiguration.get()); + } + + /** + * @param converterConfiguration the {@link ConverterConfiguration} to apply. + * @since 2.3 + */ + protected CustomConversions(ConverterConfiguration converterConfiguration) { + + this.converterConfiguration = converterConfiguration; + + List registeredConverters = collectPotentialConverterRegistrations( + converterConfiguration.getStoreConversions(), converterConfiguration.getUserConverters()).stream() // + .filter(this::isSupportedConverter) // + .filter(it -> !skip(it)) // + .map(ConverterRegistrationIntent::getConverterRegistration) // + .map(this::register) // + .distinct() // + .collect(Collectors.toList()); + + Collections.reverse(registeredConverters); + + this.converters = Collections.unmodifiableList(registeredConverters); + this.simpleTypeHolder = new SimpleTypeHolder(customSimpleTypes, + converterConfiguration.getStoreConversions().getStoreTypeHolder()); + } + + /** + * 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}. */ public CustomConversions(StoreConversions storeConversions, Collection converters) { - - Assert.notNull(storeConversions, "StoreConversions must not be null!"); - Assert.notNull(converters, "List of converters must not be null!"); - - List toRegister = new ArrayList<>(); - - // Add user provided converters to make sure they can override the defaults - toRegister.addAll(converters); - toRegister.addAll(storeConversions.getStoreConverters()); - toRegister.addAll(DEFAULT_CONVERTERS); - - toRegister.stream()// - .flatMap(it -> storeConversions.getRegistrationsFor(it).stream())// - .forEach(this::register); - - Collections.reverse(toRegister); - - this.converters = Collections.unmodifiableList(toRegister); - this.simpleTypeHolder = new SimpleTypeHolder(customSimpleTypes, storeConversions.getStoreTypeHolder()); + this(new ConverterConfiguration(storeConversions, new ArrayList<>(converters))); } /** @@ -152,6 +189,37 @@ public class CustomConversions { converters.forEach(it -> registerConverterIn(it, conversionService)); } + /** + * Get all converters and add origin information + * + * @param storeConversions + * @param converters + * @return + * @since 2.3 + */ + private List collectPotentialConverterRegistrations(StoreConversions storeConversions, + Collection converters) { + + List 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}. * @@ -160,31 +228,27 @@ public class CustomConversions { */ private void registerConverterIn(Object candidate, ConverterRegistry conversionService) { - boolean added = false; - if (candidate instanceof Converter) { conversionService.addConverter(Converter.class.cast(candidate)); - added = true; + return; } if (candidate instanceof ConverterFactory) { conversionService.addConverterFactory(ConverterFactory.class.cast(candidate)); - added = true; + return; } if (candidate instanceof GenericConverter) { conversionService.addConverter(GenericConverter.class.cast(candidate)); - added = true; + return; } if (candidate instanceof ConverterAware) { ConverterAware.class.cast(candidate).getConverters().forEach(it -> registerConverterIn(it, conversionService)); - added = true; + return; } - if (!added) { - throw new IllegalArgumentException(String.format(NOT_A_CONVERTER, candidate)); - } + throw new IllegalArgumentException(String.format(NOT_A_CONVERTER, candidate)); } /** @@ -193,7 +257,7 @@ public class CustomConversions { * * @param converterRegistration */ - private void register(ConverterRegistration converterRegistration) { + private Object register(ConverterRegistration converterRegistration) { Assert.notNull(converterRegistration, "Converter registration must not be null!"); @@ -217,6 +281,50 @@ public class CustomConversions { LOG.warn(String.format(WRITE_CONVERTER_NOT_SIMPLE, pair.getSourceType(), pair.getTargetType())); } } + + return converterRegistration.getConverter(); + } + + /** + * Validate a given {@link ConverterRegistration} in a specific setup.
+ * 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 registrationIntent + * @return {@literal true} if supported. + * @since 2.3 + */ + private boolean isSupportedConverter(ConverterRegistrationIntent registrationIntent) { + + boolean register = registrationIntent.isUserConverter() + || (registrationIntent.isReading() && registrationIntent.isSimpleSourceType()) + || (registrationIntent.isWriting() && registrationIntent.isSimpleTargetType()); + + if (LOG.isDebugEnabled()) { + + if (register) { + LOG.debug(String.format(ADD_CONVERTER, registrationIntent.isUserConverter() ? "user defined " : "", + registrationIntent.getSourceType(), registrationIntent.getTargetType(), + registrationIntent.isReading() ? "reading" : "writing")); + } else { + LOG.debug(String.format(SKIP_CONVERTER, registrationIntent.getSourceType(), registrationIntent.getTargetType(), + registrationIntent.isReading() ? "reading" : "writing", + registrationIntent.isReading() ? registrationIntent.getSourceType() : registrationIntent.getTargetType())); + } + } + + return register; + } + + /** + * @param intent must not be {@literal null}. + * @return {@literal true} if the given {@link ConverterRegistration} shall be skipped. + * @since 2.3 + */ + private boolean skip(ConverterRegistrationIntent intent) { + + return intent.isDefaultConveter() + && converterConfiguration.getSkipFor().contains(intent.getConverterRegistration().getConvertiblePair()); } /** @@ -442,6 +550,80 @@ public class CustomConversions { } } + /** + * 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. * @@ -449,8 +631,9 @@ public class CustomConversions { * @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 +683,13 @@ public class CustomConversions { public boolean isSimpleTargetType() { return storeConversions.isStoreSimpleType(convertiblePair.getTargetType()); } + + /** + * @return + */ + Object getConverter() { + return converter; + } } /** @@ -575,7 +765,7 @@ public class CustomConversions { 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,19 +790,85 @@ public class CustomConversions { 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) { return storeTypeHolder.isSimpleType(type); } } + + /** + * Value object holding the actual {@link StoreConversions} and custom {@link Converter converters} configured for + * registration. + * + * @author Christoph Strobl + * @since 2.3 + */ + protected static class ConverterConfiguration { + + private final StoreConversions storeConversions; + private final List userConverters; + private final Set skipFor; + + /** + * Create a new ConverterConfiguration holding the given {@link StoreConversions} and user defined converters. + * + * @param storeConversions must not be {@literal null}. + * @param userConverters must not be {@literal null} use {@link Collections#emptyList()} instead. + */ + public ConverterConfiguration(StoreConversions storeConversions, List userConverters) { + this(storeConversions, userConverters, Collections.emptySet()); + } + + /** + * Create a new ConverterConfiguration holding the given {@link StoreConversions} and user defined converters as + * well as a {@link Collection} of {@link ConvertiblePair} for which to skip the registration of default converters. + *
+ * This allows store implementations to modify default converter registration based on specific needs and + * configurations. User defined converters will are never subject of filtering. + * + * @param storeConversions must not be {@literal null}. + * @param userConverters must not be {@literal null} use {@link Collections#emptyList()} instead. + * @param skipFor must not be {@literal null} use {@link Collections#emptyList()} instead. + */ + public ConverterConfiguration(StoreConversions storeConversions, List userConverters, + Collection skipFor) { + + this.storeConversions = storeConversions; + this.userConverters = new ArrayList<>(userConverters); + this.skipFor = new HashSet<>(skipFor); + } + + /** + * @return never {@literal null} + */ + StoreConversions getStoreConversions() { + return storeConversions; + } + + /** + * @return never {@literal null}. + */ + List getUserConverters() { + return userConverters; + } + + /** + * @return never {@literal null}. + */ + Set getSkipFor() { + return skipFor; + } + } } diff --git a/src/test/java/org/springframework/data/convert/CustomConversionsUnitTests.java b/src/test/java/org/springframework/data/convert/CustomConversionsUnitTests.java index c9ab4d6e8..d43c15976 100644 --- a/src/test/java/org/springframework/data/convert/CustomConversionsUnitTests.java +++ b/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; @@ -26,18 +27,21 @@ import java.util.Date; import java.util.Locale; import org.joda.time.DateTime; -import org.junit.Test; - +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; +import org.springframework.core.convert.converter.ConverterRegistry; +import org.springframework.core.convert.converter.GenericConverter.ConvertiblePair; 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.ConverterConfiguration; import org.springframework.data.convert.CustomConversions.StoreConversions; +import org.springframework.data.convert.Jsr310Converters.LocalDateTimeToDateConverter; +import org.springframework.data.convert.ThreeTenBackPortConverters.LocalDateTimeToJavaTimeInstantConverter; import org.springframework.data.mapping.model.SimpleTypeHolder; - import org.threeten.bp.LocalDateTime; /** @@ -50,6 +54,15 @@ import org.threeten.bp.LocalDateTime; */ public class CustomConversionsUnitTests { + static final SimpleTypeHolder DATE_EXCLUDING_SIMPLE_TYPE_HOLDER = new SimpleTypeHolder( + Collections.singleton(Date.class), true) { + + @Override + public boolean isSimpleType(Class type) { + return type.getName().startsWith("java.time") ? false : super.isSimpleType(type); + } + }; + @Test // DATACMNS-1035 public void findsBasicReadAndWriteConversions() { @@ -194,6 +207,53 @@ public class CustomConversionsUnitTests { assertThat(conversionService.canConvert(Locale.class, CustomType.class)).isTrue(); } + @Test // DATACMNS-1615 + public void skipsUnsupportedDefaultWritingConverter() { + + ConverterRegistry registry = mock(ConverterRegistry.class); + + new CustomConversions(StoreConversions.of(DATE_EXCLUDING_SIMPLE_TYPE_HOLDER), Collections.emptyList()) + .registerConvertersIn(registry); + + verify(registry, never()).addConverter(any(LocalDateTimeToJavaTimeInstantConverter.class)); + } + + @Test // DATACMNS-1615 + public void doesNotSkipUnsupportedUserConverter() { + + ConverterRegistry registry = mock(ConverterRegistry.class); + + new CustomConversions(StoreConversions.of(DATE_EXCLUDING_SIMPLE_TYPE_HOLDER), + Collections.singletonList(LocalDateTimeToJavaTimeInstantConverter.INSTANCE)).registerConvertersIn(registry); + + verify(registry).addConverter(any(LocalDateTimeToJavaTimeInstantConverter.class)); + } + + @Test // DATACMNS-1615 + public void skipsConverterBasedOnConfiguration() { + + ConverterRegistry registry = mock(ConverterRegistry.class); + + ConverterConfiguration config = new ConverterConfiguration(StoreConversions.NONE, Collections.emptyList(), + Collections.singleton(new ConvertiblePair(java.time.LocalDateTime.class, Date.class))); + new CustomConversions(config).registerConvertersIn(registry); + + verify(registry, never()).addConverter(any(LocalDateTimeToDateConverter.class)); + } + + @Test // DATACMNS-1615 + public void doesNotSkipUserConverterConverterEvenWhenConfigurationWouldNotAllowIt() { + + ConverterRegistry registry = mock(ConverterRegistry.class); + + ConverterConfiguration config = new ConverterConfiguration(StoreConversions.NONE, + Collections.singletonList(LocalDateTimeToDateConverter.INSTANCE), + Collections.singleton(new ConvertiblePair(java.time.LocalDateTime.class, Date.class))); + new CustomConversions(config).registerConvertersIn(registry); + + verify(registry).addConverter(any(LocalDateTimeToDateConverter.class)); + } + private static Class createProxyTypeFor(Class type) { ProxyFactory factory = new ProxyFactory();