From 16e7e7c79b5dbfbbcfdecb093b74670ce7f7bc38 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 31 Jan 2023 18:04:12 +0100 Subject: [PATCH] Query method parameters are now aware of aggregate reference type. To support the binding of Class parameters to queries for declared query methods, we have to be able to differentiate them from Class parameters that are supposed to represent projections. We can do that by relating the declared Class' element type to the aggregate root type as a Class typed to that or any subtype of it will never trigger a projection by definition. So far the Parameter(s) abstraction was solely created from a query method's Method. We now changed that for QueryMethod to forward the aggregate type detected on the RepositoryMetadata and consider it during the detection of dynamic projection parameters. As a mitigating measure, we now also support @Param on Class-typed parameters to explicitly mark them for query binding. This is primarily to be able to add this support to the 2.7 The changes are built in a way that modules extending that mechanism will continue to work as is but see deprecation warnings on methods and constructors involved. Adapting extending code to the new APIs will automatically enable the support for bindable Class parameters on query methods. Fixes #2770. Original pull request: #2771 --- .../repository/query/DefaultParameters.java | 20 ++++++--- .../data/repository/query/Parameter.java | 35 ++++++++++++--- .../data/repository/query/Parameters.java | 41 +++++++++++++---- .../data/repository/query/QueryMethod.java | 21 +++++++-- .../repository/query/ParameterUnitTests.java | 44 ++++++++++++++++++- 5 files changed, 135 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/springframework/data/repository/query/DefaultParameters.java b/src/main/java/org/springframework/data/repository/query/DefaultParameters.java index e67c8927e..1cadc973d 100644 --- a/src/main/java/org/springframework/data/repository/query/DefaultParameters.java +++ b/src/main/java/org/springframework/data/repository/query/DefaultParameters.java @@ -18,7 +18,7 @@ package org.springframework.data.repository.query; import java.lang.reflect.Method; import java.util.List; -import org.springframework.core.MethodParameter; +import org.springframework.data.util.TypeInformation; /** * Default implementation of {@link Parameters}. @@ -31,18 +31,26 @@ public final class DefaultParameters extends Parameters parameters) { - super(parameters); + /** + * Creates a new {@link DefaultParameters} instance from the given {@link Method} and aggregate + * {@link TypeInformation}. + * + * @param method must not be {@literal null}. + * @param aggregateType must not be {@literal null}. + */ + public DefaultParameters(Method method, TypeInformation aggregateType) { + super(method, param -> new Parameter(param, aggregateType)); } - @Override - protected Parameter createParameter(MethodParameter parameter) { - return new Parameter(parameter); + private DefaultParameters(List parameters) { + super(parameters); } @Override diff --git a/src/main/java/org/springframework/data/repository/query/Parameter.java b/src/main/java/org/springframework/data/repository/query/Parameter.java index 2bcfef10c..d1f792f9f 100644 --- a/src/main/java/org/springframework/data/repository/query/Parameter.java +++ b/src/main/java/org/springframework/data/repository/query/Parameter.java @@ -17,7 +17,6 @@ package org.springframework.data.repository.query; import static java.lang.String.*; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -72,14 +71,27 @@ public class Parameter { * Creates a new {@link Parameter} for the given {@link MethodParameter}. * * @param parameter must not be {@literal null}. + * @deprecated since 3.1, use {@link #Parameter(MethodParameter, TypeInformation)} instead. */ + @Deprecated(since = "3.1", forRemoval = true) protected Parameter(MethodParameter parameter) { + this(parameter, TypeInformation.of(Parameter.class)); + } + + /** + * Creates a new {@link Parameter} for the given {@link MethodParameter} and aggregate {@link TypeInformation}. + * + * @param parameter must not be {@literal null}. + * @param aggregateType must not be {@literal null}. + */ + protected Parameter(MethodParameter parameter, TypeInformation aggregateType) { Assert.notNull(parameter, "MethodParameter must not be null"); + Assert.notNull(aggregateType, "TypeInformation must not be null!"); this.parameter = parameter; this.parameterType = potentiallyUnwrapParameterType(parameter); - this.isDynamicProjectionParameter = isDynamicProjectionParameter(parameter); + this.isDynamicProjectionParameter = isDynamicProjectionParameter(parameter, aggregateType); this.name = isSpecialParameterType(parameter.getParameterType()) ? Lazy.of(Optional.empty()) : Lazy.of(() -> { Param annotation = parameter.getParameterAnnotation(Param.class); return Optional.ofNullable(annotation == null ? parameter.getParameterName() : annotation.value()); @@ -206,23 +218,32 @@ public class Parameter { * * * @param parameter must not be {@literal null}. + * @param aggregateType the reference aggregate type, must not be {@literal null}. * @return */ - private static boolean isDynamicProjectionParameter(MethodParameter parameter) { + private static boolean isDynamicProjectionParameter(MethodParameter parameter, TypeInformation aggregateType) { if (!parameter.getParameterType().equals(Class.class)) { return false; } - Method method = parameter.getMethod(); + if (parameter.hasParameterAnnotation(Param.class)) { + return false; + } + + var method = parameter.getMethod(); if (method == null) { throw new IllegalArgumentException("Parameter is not associated with any method"); } - TypeInformation returnType = TypeInformation.fromReturnTypeOf(method); - TypeInformation unwrapped = QueryExecutionConverters.unwrapWrapperTypes(returnType); - TypeInformation reactiveUnwrapped = ReactiveWrapperConverters.unwrapWrapperTypes(unwrapped); + var returnType = TypeInformation.fromReturnTypeOf(method); + var unwrapped = QueryExecutionConverters.unwrapWrapperTypes(returnType); + var reactiveUnwrapped = ReactiveWrapperConverters.unwrapWrapperTypes(unwrapped); + + if (aggregateType.isAssignableFrom(reactiveUnwrapped)) { + return false; + } return reactiveUnwrapped.equals(TypeInformation.fromMethodParameter(parameter).getComponentType()); } diff --git a/src/main/java/org/springframework/data/repository/query/Parameters.java b/src/main/java/org/springframework/data/repository/query/Parameters.java index b5bb5ed04..7dfdf74c0 100644 --- a/src/main/java/org/springframework/data/repository/query/Parameters.java +++ b/src/main/java/org/springframework/data/repository/query/Parameters.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.function.Function; import org.springframework.core.DefaultParameterNameDiscoverer; import org.springframework.core.MethodParameter; @@ -62,11 +63,28 @@ public abstract class Parameters, T extends Parameter * Creates a new instance of {@link Parameters}. * * @param method must not be {@literal null}. + * @deprecated since 3.1, use {@link #Parameters(Method, Function)} instead. */ + @SuppressWarnings("null") + @Deprecated(since = "3.1", forRemoval = true) public Parameters(Method method) { + this(method, null); + } + + /** + * Creates a new {@link Parameters} instance for the given {@link Method} and {@link Function} to create a + * {@link Parameter} instance from a {@link MethodParameter}. + * + * @param method must not be {@literal null}. + * @param parameterFactory must not be {@literal null}. + */ + protected Parameters(Method method, Function parameterFactory) { Assert.notNull(method, "Method must not be null"); + // Factory nullability not enforced yet to support falling back to the deprecated + // createParameter(MethodParameter). Add assertion when the deprecation is removed. + int parameterCount = method.getParameterCount(); this.parameters = new ArrayList<>(parameterCount); @@ -80,7 +98,9 @@ public abstract class Parameters, T extends Parameter MethodParameter methodParameter = new MethodParameter(method, i); methodParameter.initParameterNameDiscovery(PARAMETER_NAME_DISCOVERER); - T parameter = createParameter(methodParameter); + T parameter = parameterFactory == null // + ? createParameter(methodParameter) // + : parameterFactory.apply(methodParameter); if (parameter.isSpecialParameter() && parameter.isNamedParameter()) { throw new IllegalArgumentException(PARAM_ON_SPECIAL); @@ -156,8 +176,13 @@ public abstract class Parameters, T extends Parameter * * @param parameter will never be {@literal null}. * @return + * @deprecated since 3.1, in your extension, call {@link #Parameters(Method, ParameterFactory)} instead. */ - protected abstract T createParameter(MethodParameter parameter); + @SuppressWarnings("unchecked") + @Deprecated(since = "3.1", forRemoval = true) + protected T createParameter(MethodParameter parameter) { + return (T) new Parameter(parameter); + } /** * Returns whether the method the {@link Parameters} was created for contains a {@link Pageable} argument. @@ -169,8 +194,8 @@ public abstract class Parameters, T extends Parameter } /** - * Returns the index of the {@link Pageable} {@link Method} parameter if available. Will return {@literal -1} if there - * is no {@link Pageable} argument in the {@link Method}'s parameter list. + * Returns the index of the {@link Pageable} {@link Method} parameter if available. Will return {@literal -1} if + * there is no {@link Pageable} argument in the {@link Method}'s parameter list. * * @return the pageableIndex */ @@ -179,8 +204,8 @@ public abstract class Parameters, T extends Parameter } /** - * Returns the index of the {@link Sort} {@link Method} parameter if available. Will return {@literal -1} if there is - * no {@link Sort} argument in the {@link Method}'s parameter list. + * Returns the index of the {@link Sort} {@link Method} parameter if available. Will return {@literal -1} if there + * is no {@link Sort} argument in the {@link Method}'s parameter list. * * @return */ @@ -289,8 +314,8 @@ public abstract class Parameters, T extends Parameter /** * Returns a bindable parameter with the given index. So for a method with a signature of - * {@code (Pageable pageable, String name)} a call to {@code #getBindableParameter(0)} will return the {@link String} - * parameter. + * {@code (Pageable pageable, String name)} a call to {@code #getBindableParameter(0)} will return the + * {@link String} parameter. * * @param bindableIndex * @return diff --git a/src/main/java/org/springframework/data/repository/query/QueryMethod.java b/src/main/java/org/springframework/data/repository/query/QueryMethod.java index 56d56e67c..76120505b 100644 --- a/src/main/java/org/springframework/data/repository/query/QueryMethod.java +++ b/src/main/java/org/springframework/data/repository/query/QueryMethod.java @@ -79,8 +79,8 @@ public class QueryMethod { this.method = method; this.unwrappedReturnType = potentiallyUnwrapReturnTypeFor(metadata, method); - this.parameters = createParameters(method); this.metadata = metadata; + this.parameters = createParameters(method); if (hasParameterOfType(method, Pageable.class)) { @@ -107,7 +107,7 @@ public class QueryMethod { Class repositoryDomainClass = metadata.getDomainType(); Class methodDomainClass = metadata.getReturnedDomainClass(method); - return (repositoryDomainClass == null) || repositoryDomainClass.isAssignableFrom(methodDomainClass) + return repositoryDomainClass == null || repositoryDomainClass.isAssignableFrom(methodDomainClass) ? methodDomainClass : repositoryDomainClass; }); @@ -119,11 +119,24 @@ public class QueryMethod { /** * Creates a {@link Parameters} instance. * - * @param method + * @param method must not be {@literal null}. * @return must not return {@literal null}. + * @deprecated since 3.1, call or override {@link #createParameters(Method, TypeInformation)} instead. */ + @Deprecated(since = "3.1", forRemoval = true) protected Parameters createParameters(Method method) { - return new DefaultParameters(method); + return createParameters(method, metadata.getDomainTypeInformation()); + } + + /** + * Creates a {@link Parameters} instance. + * + * @param method must not be {@literal null}. + * @param aggregateType must not be {@literal null}. + * @return must not return {@literal null}. + */ + protected Parameters createParameters(Method method, TypeInformation aggregateType) { + return new DefaultParameters(method, aggregateType); } /** diff --git a/src/test/java/org/springframework/data/repository/query/ParameterUnitTests.java b/src/test/java/org/springframework/data/repository/query/ParameterUnitTests.java index 516bac363..f0e08bba1 100644 --- a/src/test/java/org/springframework/data/repository/query/ParameterUnitTests.java +++ b/src/test/java/org/springframework/data/repository/query/ParameterUnitTests.java @@ -23,15 +23,19 @@ import java.util.Optional; import java.util.stream.Stream; import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; import org.springframework.core.MethodParameter; +import org.springframework.data.repository.query.ParametersUnitTests.User; +import org.springframework.data.util.TypeInformation; /** * Unit tests for {@link Parameter}. * * @author Jens Schauder */ -class KParameterUnitTests { +class ParameterUnitTests { @Test // DATAJPA-1185 void classParameterWithSameTypeParameterAsReturnedListIsDynamicProjectionParameter() throws Exception { @@ -57,6 +61,28 @@ class KParameterUnitTests { assertThat(parameter.isDynamicProjectionParameter()).isTrue(); } + @TestFactory // #2770 + Stream doesNotConsiderClassParametersDynamicProjectionOnes() { + + var methods = Stream.of( // + "genericReturnNonDynamicBind", // + "staticReturnNonDynamicBindWildcard", // + "staticReturnNonDynamicBindWildcardExtends"); + + return DynamicTest.stream(methods, it -> it, it -> { + assertThat(new Parameter(getMethodParameter(it), TypeInformation.of(User.class)) + .isDynamicProjectionParameter()).isFalse(); + }); + } + + @Test // #2770 + void doesNotConsiderAtParamAnnotatedClassParameterDynamicProjectionOne() throws Exception { + + var parameter = new Parameter(getMethodParameter("atParamOnClass")); + + assertThat(parameter.isDynamicProjectionParameter()).isFalse(); + } + @NotNull private MethodParameter getMethodParameter(String methodName) throws NoSuchMethodException { return new MethodParameter(this.getClass().getDeclaredMethod(methodName, Class.class), 0); @@ -73,4 +99,20 @@ class KParameterUnitTests { Optional dynamicProjectionWithOptional(Class type) { return Optional.empty(); } + + T genericReturnNonDynamicBind(Class one) { + return null; + } + + User staticReturnNonDynamicBindWildcard(Class two) { + return null; + } + + User staticReturnNonDynamicBindWildcardExtends(Class one) { + return null; + } + + T atParamOnClass(@Param("type") Class type) { + return null; + } }