Skip to content
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.

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

Avoid implicit autowiring with Kotlin secondary constructors [SPR-16022] #20571

Closed
spring-issuemaster opened this issue Sep 28, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Sep 28, 2017

Alexander Chernikov opened SPR-16022 and commented

Looks similar to #20561, which is marked fixed in 5.0, but I observe this with 5.0.0.RELEASE, so creating new issue for a case.

Dummy Kotlin bean class:

class KotlinCtor(val intVal: Int) {
    constructor(p: String) : this(p.length)
    constructor(p0: Int, p1: String) : this(p0 + p1.length)
}

Dummy beans in spring.xml:

...
    <bean class="temp.KotlinCtor" name="ctor0">
        <constructor-arg value="0"/>
    </bean>
    <bean class="temp.KotlinCtor" name="ctor1">
        <constructor-arg value="a"/>
    </bean>
    <bean class="temp.KotlinCtor" name="ctor2">
        <constructor-arg value="2"/>
        <constructor-arg value="b"/>
    </bean>
...

Add configuration:

@Configuration @ImportResource("spring.xml")
open class KotlinAnnotator

Dummy application, this time in Java:

import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.support.ClassPathXmlApplicationContext;

public class Main {
    public static void main(String[] args) {
        ClassPathXmlApplicationContext context = new ClassPathXmlApplicationContext("spring.xml");
//        AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(KotlinAnnotator.class);
    }
}

Execute this main() method: no problem.
Comment out XML context creation, uncomment annotation context creation, execute and fail:

WARNING: Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'ctor1' defined in class path resource [spring.xml]: Unsatisfied dependency expressed through constructor parameter 0: Could not convert argument value of type [java.lang.String] to required type [int]: Failed to convert value of type 'java.lang.String' to required type 'int'; nested exception is java.lang.NumberFormatException: For input string: "a"
Exception in thread "main" org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'ctor1' defined in class path resource [spring.xml]: Unsatisfied dependency expressed through constructor parameter 0: Could not convert argument value of type [java.lang.String] to required type [int]: Failed to convert value of type 'java.lang.String' to required type 'int'; nested exception is java.lang.NumberFormatException: For input string: "a"
	at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:691)
	at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:192)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.autowireConstructor(AbstractAutowireCapableBeanFactory.java:1269)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1126)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:545)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:502)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:312)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:228)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:310)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:756)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:868)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:549)
	at org.springframework.context.annotation.AnnotationConfigApplicationContext.<init>(AnnotationConfigApplicationContext.java:88)
	at temp.Main.main(Main.java:9)

Affects: 5.0 GA

Issue Links:

  • #20561 AutowiredAnnotationBeanPostProcessor picks "wrong" constructor for Kotlin class
  • #20836 BeanCreationException when using c-namespace, Kotlin class with default constructor and annotation configuration

Referenced from: commits 19a1477, edf8232

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 16, 2017

Sébastien Deleuze commented

Alexander Chernikov Based on similar Java tests and a variant of your use case, I tend to think for such use case the type of the arguments should be specified explicitly to disambiguate which constructor should be invoked. Could you have a look to see this branch and send me your feedback ?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 20, 2017

Alexander Chernikov commented

Sorry for delay. Actually I'm testing Kotlin/Spring integration in JetBrains. So, for real users feedback (about usability of current decision and alike) it's probably better to ask people who write real applications.

However, I can say that currently I can't find a way to call secondary constructor. I've changed, just for testing, my beans from the description to:

<bean class="secret.KotlinCtor" name="ctor0">
    <constructor-arg value="0" type="int" name="intVal" index="0"/> <!-- BTW, we have to specify resulted Java types instead of Kotlin built-in types from sources. -->
</bean>
<bean class="secret.KotlinCtor" name="ctor1">
    <constructor-arg value="a" type="java.lang.String" name="p" index="0"/>
</bean>
<bean class="secret.KotlinCtor" name="ctor2">
    <constructor-arg value="2" type="int" name="p0" index="0"/>
    <constructor-arg value="b" type="java.lang.String" name="p1" index="1"/>
