From 7eb313e3b16299abe030a1d19b534460a17c0315 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 4 Dec 2024 10:21:12 +0100 Subject: [PATCH] Guard potential NPE in TupleConverter. Also fix a broken reference in javadoc. Original Pull Request: #3654 --- .../repository/query/AbstractJpaQuery.java | 53 +++++++++++++++++-- .../jpa/repository/query/QueryEnhancer.java | 2 +- .../query/TupleConverterUnitTests.java | 25 +++++++++ .../ROOT/pages/repositories/projections.adoc | 2 +- 4 files changed, 75 insertions(+), 7 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java index 4914beb4d7..ec90ced24b 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java @@ -23,6 +23,7 @@ import jakarta.persistence.TupleElement; import jakarta.persistence.TypedQuery; +import java.lang.reflect.Constructor; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; @@ -54,6 +55,7 @@ import org.springframework.jdbc.support.JdbcUtils; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; /** @@ -345,7 +347,7 @@ public TupleConverter(ReturnedType type, boolean nativeQuery) { && !type.getInputProperties().isEmpty(); if (this.dtoProjection) { - this.preferredConstructor = PreferredConstructorDiscoverer.discover(String.class); + this.preferredConstructor = PreferredConstructorDiscoverer.discover(type.getReturnedType()); } else { this.preferredConstructor = null; } @@ -373,18 +375,35 @@ public Object convert(Object source) { Object[] ctorArgs = new Object[type.getInputProperties().size()]; + boolean containsNullValue = false; for (int i = 0; i < type.getInputProperties().size(); i++) { - ctorArgs[i] = tuple.get(i); + Object value = tuple.get(i); + ctorArgs[i] = value; + if (!containsNullValue && value == null) { + containsNullValue = true; + } } try { - if (preferredConstructor.getParameterCount() == ctorArgs.length) { + if (preferredConstructor != null && preferredConstructor.getParameterCount() == ctorArgs.length) { return BeanUtils.instantiateClass(preferredConstructor.getConstructor(), ctorArgs); } - return BeanUtils.instantiateClass(type.getReturnedType() - .getConstructor(Arrays.stream(ctorArgs).map(Object::getClass).toArray(Class[]::new)), ctorArgs); + Constructor ctor = null; + + if (!containsNullValue) { // let's seem if we have an argument type match + ctor = type.getReturnedType() + .getConstructor(Arrays.stream(ctorArgs).map(Object::getClass).toArray(Class[]::new)); + } + + if (ctor == null) { // let's see if there's more general constructor we could use that accepts our args + ctor = findFirstMatchingConstructor(ctorArgs); + } + + if (ctor != null) { + return BeanUtils.instantiateClass(ctor, ctorArgs); + } } catch (ReflectiveOperationException e) { ReflectionUtils.handleReflectionException(e); } @@ -393,6 +412,30 @@ public Object convert(Object source) { return new TupleBackedMap(tupleWrapper.apply(tuple)); } + @Nullable + private Constructor findFirstMatchingConstructor(Object[] ctorArgs) { + + for (Constructor ctor : type.getReturnedType().getDeclaredConstructors()) { + + if (ctor.getParameterCount() == ctorArgs.length) { + boolean itsAMatch = true; + for (int i = 0; i < ctor.getParameterCount(); i++) { + if (ctorArgs[i] == null) { + continue; + } + if (!ClassUtils.isAssignable(ctor.getParameterTypes()[i], ctorArgs[i].getClass())) { + itsAMatch = false; + break; + } + } + if (itsAMatch) { + return ctor; + } + } + } + return null; + } + /** * A {@link Map} implementation which delegates all calls to a {@link Tuple}. Depending on the provided * {@link Tuple} implementation it might return the same value for various keys of which only one will appear in the diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancer.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancer.java index 0995dd8700..3697c22980 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancer.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryEnhancer.java @@ -82,7 +82,7 @@ public interface QueryEnhancer { * @param sort the sort specification to apply. * @param alias the alias to be used in the order by clause. May be {@literal null} or empty. * @return the modified query string. - * @deprecated since 3.5, use {@link #rewrite(Sort, ReturnedType)} instead. + * @deprecated since 3.5, use {@link #rewrite(QueryRewriteInformation)} instead. */ @Deprecated(since = "3.5", forRemoval = true) String applySorting(Sort sort, @Nullable String alias); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/TupleConverterUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/TupleConverterUnitTests.java index 13dc550fb0..e16ebaafe2 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/TupleConverterUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/TupleConverterUnitTests.java @@ -37,6 +37,7 @@ import org.springframework.data.jpa.repository.query.AbstractJpaQuery.TupleConverter; import org.springframework.data.projection.ProjectionFactory; +import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.data.repository.CrudRepository; import org.springframework.data.repository.core.RepositoryMetadata; import org.springframework.data.repository.core.support.DefaultRepositoryMetadata; @@ -113,6 +114,16 @@ void findsValuesForAllVariantsSupportedByTheTuple() { softly.assertAll(); } + @Test // GH-3076 + void dealsWithNullsInArgumetents() { + + ReturnedType returnedType = ReturnedType.of(WithPC.class, DomainType.class, new SpelAwareProxyProjectionFactory()); + + when(tuple.get(eq(0))).thenReturn("one"); + when(tuple.get(eq(1))).thenReturn(null); + new TupleConverter(returnedType).convert(tuple); + } + interface SampleRepository extends CrudRepository { String someMethod(); } @@ -177,4 +188,18 @@ public String getAlias() { } } } + + static class DomainType { + String one, two, three; + } + + static class WithPC { + String one; + String two; + + public WithPC(String one, String two) { + this.one = one; + this.two = two; + } + } } diff --git a/src/main/antora/modules/ROOT/pages/repositories/projections.adoc b/src/main/antora/modules/ROOT/pages/repositories/projections.adoc index 95366d30aa..c5e113c8f4 100644 --- a/src/main/antora/modules/ROOT/pages/repositories/projections.adoc +++ b/src/main/antora/modules/ROOT/pages/repositories/projections.adoc @@ -68,7 +68,7 @@ This query gets rewritten to `SELECT new UserDto(u.firstname, u.lastname) FROM U [WARNING] ==== -JPQL constructor expressions must not contain aliases for selected columns. +JPQL constructor expressions must not contain aliases for selected columns and query rewriting will not remove them for you. While `SELECT u as user, count(u.roles) as roleCount FROM USER u …` is a valid query for interface-based projections that rely on column names from the returned `Tuple`, the same construct is invalid when requesting a DTO where it needs to be `SELECT u, count(u.roles) FROM USER u …`. + Some persistence providers may be lenient about this, others not. ====