From 58d7ff90a5f688713eb81c0cc6db384827860fe7 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 10 Apr 2018 15:32:02 +0200 Subject: [PATCH] DATAREDIS-544 - Read constructor property values through MappingRedisConverter. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now read and convert constructor property values through MappingRedisConverter.readProperty(…) to apply proper conversion. This way, constructor properties are read through the same methods as instance properties allowing to read back nested level entities. Previously, we were only reading simple properties or properties associated with a registered Converter. Original Pull Request: #329 --- .../core/convert/MappingRedisConverter.java | 146 +++++++++--------- .../core/convert/ConversionTestEntities.java | 19 +++ .../MappingRedisConverterUnitTests.java | 44 ++++++ 3 files changed, 132 insertions(+), 77 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/core/convert/MappingRedisConverter.java b/src/main/java/org/springframework/data/redis/core/convert/MappingRedisConverter.java index db931aeb6..3a2bfe716 100644 --- a/src/main/java/org/springframework/data/redis/core/convert/MappingRedisConverter.java +++ b/src/main/java/org/springframework/data/redis/core/convert/MappingRedisConverter.java @@ -21,17 +21,8 @@ import lombok.Getter; import lombok.RequiredArgsConstructor; import java.lang.reflect.Array; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.Map.Entry; -import java.util.Optional; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -232,73 +223,16 @@ public class MappingRedisConverter implements RedisConverter, InitializingBean { entity.doWithProperties((PropertyHandler) persistentProperty -> { - String currentPath = !path.isEmpty() ? path + "." + persistentProperty.getName() : persistentProperty.getName(); - PreferredConstructor constructor = entity.getPersistenceConstructor(); if (constructor.isConstructorParameter(persistentProperty)) { return; } - if (persistentProperty.isMap()) { + Object targetValue = readProperty(path, source, persistentProperty); - Map targetValue = null; - Class mapValueType = persistentProperty.getMapValueType(); - - if (mapValueType == null) { - throw new IllegalArgumentException("Unable to retrieve MapValueType!"); - } - - if (conversionService.canConvert(byte[].class, mapValueType)) { - targetValue = readMapOfSimpleTypes(currentPath, persistentProperty.getType(), - persistentProperty.getComponentType(), mapValueType, source); - } else { - targetValue = readMapOfComplexTypes(currentPath, persistentProperty.getType(), - persistentProperty.getComponentType(), mapValueType, source); - } - - if (targetValue != null) { - accessor.setProperty(persistentProperty, targetValue); - } - } - - else if (persistentProperty.isCollectionLike()) { - - Object targetValue = readCollectionOrArray(currentPath, persistentProperty.getType(), - persistentProperty.getTypeInformation().getRequiredComponentType().getActualType().getType(), - source.getBucket()); - if (targetValue != null) { - accessor.setProperty(persistentProperty, targetValue); - } - - } else if (persistentProperty.isEntity() && !conversionService.canConvert(byte[].class, - persistentProperty.getTypeInformation().getActualType().getType())) { - - Bucket bucket = source.getBucket().extract(currentPath + "."); - - RedisData newBucket = new RedisData(bucket); - TypeInformation typeInformation = typeMapper.readType(bucket.getPropertyPath(currentPath), - persistentProperty.getTypeInformation().getActualType()); - - Class targetType = (Class) typeInformation.getType(); - - R val = readInternal(currentPath, targetType, newBucket); - - accessor.setProperty(persistentProperty, val); - } else { - - if (persistentProperty.isIdProperty() && StringUtils.isEmpty(path.isEmpty())) { - - if (source.getBucket().get(currentPath) == null) { - accessor.setProperty(persistentProperty, - fromBytes(source.getBucket().get(currentPath), persistentProperty.getActualType())); - } else { - accessor.setProperty(persistentProperty, source.getId()); - } - } - - Class typeToUse = getTypeHint(currentPath, source.getBucket(), persistentProperty.getActualType()); - accessor.setProperty(persistentProperty, fromBytes(source.getBucket().get(currentPath), typeToUse)); + if (targetValue != null) { + accessor.setProperty(persistentProperty, targetValue); } }); @@ -307,6 +241,59 @@ public class MappingRedisConverter implements RedisConverter, InitializingBean { return (R) instance; } + @SuppressWarnings("unchecked") + @Nullable + protected Object readProperty(String path, RedisData source, RedisPersistentProperty persistentProperty) { + + String currentPath = !path.isEmpty() ? path + "." + persistentProperty.getName() : persistentProperty.getName(); + + TypeInformation typeInformation = persistentProperty.getTypeInformation(); + + if (persistentProperty.isMap()) { + + Class mapValueType = persistentProperty.getMapValueType(); + + if (mapValueType == null) { + throw new IllegalArgumentException("Unable to retrieve MapValueType!"); + } + + if (conversionService.canConvert(byte[].class, mapValueType)) { + return readMapOfSimpleTypes(currentPath, typeInformation.getType(), + typeInformation.getRequiredComponentType().getType(), mapValueType, source); + } + + return readMapOfComplexTypes(currentPath, typeInformation.getType(), + typeInformation.getRequiredComponentType().getType(), mapValueType, source); + } + + if (typeInformation.isCollectionLike()) { + + return readCollectionOrArray(currentPath, typeInformation.getType(), + typeInformation.getRequiredComponentType().getActualType().getType(), source.getBucket()); + } + + if (persistentProperty.isEntity() + && !conversionService.canConvert(byte[].class, typeInformation.getActualType().getType())) { + + Bucket bucket = source.getBucket().extract(currentPath + "."); + + RedisData newBucket = new RedisData(bucket); + TypeInformation typeToRead = typeMapper.readType(bucket.getPropertyPath(currentPath), + typeInformation.getActualType()); + + return readInternal(currentPath, typeToRead.getType(), newBucket); + } + + byte[] sourceBytes = source.getBucket().get(currentPath); + + if (persistentProperty.isIdProperty() && StringUtils.isEmpty(path.isEmpty())) { + return sourceBytes == null ? fromBytes(sourceBytes, typeInformation.getActualType().getType()) : source.getId(); + } + + Class typeToUse = getTypeHint(currentPath, source.getBucket(), persistentProperty.getActualType()); + return fromBytes(sourceBytes, typeToUse); + } + private void readAssociation(String path, RedisData source, RedisPersistentEntity entity, PersistentPropertyAccessor accessor) { @@ -463,10 +450,9 @@ public class MappingRedisConverter implements RedisConverter, InitializingBean { targetProperty = getTargetPropertyOrNullForPath(path.replaceAll("\\.\\[.*\\]", ""), update.getTarget()); TypeInformation ti = targetProperty == null ? ClassTypeInformation.OBJECT - : (targetProperty.isMap() - ? (targetProperty.getTypeInformation().getMapValueType() != null - ? targetProperty.getTypeInformation().getRequiredMapValueType() : ClassTypeInformation.OBJECT) - : targetProperty.getTypeInformation().getActualType()); + : (targetProperty.isMap() ? (targetProperty.getTypeInformation().getMapValueType() != null + ? targetProperty.getTypeInformation().getRequiredMapValueType() + : ClassTypeInformation.OBJECT) : targetProperty.getTypeInformation().getActualType()); writeInternal(entity.getKeySpace(), pUpdate.getPropertyPath(), pUpdate.getValue(), ti, sink); return; @@ -1037,9 +1023,10 @@ public class MappingRedisConverter implements RedisConverter, InitializingBean { /** * @author Christoph Strobl + * @author Mark Paluch */ @RequiredArgsConstructor - private static class ConverterAwareParameterValueProvider implements PropertyValueProvider { + private class ConverterAwareParameterValueProvider implements PropertyValueProvider { private final String path; private final RedisData source; @@ -1049,8 +1036,13 @@ public class MappingRedisConverter implements RedisConverter, InitializingBean { @SuppressWarnings("unchecked") public T getPropertyValue(RedisPersistentProperty property) { - String name = StringUtils.hasText(path) ? path + "." + property.getName() : property.getName(); - return (T) conversionService.convert(source.getBucket().get(name), property.getActualType()); + Object value = readProperty(path, source, property); + + if (value != null && !property.getActualType().isInstance(value)) { + return (T) conversionService.convert(value, property.getActualType()); + } + + return (T) value; } } diff --git a/src/test/java/org/springframework/data/redis/core/convert/ConversionTestEntities.java b/src/test/java/org/springframework/data/redis/core/convert/ConversionTestEntities.java index a88a7107a..98b9e98b8 100644 --- a/src/test/java/org/springframework/data/redis/core/convert/ConversionTestEntities.java +++ b/src/test/java/org/springframework/data/redis/core/convert/ConversionTestEntities.java @@ -15,6 +15,7 @@ */ package org.springframework.data.redis.core.convert; +import lombok.Data; import lombok.EqualsAndHashCode; import java.time.Duration; @@ -84,6 +85,24 @@ public class ConversionTestEntities { Species species; } + @RedisHash(KEYSPACE_PERSON) + @Data + public static class RecursiveConstructorPerson { + + final @Id String id; + final String firstname; + final RecursiveConstructorPerson father; + String lastname; + } + + @RedisHash(KEYSPACE_PERSON) + @Data + public static class PersonWithConstructorAndAddress { + + final @Id String id; + final Address address; + } + public static class PersonWithAddressReference extends Person { @Reference AddressWithId addressRef; diff --git a/src/test/java/org/springframework/data/redis/core/convert/MappingRedisConverterUnitTests.java b/src/test/java/org/springframework/data/redis/core/convert/MappingRedisConverterUnitTests.java index 697753a97..782bdf351 100644 --- a/src/test/java/org/springframework/data/redis/core/convert/MappingRedisConverterUnitTests.java +++ b/src/test/java/org/springframework/data/redis/core/convert/MappingRedisConverterUnitTests.java @@ -231,6 +231,31 @@ public class MappingRedisConverterUnitTests { assertThat(target.address, instanceOf(AddressWithPostcode.class)); } + @Test // DATAREDIS-544 + public void readEntityViaConstructor() { + + Map map = new HashMap<>(); + map.put("id", "bart"); + map.put("firstname", "Bart"); + map.put("lastname", "Simpson"); + + map.put("father.id", "homer"); + map.put("father.firstname", "Homer"); + map.put("father.lastname", "Simpson"); + + RecursiveConstructorPerson target = converter.read(RecursiveConstructorPerson.class, + new RedisData(Bucket.newBucketFromStringMap(map))); + + assertThat(target.id, is("bart")); + assertThat(target.firstname, is("Bart")); + assertThat(target.lastname, is("Simpson")); + assertThat(target.father, is(notNullValue())); + assertThat(target.father.id, is("homer")); + assertThat(target.father.firstname, is("Homer")); + assertThat(target.father.lastname, is("Simpson")); + assertThat(target.father.father, is(nullValue())); + } + @Test // DATAREDIS-425 public void writeAddsClassTypeInformationCorrectlyForNonMatchingTypesInCollections() { @@ -1093,6 +1118,25 @@ public class MappingRedisConverterUnitTests { assertThat(target.address.country, is("Tel'aran'rhiod")); } + @Test // DATAREDIS-544 + public void readShouldHonorCustomConversionOnNestedTypeViaConstructorCreation() { + + this.converter = new MappingRedisConverter(new RedisMappingContext(), null, resolverMock); + this.converter + .setCustomConversions(new RedisCustomConversions(Collections.singletonList(new BytesToAddressConverter()))); + this.converter.afterPropertiesSet(); + + Map map = new LinkedHashMap<>(); + map.put("address", "{\"city\":\"unknown\",\"country\":\"Tel'aran'rhiod\"}"); + + PersonWithConstructorAndAddress target = converter.read(PersonWithConstructorAndAddress.class, + new RedisData(Bucket.newBucketFromStringMap(map))); + + assertThat(target.address, notNullValue()); + assertThat(target.address.city, is("unknown")); + assertThat(target.address.country, is("Tel'aran'rhiod")); + } + @Test // DATAREDIS-425 public void writeShouldPickUpTimeToLiveFromPropertyIfPresent() {