-
Notifications
You must be signed in to change notification settings - Fork 687
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> 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}. | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @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) | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (MultipartResolutionDelegate.isMultipartArgument(parameter)) { | ||
return false; | ||
} | ||
|
||
// Type or parameter explicitly annotated with @ProjectedPayload | ||
if (parameter.hasParameterAnnotation(ProjectedPayload.class) || AnnotatedElementUtils.findMergedAnnotation(type, | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ProjectedPayload.class) != null) { | ||
return true; | ||
} | ||
|
||
// Annotated type | ||
if (AnnotatedElementUtils.findMergedAnnotation(type, ProjectedPayload.class) != null) { | ||
// Parameter annotated with @ModelAttribute | ||
if (parameter.hasParameterAnnotation(ModelAttribute.class)) { | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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); | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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}. | ||
* <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); | ||
onobc marked this conversation as resolved.
Show resolved
Hide resolved
onobc marked this conversation as resolved.
Show resolved
Hide resolved
onobc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private final ConcurrentHashMap<MethodParameter, Boolean> loggedParameters = new ConcurrentHashMap<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 What is the biggest number of method params we have encountered in the wilderness via bug reports etc..??? IOW - you good w/ this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
|
||
/** | ||
* 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)); | ||
} | ||
} | ||
|
||
} | ||
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.