From a34b1d3c6e5045f36a1738eaca234cf3c6bb0101 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 18 Oct 2022 00:45:05 -0700 Subject: [PATCH] Don't detect private constructors on member classes for binding Refine constructor binding detection logic so that `private` constructors on member classes are no longer automatically picked for constructor binding. This provides users a way of signalling that they wish to use the constructor directly. Closes gh-32639 --- .../PropertyDescriptorResolver.java | 22 +++++--- ...ationMetadataAnnotationProcessorTests.java | 8 +++ .../InnerClassWithPrivateConstructor.java | 56 +++++++++++++++++++ .../bind/DefaultBindConstructorProvider.java | 7 ++- .../DefaultBindConstructorProviderTests.java | 18 ++++++ 5 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/simple/InnerClassWithPrivateConstructor.java diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/PropertyDescriptorResolver.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/PropertyDescriptorResolver.java index bbde41cdd7d..0e4357983be 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/PropertyDescriptorResolver.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/PropertyDescriptorResolver.java @@ -24,6 +24,8 @@ import java.util.stream.Stream; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Modifier; +import javax.lang.model.element.NestingKind; import javax.lang.model.element.TypeElement; import javax.lang.model.element.VariableElement; import javax.lang.model.type.TypeMirror; @@ -186,24 +188,30 @@ class PropertyDescriptorResolver { static ConfigurationPropertiesTypeElement of(TypeElement type, MetadataGenerationEnvironment env) { List constructors = ElementFilter.constructorsIn(type.getEnclosedElements()); - List boundConstructors = getBoundConstructors(env, constructors); + List boundConstructors = getBoundConstructors(type, env, constructors); return new ConfigurationPropertiesTypeElement(type, constructors, boundConstructors); } - private static List getBoundConstructors(MetadataGenerationEnvironment env, + private static List getBoundConstructors(TypeElement type, MetadataGenerationEnvironment env, List constructors) { - ExecutableElement bindConstructor = deduceBindConstructor(constructors, env); + ExecutableElement bindConstructor = deduceBindConstructor(type, constructors, env); if (bindConstructor != null) { return Collections.singletonList(bindConstructor); } return constructors.stream().filter(env::hasConstructorBindingAnnotation).toList(); } - private static ExecutableElement deduceBindConstructor(List constructors, + private static ExecutableElement deduceBindConstructor(TypeElement type, List constructors, MetadataGenerationEnvironment env) { - if (constructors.size() == 1 && constructors.get(0).getParameters().size() > 0 - && !env.hasAutowiredAnnotation(constructors.get(0))) { - return constructors.get(0); + if (constructors.size() == 1) { + ExecutableElement candidate = constructors.get(0); + if (candidate.getParameters().size() > 0 && !env.hasAutowiredAnnotation(constructors.get(0))) { + if (type.getNestingKind() == NestingKind.MEMBER + && candidate.getModifiers().contains(Modifier.PRIVATE)) { + return null; + } + return candidate; + } } return null; } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java index a140709db05..f2263ed27fb 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/ConfigurationMetadataAnnotationProcessorTests.java @@ -31,6 +31,7 @@ import org.springframework.boot.configurationsample.simple.DescriptionProperties import org.springframework.boot.configurationsample.simple.HierarchicalProperties; import org.springframework.boot.configurationsample.simple.HierarchicalPropertiesGrandparent; import org.springframework.boot.configurationsample.simple.HierarchicalPropertiesParent; +import org.springframework.boot.configurationsample.simple.InnerClassWithPrivateConstructor; import org.springframework.boot.configurationsample.simple.NotAnnotated; import org.springframework.boot.configurationsample.simple.SimpleArrayProperties; import org.springframework.boot.configurationsample.simple.SimpleCollectionProperties; @@ -459,4 +460,11 @@ class ConfigurationMetadataAnnotationProcessorTests extends AbstractMetadataGene assertThat(metadata).doesNotHave(Metadata.withProperty("multi.some-integer")); } + @Test + void innerClassWithPrivateConstructor() { + ConfigurationMetadata metadata = compile(InnerClassWithPrivateConstructor.class); + assertThat(metadata).has(Metadata.withProperty("config.nested.name")); + assertThat(metadata).doesNotHave(Metadata.withProperty("config.nested.ignored")); + } + } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/simple/InnerClassWithPrivateConstructor.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/simple/InnerClassWithPrivateConstructor.java new file mode 100644 index 00000000000..474d7691b63 --- /dev/null +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationsample/simple/InnerClassWithPrivateConstructor.java @@ -0,0 +1,56 @@ +/* + * Copyright 2012-2022 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 + * + * https://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.simple; + +import org.springframework.boot.configurationsample.ConfigurationProperties; + +/** + * Nested properties with a private constructor. + * + * @author Phillip Webb + */ +@ConfigurationProperties(prefix = "config") +public class InnerClassWithPrivateConstructor { + + private Nested nested = new Nested("whatever"); + + public Nested getNested() { + return this.nested; + } + + public void setNested(Nested nested) { + this.nested = nested; + } + + public static final class Nested { + + private String name; + + private Nested(String ignored) { + } + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + + } + +} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java index 6d65256d284..c9bdb2755ba 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java @@ -80,7 +80,7 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { boolean hasAutowiredConstructor = isAutowiredPresent(candidateAnnotations); Constructor bind = getConstructorBindingAnnotated(type, candidates, candidateAnnotations); if (bind == null && !hasAutowiredConstructor) { - bind = deduceBindConstructor(candidates); + bind = deduceBindConstructor(type, candidates); } if (bind == null && !hasAutowiredConstructor && isKotlinType(type)) { bind = deduceKotlinBindConstructor(type); @@ -142,8 +142,11 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { } - private static Constructor deduceBindConstructor(Constructor[] candidates) { + private static Constructor deduceBindConstructor(Class type, Constructor[] candidates) { if (candidates.length == 1 && candidates[0].getParameterCount() > 0) { + if (type.isMemberClass() && Modifier.isPrivate(candidates[0].getModifiers())) { + return null; + } return candidates[0]; } Constructor result = null; diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java index e6d0e904515..5c646e3c409 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java @@ -90,6 +90,13 @@ class DefaultBindConstructorProviderTests { .withMessageContaining("has more than one @ConstructorBinding"); } + @Test + void getBindConstructorWhenIsMemberTypeWithPrivateConstructorReturnsNull() { + Constructor constructor = this.provider.getBindConstructor(MemberTypeWithPrivateConstructor.Member.class, + false); + assertThat(constructor).isNotNull(); + } + static class OnlyDefaultConstructor { } @@ -170,4 +177,15 @@ class DefaultBindConstructorProviderTests { } + static class MemberTypeWithPrivateConstructor { + + static final class Member { + + private Member(String name) { + } + + } + + } + }