From 08710d8e16fd22999fa0dcf711e8ed4c7f6bf6e4 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Mon, 29 Apr 2024 16:57:08 +0200 Subject: [PATCH] Reuse custom converters from `AggregateReferenceConverters`. Let the AggregateReference converters now receive also custom converters as delegates. Remove the ArrayToObjectConverter which just uses the first element of the array from the DefaultConversion service. This does NOT solve the underlying problem, of having two DefaultConversionServices at work. One is in the store conversion, one constructed in the AbstractRelationalConverter. Although it looks like they get merged, the former contains converters, using that ConversionService as a delegate. Attempts were made to move AggregateReferenceConverters out of the store converters and just register them directly, but then no custom read/write targets are detected, leading to failed conversions. The same problem prevents a registration in CustomConversions as a Function or similar, which would allow late registration. The problem here is that custom read/write targets require the function to get evaluated, before custom read/write targets can be determined. Closes #1750 Original pull request: #1785 --- .../core/convert/JdbcCustomConversions.java | 37 +++++++--- .../MappingJdbcConverterUnitTests.java | 71 +++++++++++++++++-- .../AbstractRelationalConverter.java | 13 +++- .../MappingRelationalConverter.java | 8 --- 4 files changed, 105 insertions(+), 24 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcCustomConversions.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcCustomConversions.java index 52ddb460..f6d8a768 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcCustomConversions.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/JdbcCustomConversions.java @@ -20,7 +20,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; -import org.springframework.core.convert.ConversionService; +import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.GenericConverter.ConvertiblePair; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.convert.CustomConversions; @@ -52,9 +52,6 @@ public class JdbcCustomConversions extends CustomConversions { } - private static final StoreConversions STORE_CONVERSIONS = StoreConversions.of(JdbcSimpleTypes.HOLDER, - STORE_CONVERTERS); - /** * Creates an empty {@link JdbcCustomConversions} object. */ @@ -70,11 +67,7 @@ public class JdbcCustomConversions extends CustomConversions { */ public JdbcCustomConversions(List converters) { - super(new ConverterConfiguration( // - STORE_CONVERSIONS, // - converters, // - JdbcCustomConversions::excludeConversionsBetweenDateAndJsr310Types // - )); + super(constructConverterConfiguration(converters)); } /** @@ -103,6 +96,32 @@ public class JdbcCustomConversions extends CustomConversions { super(converterConfiguration); } + private static ConverterConfiguration constructConverterConfiguration(List converters) { + + StoreConversions storeConversions = storeConversions(converters); + + return new ConverterConfiguration( // + storeConversions, // + converters, // + JdbcCustomConversions::excludeConversionsBetweenDateAndJsr310Types // + ); + } + + private static StoreConversions storeConversions(List userConverters) { + + List converters = new ArrayList<>(Jsr310TimestampBasedConverters.getConvertersToRegister()); + + DefaultConversionService defaultConversionService = new DefaultConversionService(); + for (Object userConverter : userConverters) { + if (userConverter instanceof Converter converter) + defaultConversionService.addConverter(converter); + } + + converters.addAll(AggregateReferenceConverters.getConvertersToRegister(defaultConversionService)); + + return StoreConversions.of(JdbcSimpleTypes.HOLDER, Collections.unmodifiableCollection(converters)); + } + /** * Obtain a read only copy of default store converters. * diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverterUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverterUnitTests.java index c56f6d41..186a1e09 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverterUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverterUnitTests.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.*; import static org.assertj.core.api.SoftAssertions.*; import static org.mockito.Mockito.*; +import java.nio.ByteBuffer; import java.sql.Array; import java.sql.Timestamp; import java.time.Instant; @@ -28,13 +29,16 @@ import java.time.LocalTime; import java.time.OffsetDateTime; import java.time.ZoneOffset; import java.time.ZonedDateTime; +import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.UUID; import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.Test; +import org.springframework.core.convert.converter.Converter; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.core.mapping.AggregateReference; import org.springframework.data.jdbc.core.mapping.JdbcMappingContext; @@ -50,9 +54,14 @@ import org.springframework.data.util.TypeInformation; * Unit tests for {@link MappingJdbcConverter}. * * @author Mark Paluch + * @author Jens Schauder */ public class MappingJdbcConverterUnitTests { + public static final UUID UUID = java.util.UUID.fromString("87a48aa8-a071-705e-54a9-e52fe3a012f1"); + public static final byte[] BYTES_REPRESENTING_UUID = { -121, -92, -118, -88, -96, 113, 112, 94, 84, -87, -27, 47, -29, + -96, 18, -15 }; + JdbcMappingContext context = new JdbcMappingContext(); StubbedJdbcTypeFactory typeFactory = new StubbedJdbcTypeFactory(); MappingJdbcConverter converter = new MappingJdbcConverter( // @@ -61,7 +70,7 @@ public class MappingJdbcConverterUnitTests { throw new UnsupportedOperationException(); }, // new JdbcCustomConversions(), // - typeFactory // + typeFactory // ); @Test // DATAJDBC-104, DATAJDBC-1384 @@ -152,6 +161,39 @@ public class MappingJdbcConverterUnitTests { assertThat(result).isEqualTo(new WithOneToOne("one", new Referenced(23L))); } + @Test // GH-1750 + void readByteArrayToNestedUuidWithCustomConverter() { + + JdbcMappingContext context = new JdbcMappingContext(); + StubbedJdbcTypeFactory typeFactory = new StubbedJdbcTypeFactory(); + Converter customConverter = new ByteArrayToUuid(); + MappingJdbcConverter converter = new MappingJdbcConverter( // + context, // + (identifier, path) -> { + throw new UnsupportedOperationException(); + }, // + new JdbcCustomConversions(Collections.singletonList(customConverter)), // + typeFactory // + ); + + SoftAssertions.assertSoftly(softly -> { + checkReadConversion(softly, converter, "uuidRef", AggregateReference.to(UUID)); + checkReadConversion(softly, converter, "uuid", UUID); + checkReadConversion(softly, converter, "optionalUuid", Optional.of(UUID)); + }); + + } + + private static void checkReadConversion(SoftAssertions softly, MappingJdbcConverter converter, String propertyName, + Object expected) { + + RelationalPersistentProperty property = converter.getMappingContext().getRequiredPersistentEntity(DummyEntity.class) + .getRequiredPersistentProperty(propertyName); + Object value = converter.readValue(BYTES_REPRESENTING_UUID, property.getTypeInformation() // + ); + + softly.assertThat(value).isEqualTo(expected); + } private void checkConversionToTimestampAndBack(SoftAssertions softly, RelationalPersistentEntity persistentEntity, String propertyName, Object value) { @@ -187,6 +229,8 @@ public class MappingJdbcConverterUnitTests { private final Timestamp timestamp; private final AggregateReference reference; private final UUID uuid; + private final AggregateReference uuidRef; + private final Optional optionalUuid; // DATAJDBC-259 private final List listOfString; @@ -195,9 +239,10 @@ public class MappingJdbcConverterUnitTests { private final OtherEntity[] arrayOfEntity; private DummyEntity(Long id, SomeEnum someEnum, LocalDateTime localDateTime, LocalDate localDate, - LocalTime localTime, ZonedDateTime zonedDateTime, OffsetDateTime offsetDateTime, Instant instant, Date date, - Timestamp timestamp, AggregateReference reference, UUID uuid, List listOfString, - String[] arrayOfString, List listOfEntity, OtherEntity[] arrayOfEntity) { + LocalTime localTime, ZonedDateTime zonedDateTime, OffsetDateTime offsetDateTime, Instant instant, Date date, + Timestamp timestamp, AggregateReference reference, UUID uuid, + AggregateReference uuidRef, Optional optionalUUID, List listOfString, String[] arrayOfString, + List listOfEntity, OtherEntity[] arrayOfEntity) { this.id = id; this.someEnum = someEnum; this.localDateTime = localDateTime; @@ -210,6 +255,8 @@ public class MappingJdbcConverterUnitTests { this.timestamp = timestamp; this.reference = reference; this.uuid = uuid; + this.uuidRef = uuidRef; + this.optionalUuid = optionalUUID; this.listOfString = listOfString; this.arrayOfString = arrayOfString; this.listOfEntity = listOfEntity; @@ -299,9 +346,23 @@ public class MappingJdbcConverterUnitTests { } } - record WithOneToOne(@Id String id,@MappedCollection(idColumn = "renamed") Referenced referenced){} + record WithOneToOne(@Id String id, @MappedCollection(idColumn = "renamed") Referenced referenced) { + } record Referenced(@Id Long id) { } + record ReferencedByUuid(@Id UUID id) { + } + + class ByteArrayToUuid implements Converter { + @Override + public UUID convert(byte[] source) { + + ByteBuffer byteBuffer = ByteBuffer.wrap(source); + long high = byteBuffer.getLong(); + long low = byteBuffer.getLong(); + return new UUID(high, low); + } + } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/AbstractRelationalConverter.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/AbstractRelationalConverter.java index a40dfb22..f9fe08a9 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/AbstractRelationalConverter.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/AbstractRelationalConverter.java @@ -17,6 +17,7 @@ package org.springframework.data.relational.core.conversion; import java.util.Collections; +import org.jetbrains.annotations.NotNull; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.ConfigurableConversionService; import org.springframework.core.convert.support.DefaultConversionService; @@ -47,7 +48,7 @@ public abstract class AbstractRelationalConverter implements RelationalConverter * @param context must not be {@literal null}. */ public AbstractRelationalConverter(RelationalMappingContext context) { - this(context, new CustomConversions(StoreConversions.NONE, Collections.emptyList()), new DefaultConversionService(), + this(context, new CustomConversions(StoreConversions.NONE, Collections.emptyList()), createBaseConversionService(), new EntityInstantiators()); } @@ -58,7 +59,7 @@ public abstract class AbstractRelationalConverter implements RelationalConverter * @param conversions must not be {@literal null}. */ public AbstractRelationalConverter(RelationalMappingContext context, CustomConversions conversions) { - this(context, conversions, new DefaultConversionService(), new EntityInstantiators()); + this(context, conversions, createBaseConversionService(), new EntityInstantiators()); } private AbstractRelationalConverter(RelationalMappingContext context, CustomConversions conversions, @@ -75,6 +76,14 @@ public abstract class AbstractRelationalConverter implements RelationalConverter conversions.registerConvertersIn(this.conversionService); } + @NotNull + private static DefaultConversionService createBaseConversionService() { + + DefaultConversionService conversionService = new DefaultConversionService(); + conversionService.removeConvertible(Object[].class, Object.class); + return conversionService; + } + @Override public ConversionService getConversionService() { return conversionService; diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java index b3fbfc44..c10648d5 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java @@ -624,14 +624,6 @@ public class MappingRelationalConverter extends AbstractRelationalConverter return null; } - if (getConversions().hasCustomReadTarget(value.getClass(), type.getType())) { - - TypeDescriptor sourceDescriptor = TypeDescriptor.valueOf(value.getClass()); - TypeDescriptor targetDescriptor = createTypeDescriptor(type); - - return getConversionService().convert(value, sourceDescriptor, targetDescriptor); - } - return getPotentiallyConvertedSimpleRead(value, type); }