Skip to content

Remove @ProjectedPayload warning logging. #3509

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.

Sign up for GitHub

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

Open
hutiefang76 wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −108
Open

Remove @ProjectedPayload warning logging. #3509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/main/antora/modules/ROOT/pages/repositories/core-extensions-web.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,9 @@ public interface UserPayload {
}
----

For Spring MVC handler method argument resolution, annotate either the handler method parameter or the projection interface with `@ProjectedPayload`.
Interfaces without `@ProjectedPayload` on either the handler method parameter or the interface type are not considered projection payloads.

You can use the type shown in the preceding example as a Spring MVC handler method argument or by using `ParameterizedTypeReference` on one of methods of the `RestTemplate`.
The preceding method declarations would try to find `firstname` anywhere in the given document.
The `lastname` XML lookup is performed on the top-level of the incoming document.
Expand Down
78 changes: 3 additions & 75 deletions src/main/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@
*/
package org.springframework.data.web;

import java.lang.annotation.Annotation;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.springframework.beans.BeansException;
import org.springframework.beans.MutablePropertyValues;
import org.springframework.beans.factory.BeanClassLoaderAware;
Expand All @@ -32,13 +25,10 @@
import org.springframework.core.SpringProperties;
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.util.NumberUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.bind.WebDataBinder;
import org.springframework.web.bind.annotation.ModelAttribute;
import org.springframework.web.bind.support.WebDataBinderFactory;
import org.springframework.web.context.request.NativeWebRequest;
import org.springframework.web.method.annotation.ModelAttributeMethodProcessor;
Expand All @@ -55,20 +45,16 @@
* @author Chris Bono
* @author Mark Paluch
* @author Christoph Strobl
* @author hutiefang
* @since 1.10
*/
public class ProxyingHandlerMethodArgumentResolver extends ModelAttributeMethodProcessor
implements BeanFactoryAware, BeanClassLoaderAware {

// NonFinalForTesting
private static LogAccessor LOGGER = new LogAccessor(ProxyingHandlerMethodArgumentResolver.class);
public static final String COLLECTION_SIZE_LIMIT_PARAM = "spring.data.web.projection.collection-limit";

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();
private final int collectionSizeLimit;

/**
Expand Down Expand Up @@ -122,35 +108,8 @@ public boolean supportsParameter(MethodParameter parameter) {
return false;
}

// Type or parameter explicitly annotated with @ProjectedPayload
if (parameter.hasParameterAnnotation(ProjectedPayload.class) || AnnotatedElementUtils.findMergedAnnotation(type,
ProjectedPayload.class) != null) {
return true;
}

// Parameter annotated with @ModelAttribute
if (parameter.hasParameterAnnotation(ModelAttribute.class)) {
this.deprecationLogger.logDeprecationForParameter(parameter);
return false;
}

// Exclude any other parameters annotated with Spring annotation
if (Arrays.stream(parameter.getParameterAnnotations())
.map(Annotation::annotationType)
.map(Class::getPackageName)
.anyMatch(it -> it.startsWith("org.springframework"))) {

return false;
}

// Fallback for only user defined interfaces
String packageName = ClassUtils.getPackageName(type);
if (IGNORED_PACKAGES.stream().noneMatch(packageName::startsWith)) {
this.deprecationLogger.logDeprecationForParameter(parameter);
return false;
}

return false;
return parameter.hasParameterAnnotation(ProjectedPayload.class)
|| AnnotatedElementUtils.findMergedAnnotation(type, ProjectedPayload.class) != null;
}

@Override
Expand All @@ -166,35 +125,4 @@ 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 %sat index %s in [%s] is not annotated with @ProjectedPayload. Make sure to annotate it with @ProjectedPayload (at the parameter or type level) to use it for projections.";

private final Set<MethodParameter> loggedParameters = Collections.synchronizedSet(new HashSet<>());

/**
* Log a warning the first time a non-annotated method parameter is encountered.
*
* @param parameter the parameter
*/
void logDeprecationForParameter(MethodParameter parameter) {

if (!this.loggedParameters.add(parameter)) {
return;
}

String paramName = parameter.getParameterName();
String paramNameOrEmpty = paramName != null ? ("'" + paramName + "' ") : "";
String methodName = parameter.getMethod() != null ? parameter.getMethod().toGenericString() : "constructor";

LOGGER.warn(() -> MESSAGE.formatted(paramNameOrEmpty, parameter.getParameterIndex(), methodName));
}

}

}
55 changes: 22 additions & 33 deletions ...est/java/org/springframework/data/web/ProxyingHandlerMethodArgumentResolverUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,20 @@

import java.lang.reflect.Method;
import java.util.List;
import java.util.function.Supplier;

import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.Appender;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.slf4j.LoggerFactory;

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;
Expand All @@ -44,6 +47,7 @@
* @author Oliver Gierke
* @author Chris Bono
* @author Mark Paluch
* @author hutiefang
* @soundtrack Karlijn Langendijk & Sönke Meinen - Englishman In New York (Sting,
* https://www.youtube.com/watch?v=O7LZsqrnaaA)
*/
Expand Down Expand Up @@ -132,39 +136,24 @@ void doesNotSupportAtProjectedPayloadForMultipartParam() {
assertThat(resolver.supportsParameter(parameter)).isFalse();
}

@ParameterizedTest // GH-3300
@ParameterizedTest // GH-3302
@ValueSource(strings = { "withModelAttribute", "withUserUnannotatedInterface" })
@SuppressWarnings("unchecked")
void deprecationLoggerOnlyLogsOncePerParameter(String methodName) {
void doesNotLogDeprecationForUnsupportedParameter(String methodName) {

var parameter = getParameter(methodName);

// Spy on the actual logger
var actualLoggerSpy = spy(new LogAccessor(ProxyingHandlerMethodArgumentResolver.class));
ReflectionTestUtils.setField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER", actualLoggerSpy,
LogAccessor.class);

// Invoke twice but should only log the first time
assertThat(resolver.supportsParameter(parameter)).isFalse();
verify(actualLoggerSpy, times(1)).warn(any(Supplier.class));
assertThat(resolver.supportsParameter(parameter)).isFalse();
verifyNoMoreInteractions(actualLoggerSpy);
}

@ParameterizedTest // GH-3300
@ValueSource(strings = { "withProjectedPayload", "withSpringAnnotatedInterface", "withUserAnnotatedInterface" })
void shouldNotLogDeprecationForValidUsage(String methodName) {

var parameter = getParameter(methodName);

// Spy on the actual logger
var actualLoggerSpy = spy(new LogAccessor(ProxyingHandlerMethodArgumentResolver.class));
ReflectionTestUtils.setField(ProxyingHandlerMethodArgumentResolver.class, "LOGGER", actualLoggerSpy,
LogAccessor.class);

// Invoke should not log
assertThat(resolver.supportsParameter(parameter)).isTrue();
verifyNoInteractions(actualLoggerSpy);
Appender<ILoggingEvent> appender = mock();
Logger logger = (Logger) LoggerFactory.getLogger(ProxyingHandlerMethodArgumentResolver.class);
Level previousLevel = logger.getLevel();
logger.addAppender(appender);
logger.setLevel(Level.WARN);

try {
assertThat(resolver.supportsParameter(parameter)).isFalse();
verify(appender, never()).doAppend(any(ILoggingEvent.class));
} finally {
logger.setLevel(previousLevel);
logger.detachAppender(appender);
}
}

private static MethodParameter getParameter(String methodName, Class<?> parameterType) {
Expand Down
Loading