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

Document Kotlin internal modifier impact on @Bean #31985

Closed
richmeyer7 opened this issue Jan 9, 2024 · 4 comments
Closed

Document Kotlin internal modifier impact on @Bean #31985

richmeyer7 opened this issue Jan 9, 2024 · 4 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support type: documentation A documentation task
Milestone

Comments

@richmeyer7
Copy link

richmeyer7 commented Jan 9, 2024

Affects: <Spring Framework version 5.3.31>


Spring discriminates public beans by implicit bean name and injected parameter name. If multiple public beans match an unqualified injection parameter, Spring matches the parameter name to the implicit bean name that it derives from the @Bean function name.

However, we find different behavior when the beans involved are Kotlin internal. Spring fails to use implicit bean names and injection parameter names to discriminate between multiple Kotlin internal beans that match an injection parameter type. Instead, NoUniqueBeanDefinitionException is thrown.

Our project has many Kotlin @Bean functions declared internal to restrict their visibility to the declaring module. Many of those beans are of internal types because we want visibility of those types restricted to the declaring module. When a bean's type is internal, the @Bean function must also be internal.

Our developers found that, to make Spring discriminate between multiple such internal beans of the same type, we must provide both explicit @Bean name Strings and injection parameter qualifier Strings (e.g. @Named("someBeanName")).

We find it counterintuitive that Kotlin internal bean implicit names and injection behave differently than public beans. Preferably, they should behave the same. Else, document the different behavior. Apologies if such documentation exists; we have not found it.

Minimal reproducible example:

SpringInternalBeanFunctionBugReproducerTest.kt:

package springinternalbeanfunctionbugreproducer

import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.boot.test.context.SpringBootTest
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import javax.inject.Named

/**
 * Demonstrates that Spring 2.7.18 fails to use implicit bean names
 * and injected parameter names to discriminate between multiple
 * Kotlin 1.9.21 `internal` beans of the same type.
 *
 * When multiple `internal` beans match an injected parameter type,
 * we found it necessary to provide explicit `@Bean` name Strings
 * AND qualifiers on the injected parameters to make Spring discriminate between
 * the beans.
 *
 * Public beans do not require that. Prefer that `internal` beans behave
 * the same, else perhaps Spring should document the different behavior.
 */
@SpringBootTest
internal class SpringInternalBeanFunctionBugReproducerTest {

   companion object {

      interface PublicInterface

      class PublicClass : PublicInterface

      internal interface InternalInterface1

      internal class InternalClass1 : InternalInterface1

      internal interface InternalInterface2

      internal class InternalClass2 : InternalInterface2
   }

   @Configuration
   class TestConfig {

      @Bean
      fun implicitNamePublicBeanA(): PublicInterface = PublicClass()

      @Bean
      fun implicitNamePublicBeanB(): PublicInterface = PublicClass()

      @Bean("beanA")
      internal fun explicitNameInternalBeanA(): InternalInterface1 = InternalClass1()

      @Bean("beanB")
      internal fun explicitNameInternalBeanB(): InternalInterface1 = InternalClass1()

      @Bean
      internal fun implicitNameInternalBeanC(): InternalInterface2 = InternalClass2()

      @Bean
      internal fun implicitNameInternalBeanD(): InternalInterface2 = InternalClass2()
   }

   @Test
   fun `test public beans with implicit names with unqualified injection`(
      @Autowired
      implicitNamePublicBeanA: PublicInterface?,
      @Autowired
      implicitNamePublicBeanB: PublicInterface?,
   ) {
      assertThat(implicitNamePublicBeanA).isInstanceOf(PublicClass::class.java)
      assertThat(implicitNamePublicBeanB).isInstanceOf(PublicClass::class.java)
      // Because Spring matches the bean function name to the injected parameter name,
      // we expect two different beans provided by the two different bean functions.
      assertThat(implicitNamePublicBeanA).isNotSameAs(implicitNamePublicBeanB)
   }

