Prevent @Bean method overloading by default (avoiding accidental overloading and condition mismatches)

[rfelgent]

Hello poeple,

I lost some hours configuring a bean with same id but different properties in java config, as the raised error message was not very helpful.

The bean in question must be created, as it is required via declarative @DependsOn configuration.

The error

A component required a bean named 'postgresqlContainer' that could not be found.
	- Bean method 'postgresqlContainer' in 'PersistenceConfig.DbServersConfig' not loaded because @ConditionalOnProperty (app.persistence.servers.postgresql.enabled=true) found different value in property 'enabled'
	- Bean method 'postgresqlContainer' in 'PersistenceConfig.DbServersConfig' not loaded because @ConditionalOnProperty (app.persistence.servers.postgresql.enabled=true) found different value in property 'enabled'

This config fails:

@Configuration
public class PersistenceConfig {

  @Configuration
  public class DbServersConfig {

    @Bean(value = "postgresqlContainer", initMethod = "start", destroyMethod = "stop")
    @ConditionalOnProperty(prefix = "app.persistence.servers.postgresql", name = "enabled", havingValue = "true")
    public PostgreSQLContainer postgresqlContainer(TCPostgresqlProperties postgresqlProperties) {
        Instance retVal = <do_your_logic>
        return retVal;
    }

    @Bean("postgresqlContainer")
    @ConditionalOnProperty(prefix = "app.persistence.servers.postgresql", name = "enabled", havingValue = "false", matchIfMissing = true)
    public PostgreSQLContainer postgresqlContainer() {
      return null;
    }
  }
}

This config works:

@Configuration
public class PersistenceConfig {

  @Configuration
  public class DbServersConfig {

    @Bean(value = "postgresqlContainer", initMethod = "start", destroyMethod = "stop")
    @ConditionalOnProperty(prefix = "app.persistence.servers.postgresql", name = "enabled", havingValue = "true")
    public PostgreSQLContainer  postgresqlContainer(TCPostgresqlProperties postgresqlProperties) {
        Instance retVal = <do_your_logic>
        return retVal;
    }

    @Bean("postgresqlContainer")
    @ConditionalOnProperty(prefix = "app.persistence.servers.postgresql", name = "enabled", havingValue = "false", matchIfMissing = true)
    public PostgreSQLContainer postgresqlContainer2() {
      return null;
    }
  }
}

If you name both methods differently (postgresqlContainer and postgresqlContainer2) then everything works as expected otherwise you get an error.

Is this desired behavior ?

I am unsure if my scenario could indicate a bug, too. I do not know if this problem happens only to @ConditionalOnProperty or any other condition like @ConditionalOnExpression.

Best regards

[mbhave]

@rfelgent I was able to replicate this behavior. It appears to be happening because of this in Spring Framework. That check can cause unpredictable behavior because it depends on the order in which the bean definitions were processed. In the example above, if the bean definition corresponding to the second bean is processed first, it will be loaded as configClass.skippedBeanMethods.contains(methodName) will return false.

I don't think there is anything we can do in Spring Boot about this. We can move it to the Spring Framework issue tracker if the rest of the team think that it's something that can be fixed there.

[wilkinsona]

Framework's behaviour should be predictable thanks to this logic.

I think it's worth moving this to Framework to consider an enhancement to use something more distinct than the method name when determining if a method has already been skipped. The simple workaround, as you have already noted, @rfelgent, is to avoid overloading @Bean methods and use distinct method names instead.

[sbrannen]

@mbhave and @wilkinsona, thanks for the detective work!

I agree that the algorithm in question should take overloaded methods into account and thus track skipped methods based on each method's name plus its formal parameter list.

Let's see if the rest of the team agrees.

[jhoeller]

The current behavior is more or less by design: There is only one bean definition per bean name, so we're conceptutally merging all relevant metadata into one definition per name. Overloaded @Bean methods are effectively just like overloaded constructors on a particular bean class, still constituting one bean definition only. And for condition handling, all conditions on all overloaded methods effectively apply to that same single bean. Conditions are not straightforward to isolate per bean method since we'd have to apply them to the overloaded factory method selection instead of to the bean definition.