</bean>

The ctor0 bean, without others, is accepted. With 2 others initialization fails.

Looks like the branch sdeleuze@SPR-160 contains only changes in tests, so it probably gives the same output.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 20, 2017

Sébastien Deleuze commented

I have updated the branch to match more closely your example, and it works correctly with secondary constructors and following beans definition:

<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans"
	   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	   xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">

	<bean class="org.springframework.beans.factory.xml.KotlinXmlBeanFactoryTests.KotlinCtor" name="ctor1">
		<constructor-arg value="0" type="int" />
	</bean>

	<bean class="org.springframework.beans.factory.xml.KotlinXmlBeanFactoryTests.KotlinCtor" name="ctor2">
		<constructor-arg value="a" type="java.lang.String" />
	</bean>

	<bean class="org.springframework.beans.factory.xml.KotlinXmlBeanFactoryTests.KotlinCtor" name="ctor3">
		<constructor-arg value="2" type="int" />
		<constructor-arg value="b" type="java.lang.String" />
	</bean>

</beans>

Indeed it works only with Java types not Kotlin ones, we probably could support Kotlin type as well, but I tink I prefer to wait this request come from users with real application and use cases, since I tend to think the vast majority of Kotlin users will use JavaConfig or Functional bean DSL.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 21, 2017

Alexander Chernikov commented

Thank you. So, "how to do it correctly" is not a question, I just specify the types.

However, I've added "type" attribute to the beans from my example, and the output of my sandbox application is the same: no problem with ClassPathXmlApplicationContext, but failure with AnnotationConfigApplicationContext. Even though it just adds the same XML via ImportResource.

The exception trace is similar, but message slightly differs:

Oct 21, 2017 2:44:00 AM org.springframework.context.support.AbstractApplicationContext refresh
WARNING: Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'ctor2' defined in class path resource [spring.xml]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'int' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}
Exception in thread "main" org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'ctor2' defined in class path resource [spring.xml]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'int' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}
	at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:723)
	at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:192)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.autowireConstructor(AbstractAutowireCapableBeanFactory.java:1269)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1126)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:545)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:502)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:312)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:228)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:310)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:200)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:756)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:868)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:549)
	at org.springframework.context.annotation.AnnotationConfigApplicationContext.<init>(AnnotationConfigApplicationContext.java:88)
	at pack.Main.main(Main.java:9)
Caused by: org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'int' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.raiseNoMatchingBeanFound(DefaultListableBeanFactory.java:1501)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1099)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1060)
	at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:809)
	at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:715)
	... 14 more

Process finished with exit code 1
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 23, 2017

Sébastien Deleuze commented

After more tests, it seems the difference of behavior between Java and Kotlin comes from AutowiredAnnotationBeanPostProcessor which returns the primary constructor in Kotlin and an empty constructor list in Java.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 13, 2017

Sébastien Deleuze commented

Juergen Hoeller Could you please have a look to this pull-request which introduces changes that prevent implicit autowiring when there are secondary constructors? An alternative solution would be to tune ConstructorResolver with more primary constructor knowledge, but I am not sure that's a way I would like to follow, since other issues could arise in other place with the current implicit autowiring policy. Only supporting implicit autowiring for Kotlin classes with primary constructor or primary + default constructors seems to be a reasonable behavior that should avoid this kind of issue. Any thoughts?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 13, 2017

Juergen Hoeller commented

Sébastien Deleuze, this looks feasible to me. The constructor resolution logic in AutowiredAnnotationBeanPostProcessor is quite involved already, so I wouldn't try to extract any specifics there for the time being: Refining the algorithm right there - as in your pull request - seems like the better option.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 14, 2017

Sébastien Deleuze commented

Fixed via this commit.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

Alexander Chernikov commented

Thank you, looks like the issue is fixed for me in 5.0.2.
Met one a bit similar problem with c-namespace, filed as #20836.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.