From edf82325557f05f482b0b9c0333d99908c8f1378 Mon Sep 17 00:00:00 2001 From: sdeleuze Date: Mon, 13 Nov 2017 12:04:39 +0100 Subject: [PATCH] Avoid implicit autowiring with Kotlin secondary ctors Autowiring implicitely Kotlin primary constructors when there are secondary constructors has side effects on ConstructorResolver. It seems reasonable to require explicit @Autowired annotation in such case. With this commit, implicit autowiring of Kotlin primary constructors is only performed when there is a primary constructor defined alone or with a default constructor (define explicitly or generated via the kotlin-noarg compiler plugin or via optional constructor parameters with default values). Issue: SPR-16022 --- .../AutowiredAnnotationBeanPostProcessor.java | 15 ++-- .../annotation/KotlinAutowiredTests.kt | 70 +++++++++++++++++-- .../context/annotation/Spr16022Tests.kt | 59 ++++++++++++++++ .../annotation/multipleConstructors.xml | 19 +++++ 4 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 spring-context/src/test/kotlin/org/springframework/context/annotation/Spr16022Tests.kt create mode 100644 spring-context/src/test/resources/org/springframework/context/annotation/multipleConstructors.xml diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java index e2ed0eca8c3..d3810f4e8bb 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java @@ -284,8 +284,12 @@ public class AutowiredAnnotationBeanPostProcessor extends InstantiationAwareBean Constructor requiredConstructor = null; Constructor defaultConstructor = null; Constructor primaryConstructor = BeanUtils.findPrimaryConstructor(beanClass); + int nonSyntheticConstructors = 0; for (Constructor candidate : rawCandidates) { - if (primaryConstructor != null && candidate.isSynthetic()) { + if (!candidate.isSynthetic()) { + nonSyntheticConstructors++; + } + else if (primaryConstructor != null) { continue; } AnnotationAttributes ann = findAutowiredAnnotation(candidate); @@ -343,10 +347,11 @@ public class AutowiredAnnotationBeanPostProcessor extends InstantiationAwareBean else if (rawCandidates.length == 1 && rawCandidates[0].getParameterCount() > 0) { candidateConstructors = new Constructor[] {rawCandidates[0]}; } - else if (primaryConstructor != null) { - candidateConstructors = (defaultConstructor != null ? - new Constructor[] {primaryConstructor, defaultConstructor} : - new Constructor[] {primaryConstructor}); + else if (nonSyntheticConstructors == 2 && primaryConstructor != null && defaultConstructor != null) { + candidateConstructors = new Constructor[] {primaryConstructor, defaultConstructor}; + } + else if (nonSyntheticConstructors == 1 && primaryConstructor != null) { + candidateConstructors = new Constructor[] {primaryConstructor}; } else { candidateConstructors = new Constructor[0]; diff --git a/spring-beans/src/test/kotlin/org/springframework/beans/factory/annotation/KotlinAutowiredTests.kt b/spring-beans/src/test/kotlin/org/springframework/beans/factory/annotation/KotlinAutowiredTests.kt index b8e05974fc2..07c541e25b0 100644 --- a/spring-beans/src/test/kotlin/org/springframework/beans/factory/annotation/KotlinAutowiredTests.kt +++ b/spring-beans/src/test/kotlin/org/springframework/beans/factory/annotation/KotlinAutowiredTests.kt @@ -23,6 +23,7 @@ import org.springframework.beans.factory.support.RootBeanDefinition import org.springframework.tests.sample.beans.TestBean import org.junit.Assert.* +import org.springframework.beans.factory.BeanCreationException import org.springframework.tests.sample.beans.Colour /** @@ -68,23 +69,39 @@ class KotlinAutowiredTests { } @Test // SPR-15847 - fun `Autowiring by primary constructor with optional parameter`() { + fun `Autowiring by primary constructor with mandatory and optional parameters`() { val bf = DefaultListableBeanFactory() val bpp = AutowiredAnnotationBeanPostProcessor() bpp.setBeanFactory(bf) bf.addBeanPostProcessor(bpp) - val bd = RootBeanDefinition(KotlinBeanWithOptionalParameter::class.java) + val bd = RootBeanDefinition(KotlinBeanWithMandatoryAndOptionalParameters::class.java) bd.scope = RootBeanDefinition.SCOPE_PROTOTYPE bf.registerBeanDefinition("bean", bd) val tb = TestBean() bf.registerSingleton("testBean", tb) - val kb = bf.getBean("bean", KotlinBeanWithOptionalParameter::class.java) + val kb = bf.getBean("bean", KotlinBeanWithMandatoryAndOptionalParameters::class.java) assertSame(tb, kb.injectedFromConstructor) assertEquals("foo", kb.optional) assertEquals("bar", kb.initializedField) } + @Test + fun `Autowiring by primary constructor with optional parameters`() { + val bf = DefaultListableBeanFactory() + val bpp = AutowiredAnnotationBeanPostProcessor() + bpp.setBeanFactory(bf) + bf.addBeanPostProcessor(bpp) + val bd = RootBeanDefinition(KotlinBeanWithOptionalParameters::class.java) + bd.scope = RootBeanDefinition.SCOPE_PROTOTYPE + bf.registerBeanDefinition("bean", bd) + + val kb = bf.getBean("bean", KotlinBeanWithOptionalParameters::class.java) + assertNotNull(kb.optional1) + assertEquals("foo", kb.optional2) + assertEquals("bar", kb.initializedField) + } + @Test // SPR-15847 fun `Autowiring by annotated primary constructor with optional parameter`() { val bf = DefaultListableBeanFactory() @@ -108,7 +125,7 @@ class KotlinAutowiredTests { val bpp = AutowiredAnnotationBeanPostProcessor() bpp.setBeanFactory(bf) bf.addBeanPostProcessor(bpp) - val bd = RootBeanDefinition(KotlinBeanWithSecondaryConstructor::class.java) + val bd = RootBeanDefinition(KotlinBeanWithAutowiredSecondaryConstructor::class.java) bd.scope = RootBeanDefinition.SCOPE_PROTOTYPE bf.registerBeanDefinition("bean", bd) val tb = TestBean() @@ -116,7 +133,7 @@ class KotlinAutowiredTests { val colour = Colour.BLUE bf.registerSingleton("colour", colour) - val kb = bf.getBean("bean", KotlinBeanWithSecondaryConstructor::class.java) + val kb = bf.getBean("bean", KotlinBeanWithAutowiredSecondaryConstructor::class.java) assertSame(tb, kb.injectedFromConstructor) assertEquals("bar", kb.optional) assertSame(colour, kb.injectedFromSecondaryConstructor) @@ -152,6 +169,23 @@ class KotlinAutowiredTests { assertEquals(tb, kb.testBean) } + @Test(expected = BeanCreationException::class) // SPR-16022 + fun `No autowiring with primary and secondary non annotated constructors`() { + val bf = DefaultListableBeanFactory() + val bpp = AutowiredAnnotationBeanPostProcessor() + bpp.setBeanFactory(bf) + bf.addBeanPostProcessor(bpp) + val bd = RootBeanDefinition(KotlinBeanWithSecondaryConstructor::class.java) + bd.scope = RootBeanDefinition.SCOPE_PROTOTYPE + bf.registerBeanDefinition("bean", bd) + val tb = TestBean() + bf.registerSingleton("testBean", tb) + val colour = Colour.BLUE + bf.registerSingleton("colour", colour) + + bf.getBean("bean", KotlinBeanWithSecondaryConstructor::class.java) + } + class KotlinBean(val injectedFromConstructor: TestBean?) { @@ -166,7 +200,7 @@ class KotlinAutowiredTests { } } - class KotlinBeanWithOptionalParameter( + class KotlinBeanWithMandatoryAndOptionalParameters( val injectedFromConstructor: TestBean, val optional: String = "foo" ) { @@ -177,12 +211,23 @@ class KotlinAutowiredTests { } } + class KotlinBeanWithOptionalParameters( + val optional1: TestBean = TestBean(), + val optional2: String = "foo" + ) { + var initializedField: String? = null + + init { + initializedField = "bar" + } + } + class KotlinBeanWithOptionalParameterAndExplicitConstructor @Autowired constructor( val optional: String = "foo", val injectedFromConstructor: TestBean ) - class KotlinBeanWithSecondaryConstructor( + class KotlinBeanWithAutowiredSecondaryConstructor( val optional: String = "foo", val injectedFromConstructor: TestBean ) { @@ -198,4 +243,15 @@ class KotlinAutowiredTests { constructor() : this(TestBean()) } + class KotlinBeanWithSecondaryConstructor( + val optional: String = "foo", + val injectedFromConstructor: TestBean + ) { + constructor(injectedFromSecondaryConstructor: Colour, injectedFromConstructor: TestBean, optional: String = "bar") : this(optional, injectedFromConstructor) { + this.injectedFromSecondaryConstructor = injectedFromSecondaryConstructor + } + + var injectedFromSecondaryConstructor: Colour? = null + } + } diff --git a/spring-context/src/test/kotlin/org/springframework/context/annotation/Spr16022Tests.kt b/spring-context/src/test/kotlin/org/springframework/context/annotation/Spr16022Tests.kt new file mode 100644 index 00000000000..71a02c15b8f --- /dev/null +++ b/spring-context/src/test/kotlin/org/springframework/context/annotation/Spr16022Tests.kt @@ -0,0 +1,59 @@ +/* + * Copyright 2002-2017 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.context.annotation + +import org.junit.Assert.assertEquals +import org.junit.Test +import org.springframework.beans.factory.BeanFactory +import org.springframework.beans.factory.getBean +import org.springframework.context.support.ClassPathXmlApplicationContext + +/** + * @author Sebastien Deleuze + */ +class Spr16022Tests { + + @Test + fun `Register beans with multiple constructors with AnnotationConfigApplicationContext`() { + assert(AnnotationConfigApplicationContext(Config::class.java)) + } + + @Test + fun `Register beans with multiple constructors with ClassPathXmlApplicationContext`() { + assert(ClassPathXmlApplicationContext(CONTEXT)) + } + + private fun assert(context: BeanFactory) { + val bean1 = context.getBean("bean1") + assertEquals(0, bean1.foo) + val bean2 = context.getBean("bean2") + assertEquals(1, bean2.foo) + val bean3 = context.getBean("bean3") + assertEquals(3, bean3.foo) + + } + + @Suppress("unused") + class MultipleConstructorsTestBean(val foo: Int) { + constructor(bar: String) : this(bar.length) + constructor(foo: Int, bar: String) : this(foo + bar.length) + } + + @Configuration @ImportResource(CONTEXT) + open class Config +} + +private const val CONTEXT = "org/springframework/context/annotation/multipleConstructors.xml" \ No newline at end of file diff --git a/spring-context/src/test/resources/org/springframework/context/annotation/multipleConstructors.xml b/spring-context/src/test/resources/org/springframework/context/annotation/multipleConstructors.xml new file mode 100644 index 00000000000..3eaa5e5627f --- /dev/null +++ b/spring-context/src/test/resources/org/springframework/context/annotation/multipleConstructors.xml @@ -0,0 +1,19 @@ + + + + + + + + + + + + + + + + +