From 62a3e414734b24d1801b34223df5faaae9e32b5f Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 30 Jul 2019 15:22:05 +0100 Subject: [PATCH] Deprecate mutable methods of MethodParameter Deprecate all mutation methods in `MethodParameter` in favor of factory methods that return a new instance. Existing code that previously relied on mutation has been updated to use the replacement methods. Closes gh-23385 --- .../core/GenericTypeResolver.java | 5 +- .../springframework/core/MethodParameter.java | 127 ++++++++++++++---- .../springframework/core/ResolvableType.java | 38 +++--- .../core/convert/Property.java | 13 +- .../core/MethodParameterTests.java | 62 ++++++++- .../messaging/handler/HandlerMethod.java | 5 +- .../web/method/HandlerMethod.java | 5 +- 7 files changed, 187 insertions(+), 68 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/GenericTypeResolver.java b/spring-core/src/main/java/org/springframework/core/GenericTypeResolver.java index 55c7aab189..55a0f60b31 100644 --- a/spring-core/src/main/java/org/springframework/core/GenericTypeResolver.java +++ b/spring-core/src/main/java/org/springframework/core/GenericTypeResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -57,12 +57,13 @@ public final class GenericTypeResolver { * @param methodParameter the method parameter specification * @param implementationClass the class to resolve type variables against * @return the corresponding generic parameter or return type + * @deprecated since 5.2 in favor of {@code methodParameter.withContainingClass(implementationClass).getParameterType()} */ + @Deprecated public static Class resolveParameterType(MethodParameter methodParameter, Class implementationClass) { Assert.notNull(methodParameter, "MethodParameter must not be null"); Assert.notNull(implementationClass, "Class must not be null"); methodParameter.setContainingClass(implementationClass); - ResolvableType.resolveMethodParameter(methodParameter); return methodParameter.getParameterType(); } diff --git a/spring-core/src/main/java/org/springframework/core/MethodParameter.java b/spring-core/src/main/java/org/springframework/core/MethodParameter.java index b8eba8a3ef..d6b6be0471 100644 --- a/spring-core/src/main/java/org/springframework/core/MethodParameter.java +++ b/spring-core/src/main/java/org/springframework/core/MethodParameter.java @@ -55,6 +55,7 @@ import org.springframework.util.ObjectUtils; * @author Andy Clement * @author Sam Brannen * @author Sebastien Deleuze + * @author Phillip Webb * @since 2.0 * @see org.springframework.core.annotation.SynthesizingMethodParameter */ @@ -76,6 +77,7 @@ public class MethodParameter { @Nullable Map typeIndexesPerLevel; + /** The containing class. Could also be supplied by overriding {@link #getContainingClass()} */ @Nullable private volatile Class containingClass; @@ -150,6 +152,24 @@ public class MethodParameter { this.nestingLevel = nestingLevel; } + /** + * Internal constructor used to create a {@link MethodParameter} with a + * containing class already set. + * @param executable the Executable to specify a parameter for + * @param parameterIndex the index of the parameter + * @param containingClass the containing class + * @since 5.2 + */ + MethodParameter(Executable executable, int parameterIndex, + @Nullable Class containingClass) { + + Assert.notNull(executable, "Executable must not be null"); + this.executable = executable; + this.parameterIndex = validateIndex(executable, parameterIndex); + this.nestingLevel = 1; + this.containingClass = containingClass; + } + /** * Copy constructor, resulting in an independent MethodParameter object * based on the same metadata and cache state that the original object was in. @@ -252,7 +272,9 @@ public class MethodParameter { /** * Increase this parameter's nesting level. * @see #getNestingLevel() + * @deprecated since 5.2 in favor of {@link #nested(Integer)} */ + @Deprecated public void increaseNestingLevel() { this.nestingLevel++; } @@ -260,7 +282,10 @@ public class MethodParameter { /** * Decrease this parameter's nesting level. * @see #getNestingLevel() + * @deprecated since 5.2 in favor of retaining the original MethodParameter and + * using {@link #nested(Integer)} if nesting is required */ + @Deprecated public void decreaseNestingLevel() { getTypeIndexesPerLevel().remove(this.nestingLevel); this.nestingLevel--; @@ -280,11 +305,22 @@ public class MethodParameter { * @param typeIndex the corresponding type index * (or {@code null} for the default type index) * @see #getNestingLevel() + * @deprecated since 5.2 in favor of {@link #withTypeIndex} */ + @Deprecated public void setTypeIndexForCurrentLevel(int typeIndex) { getTypeIndexesPerLevel().put(this.nestingLevel, typeIndex); } + /** + * Return a variant of this {@code MethodParameter} with the type for the current + * level set to the specified value. + * @param typeIndex the new type index + */ + public MethodParameter withTypeIndex(int typeIndex) { + return nested(this.nestingLevel, typeIndex); + } + /** * Return the type index for the current nesting level. * @return the corresponding type index, or {@code null} @@ -319,22 +355,45 @@ public class MethodParameter { /** * Return a variant of this {@code MethodParameter} which points to the - * same parameter but one nesting level deeper. This is effectively the - * same as {@link #increaseNestingLevel()}, just with an independent - * {@code MethodParameter} object (e.g. in case of the original being cached). + * same parameter but one nesting level deeper. * @since 4.3 */ public MethodParameter nested() { + return nested(null); + } + + /** + * Return a variant of this {@code MethodParameter} which points to the + * same parameter but one nesting level deeper. + * @param typeIndex the type index for the new nesting level + * @since 5.2 + */ + public MethodParameter nested(@Nullable Integer typeIndex) { MethodParameter nestedParam = this.nestedMethodParameter; - if (nestedParam != null) { + if (nestedParam != null && typeIndex == null) { return nestedParam; } - nestedParam = clone(); - nestedParam.nestingLevel = this.nestingLevel + 1; - this.nestedMethodParameter = nestedParam; + nestedParam = nested(this.nestingLevel + 1, typeIndex); + if (typeIndex == null) { + this.nestedMethodParameter = nestedParam; + } return nestedParam; } + private MethodParameter nested(int nestingLevel, @Nullable Integer typeIndex) { + MethodParameter copy = clone(); + copy.nestingLevel = nestingLevel; + if (this.typeIndexesPerLevel != null) { + copy.typeIndexesPerLevel = new HashMap<>(this.typeIndexesPerLevel); + } + if (typeIndex != null) { + copy.getTypeIndexesPerLevel().put(copy.nestingLevel, typeIndex); + } + copy.parameterType = null; + copy.genericParameterType = null; + return copy; + } + /** * Return whether this method indicates a parameter which is not required: * either in the form of Java 8's {@link java.util.Optional}, any variant @@ -376,21 +435,28 @@ public class MethodParameter { return (getParameterType() == Optional.class ? nested() : this); } - /** - * Set a containing class to resolve the parameter type against. - */ + public Class getContainingClass() { + Class containingClass = this.containingClass; + return (containingClass != null ? containingClass : getDeclaringClass()); + } + + @Deprecated void setContainingClass(Class containingClass) { this.containingClass = containingClass; + this.parameterType = null; } - public Class getContainingClass() { - Class containingClass = this.containingClass; - return (containingClass != null ? containingClass : getDeclaringClass()); + public MethodParameter withContainingClass(@Nullable Class containingClass) { + MethodParameter result = clone(); + result.containingClass = containingClass; + result.parameterType = null; + return result; } /** * Set a resolved (generic) parameter type. */ + @Deprecated void setParameterType(@Nullable Class parameterType) { this.parameterType = parameterType; } @@ -401,18 +467,16 @@ public class MethodParameter { */ public Class getParameterType() { Class paramType = this.parameterType; + if (paramType != null) { + return paramType; + } + if (getContainingClass() != null) { + paramType = ResolvableType.forMethodParameter(this, null, 1, null).resolve(); + } if (paramType == null) { - if (this.parameterIndex < 0) { - Method method = getMethod(); - paramType = (method != null ? - (KotlinDetector.isKotlinReflectPresent() && KotlinDetector.isKotlinType(getContainingClass()) ? - KotlinDelegate.getReturnType(method) : method.getReturnType()) : void.class); - } - else { - paramType = this.executable.getParameterTypes()[this.parameterIndex]; - } - this.parameterType = paramType; + paramType = computeParameterType(); } + this.parameterType = paramType; return paramType; } @@ -442,13 +506,27 @@ public class MethodParameter { index = this.parameterIndex - 1; } paramType = (index >= 0 && index < genericParameterTypes.length ? - genericParameterTypes[index] : getParameterType()); + genericParameterTypes[index] : computeParameterType()); } this.genericParameterType = paramType; } return paramType; } + private Class computeParameterType() { + if (this.parameterIndex < 0) { + Method method = getMethod(); + if (method == null) { + return void.class; + } + if (KotlinDetector.isKotlinReflectPresent() && KotlinDetector.isKotlinType(getContainingClass())) { + return KotlinDelegate.getReturnType(method); + } + return method.getReturnType(); + } + return this.executable.getParameterTypes()[this.parameterIndex]; + } + /** * Return the nested type of the method/constructor parameter. * @return the parameter type (never {@code null}) @@ -688,7 +766,6 @@ public class MethodParameter { return new MethodParameter(this); } - /** * Create a new MethodParameter for the given method or constructor. *

This is a convenience factory method for scenarios where a diff --git a/spring-core/src/main/java/org/springframework/core/ResolvableType.java b/spring-core/src/main/java/org/springframework/core/ResolvableType.java index 18f8fd45b8..055b81ee22 100644 --- a/spring-core/src/main/java/org/springframework/core/ResolvableType.java +++ b/spring-core/src/main/java/org/springframework/core/ResolvableType.java @@ -1200,8 +1200,7 @@ public class ResolvableType implements Serializable { Class implementationClass) { Assert.notNull(constructor, "Constructor must not be null"); - MethodParameter methodParameter = new MethodParameter(constructor, parameterIndex); - methodParameter.setContainingClass(implementationClass); + MethodParameter methodParameter = new MethodParameter(constructor, parameterIndex, implementationClass); return forMethodParameter(methodParameter); } @@ -1227,8 +1226,7 @@ public class ResolvableType implements Serializable { */ public static ResolvableType forMethodReturnType(Method method, Class implementationClass) { Assert.notNull(method, "Method must not be null"); - MethodParameter methodParameter = new MethodParameter(method, -1); - methodParameter.setContainingClass(implementationClass); + MethodParameter methodParameter = new MethodParameter(method, -1, implementationClass); return forMethodParameter(methodParameter); } @@ -1258,8 +1256,7 @@ public class ResolvableType implements Serializable { */ public static ResolvableType forMethodParameter(Method method, int parameterIndex, Class implementationClass) { Assert.notNull(method, "Method must not be null"); - MethodParameter methodParameter = new MethodParameter(method, parameterIndex); - methodParameter.setContainingClass(implementationClass); + MethodParameter methodParameter = new MethodParameter(method, parameterIndex, implementationClass); return forMethodParameter(methodParameter); } @@ -1303,22 +1300,29 @@ public class ResolvableType implements Serializable { */ public static ResolvableType forMethodParameter(MethodParameter methodParameter, @Nullable Type targetType) { Assert.notNull(methodParameter, "MethodParameter must not be null"); - ResolvableType owner = forType(methodParameter.getContainingClass()).as(methodParameter.getDeclaringClass()); - return forType(targetType, new MethodParameterTypeProvider(methodParameter), owner.asVariableResolver()). - getNested(methodParameter.getNestingLevel(), methodParameter.typeIndexesPerLevel); + int nestingLevel = methodParameter.getNestingLevel(); + Map typeIndexesPerLevel = methodParameter.typeIndexesPerLevel; + return forMethodParameter(methodParameter, targetType, nestingLevel, + typeIndexesPerLevel); } /** - * Resolve the top-level parameter type of the given {@code MethodParameter}. - * @param methodParameter the method parameter to resolve - * @since 4.1.9 - * @see MethodParameter#setParameterType + * Return a {@link ResolvableType} for the specified {@link MethodParameter} at + * a specific nesting level, overriding the target type to resolve with a specific + * given type. + * @param methodParameter the source method parameter (must not be {@code null}) + * @param targetType the type to resolve (a part of the method parameter's type) + * @param nestingLevel the nesting level to use + * @param typeIndexesPerLevel the type indexes per nesting level + * @return a {@link ResolvableType} for the specified method parameter + * @since 5.2 + * @see #forMethodParameter(Method, int) */ - static void resolveMethodParameter(MethodParameter methodParameter) { - Assert.notNull(methodParameter, "MethodParameter must not be null"); + static ResolvableType forMethodParameter(MethodParameter methodParameter, @Nullable Type targetType, + int nestingLevel, @Nullable Map typeIndexesPerLevel) { ResolvableType owner = forType(methodParameter.getContainingClass()).as(methodParameter.getDeclaringClass()); - methodParameter.setParameterType( - forType(null, new MethodParameterTypeProvider(methodParameter), owner.asVariableResolver()).resolve()); + return forType(targetType, new MethodParameterTypeProvider(methodParameter), owner.asVariableResolver()). + getNested(nestingLevel, typeIndexesPerLevel); } /** diff --git a/spring-core/src/main/java/org/springframework/core/convert/Property.java b/spring-core/src/main/java/org/springframework/core/convert/Property.java index 134d16eaf7..810987917d 100644 --- a/spring-core/src/main/java/org/springframework/core/convert/Property.java +++ b/spring-core/src/main/java/org/springframework/core/convert/Property.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,7 +23,6 @@ import java.lang.reflect.Method; import java.util.LinkedHashMap; import java.util.Map; -import org.springframework.core.GenericTypeResolver; import org.springframework.core.MethodParameter; import org.springframework.lang.Nullable; import org.springframework.util.ConcurrentReferenceHashMap; @@ -187,7 +186,7 @@ public final class Property { if (getReadMethod() == null) { return null; } - return resolveParameterType(new MethodParameter(getReadMethod(), -1)); + return new MethodParameter(getReadMethod(), -1).withContainingClass(getObjectType()); } @Nullable @@ -195,13 +194,7 @@ public final class Property { if (getWriteMethod() == null) { return null; } - return resolveParameterType(new MethodParameter(getWriteMethod(), 0)); - } - - private MethodParameter resolveParameterType(MethodParameter parameter) { - // needed to resolve generic property types that parameterized by sub-classes e.g. T getFoo(); - GenericTypeResolver.resolveParameterType(parameter, getObjectType()); - return parameter; + return new MethodParameter(getWriteMethod(), 0).withContainingClass(getObjectType()); } private Annotation[] resolveAnnotations() { diff --git a/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java b/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java index 1ddd5d35d5..12beb094fa 100644 --- a/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java +++ b/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java @@ -32,9 +32,12 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** + * Tests for {@link MethodParameter}. + * * @author Arjen Poutsma * @author Juergen Hoeller * @author Sam Brannen + * @author Phillip Webb */ public class MethodParameterTests { @@ -151,6 +154,7 @@ public class MethodParameterTests { } @Test + @Deprecated public void multipleResolveParameterTypeCalls() throws Exception { Method method = ArrayList.class.getMethod("get", int.class); MethodParameter methodParameter = MethodParameter.forExecutable(method, -1); @@ -174,17 +178,63 @@ public class MethodParameterTests { @Test public void equalsAndHashCodeConsidersNesting() throws Exception { Method method = ArrayList.class.getMethod("get", int.class); - MethodParameter m1 = MethodParameter.forExecutable(method, -1); - GenericTypeResolver.resolveParameterType(m1, StringList.class); - MethodParameter m2 = MethodParameter.forExecutable(method, -1); - GenericTypeResolver.resolveParameterType(m2, StringList.class); - MethodParameter m3 = MethodParameter.forExecutable(method, -1); - GenericTypeResolver.resolveParameterType(m3, IntegerList.class); + MethodParameter m1 = MethodParameter.forExecutable(method, -1) + .withContainingClass(StringList.class); + MethodParameter m2 = MethodParameter.forExecutable(method, -1) + .withContainingClass(StringList.class); + MethodParameter m3 = MethodParameter.forExecutable(method, -1) + .withContainingClass(IntegerList.class); MethodParameter m4 = MethodParameter.forExecutable(method, -1); assertThat(m1).isEqualTo(m2).isNotEqualTo(m3).isNotEqualTo(m4); assertThat(m1.hashCode()).isEqualTo(m2.hashCode()); } + public void withContainingClassReturnsNewInstance() throws Exception { + Method method = ArrayList.class.getMethod("get", int.class); + MethodParameter m1 = MethodParameter.forExecutable(method, -1); + MethodParameter m2 = m1.withContainingClass(StringList.class); + MethodParameter m3 = m1.withContainingClass(IntegerList.class); + assertThat(m1).isNotSameAs(m2).isNotSameAs(m3); + assertThat(m1.getParameterType()).isEqualTo(Object.class); + assertThat(m2.getParameterType()).isEqualTo(String.class); + assertThat(m3.getParameterType()).isEqualTo(Integer.class); + } + + @Test + public void withTypeIndexReturnsNewInstance() throws Exception { + Method method = ArrayList.class.getMethod("get", int.class); + MethodParameter m1 = MethodParameter.forExecutable(method, -1); + MethodParameter m2 = m1.withTypeIndex(2); + MethodParameter m3 = m1.withTypeIndex(3); + assertThat(m1).isNotSameAs(m2).isNotSameAs(m3); + assertThat(m1.getTypeIndexForCurrentLevel()).isNull(); + assertThat(m2.getTypeIndexForCurrentLevel()).isEqualTo(2); + assertThat(m3.getTypeIndexForCurrentLevel()).isEqualTo(3); + } + + @Test + @SuppressWarnings("deprecation") + public void mutatingNestingLevelShouldNotChangeNewInstance() throws Exception { + Method method = ArrayList.class.getMethod("get", int.class); + MethodParameter m1 = MethodParameter.forExecutable(method, -1); + MethodParameter m2 = m1.withTypeIndex(2); + assertThat(m2.getTypeIndexForCurrentLevel()).isEqualTo(2); + m1.setTypeIndexForCurrentLevel(1); + m2.decreaseNestingLevel(); + assertThat(m2.getTypeIndexForCurrentLevel()).isNull(); + } + + @Test + public void nestedWithTypeIndexReturnsNewInstance() throws Exception { + Method method = ArrayList.class.getMethod("get", int.class); + MethodParameter m1 = MethodParameter.forExecutable(method, -1); + MethodParameter m2 = m1.nested(2); + MethodParameter m3 = m1.nested(3); + assertThat(m1).isNotSameAs(m2).isNotSameAs(m3); + assertThat(m1.getTypeIndexForCurrentLevel()).isNull(); + assertThat(m2.getTypeIndexForCurrentLevel()).isEqualTo(2); + assertThat(m3.getTypeIndexForCurrentLevel()).isEqualTo(3); + } public int method(String p1, long p2) { return 42; diff --git a/spring-messaging/src/main/java/org/springframework/messaging/handler/HandlerMethod.java b/spring-messaging/src/main/java/org/springframework/messaging/handler/HandlerMethod.java index 6294c6dd9a..496266b108 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/handler/HandlerMethod.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/handler/HandlerMethod.java @@ -26,7 +26,6 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.core.BridgeMethodResolver; -import org.springframework.core.GenericTypeResolver; import org.springframework.core.MethodParameter; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.annotation.SynthesizingMethodParameter; @@ -161,9 +160,7 @@ public class HandlerMethod { int count = this.bridgedMethod.getParameterCount(); MethodParameter[] result = new MethodParameter[count]; for (int i = 0; i < count; i++) { - HandlerMethodParameter parameter = new HandlerMethodParameter(i); - GenericTypeResolver.resolveParameterType(parameter, this.beanType); - result[i] = parameter; + result[i] = new HandlerMethodParameter(i); } return result; } diff --git a/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java b/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java index 938afbae31..74aea1a986 100644 --- a/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java +++ b/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java @@ -30,7 +30,6 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.core.BridgeMethodResolver; -import org.springframework.core.GenericTypeResolver; import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.core.annotation.AnnotatedElementUtils; @@ -188,9 +187,7 @@ public class HandlerMethod { int count = this.bridgedMethod.getParameterCount(); MethodParameter[] result = new MethodParameter[count]; for (int i = 0; i < count; i++) { - HandlerMethodParameter parameter = new HandlerMethodParameter(i); - GenericTypeResolver.resolveParameterType(parameter, this.beanType); - result[i] = parameter; + result[i] = new HandlerMethodParameter(i); } return result; }