The general recommendation is indeed not to use overloading but distinct bean/method names. And in the case of overloading, to declare the same annotations on all overloaded methods because that's closest to the actual semantics at runtime. This is not really obvious, so at the very least we need to document this properly... and we could possibly raise exceptions in case of condition mismatches among overloaded methods for the same bean definition, suggesting unified conditions or distinct bean names.

[mbhave]

Framework's behaviour should be predictable thanks to this logic.

@wilkinsona Sorry, I didn't explain myself very well in the previous comment. The behavior would be predictable for a given class. The unpredictability I was referring was across classes, where the Condition would match in some cases and not in others, depending on the order of the methods returned.

[fprochazka]

I'd like to add my two cents - I've just lost several hours trying to debug why one of my beans was not registered (it was declared in a @Configuration class by @Bean method)... the problem was, I've made a typo and given the two methods the same name... because they had different arguments I didn't notice anything (no error anywhere) and the app just wasn't starting

@ConditionalOnWebApplication
@Configuration
public class HttpRequestHelpersConfiguration
{

    @Bean
    public RequestContextHolderFacade requestContextHolderFacade()
    {
        return new RequestContextHolderFacade();
    }

    @Bean
    public RequestHandlerProvider requestContextHolderFacade(
        final ApplicationContext applicationContext,
        final RequestContextHolderFacade requestContextHolderFacade
    )
    {
        return new RequestHandlerProvider(
            applicationContext,
            requestContextHolderFacade
        );
    }

}

->

15:07:27.203 [restartedMain  ] ERROR        o.s.b.d.LoggingFailureAnalysisReporter:	

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 2 of constructor in ThymeleafMvcTemplatesConfiguration required a bean of type 'RequestHandlerProvider' that could not be found.


Action:

Consider defining a bean of type 'RequestHandlerProvider' in your configuration.

I don't have any strong opinion about the behaviour, as I have no problem renaming the method... but there should be a better error message provided by Spring.

Thanks :)

[alfonz19]

Just to add my experience. Just like other I made typo in method name. Same return type, same method name, different parameters. 2 beans with 2 different qualifiers. First bean declaration wasn't called at all (System.out.println in it wasn't printed, breakpoint wasn't hit), the other method was called twice. Both bean were injectable using 2 different qualifiers, however bean declaration of first qualifier were ever called.

[Hakky54]

I also have the same issue. I wanted to refactor the code below:

github/mutual-tls-ssl/SSLConfig.java

@Component
public class SSLConfig {

    @Bean
    @Scope("prototype")
    public SSLFactory sslFactory(
            @Value("${client.ssl.one-way-authentication-enabled:false}") boolean oneWayAuthenticationEnabled,
            @Value("${client.ssl.two-way-authentication-enabled:false}") boolean twoWayAuthenticationEnabled,
            @Value("${client.ssl.key-store:}") String keyStorePath,
            @Value("${client.ssl.key-store-password:}") char[] keyStorePassword,
            @Value("${client.ssl.trust-store:}") String trustStorePath,
            @Value("${client.ssl.trust-store-password:}") char[] trustStorePassword) {
        SSLFactory sslFactory = null;

        if (oneWayAuthenticationEnabled) {
            sslFactory = SSLFactory.builder()
                    .withTrustMaterial(trustStorePath, trustStorePassword)
                    .withProtocols("TLSv1.3")
                    .build();
        }

        if (twoWayAuthenticationEnabled) {
            sslFactory = SSLFactory.builder()
                    .withIdentityMaterial(keyStorePath, keyStorePassword)
                    .withTrustMaterial(trustStorePath, trustStorePassword)
                    .withProtocols("TLSv1.3")
                    .build();
        }

        return sslFactory;
    }

}

Into the following snippet:

@Component
public class SSLConfig {

    @Bean
    @Scope("prototype")
    @ConditionalOnExpression("${client.ssl.one-way-authentication-enabled} == true and ${client.ssl.two-way-authentication-enabled} == false")
    public SSLFactory sslFactory(@Value("${client.ssl.trust-store:}") String trustStorePath,
                                 @Value("${client.ssl.trust-store-password:}") char[] trustStorePassword) {

        return SSLFactory.builder()
                .withTrustMaterial(trustStorePath, trustStorePassword)
                .withProtocols("TLSv1.3")
                .build();
    }