   @Test
   fun `test public beans with implicit names with qualified injection`(
      @Autowired
      @Named("implicitNamePublicBeanA")
      implicitNamePublicBeanA: PublicInterface?,
      @Autowired
      @Named("implicitNamePublicBeanB")
      implicitNamePublicBeanB: PublicInterface?,
   ) {
      assertThat(implicitNamePublicBeanA).isInstanceOf(PublicClass::class.java)
      assertThat(implicitNamePublicBeanB).isInstanceOf(PublicClass::class.java)
      // Because Spring matches the explicit @Bean name to the @Named parameter name,
      // we expect two different beans provided by the two different bean functions.
      assertThat(implicitNamePublicBeanA).isNotSameAs(implicitNamePublicBeanB)
   }

   @Test
   fun `test internal bean with implicit name with unqualified injection`(
      @Autowired
      implicitNameInternalBeanC: InternalInterface2?,
   ) {
      // This test fails because Spring fails to resolve the injected parameter.
      // Spring finds 2 beans of type InternalInterface2
      // and does not use the bean function names as implicit bean names.
      // Thus, NoUniqueBeanDefinitionException is thrown.

      // Because Spring should match the bean function name to the injected parameter name,
      // expect that implicitNameInternalBeanC is not null.
      assertThat(implicitNameInternalBeanC).isNotNull
   }

   @Test
   fun `test internal beans with implicit names with unqualified injection`(
      @Autowired
      implicitNameInternalBeanC: InternalInterface2?,
      @Autowired
      implicitNameInternalBeanD: InternalInterface2?,
   ) {
      // This test fails because Spring fails to resolve the injected parameter.
      // Spring finds 2 beans of type InternalInterface2
      // and does not use the bean function names as implicit bean names.
      // Thus, NoUniqueBeanDefinitionException is thrown.

      assertThat(implicitNameInternalBeanC).isNotNull
      assertThat(implicitNameInternalBeanD).isNotNull
      // Because Spring should match the bean function name to the injected parameter name,
      // we expect two different beans provided by the two different bean functions.
      assertThat(implicitNameInternalBeanC).isNotSameAs(implicitNameInternalBeanD)
   }

   @Test
   fun `test internal beans with implicit names with qualified injection`(
      @Autowired
      @Named("implicitNameInternalBeanC")
      implicitNameInternalBeanC: InternalInterface2?,
      @Autowired
      @Named("implicitNameInternalBeanD")
      implicitNameInternalBeanD: InternalInterface2?,
   ) {
      // This test fails because Spring fails to resolve the injected parameter.
      // Spring finds 2 beans of type InternalInterface2
      // and does not use the bean function names as implicit bean names.
      // Thus, NoUniqueBeanDefinitionException is thrown.

      assertThat(implicitNameInternalBeanC).isNotNull
      assertThat(implicitNameInternalBeanD).isNotNull
      // Because Spring should match the bean function name to the @Named parameter name,
      // we expect two different beans provided by the two different bean functions.
      assertThat(implicitNameInternalBeanC).isNotSameAs(implicitNameInternalBeanD)
   }

   @Test
   fun `test internal beans with explicit names with qualified injection`(
      @Autowired
      @Named("beanA")
      explicitNameInternalBeanA: InternalInterface1?,
      @Autowired
      @Named("beanB")
      explicitNameInternalBeanB: InternalInterface1?,
   ) {
      // With multiple internal beans of a requested type,
      // we found that we must supply explicit @Bean names on the bean functions
      // AND injection qualifiers such as @Named()
      // to get Spring to discriminate between the different beans.
      // Because we do that in this test, it passes.

      assertThat(explicitNameInternalBeanA).isInstanceOf(InternalClass1::class.java)
      assertThat(explicitNameInternalBeanB).isInstanceOf(InternalClass1::class.java)
      // Because Spring matches the explicit @Bean name to the @Named parameter name,
      // we expect two different beans provided by the two different bean functions.
      assertThat(explicitNameInternalBeanA).isNotSameAs(explicitNameInternalBeanB)
   }
}

build.gradle:

import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
    id 'io.spring.dependency-management' version '1.1.4'
    id 'org.jetbrains.kotlin.jvm' version '1.9.21'
    id 'org.jetbrains.kotlin.plugin.spring' version '1.9.21'
    id 'org.springframework.boot' version '2.7.18'
}

group = 'com.example'
version = '0.0.1-SNAPSHOT'

