Skip to content

Log a warning if param not annotated with @ProjectedPayload #3303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-commons</artifactId>
<version>4.0.0-SNAPSHOT</version>
<version>4.0.0-GH-3300-SNAPSHOT</version>

<name>Spring Data Core</name>
<description>Core Spring concepts underpinning every Spring Data module.</description>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -52,9 +54,10 @@ public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodP

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}.
*/
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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}.
* <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 %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<MethodParameter, Boolean> loggedParameters = new ConcurrentHashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My spidey-senses are typically activated when I add a map of things that could ultimately grow unbounded and OOME if something went awry. That would lead me to use the ConcurrentLruCache from framework. However, the added complexity plus the LruCache get API has no signal as to whether the cache was originally empty (this is the signal I needed). I chose simplicity over paranoia here.

What is the biggest number of method params we have encountered in the wilderness via bug reports etc..???

IOW - you good w/ this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to switch to a simple static boolean? I guess a single warning is just enough. That would reduce this to a Logger declaration in ProxyingHandlerMethodArgumentResolver and a simple if?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thinking behind logging per parameter is that the user will get a more directly actionable message. Otherwise they may end up doing wak-a-mole to find the problematic parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR to include more context/location in the log message. It now looks like:

2025-06-02 11:18:57,706 WARN eb.ProxyingHandlerMethodArgumentResolver: 271 - Parameter 'foo' at position 0 in org.springframework.data.web.ProxyingHandlerMethodArgumentResolverUnitTests$Controller.withModelAttribute 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.


/**
* 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));
}
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down