From 13d9d5ed66256f4087647d6c5137cedec3caad1a Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 4 Feb 2021 13:26:22 +0100 Subject: [PATCH 1/4] Use contextual information when creating PersistentPropertyPaths. In oder to preserve contextual information the PersistentPropertyPathFactory now obtains EntityInformation for properties from the MappingContext via their PersistentProperty representation instead of plain the TypeInformation as the former contains more information about the actual type and signatures. --- .../context/PersistentPropertyPathFactory.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/context/PersistentPropertyPathFactory.java b/src/main/java/org/springframework/data/mapping/context/PersistentPropertyPathFactory.java index bdf9940f56..6d1f76aa41 100644 --- a/src/main/java/org/springframework/data/mapping/context/PersistentPropertyPathFactory.java +++ b/src/main/java/org/springframework/data/mapping/context/PersistentPropertyPathFactory.java @@ -222,8 +222,7 @@ private Pair, E> getPair(DefaultPersistentPrope return null; } - TypeInformation type = property.getTypeInformation().getRequiredActualType(); - return Pair.of(path.append(property), iterator.hasNext() ? context.getRequiredPersistentEntity(type) : entity); + return Pair.of(path.append(property), iterator.hasNext() ? context.getRequiredPersistentEntity(property) : entity); } private Collection> from(TypeInformation type, Predicate filter, @@ -236,6 +235,12 @@ private Collection> from(TypeInformation type, } E entity = context.getRequiredPersistentEntity(actualType); + return from(entity, filter, traversalGuard, basePath); + } + + private Collection> from(E entity, Predicate filter, Predicate

traversalGuard, + DefaultPersistentPropertyPath

basePath) { + Set> properties = new HashSet<>(); PropertyHandler

propertyTester = persistentProperty -> { @@ -254,7 +259,7 @@ private Collection> from(TypeInformation type, } if (traversalGuard.and(IS_ENTITY).test(persistentProperty)) { - properties.addAll(from(typeInformation, filter, traversalGuard, currentPath)); + properties.addAll(from(context.getPersistentEntity(persistentProperty), filter, traversalGuard, currentPath)); } }; From d8cfd5dcdbcff5fb09f23795f63335a38608960c Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 2 Feb 2021 16:36:11 +0100 Subject: [PATCH 2/4] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 9be85dfb76..b192ee23a0 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.5.0-SNAPSHOT + 2.5.0-GH-2290-SNAPSHOT Spring Data Core From 99d540e889860fe48015ec359f71451990d8c5b7 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 2 Feb 2021 16:51:40 +0100 Subject: [PATCH 3/4] Throw UnsupportedOperationException when a projected value cannot be returned. We now throw UnsupportedOperationException when a projected value cannot be returned because it cannot be brought into the target type, either via conversion or projection. This exception improves the error message by avoiding throwing IllegalArgumentException: Projection type must be an interface from the last branch that falls back into projections. Closes #2290. --- .../ProjectingMethodInterceptor.java | 10 ++++++++-- .../ProjectingMethodInterceptorUnitTests.java | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java b/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java index ae75bf3381..9683d7b583 100644 --- a/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java +++ b/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java @@ -104,8 +104,14 @@ protected Object potentiallyConvertResult(TypeInformation type, @Nullable Obj return projectMapValues((Map) result, type); } else if (conversionRequiredAndPossible(result, type.getType())) { return conversionService.convert(result, type.getType()); - } else { + } else if (ClassUtils.isAssignable(type.getType(), result.getClass())) { + return result; + } else if (type.getType().isInterface()) { return getProjection(result, type.getType()); + } else { + throw new UnsupportedOperationException(String.format( + "Cannot convert value '%s' of type '%s' to '%s' and cannot create a projection as the target type is not an interface", + result, ClassUtils.getDescriptiveType(result), ClassUtils.getQualifiedName(type.getType()))); } } @@ -155,7 +161,7 @@ private Map projectMapValues(Map sources, TypeInformation< } @Nullable - private Object getProjection(Object result, Class returnType) { + private Object getProjection(@Nullable Object result, Class returnType) { return result == null || ClassUtils.isAssignable(returnType, result.getClass()) ? result : factory.createProjection(returnType, result); } diff --git a/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java b/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java index f1943ef459..982000aba1 100755 --- a/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java +++ b/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java @@ -19,6 +19,7 @@ import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.*; +import java.math.BigInteger; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; @@ -95,6 +96,21 @@ void considersPrimitivesAsWrappers() throws Throwable { verify(factory, times(0)).createProjection((Class) any(), any()); } + @Test // #2290 + void failsForNonConvertibleTypes() throws Throwable { + + MethodInterceptor methodInterceptor = new ProjectingMethodInterceptor(factory, interceptor, conversionService); + + when(invocation.getMethod()).thenReturn(Helper.class.getMethod("getBoolean")); + when(interceptor.invoke(invocation)).thenReturn(BigInteger.valueOf(1)); + + assertThatThrownBy(() -> methodInterceptor.invoke(invocation)) // + .isInstanceOf(UnsupportedOperationException.class) // + .hasMessageContaining("'1'") // + .hasMessageContaining("BigInteger") // + .hasMessageContaining("boolean"); + } + @Test // DATAREST-394, DATAREST-408 @SuppressWarnings("unchecked") void appliesProjectionToNonEmptySets() throws Throwable { @@ -213,6 +229,8 @@ interface Helper { long getPrimitive(); + boolean getBoolean(); + Collection getHelperCollection(); List getHelperList(); From 29bb8d04337bfe7b2dc3febd6cc02850261ebef8 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 9 Feb 2021 10:28:23 +0100 Subject: [PATCH 4/4] Avoid leaking data via Exception message. Do not include the result value in the exception message to avoid data exposure. improve method order to avoid superfluous null checks. --- .../ProjectingMethodInterceptor.java | 39 ++++++------------- .../ProjectingMethodInterceptorUnitTests.java | 4 +- 2 files changed, 14 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java b/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java index 9683d7b583..322948c4d9 100644 --- a/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java +++ b/src/main/java/org/springframework/data/projection/ProjectingMethodInterceptor.java @@ -27,7 +27,6 @@ import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; - import org.springframework.core.CollectionFactory; import org.springframework.core.convert.ConversionService; import org.springframework.data.util.ClassTypeInformation; @@ -45,6 +44,7 @@ * * @author Oliver Gierke * @author Mark Paluch + * @author Christoph Strobl * @since 1.10 */ class ProjectingMethodInterceptor implements MethodInterceptor { @@ -98,20 +98,22 @@ protected Object potentiallyConvertResult(TypeInformation type, @Nullable Obj return null; } - if (type.isCollectionLike() && !ClassUtils.isPrimitiveArray(type.getType())) { + Class targetType = type.getType(); + + if (type.isCollectionLike() && !ClassUtils.isPrimitiveArray(targetType)) { return projectCollectionElements(asCollection(result), type); } else if (type.isMap()) { return projectMapValues((Map) result, type); - } else if (conversionRequiredAndPossible(result, type.getType())) { - return conversionService.convert(result, type.getType()); - } else if (ClassUtils.isAssignable(type.getType(), result.getClass())) { + } else if (ClassUtils.isAssignable(targetType, result.getClass())) { return result; - } else if (type.getType().isInterface()) { - return getProjection(result, type.getType()); + } else if (conversionService.canConvert(result.getClass(), targetType)) { + return conversionService.convert(result, targetType); + } else if (targetType.isInterface()) { + return getProjection(result, targetType); } else { - throw new UnsupportedOperationException(String.format( - "Cannot convert value '%s' of type '%s' to '%s' and cannot create a projection as the target type is not an interface", - result, ClassUtils.getDescriptiveType(result), ClassUtils.getQualifiedName(type.getType()))); + throw new UnsupportedOperationException( + String.format("Cannot project %s to %s. Target type is not an interface and no matching Converter found!", + ClassUtils.getDescriptiveType(result), ClassUtils.getQualifiedName(targetType))); } } @@ -166,23 +168,6 @@ private Object getProjection(@Nullable Object result, Class returnType) { : factory.createProjection(returnType, result); } - /** - * Returns whether the source object needs to be converted to the given target type and whether we can convert it at - * all. - * - * @param source can be {@literal null}. - * @param targetType must not be {@literal null}. - * @return - */ - private boolean conversionRequiredAndPossible(Object source, Class targetType) { - - if (source == null || targetType.isInstance(source)) { - return false; - } - - return conversionService.canConvert(source.getClass(), targetType); - } - /** * Turns the given value into a {@link Collection}. Will turn an array into a collection an wrap all other values into * a single-element collection. diff --git a/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java b/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java index 982000aba1..5051f70878 100755 --- a/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java +++ b/src/test/java/org/springframework/data/projection/ProjectingMethodInterceptorUnitTests.java @@ -43,6 +43,7 @@ * @author Oliver Gierke * @author Saulo Medeiros de Araujo * @author Mark Paluch + * @author Christoph Strobl */ @ExtendWith(MockitoExtension.class) class ProjectingMethodInterceptorUnitTests { @@ -96,7 +97,7 @@ void considersPrimitivesAsWrappers() throws Throwable { verify(factory, times(0)).createProjection((Class) any(), any()); } - @Test // #2290 + @Test // GH-2290 void failsForNonConvertibleTypes() throws Throwable { MethodInterceptor methodInterceptor = new ProjectingMethodInterceptor(factory, interceptor, conversionService); @@ -106,7 +107,6 @@ void failsForNonConvertibleTypes() throws Throwable { assertThatThrownBy(() -> methodInterceptor.invoke(invocation)) // .isInstanceOf(UnsupportedOperationException.class) // - .hasMessageContaining("'1'") // .hasMessageContaining("BigInteger") // .hasMessageContaining("boolean"); }