java {
    sourceCompatibility = '11'
}

repositories {
    mavenCentral()
}

dependencies {
    implementation 'org.springframework.boot:spring-boot-starter'
    implementation 'org.jetbrains.kotlin:kotlin-reflect'
    testImplementation 'org.springframework.boot:spring-boot-starter-test'
    testImplementation 'javax.inject:javax.inject:1'
    testImplementation 'org.assertj:assertj-core:3.20.2'
}

tasks.withType(KotlinCompile) {
    kotlinOptions {
        freeCompilerArgs += '-Xjsr305=strict'
        jvmTarget = '11'
    }
}

tasks.named('test') {
    useJUnitPlatform()
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 9, 2024
@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Jan 9, 2024
@sdeleuze sdeleuze self-assigned this Jan 9, 2024
@sdeleuze sdeleuze added the theme: kotlin An issue related to Kotlin support label Jan 9, 2024
@sdeleuze
Copy link
Contributor

sdeleuze commented Jan 9, 2024

This is likely a side effect of Kotlin name mangling of internal functions.

If you decompile the JVM bytecode, you will see:

@Bean({"beanA"})
@NotNull
public Companion.InternalInterface1 explicitNameInternalBeanA$demo_kotlin_internal_test() {
    return (Companion.InternalInterface1)(new Companion.InternalClass1());
}

You use case should work when using mangled name like explicitNameInternalBeanA$demo_kotlin_internal_test even if I understand this is not very convenient. I will experiment a bit and discuss with the team to decide if we are willing to use Kotlin reflection for such use case to get the Kotlin name, or if we prefer to stick with what is seen at JVM bytecode level.

@richmeyer7
Copy link
Author

Thank you, @sdeleuze .
I checked, and using a qualifier annotation with the mangled bean name does indeed work.
Of course, we cannot do this without a qualifier annotation, because the mangled bean name is not a legal Kotlin name for the injected parameter.
If we choose to use the mangled name in the qualifier annotation (which predominantly happens in our Configuration tests), at least it avoids supplying an explicit bean name String in the Configuration.
We will watch for further updates.

@sdeleuze
Copy link
Contributor

After a deeper look, it does not seem obvious how we can support resolving Kotlin internal function names at ConfigurationClassBeanDefinitionReader#loadBeanDefinitionsForBeanMethod level, where Spring only deals with metadata and not with concrete Class and Method instances.

It would be doable to identify Kotlin classes using kotlin.Metadata annotation metadata, but then it seems not possible to get the Kotlin function name because:

  • We can't just parse to function name to remove the $demo_kotlin_internal_test part because Kotlin developers can craft that manually using escaped method name (+ it would be fragile).
  • ReflectJvmMapping#getKotlinFunction takes a Method parameter, and it is problematic to create such instance at that level.

As a consequence, I am turning this issue into a documentation one.

@sdeleuze sdeleuze added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 10, 2024
@sdeleuze sdeleuze added this to the 6.1.3 milestone Jan 10, 2024
@sdeleuze sdeleuze changed the title Kotlin internal beans are not discriminated by implicit bean name and injected parameter name Document Kotlin internal function impact on @Bean Jan 10, 2024
@sdeleuze sdeleuze changed the title Document Kotlin internal function impact on @Bean Document Kotlin internal modifier impact on @Bean Jan 10, 2024
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Jan 10, 2024
@ianbrandt
Copy link
Contributor

ianbrandt commented Jan 10, 2024

@sdeleuze, Thank you for looking into this and adding documentation for it. Regarding:

Make sure to use the mangled name when injecting such bean by name.

It might be good to also suggest in the docs the alternative of adding an explicit bean name, i.e.

@Bean("sampleBean")
internal fun sampleBean() = SampleBean()

In fact, I'd suggest making that the primary recommendation over using the mangled name when injecting. I don't see the specific convention of appending the module name for mangling internal functions documented anywhere in the Kotlin docs, so perhaps that could change. Moreover, if one renames a module, that could break Spring configuration in a potentially unanticipated way.

izeye added a commit to izeye/hello-spring-boot-kotlin that referenced this issue Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) theme: kotlin An issue related to Kotlin support type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

5 participants