From 0ad5a4935659958b43bc5ae0ccb777fd59d9687d Mon Sep 17 00:00:00 2001 From: Chris Bono Date: Tue, 27 May 2025 16:56:04 -0500 Subject: [PATCH 1/4] GH-3300 - Prepare branch --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index a6dc167a03..f60f531acf 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 4.0.0-SNAPSHOT + 4.0.0-GH-3300-SNAPSHOT Spring Data Core Core Spring concepts underpinning every Spring Data module. From 261dba2cf4be0545438444b70a60ffccb22fe93b Mon Sep 17 00:00:00 2001 From: Chris Bono Date: Thu, 29 May 2025 15:38:43 -0500 Subject: [PATCH 2/4] Log a warning if param not annotated with @ProjectedPayload. This commit introduces a warning log if a parameter is not annotated with `@ProjectedPayload` that this style is deprecated and that we will drop support for projections if a parameter (or the parameter type) is not explicitly annotated with `@ProjectedPayload`. Resolves #3300 Signed-off-by: Chris Bono --- ...ProxyingHandlerMethodArgumentResolver.java | 60 ++++++++++++++++--- ...andlerMethodArgumentResolverUnitTests.java | 24 ++++++++ 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java index c47881852c..2d92765b22 100644 --- a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java @@ -18,6 +18,7 @@ import java.lang.annotation.Annotation; import java.util.Arrays; import java.util.List; +import java.util.concurrent.ConcurrentHashMap; import org.springframework.beans.BeansException; import org.springframework.beans.MutablePropertyValues; @@ -28,6 +29,7 @@ import org.springframework.core.MethodParameter; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.convert.ConversionService; +import org.springframework.core.log.LogAccessor; import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.util.ClassUtils; import org.springframework.web.bind.WebDataBinder; @@ -52,9 +54,10 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodP private final SpelAwareProxyProjectionFactory proxyFactory; private final ObjectFactory conversionService; + private final ProjectedPayloadDeprecationLogger deprecationLogger = new ProjectedPayloadDeprecationLogger(); /** - * Creates a new {@link PageableHandlerMethodArgumentResolver} using the given {@link ConversionService}. + * Creates a new {@link ProxyingHandlerMethodArgumentResolver} using the given {@link ConversionService}. * * @param conversionService must not be {@literal null}. */ @@ -80,28 +83,36 @@ public void setBeanClassLoader(ClassLoader classLoader) { @Override public boolean supportsParameter(MethodParameter parameter) { + // Simple type or not annotated with @ModelAttribute (and flag set to require annotation) if (!super.supportsParameter(parameter)) { return false; } Class type = parameter.getParameterType(); + // Only interfaces can be proxied if (!type.isInterface()) { return false; } - // Annotated parameter (excluding multipart) - if ((parameter.hasParameterAnnotation(ProjectedPayload.class) || parameter.hasParameterAnnotation( - ModelAttribute.class)) && !MultipartResolutionDelegate.isMultipartArgument(parameter)) { + // Multipart not currently supported + if (MultipartResolutionDelegate.isMultipartArgument(parameter)) { + return false; + } + + // Type or parameter explicitly annotated with @ProjectedPayload + if (parameter.hasParameterAnnotation(ProjectedPayload.class) || AnnotatedElementUtils.findMergedAnnotation(type, + ProjectedPayload.class) != null) { return true; } - // Annotated type - if (AnnotatedElementUtils.findMergedAnnotation(type, ProjectedPayload.class) != null) { + // Parameter annotated with @ModelAttribute + if (parameter.hasParameterAnnotation(ModelAttribute.class)) { + this.deprecationLogger.logDeprecationForParameter(parameter); return true; } - // Exclude parameters annotated with Spring annotation + // Exclude any other parameters annotated with Spring annotation if (Arrays.stream(parameter.getParameterAnnotations()) .map(Annotation::annotationType) .map(Class::getPackageName) @@ -112,8 +123,12 @@ public boolean supportsParameter(MethodParameter parameter) { // Fallback for only user defined interfaces String packageName = ClassUtils.getPackageName(type); + if (IGNORED_PACKAGES.stream().noneMatch(packageName::startsWith)) { + this.deprecationLogger.logDeprecationForParameter(parameter); + return true; + } - return !IGNORED_PACKAGES.stream().anyMatch(it -> packageName.startsWith(it)); + return false; } @Override @@ -128,4 +143,33 @@ protected Object createAttribute(String attributeName, MethodParameter parameter @Override protected void bindRequestParameters(WebDataBinder binder, NativeWebRequest request) {} + + /** + * Logs a warning message when a parameter is proxied but not explicitly annotated with {@link @ProjectedPayload}. + *

