From 3722208f969cd8590ab78e26900a3858faa73a57 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 4 Dec 2023 11:00:00 +0100 Subject: [PATCH 1/2] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 0a9bd1723b..5e6250c8d4 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 3.3.0-SNAPSHOT + 3.3.0-GH-2995-SNAPSHOT Spring Data Core Core Spring concepts underpinning every Spring Data module. From 39697c7f93e067fb737282fafb0c882592d54a16 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 4 Dec 2023 11:01:37 +0100 Subject: [PATCH 2/2] Introduce ParametersSource to Parameters. We now provide a ParametersSource object to create MethodParameter objects associated with the enclosing class so parameters can resolve generics properly. --- .../repository/query/DefaultParameters.java | 11 +- .../data/repository/query/Parameters.java | 27 ++++- .../repository/query/ParametersSource.java | 112 ++++++++++++++++++ .../data/repository/query/QueryMethod.java | 17 ++- .../RepositoryMethodInvokerUnitTests.java | 6 +- .../ParametersParameterAccessorUnitTests.java | 28 +++-- .../repository/query/ParametersUnitTests.java | 40 +++++-- 7 files changed, 207 insertions(+), 34 deletions(-) create mode 100644 src/main/java/org/springframework/data/repository/query/ParametersSource.java 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 1cadc973d1..636bf17737 100644 --- a/src/main/java/org/springframework/data/repository/query/DefaultParameters.java +++ b/src/main/java/org/springframework/data/repository/query/DefaultParameters.java @@ -39,14 +39,13 @@ public DefaultParameters(Method method) { } /** - * Creates a new {@link DefaultParameters} instance from the given {@link Method} and aggregate - * {@link TypeInformation}. + * Creates a new {@link DefaultParameters} instance from the given {@link ParametersSource}. * - * @param method must not be {@literal null}. - * @param aggregateType must not be {@literal null}. + * @param parametersSource must not be {@literal null}. + * @since 3.2.1 */ - public DefaultParameters(Method method, TypeInformation aggregateType) { - super(method, param -> new Parameter(param, aggregateType)); + public DefaultParameters(ParametersSource parametersSource) { + super(parametersSource, param -> new Parameter(param, parametersSource.getDomainType())); } private DefaultParameters(List parameters) { 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 f98e2a2179..661334b70f 100644 --- a/src/main/java/org/springframework/data/repository/query/Parameters.java +++ b/src/main/java/org/springframework/data/repository/query/Parameters.java @@ -44,7 +44,8 @@ */ public abstract class Parameters, T extends Parameter> implements Streamable { - public static final List> TYPES = Arrays.asList(ScrollPosition.class, Pageable.class, Sort.class, Limit.class); + public static final List> TYPES = Arrays.asList(ScrollPosition.class, Pageable.class, Sort.class, + Limit.class); private static final String PARAM_ON_SPECIAL = format("You must not use @%s on a parameter typed %s or %s", Param.class.getSimpleName(), Pageable.class.getSimpleName(), Sort.class.getSimpleName()); @@ -82,14 +83,30 @@ public Parameters(Method method) { * @param method must not be {@literal null}. * @param parameterFactory must not be {@literal null}. * @since 3.0.2 + * @deprecated since 3.2.1, use {@link Parameters(ParametersSource, Function)} instead. */ + @Deprecated(since = "3.2.1", forRemoval = true) protected Parameters(Method method, Function parameterFactory) { + this(ParametersSource.of(method), parameterFactory); + } + + /** + * 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 parametersSource must not be {@literal null}. + * @param parameterFactory must not be {@literal null}. + * @since 3.2.1 + */ + protected Parameters(ParametersSource parametersSource, + Function parameterFactory) { - Assert.notNull(method, "Method must not be null"); + Assert.notNull(parametersSource, "ParametersSource 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. + Method method = parametersSource.getMethod(); int parameterCount = method.getParameterCount(); this.parameters = new ArrayList<>(parameterCount); @@ -102,7 +119,9 @@ protected Parameters(Method method, Function parameterFactor for (int i = 0; i < parameterCount; i++) { - MethodParameter methodParameter = new MethodParameter(method, i); + MethodParameter methodParameter = new MethodParameter(method, i) + .withContainingClass(parametersSource.getContainingClass()); + methodParameter.initParameterNameDiscovery(PARAMETER_NAME_DISCOVERER); T parameter = parameterFactory == null // @@ -421,7 +440,9 @@ public static boolean isBindable(Class type) { return !TYPES.contains(type); } + @Override public Iterator iterator() { return parameters.iterator(); } + } diff --git a/src/main/java/org/springframework/data/repository/query/ParametersSource.java b/src/main/java/org/springframework/data/repository/query/ParametersSource.java new file mode 100644 index 0000000000..de4beae77c --- /dev/null +++ b/src/main/java/org/springframework/data/repository/query/ParametersSource.java @@ -0,0 +1,112 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.repository.query; + +import java.lang.reflect.Method; + +import org.springframework.data.repository.core.RepositoryMetadata; +import org.springframework.data.util.TypeInformation; +import org.springframework.util.Assert; + +/** + * Interface providing access to the method, containing class and domain type as source object for parameter + * descriptors. + * + * @author Mark Paluch + * @since 3.2.1 + */ +public interface ParametersSource { + + /** + * Create a new parameter source for the given {@link Method}. + * + * @param method the method to use. + * @return a new parameter source for the given {@link Method}. + */ + static ParametersSource of(Method method) { + + Assert.notNull(method, "Method must not be null"); + + return new ParametersSource() { + + @Override + public Class getContainingClass() { + return method.getDeclaringClass(); + } + + @Override + public Method getMethod() { + return method; + } + + @Override + public TypeInformation getDomainType() { + return TypeInformation.OBJECT; + } + }; + } + + /** + * Create a new parameter source for the given {@link Method} in the context of {@link RepositoryMetadata}. + * + * @param method the method to use. + * @return a new parameter source for the given {@link Method}. + */ + static ParametersSource of(RepositoryMetadata metadata, Method method) { + + Assert.notNull(metadata, "RepositoryMetadata must not be null"); + Assert.notNull(method, "Method must not be null"); + + if (!method.getDeclaringClass().isAssignableFrom(metadata.getRepositoryInterface())) { + throw new IllegalArgumentException( + "Method " + method + " is not defined in the type hierarchy of " + metadata.getRepositoryInterface()); + } + + return new ParametersSource() { + + @Override + public Class getContainingClass() { + return metadata.getRepositoryInterface(); + } + + @Override + public Method getMethod() { + return method; + } + + @Override + public TypeInformation getDomainType() { + return metadata.getDomainTypeInformation(); + } + }; + } + + /** + * @return the class that contains the {@link #getMethod()}. + */ + Class getContainingClass(); + + /** + * @return the method for which the parameter source is defined. + */ + Method getMethod(); + + /** + * @return the domain type associated with the parameter source. + */ + TypeInformation getDomainType(); + +} 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 bc3e66cf7a..9b0fd28254 100644 --- a/src/main/java/org/springframework/data/repository/query/QueryMethod.java +++ b/src/main/java/org/springframework/data/repository/query/QueryMethod.java @@ -183,16 +183,29 @@ private boolean calculateIsCollectionQuery() { * @param method must not be {@literal null}. * @param domainType must not be {@literal null}. * @return must not return {@literal null}. + * @deprecated since 3.2.1 * @since 3.0.2 */ + @Deprecated(since = "3.2.1", forRemoval = true) protected Parameters createParameters(Method method, TypeInformation domainType) { - return new DefaultParameters(method, domainType); + return createParameters(ParametersSource.of(getMetadata(), method)); + } + + /** + * Creates a {@link Parameters} instance. + * + * @param parametersSource must not be {@literal null}. + * @return must not return {@literal null}. + * @since 3.2.1 + */ + protected Parameters createParameters(ParametersSource parametersSource) { + return new DefaultParameters(parametersSource); } /** * Returns the method's name. * - * @return + * @return the method's name. */ public String getName() { return method.getName(); diff --git a/src/test/java/org/springframework/data/repository/core/support/RepositoryMethodInvokerUnitTests.java b/src/test/java/org/springframework/data/repository/core/support/RepositoryMethodInvokerUnitTests.java index 5a185737f9..27bb191c77 100644 --- a/src/test/java/org/springframework/data/repository/core/support/RepositoryMethodInvokerUnitTests.java +++ b/src/test/java/org/springframework/data/repository/core/support/RepositoryMethodInvokerUnitTests.java @@ -114,7 +114,8 @@ void capturesImperativeDurationCorrectly() throws Exception { repositoryMethodInvoker("findAll").invoke(); - assertThat(multicaster.first().getDuration(TimeUnit.MILLISECONDS)).isCloseTo(250, Percentage.withPercentage(10)); + assertThat(multicaster.first() + .getDuration(TimeUnit.MILLISECONDS)).isCloseTo(500, Percentage.withPercentage(50)); } @Test // DATACMNS-1764 @@ -124,7 +125,8 @@ void capturesReactiveDurationCorrectly() throws Exception { repositoryMethodInvokerForReactive("findAll").> invoke().subscribe(); - assertThat(multicaster.first().getDuration(TimeUnit.MILLISECONDS)).isCloseTo(250, Percentage.withPercentage(10)); + assertThat(multicaster.first() + .getDuration(TimeUnit.MILLISECONDS)).isCloseTo(500, Percentage.withPercentage(50)); } @Test // DATACMNS-1764 diff --git a/src/test/java/org/springframework/data/repository/query/ParametersParameterAccessorUnitTests.java b/src/test/java/org/springframework/data/repository/query/ParametersParameterAccessorUnitTests.java index 223aa0f35c..c6ae21802d 100755 --- a/src/test/java/org/springframework/data/repository/query/ParametersParameterAccessorUnitTests.java +++ b/src/test/java/org/springframework/data/repository/query/ParametersParameterAccessorUnitTests.java @@ -25,6 +25,7 @@ import org.springframework.data.domain.ScrollPosition; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Order; +import org.springframework.data.repository.core.RepositoryMetadata; /** * Unit tests for {@link ParametersParameterAccessor}. @@ -36,10 +37,11 @@ class ParametersParameterAccessorUnitTests { Parameters parameters; + RepositoryMetadata metadata; @BeforeEach void setUp() throws Exception { - parameters = new DefaultParameters(Sample.class.getMethod("method", String.class, int.class)); + parameters = new DefaultParameters(ParametersSource.of(Sample.class.getMethod("method", String.class, int.class))); } @Test @@ -62,7 +64,7 @@ void detectsNullValue() throws Exception { assertThat(accessor.hasBindableNullValue()).isTrue(); var method = Sample.class.getMethod("method", Pageable.class, String.class); - var parameters = new DefaultParameters(method); + var parameters = new DefaultParameters(ParametersSource.of(method)); accessor = new ParametersParameterAccessor(parameters, new Object[] { null, "Foo" }); assertThat(accessor.hasBindableNullValue()).isFalse(); @@ -72,7 +74,7 @@ void detectsNullValue() throws Exception { void iteratesonlyOverBindableValues() throws Exception { var method = Sample.class.getMethod("method", Pageable.class, String.class); - var parameters = new DefaultParameters(method); + var parameters = new DefaultParameters(ParametersSource.of(method)); var accessor = new ParametersParameterAccessor(parameters, new Object[] { PageRequest.of(0, 10), "Foo" }); @@ -84,7 +86,7 @@ void iteratesonlyOverBindableValues() throws Exception { void handlesScrollPositionAsAParameterType() throws NoSuchMethodException { var method = Sample.class.getMethod("method", ScrollPosition.class, String.class); - var parameters = new DefaultParameters(method); + var parameters = new DefaultParameters(ParametersSource.of(method)); var accessor = new ParametersParameterAccessor(parameters, new Object[] { ScrollPosition.offset(1), "Foo" }); @@ -96,7 +98,7 @@ void handlesScrollPositionAsAParameterType() throws NoSuchMethodException { void handlesPageRequestAsAParameterType() throws NoSuchMethodException { var method = Sample.class.getMethod("methodWithPageRequest", PageRequest.class, String.class); - var parameters = new DefaultParameters(method); + var parameters = new DefaultParameters(ParametersSource.of(method)); var accessor = new ParametersParameterAccessor(parameters, new Object[] { PageRequest.of(0, 10), "Foo" }); @@ -108,7 +110,7 @@ void handlesPageRequestAsAParameterType() throws NoSuchMethodException { void handlesLimitAsAParameterType() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Limit.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Limit.of(100), "spring" }); assertThat(accessor).hasSize(1); @@ -119,7 +121,7 @@ void handlesLimitAsAParameterType() throws NoSuchMethodException { void returnsLimitIfAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Limit.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Limit.of(100), "spring" }); assertThat(accessor.getLimit()).extracting(Limit::max).isEqualTo(100); @@ -129,7 +131,7 @@ void returnsLimitIfAvailable() throws NoSuchMethodException { void readsLimitFromPageableIfAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Pageable.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Pageable.ofSize(100), "spring" }); assertThat(accessor.getLimit()).extracting(Limit::max).isEqualTo(100); @@ -139,7 +141,7 @@ void readsLimitFromPageableIfAvailable() throws NoSuchMethodException { void returnsUnlimitedIfNoLimitingAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Sort.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Pageable.ofSize(100), "spring" }); assertThat(accessor.getLimit().isUnlimited()).isTrue(); @@ -149,7 +151,7 @@ void returnsUnlimitedIfNoLimitingAvailable() throws NoSuchMethodException { void appliesLimitToPageableIfAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Limit.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Limit.of(100), "spring" }); Pageable pageable = accessor.getPageable(); @@ -161,7 +163,7 @@ void appliesLimitToPageableIfAvailable() throws NoSuchMethodException { void appliesLimitToPageableIfRequested() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Limit.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Limit.of(100), "spring" }); assertThat(accessor).hasSize(1); @@ -172,7 +174,7 @@ void appliesLimitToPageableIfRequested() throws NoSuchMethodException { void appliesSortToPageableIfAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Sort.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Sort.by("one", "two"), "spring" }); Pageable pageable = accessor.getPageable(); @@ -184,7 +186,7 @@ void appliesSortToPageableIfAvailable() throws NoSuchMethodException { void appliesSortAndLimitToPageableIfAvailable() throws NoSuchMethodException { var method = Sample.class.getMethod("method", Sort.class, Limit.class, String.class); - var accessor = new ParametersParameterAccessor(new DefaultParameters(method), + var accessor = new ParametersParameterAccessor(new DefaultParameters(ParametersSource.of(method)), new Object[] { Sort.by("one", "two"), Limit.of(42), "spring" }); Pageable pageable = accessor.getPageable(); diff --git a/src/test/java/org/springframework/data/repository/query/ParametersUnitTests.java b/src/test/java/org/springframework/data/repository/query/ParametersUnitTests.java index 0c62d9196d..3a36a8f58e 100755 --- a/src/test/java/org/springframework/data/repository/query/ParametersUnitTests.java +++ b/src/test/java/org/springframework/data/repository/query/ParametersUnitTests.java @@ -26,12 +26,16 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.reactivestreams.Publisher; + import org.springframework.data.domain.Limit; import org.springframework.data.domain.OffsetScrollPosition; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Window; +import org.springframework.data.repository.Repository; +import org.springframework.data.repository.core.RepositoryMetadata; +import org.springframework.data.repository.core.support.DefaultRepositoryMetadata; import org.springframework.test.util.ReflectionTestUtils; /** @@ -43,11 +47,13 @@ class ParametersUnitTests { private Method valid; + private RepositoryMetadata metadata; @BeforeEach void setUp() throws SecurityException, NoSuchMethodException { valid = SampleDao.class.getMethod("valid", String.class); + metadata = new DefaultRepositoryMetadata(SampleDao.class); } @Test @@ -56,14 +62,14 @@ void checksValidMethodCorrectly() throws Exception { var validWithPageable = SampleDao.class.getMethod("validWithPageable", String.class, Pageable.class); var validWithSort = SampleDao.class.getMethod("validWithSort", String.class, Sort.class); - new DefaultParameters(valid); - new DefaultParameters(validWithPageable); - new DefaultParameters(validWithSort); + new DefaultParameters(ParametersSource.of(valid)); + new DefaultParameters(ParametersSource.of(validWithPageable)); + new DefaultParameters(ParametersSource.of(validWithSort)); } @Test void rejectsNullMethod() { - assertThatIllegalArgumentException().isThrownBy(() -> new DefaultParameters(null)); + assertThatIllegalArgumentException().isThrownBy(() -> new DefaultParameters((ParametersSource) null)); } @Test @@ -87,12 +93,12 @@ void calculatesPlaceholderPositionCorrectly() throws Exception { var method = SampleDao.class.getMethod("validWithSortFirst", Sort.class, String.class); - Parameters parameters = new DefaultParameters(method); + Parameters parameters = new DefaultParameters(ParametersSource.of(method)); assertThat(parameters.getBindableParameter(0).getIndex()).isEqualTo(1); method = SampleDao.class.getMethod("validWithSortInBetween", String.class, Sort.class, String.class); - parameters = new DefaultParameters(method); + parameters = new DefaultParameters(ParametersSource.of(method)); assertThat(parameters.getBindableParameter(0).getIndex()).isEqualTo(0); assertThat(parameters.getBindableParameter(1).getIndex()).isEqualTo(2); @@ -202,19 +208,30 @@ void acceptsLimitParameter() throws Exception { assertThat(parameters.getLimitIndex()).isOne(); } + @Test // GH-2995 + void considersGenericType() throws Exception { + + var method = TypedInterface.class.getMethod("foo", Object.class); + + var parameters = new DefaultParameters( + ParametersSource.of(new DefaultRepositoryMetadata(TypedInterface.class), method)); + + assertThat(parameters.getParameter(0).getType()).isEqualTo(Long.class); + } + private Parameters getParametersFor(String methodName, Class... parameterTypes) throws SecurityException, NoSuchMethodException { var method = SampleDao.class.getMethod(methodName, parameterTypes); - return new DefaultParameters(method); + return new DefaultParameters(ParametersSource.of(metadata, method)); } static class User { } - static interface SampleDao { + interface SampleDao extends Repository { User valid(@Param("username") String username); @@ -248,4 +265,11 @@ static interface SampleDao { } interface SomePageable extends Pageable {} + + interface Intermediate extends Repository { + void foo(ID id); + } + + interface TypedInterface extends Intermediate {} + }