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 Original pull request: #3303 Signed-off-by: Chris Bono <chris.bono@broadcom.com>
This commit is contained in:
@@ -18,6 +18,7 @@ package org.springframework.data.web;
|
||||
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.beans.factory.ObjectFactory;
|
||||
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;
|
||||
@@ -48,13 +50,17 @@ import org.springframework.web.multipart.support.MultipartResolutionDelegate;
|
||||
public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodProcessor
|
||||
implements BeanFactoryAware, BeanClassLoaderAware {
|
||||
|
||||
// NonFinalForTesting
|
||||
private static LogAccessor LOGGER = new LogAccessor(ProxyingHandlerMethodArgumentResolver.class);
|
||||
|
||||
private static final List<String> IGNORED_PACKAGES = List.of("java", "org.springframework");
|
||||
|
||||
private final SpelAwareProxyProjectionFactory proxyFactory;
|
||||
private final ObjectFactory<ConversionService> 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 +86,36 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodP
|
||||
@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 +126,12 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodP
|
||||
|
||||
// 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 +146,33 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodP
|
||||
|
||||
@Override
|
||||
protected void bindRequestParameters(WebDataBinder binder, NativeWebRequest request) {}
|
||||
|
||||
/**
|
||||
* Logs a warning message when a parameter is proxied but not explicitly annotated with {@link @ProjectedPayload}.
|
||||
* <p>
|
||||
* 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%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<MethodParameter, Boolean> 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();
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -16,16 +16,20 @@
|
||||
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.test.util.ReflectionTestUtils;
|
||||
import org.springframework.util.ReflectionUtils;
|
||||
import org.springframework.web.bind.annotation.ModelAttribute;
|
||||
import org.springframework.web.multipart.MultipartFile;
|
||||
@@ -115,6 +119,24 @@ class ProxyingHandlerMethodArgumentResolverUnitTests {
|
||||
assertThat(resolver.supportsParameter(parameter)).isFalse();
|
||||
}
|
||||
|
||||
@Test // GH-3300
|
||||
@SuppressWarnings("unchecked")
|
||||
void deprecationLoggerOnlyLogsOncePerParameter() {
|
||||
|
||||
var parameter = getParameter("withModelAttribute", SampleInterface.class);
|
||||
|
||||
// Spy on the actual logger
|
||||
var actualLogger = (LogAccessor) ReflectionTestUtils.getField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER");
|
||||
var actualLoggerSpy = spy(actualLogger);
|
||||
ReflectionTestUtils.setField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER", actualLoggerSpy, LogAccessor.class);
|
||||
|
||||
// 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);
|
||||
|
||||
Reference in New Issue
Block a user