+ * To avoid log spamming, the message is only logged the first time the parameter is encountered. + */ + static class ProjectedPayloadDeprecationLogger { + + private static final String MESSAGE = "Parameter %s (%s) is not annotated with @ProjectedPayload - support for parameters not explicitly annotated with @ProjectedPayload (at the parameter or type level) will be dropped in a future version."; + + private final LogAccessor logger = new LogAccessor(ProxyingHandlerMethodArgumentResolver.class); + + private final ConcurrentHashMap loggedParameters = new ConcurrentHashMap<>(); + + /** + * Log a warning the first time a non-annotated method parameter is encountered. + * + * @param parameter the parameter + */ + void logDeprecationForParameter(MethodParameter parameter) { + + if (this.loggedParameters.putIfAbsent(parameter, Boolean.TRUE) == null) { + var paramName = parameter.getParameterName(); + this.logger.warn(() -> MESSAGE.formatted(paramName != null ? paramName : "", parameter)); + } + } + + } + } diff --git a/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java index 7093aa0be7..1484c97dd3 100755 --- a/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java @@ -16,16 +16,21 @@ package org.springframework.data.web; import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import example.SampleInterface; import java.util.List; +import java.util.function.Supplier; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.MethodParameter; import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.core.log.LogAccessor; +import org.springframework.data.web.ProxyingHandlerMethodArgumentResolver.ProjectedPayloadDeprecationLogger; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.ReflectionUtils; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.multipart.MultipartFile; @@ -115,6 +120,25 @@ void doesNotSupportAtProjectedPayloadForMultipartParam() { assertThat(resolver.supportsParameter(parameter)).isFalse(); } + @Test // GH-3300 + @SuppressWarnings("unchecked") + void deprecationLoggerOnlyLogsOncePerParameter() { + + var parameter = getParameter("withModelAttribute", SampleInterface.class); + + // Spy on the actual logger in the helper class + var deprecationLogger = (ProjectedPayloadDeprecationLogger) ReflectionTestUtils.getField(resolver, "deprecationLogger"); + var actualLogger = (LogAccessor) ReflectionTestUtils.getField(deprecationLogger, "logger"); + var actualLoggerSpy = spy(actualLogger); + ReflectionTestUtils.setField(deprecationLogger, "logger", actualLoggerSpy); + + // Invoke twice but should only log the first time + assertThat(resolver.supportsParameter(parameter)).isTrue(); + verify(actualLoggerSpy, times(1)).warn(any(Supplier.class)); + assertThat(resolver.supportsParameter(parameter)).isTrue(); + verifyNoMoreInteractions(actualLoggerSpy); + } + private static MethodParameter getParameter(String methodName, Class parameterType) { var method = ReflectionUtils.findMethod(Controller.class, methodName, parameterType); From 4ab56932ee4ae922faa7276905ccd2267f609b2b Mon Sep 17 00:00:00 2001 From: Chris Bono Date: Mon, 2 Jun 2025 11:19:44 -0500 Subject: [PATCH 3/4] - Improve message w/ more context/location. - Move logger up to resolver Signed-off-by: Chris Bono --- .../web/ProxyingHandlerMethodArgumentResolver.java | 11 +++++++---- ...roxyingHandlerMethodArgumentResolverUnitTests.java | 8 +++----- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java index 2d92765b22..631ca87827 100644 --- a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java @@ -50,6 +50,9 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodProcessor implements BeanFactoryAware, BeanClassLoaderAware { + // NonFinalForTesting + private static LogAccessor LOGGER = new LogAccessor(ProxyingHandlerMethodArgumentResolver.class); + private static final List IGNORED_PACKAGES = List.of("java", "org.springframework"); private final SpelAwareProxyProjectionFactory proxyFactory; @@ -151,9 +154,7 @@ protected void bindRequestParameters(WebDataBinder binder, NativeWebRequest requ */ static class ProjectedPayloadDeprecationLogger { - private static final String MESSAGE = "Parameter %s (%s) is not annotated with @ProjectedPayload - support for parameters not explicitly annotated with @ProjectedPayload (at the parameter or type level) will be dropped in a future version."; - - private final LogAccessor logger = new LogAccessor(ProxyingHandlerMethodArgumentResolver.class); + private static final String MESSAGE = "Parameter%sat position %s in %s.%s is not annotated with @ProjectedPayload - support for parameters not explicitly annotated with @ProjectedPayload (at the parameter or type level) will be dropped in a future version."; private final ConcurrentHashMap loggedParameters = new ConcurrentHashMap<>(); @@ -166,7 +167,9 @@ void logDeprecationForParameter(MethodParameter parameter) { if (this.loggedParameters.putIfAbsent(parameter, Boolean.TRUE) == null) { var paramName = parameter.getParameterName(); - this.logger.warn(() -> MESSAGE.formatted(paramName != null ? paramName : "", parameter)); + var paramNameOrEmpty = paramName != null ? (" " + paramName + " ") : " "; + var methodName = parameter.getMethod() != null ? parameter.getMethod().getName() : "constructor"; + LOGGER.warn(() -> MESSAGE.formatted(paramNameOrEmpty, parameter.getParameterIndex(), parameter.getContainingClass().getName(), methodName)); } } diff --git a/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java index 1484c97dd3..a8e1d75820 100755 --- a/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java @@ -29,7 +29,6 @@ import org.springframework.core.MethodParameter; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.core.log.LogAccessor; -import org.springframework.data.web.ProxyingHandlerMethodArgumentResolver.ProjectedPayloadDeprecationLogger; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.ReflectionUtils; import org.springframework.web.bind.annotation.ModelAttribute; @@ -126,11 +125,10 @@ void deprecationLoggerOnlyLogsOncePerParameter() { var parameter = getParameter("withModelAttribute", SampleInterface.class); - // Spy on the actual logger in the helper class - var deprecationLogger = (ProjectedPayloadDeprecationLogger) ReflectionTestUtils.getField(resolver, "deprecationLogger"); - var actualLogger = (LogAccessor) ReflectionTestUtils.getField(deprecationLogger, "logger"); + // Spy on the actual logger + var actualLogger = (LogAccessor) ReflectionTestUtils.getField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER"); var actualLoggerSpy = spy(actualLogger); - ReflectionTestUtils.setField(deprecationLogger, "logger", actualLoggerSpy); + ReflectionTestUtils.setField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER", actualLoggerSpy, LogAccessor.class); // Invoke twice but should only log the first time assertThat(resolver.supportsParameter(parameter)).isTrue(); From b94d734ec74005a11653fb5875220f9ebd208fe8 Mon Sep 17 00:00:00 2001 From: Chris Bono Date: Mon, 2 Jun 2025 11:25:04 -0500 Subject: [PATCH 4/4] - Improve message w/ more context/location (take 2) Signed-off-by: Chris Bono --- .../data/web/ProxyingHandlerMethodArgumentResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java index 631ca87827..738de8c0c7 100644 --- a/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java @@ -167,7 +167,7 @@ void logDeprecationForParameter(MethodParameter parameter) { if (this.loggedParameters.putIfAbsent(parameter, Boolean.TRUE) == null) { var paramName = parameter.getParameterName(); - var paramNameOrEmpty = paramName != null ? (" " + paramName + " ") : " "; + var paramNameOrEmpty = paramName != null ? (" '" + paramName + "' ") : " "; var methodName = parameter.getMethod() != null ? parameter.getMethod().getName() : "constructor"; LOGGER.warn(() -> MESSAGE.formatted(paramNameOrEmpty, parameter.getParameterIndex(), parameter.getContainingClass().getName(), methodName)); }