    @Bean
    @Scope("prototype")
    @ConditionalOnExpression("${client.ssl.two-way-authentication-enabled} == true and ${client.ssl.one-way-authentication-enabled} == false")
    public SSLFactory sslFactory(@Value("${client.ssl.key-store:}") String keyStorePath,
                                 @Value("${client.ssl.key-store-password:}") char[] keyStorePassword,
                                 @Value("${client.ssl.trust-store:}") String trustStorePath,
                                 @Value("${client.ssl.trust-store-password:}") char[] trustStorePassword) {

        return SSLFactory.builder()
                .withIdentityMaterial(keyStorePath, keyStorePassword)
                .withTrustMaterial(trustStorePath, trustStorePassword)
                .withProtocols("TLSv1.3")
                .build();
    }

}

However it fails. When I set debug on I see that the first is not matched because the expression is evaluated into negative. But the second method should pass, however that one is never evaluated.

I don't think this is a must option as there is a workaround such as not using method overloading/ and use different method name or combine it in a single method. However it will give a better DX if this would just work out of the box. Looking forward to have this feature 😄

Any news regarding this issue?

[rfelgent]

I am sorry for you @Hakky54 - you walked into the same trap like me and others :-(

@jholler, I do understand your hint regarding "more or less" by design and I do love your suggestions regarding
"... so at the very least we need to document this properly... and we could possibly raise exceptions in case of condition mismatches among overloaded methods for the same bean definition..."

[fprochazka]

Simply disallowing to declare more methods with the same name on the same configuration class (and guarding that with an exception) would suffice to prevent others from walking into the same trap 👍

I guess this could be even easily made into an ErrorProne check for example 🤔

[Hakky54]

I have forked the repo and made it working for this specific use case. It was a bit tricky because of some unit tests which still needed to pass. Anyone of the spring-framework team here? Just curious if it is worth to submit a PR as I am wondering if the team is considering to have this kind of capability for allowing of creating a bean conditionally while using method overload.

[Hakky54]

I have created a PR to make this feature working, see here for the details: #28019 Would love to get everyones input ❤️

[jhoeller]

After a team discussion this morning, we are leaning towards a more radical step: disallowing overloaded @Bean methods completely, therefore raising an error in case of the same method name reappearing in any form. The original method overloading arrangement for factory methods was inspired by overloaded constructors, treated the same way by our constructor resolution algorithm. Unfortunately this causes more harm than good with @Bean methods where it is not obvious that all such same-named methods are part of the same bean definition, with all metadata expected to be equivalent. This was simply never meant to be used with mutually exclusive conditions, or any differing conditions to begin with.

The use cases for factory method overloading (just like with constructor overloading) are mostly related to optional arguments, with the "greediest satisfiable" factory method being calling, e.g. a variant with an optional argument, falling back to the overloaded method without the optional argument otherwise. In a modern-day Spring application, optional factory method arguments can be modeled in various forms, including @Nullable arguments and Optional<...> declarations, so there should not be any exclusive needs for factory method overloading anymore. That's why we are considering to disallow it completely for Spring Framework 6.0, ideally as of the 6.0 M3 release already.

Enforcing a strict non-overloading rule for @Bean methods would prevent accidental overloading that cannot ever work properly, as well as half-accidental overloading that might work by chance but not really by design. For exclusive conditions ("alternative beans"), it is always preferable to use distinct bean names, no matter whether there are optional arguments involved or not. After all, alternative beans with the same (or no) arguments can only be modeled with distinct method names according to Java language rules; this is a strong indication that method overloading for exclusive conditions is misguided, only accidentally working if the method arguments happen to differ between the alternative definitions.

[jhoeller]

I ended up introducing an enforceUniqueMethods flag on the @Configuration annotation, by default set to true. This prevents accidental overloading by enforcing distinct @Bean method names for separate bean definitions (which includes "alternative" bean definitions with mutually exclusive conditions). This can be switched to false for the previous lenient acceptance of overloaded methods as factory method candidates for the same bean definition.

All in all, the default behavior should provide better guidance now. The error message shown when rejecting same-named @Bean methods in 6.0 hints at the enforceUniqueMethods flag for whoever intends to opt into the overloading behavior, or whoever has existing configuration classes which happen to rely on the overloading.

[rfelgent]

@jhoeller thx

• for improved exception logging
• and for the opt in configuration