From 0b326035b0c223a1a07a04e65cbdf6183a618ed6 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 30 Oct 2015 14:39:43 +0100 Subject: [PATCH] Fix detection of property setter Previously, the meta-data annotation processor was taking the first setter that match the property name it has to handle. Contrary to getters that are enforced by a return type (no argument), multiple setter candidates may exist. If a property's type got narrowed over time, the original setter may have been marked as Deprecated. As the annotation processor takes the first setter that matches based on the name only, it may pick up the deprecated one and therefore mark the property as being (wrongly) deprecatede in the meta-data. It turns out that checking for the actual type of the setter parameter brought a side effect: some primitive properties may use the primitive or the Wrapper counter part. This commit not only look at the proper setter based on the type but also fallback on the wrapper (or) primitive if necessary. Closes gh-4338 --- ...figurationMetadataAnnotationProcessor.java | 4 +- .../TypeElementMembers.java | 49 +++++++++++++-- .../configurationprocessor/TypeUtils.java | 37 +++++++++++- ...ationMetadataAnnotationProcessorTests.java | 30 +++++++++- .../ConfigurationMetadataMatchers.java | 8 +++ .../specific/BoxingPojo.java | 52 ++++++++++++++++ .../DeprecatedUnrelatedMethodPojo.java | 60 +++++++++++++++++++ 7 files changed, 229 insertions(+), 11 deletions(-) create mode 100644 spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/BoxingPojo.java create mode 100644 spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/DeprecatedUnrelatedMethodPojo.java diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessor.java b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessor.java index a0611b6214..2ee4e35f82 100644 --- a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessor.java +++ b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessor.java @@ -204,9 +204,9 @@ public class ConfigurationMetadataAnnotationProcessor extends AbstractProcessor .entrySet()) { String name = entry.getKey(); ExecutableElement getter = entry.getValue(); - ExecutableElement setter = members.getPublicSetters().get(name); - VariableElement field = members.getFields().get(name); TypeMirror returnType = getter.getReturnType(); + ExecutableElement setter = members.getPublicSetter(name, returnType); + VariableElement field = members.getFields().get(name); Element returnTypeElement = this.processingEnv.getTypeUtils() .asElement(returnType); boolean isExcluded = this.typeExcludeFilter.isExcluded(returnType); diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeElementMembers.java b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeElementMembers.java index 1e19d2d84c..303aeb7ff9 100644 --- a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeElementMembers.java +++ b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeElementMembers.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 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. @@ -16,8 +16,10 @@ package org.springframework.boot.configurationprocessor; +import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import javax.annotation.processing.ProcessingEnvironment; @@ -27,6 +29,7 @@ import javax.lang.model.element.Modifier; import javax.lang.model.element.TypeElement; import javax.lang.model.element.VariableElement; import javax.lang.model.type.TypeKind; +import javax.lang.model.type.TypeMirror; import javax.lang.model.util.ElementFilter; /** @@ -42,14 +45,18 @@ class TypeElementMembers { private final ProcessingEnvironment env; + private final TypeUtils typeUtils; + private final Map fields = new LinkedHashMap(); private final Map publicGetters = new LinkedHashMap(); - private final Map publicSetters = new LinkedHashMap(); + private final Map> publicSetters = + new LinkedHashMap>(); TypeElementMembers(ProcessingEnvironment env, TypeElement element) { this.env = env; + this.typeUtils = new TypeUtils(this.env); process(element); } @@ -75,10 +82,29 @@ class TypeElementMembers { if (isGetter(method) && !this.publicGetters.containsKey(name)) { this.publicGetters.put(getAccessorName(name), method); } - else if (isSetter(method) && !this.publicSetters.containsKey(name)) { - this.publicSetters.put(getAccessorName(name), method); + else if (isSetter(method)) { + String propertyName = getAccessorName(name); + List matchingSetters = this.publicSetters.get(propertyName); + if (matchingSetters == null) { + matchingSetters = new ArrayList(); + this.publicSetters.put(propertyName, matchingSetters); + } + TypeMirror paramType = method.getParameters().get(0).asType(); + if (getMatchingSetter(matchingSetters, paramType) == null) { + matchingSetters.add(method); + } + } + } + } + + private ExecutableElement getMatchingSetter(List candidates, TypeMirror type) { + for (ExecutableElement candidate : candidates) { + TypeMirror paramType = candidate.getParameters().get(0).asType(); + if (this.env.getTypeUtils().isSameType(paramType, type)) { + return candidate; } } + return null; } private boolean isGetter(ExecutableElement method) { @@ -122,8 +148,19 @@ class TypeElementMembers { return Collections.unmodifiableMap(this.publicGetters); } - public Map getPublicSetters() { - return Collections.unmodifiableMap(this.publicSetters); + public ExecutableElement getPublicSetter(String name, TypeMirror type) { + List candidates = this.publicSetters.get(name); + if (candidates != null) { + ExecutableElement matching = getMatchingSetter(candidates, type); + if (matching != null) { + return matching; + } + TypeMirror alternative = this.typeUtils.getWrapperOrPrimitiveFor(type); + if (alternative != null) { + return getMatchingSetter(candidates, alternative); + } + } + return null; } } diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeUtils.java b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeUtils.java index b5a1e35852..12529b76b6 100644 --- a/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeUtils.java +++ b/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/TypeUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 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. @@ -40,6 +40,7 @@ import javax.lang.model.util.Types; class TypeUtils { private static final Map> PRIMITIVE_WRAPPERS; + private static final Map WRAPPER_TO_PRIMITIVE; static { Map> wrappers = new HashMap>(); @@ -52,6 +53,17 @@ class TypeUtils { wrappers.put(TypeKind.LONG, Long.class); wrappers.put(TypeKind.SHORT, Short.class); PRIMITIVE_WRAPPERS = Collections.unmodifiableMap(wrappers); + + Map primitives = new HashMap(); + primitives.put(Boolean.class.getName(), TypeKind.BOOLEAN); + primitives.put(Byte.class.getName(), TypeKind.BYTE); + primitives.put(Character.class.getName(), TypeKind.CHAR); + primitives.put(Double.class.getName(), TypeKind.DOUBLE); + primitives.put(Float.class.getName(), TypeKind.FLOAT); + primitives.put(Integer.class.getName(), TypeKind.INT); + primitives.put(Long.class.getName(), TypeKind.LONG); + primitives.put(Short.class.getName(), TypeKind.SHORT); + WRAPPER_TO_PRIMITIVE = primitives; } private final ProcessingEnvironment env; @@ -79,7 +91,7 @@ class TypeUtils { if (type == null) { return null; } - Class wrapper = PRIMITIVE_WRAPPERS.get(type.getKind()); + Class wrapper = getWrapperFor(type); if (wrapper != null) { return wrapper.getName(); } @@ -99,6 +111,7 @@ class TypeUtils { || this.env.getTypeUtils().isAssignable(type, this.mapType); } + public boolean isEnclosedIn(Element candidate, TypeElement element) { if (candidate == null || element == null) { return false; @@ -118,4 +131,24 @@ class TypeUtils { return ("".equals(javadoc) ? null : javadoc); } + public TypeMirror getWrapperOrPrimitiveFor(TypeMirror typeMirror) { + Class candidate = getWrapperFor(typeMirror); + if (candidate != null) { + return this.env.getElementUtils().getTypeElement(candidate.getName()).asType(); + } + TypeKind primitiveKind = getPrimitiveFor(typeMirror); + if (primitiveKind != null) { + return this.env.getTypeUtils().getPrimitiveType(primitiveKind); + } + return null; + } + + private Class getWrapperFor(TypeMirror type) { + return PRIMITIVE_WRAPPERS.get(type.getKind()); + } + + private TypeKind getPrimitiveFor(TypeMirror type) { + return WRAPPER_TO_PRIMITIVE.get(type.toString()); + } + } diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java index dc6d510e45..6016490188 100644 --- a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java +++ b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java @@ -51,7 +51,9 @@ import org.springframework.boot.configurationsample.simple.SimpleCollectionPrope import org.springframework.boot.configurationsample.simple.SimplePrefixValueProperties; import org.springframework.boot.configurationsample.simple.SimpleProperties; import org.springframework.boot.configurationsample.simple.SimpleTypeProperties; +import org.springframework.boot.configurationsample.specific.BoxingPojo; import org.springframework.boot.configurationsample.specific.BuilderPojo; +import org.springframework.boot.configurationsample.specific.DeprecatedUnrelatedMethodPojo; import org.springframework.boot.configurationsample.specific.ExcludedTypesPojo; import org.springframework.boot.configurationsample.specific.InnerClassAnnotatedGetterConfig; import org.springframework.boot.configurationsample.specific.InnerClassProperties; @@ -197,6 +199,30 @@ public class ConfigurationMetadataAnnotationProcessorTests { .withDeprecation("renamed", "singledeprecated.new-name")); } + @Test + public void deprecatedOnUnrelatedSetter() throws Exception { + Class type = DeprecatedUnrelatedMethodPojo.class; + ConfigurationMetadata metadata = compile(type); + assertThat(metadata, containsGroup("not.deprecated").fromSource(type)); + assertThat(metadata, containsProperty("not.deprecated.counter", Integer.class) + .withNoDeprecation() + .fromSource(type)); + assertThat(metadata, containsProperty("not.deprecated.flag", Boolean.class) + .withNoDeprecation() + .fromSource(type)); + } + + @Test + public void boxingOnSetter() throws IOException { + Class type = BoxingPojo.class; + ConfigurationMetadata metadata = compile(type); + assertThat(metadata, containsGroup("boxing").fromSource(type)); + assertThat(metadata, containsProperty("boxing.flag", Boolean.class) + .fromSource(type)); + assertThat(metadata, containsProperty("boxing.counter", Integer.class) + .fromSource(type)); + } + @Test public void parseCollectionConfig() throws Exception { ConfigurationMetadata metadata = compile(SimpleCollectionProperties.class); @@ -343,7 +369,9 @@ public class ConfigurationMetadataAnnotationProcessorTests { assertThat(metadata, containsProperty("simple.flag", Boolean.class) .fromSource(SimpleProperties.class) - .withDescription("A simple flag.").withDefaultValue(is(true))); + .withDescription("A simple flag.") + .withDeprecation(null, null) + .withDefaultValue(is(true))); assertThat(metadata.getItems().size(), is(4)); } diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataMatchers.java b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataMatchers.java index 95c7c7a781..d630059a44 100644 --- a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataMatchers.java +++ b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataMatchers.java @@ -125,6 +125,9 @@ public final class ConfigurationMetadataMatchers { && !this.description.equals(itemMetadata.getDescription())) { return false; } + if (this.deprecation == null && itemMetadata.getDeprecation() != null) { + return false; + } if (this.deprecation != null && !this.deprecation.equals(itemMetadata.getDeprecation())) { return false; @@ -200,6 +203,11 @@ public final class ConfigurationMetadataMatchers { new ItemDeprecation(reason, replacement)); } + public ContainsItemMatcher withNoDeprecation() { + return new ContainsItemMatcher(this.itemType, this.name, this.type, + this.sourceType, this.description, this.defaultValue, null); + } + private ItemMetadata getFirstItemWithName(ConfigurationMetadata metadata, String name) { for (ItemMetadata item : metadata.getItems()) { diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/BoxingPojo.java b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/BoxingPojo.java new file mode 100644 index 0000000000..a120c47734 --- /dev/null +++ b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/BoxingPojo.java @@ -0,0 +1,52 @@ +/* + * Copyright 2012-2015 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.configurationsample.specific; + +import org.springframework.boot.configurationsample.ConfigurationProperties; + +/** + * Demonstrate the use of boxing/unboxing. Even if the type does not + * strictly match, it should still be detected. + * + * @author Stephane Nicoll + */ +@ConfigurationProperties("boxing") +public class BoxingPojo { + + private boolean flag; + + private Integer counter; + + public boolean isFlag() { + return this.flag; + } + + // Setter use Boolean + public void setFlag(Boolean flag) { + this.flag = flag; + } + + public Integer getCounter() { + return this.counter; + } + + // Setter use int + public void setCounter(int counter) { + this.counter = counter; + } + +} diff --git a/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/DeprecatedUnrelatedMethodPojo.java b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/DeprecatedUnrelatedMethodPojo.java new file mode 100644 index 0000000000..798115b722 --- /dev/null +++ b/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/specific/DeprecatedUnrelatedMethodPojo.java @@ -0,0 +1,60 @@ +/* + * Copyright 2012-2015 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.configurationsample.specific; + +import org.springframework.boot.configurationsample.ConfigurationProperties; + +/** + * Demonstrate that an unrelated setter is not taken into account + * to detect the deprecated flag. + * + * @author Stephane Nicoll + */ +@ConfigurationProperties("not.deprecated") +public class DeprecatedUnrelatedMethodPojo { + + private Integer counter; + + private boolean flag; + + public Integer getCounter() { + return this.counter; + } + + public void setCounter(Integer counter) { + this.counter = counter; + } + + @Deprecated + public void setCounter(String counterAsString) { + this.counter = Integer.valueOf(counterAsString); + } + + public boolean isFlag() { + return this.flag; + } + + public void setFlag(boolean flag) { + this.flag = flag; + } + + @Deprecated + public void setFlag(Boolean flag) { + this.flag = flag; + } + +}