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

Enabling configuration properties scanning by default prevents conditional registration of @ConfigurationProperties-annoted types that are found by scanning #18674

Closed
chengchen opened this issue Oct 21, 2019 · 15 comments
Assignees
Labels
Milestone

Comments

@chengchen
Copy link

@chengchen chengchen commented Oct 21, 2019

Hello,

We were trying to migrate from 2.1.9 to 2.2.0 for our applications last week and encountered a startup failure due to a configuration validation failure. In short, if we are using @EnableConfigurationProperties combined with @ConditionalOnProperty for some beans, the new version is not respecting the conditional on the config part.

Here is a tiny example:

If we add this component to any Spring Boot application, it will fail to start with 2.2.0.

@Component
@ConditionalOnProperty(prefix = "foo", name = "enabled")
@EnableConfigurationProperties(FooBarConfig.class)
public class FooBar {

    public FooBar() {
        Logger.getGlobal().info("Starting FooBar...");
    }

    @ConfigurationProperties(prefix = "foo")
    @Validated
    static class FooBarConfig {

        @NotNull
        String bar;

        public void setBar(String bar) {
            this.bar = bar;
        }

    }

}

Expected behavior:
The bean initialization of FooBar and FooBarConfig should be skipped because @ConditionalOnProperty doesn't match.

Actual behavior:
FooBar is skipped but the FooBarConfig keeps initializing thus failed startup.

@snicoll

This comment has been minimized.

Copy link
Member

@snicoll snicoll commented Oct 21, 2019

I understand that FooBar is in a package that is a target of component scan given the @Component on it. Spring Boot 2.2 now scans @ConfigurationProperties automatically and that arrangement above is really two separate components (i.e. excluding the parent doesn't mean that the inner class should not be scanned).

If you want to conditionally create such bean, you shouldn't put them in a package that is target for classpath scanning (auto-configurations shouldn't use package scanning in the first place). If you EnableConfigurationProperties(FooBar.class) from your auto-configuration, things should work fine in both versions.

This is based on partial code snippet so I might be missing something. Does that make sense?

@chengchen

This comment has been minimized.

Copy link
Author

@chengchen chengchen commented Oct 21, 2019

@snicoll Thanks for the quick explanations. I see what you meant and I just noticed this on the upgrade instructions:

Classes annotated with @ConfigurationProperties can now be found via classpath scanning as an alternative to using @EnableConfigurationProperties or @component. If you use @SpringBootApplication, scanning is enabled by default for the package that contains the @SpringBootApplication-annotated class.

We wanted to put the our service bean together with its configuration, and use @EnableConfigurationProperties to conditional switch on/off the bean initialization in a consistent way.

@snicoll

This comment has been minimized.

Copy link
Member

@snicoll snicoll commented Oct 21, 2019

We wanted to put the our service bean together with its configuration, and use @EnableConfigurationProperties to conditional switch on/off the bean initialization in a consistent way.

And that's a perfectly reasonable thing to do. I can see now how scanning user-config makes it harder. Flagging for team attention to see how we could mitigate this.

@snicoll snicoll changed the title Potential regression or behaviour change in EnableConfigurationProperties after upgrading from 2.1.9 to 2.2.0 No way to conditionally register @ConfigurationProperties-annoted types that are target for classpath scanning Oct 23, 2019
@philwebb philwebb added this to the 2.2.x milestone Oct 23, 2019
@philwebb

This comment has been minimized.

Copy link
Member

@philwebb philwebb commented Oct 23, 2019

One idea we discussed is adding a @SpringBootApplication(configurationPropertiesScanning=false) option to disable scanning. Another option would be a similar attribute on @ConfigurationProperties.

@philwebb philwebb assigned philwebb and unassigned philwebb Oct 23, 2019
@derTobsch

This comment has been minimized.

Copy link

@derTobsch derTobsch commented Oct 27, 2019

I got a similar problem. We have a application that can talk with oidc provider or ldap e.g. and for each security provider we have a guarded configuration class (see here e.g. https://github.com/synyx/urlaubsverwaltung/blob/master/src/main/java/org/synyx/urlaubsverwaltung/security/oidc/OidcSecurityConfiguration.java) and this will only be executed if @ConditionalOnProperty(value = "uv.security.auth", havingValue = "oidc") is defined and therefore the @EnableConfigurationProperties(OidcSecurityProperties.class) will be collected and so on.

But now @EnableConfigurationProperties(OidcSecurityProperties.class) will be found even if v.security.auth does not have oidc. How is it now possible to only collect ConfigurationProperties based on a application property?

@wilkinsona

This comment has been minimized.

Copy link
Member

@wilkinsona wilkinsona commented Oct 27, 2019

I'm starting to feel that configuration properties scanning may have been a mistake due to the dual purposes that @ConfigurationProperties now serves. I'd rather remove it than add an attribute to @SpringBootApplication or @ConfigurationProperties. As things stand, I don't think the benefit of no longer needing @EnableConfigurationProperties is worth the additional complexity of scanning and its side-effects.

@jeffbswope

This comment has been minimized.

Copy link

@jeffbswope jeffbswope commented Oct 27, 2019

My hunch when I was disabling this as we upgraded was that @ConfigurationProperties have ended up in an uncanny valley where the separate-but-not-quite-equal @Component/bean-like behavior appears to be causing confusion, here and elsewhere. Not sure if that gap can technically be closed.

@derTobsch

This comment has been minimized.

Copy link

@derTobsch derTobsch commented Oct 28, 2019

I'm starting to feel that configuration properties scanning may have been a mistake due to the dual purposes that @ConfigurationProperties now serves. I'd rather remove it than add an attribute to @SpringBootApplication or @ConfigurationProperties. As things stand, I don't think the benefit of no longer needing @EnableConfigurationProperties is worth the additional complexity of scanning and its side-effects.

I feel the same. I have at the moment no clue how I could achieve my requirement with Spring Boot 2.2. It feels like we "destroyed" the mechanism to have a "namespaced" configuration class with ConfigurationProperties support. That feels like a step back to @Value? :/

@evpaassen

This comment has been minimized.

Copy link

@evpaassen evpaassen commented Oct 29, 2019

Wouldn't a solution be to annotate the the configuration properties class with @ConditionalOnProperty and then annotate the service with @ConditionalOnBean, like this? (And I guess the classes should both be top-level then?)

@Component
@ConditionalOnBean(FooBarConfig.class)
@EnableConfigurationProperties(FooBarConfig.class)
public class FooBar {

    public FooBar() {
        Logger.getGlobal().info("Starting FooBar...");
    }
}

@ConditionalOnProperty(prefix = "foo", name = "enabled")
@ConfigurationProperties(prefix = "foo")
@Validated
public class FooBarConfig {

    @NotNull
    String bar;

    public void setBar(String bar) {
        this.bar = bar;
    }
}
@wilkinsona

This comment has been minimized.

Copy link
Member

@wilkinsona wilkinsona commented Oct 29, 2019

Thanks for the suggestion, @evpaassen. We discourage use of @ConditionalOnBean in "normal" configuration and recommend that it's only used in auto-configuration where you have more guarantees about other beans having already been defined. It may be fine in this case as the bean is being defined via @EnableConfigurationProperties but it still risks being brittle or confusing people about when they can or cannot safely use @ConditionalOnBean.

@wilkinsona wilkinsona self-assigned this Nov 4, 2019
@wilkinsona

This comment has been minimized.

Copy link
Member

@wilkinsona wilkinsona commented Nov 4, 2019

We discussed this last week and decided that we're no longer going to enable configuration property scanning by default. Strictly speaking, this will be a breaking change from 2.2.0 but so was the loss of the ability to enable configuration properties conditionally. Making scanning opt-in will allow those that relied on 2.1.x's behaviour to continue to have it while also allowing those who want configuration property scanning to enable it by adding @ConfigurationPropertiesScan to their main application class alongside @SpringBootApplication.

@wilkinsona wilkinsona changed the title No way to conditionally register @ConfigurationProperties-annoted types that are target for classpath scanning Enabling configuration properties scanning by default prevents conditional registration of @ConfigurationProperties-annoted types that are found by scanning Nov 4, 2019
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.1 Nov 4, 2019
@wilkinsona wilkinsona closed this in e26d5d9 Nov 4, 2019
@wilkinsona

This comment has been minimized.

Copy link
Member

@wilkinsona wilkinsona commented Nov 4, 2019

I've broken some of the smoke tests.

@wilkinsona wilkinsona reopened this Nov 4, 2019
@wilkinsona wilkinsona closed this in 8c07733 Nov 4, 2019
@fkowal

This comment has been minimized.

Copy link

@fkowal fkowal commented Nov 7, 2019

So me 2.2.0 -> 2.2.1 migration is not going smoothly

I was using

@Configuration
@EnableConfigurationProperties(XXProperties::class)
class XXConfiguration {
}

@ConfigurationProperties(prefix = "xxprefix")
@ConstructorBinding
data class XXProperties(arg1: Arg1Class)

data class Arg1Class(...)

Now i am getting

Failed to bind properties under 'xxprefix' to org.example.XXProperties:

    Reason: Parameter specified as non-null is null: method org.example.XXProperties.<init>, parameter arg1

I am a bit confused about what changed and how to undo the changes from 2.2.1.

Better description in the release notes regarding breaking changes would be most welcome.

@snicoll

This comment has been minimized.

Copy link
Member

@snicoll snicoll commented Nov 7, 2019

@fkowal thanks for letting us know but that looks like a regression unrelated to this issue. Please create a separate issue.

@steklopod

This comment has been minimized.

Copy link

@steklopod steklopod commented Nov 8, 2019

My example in kotlin & Spring Boot 2.2.1:

package by.colaba.config

import by.colaba.contentFolder
import by.colaba.contextPath
import by.colaba.domain
import org.springframework.boot.context.properties.ConfigurationProperties
import org.springframework.boot.context.properties.ConfigurationPropertiesScan
import org.springframework.boot.context.properties.ConstructorBinding
import org.springframework.context.annotation.Configuration
import javax.annotation.PostConstruct

@Configuration
@ConfigurationPropertiesScan
class YamlParser(private val appInfo: ApplicationInfo, private val servletInfo: ServletInfo) {
    companion object{
        fun printDomain() = println(" \uD83C\uDFAF Target domain:  https://$domain")
    }
    @PostConstruct
    fun readApplicationProperties() {
        domain = appInfo.domain
        contentFolder = appInfo.contentFolder
        contextPath = servletInfo.contextPath
    }
}

@ConfigurationProperties(prefix = "application") @ConstructorBinding
data class ApplicationInfo(val domain: String, val contentFolder: String)

@ConfigurationProperties(prefix = "server.servlet") @ConstructorBinding
data class ServletInfo(val contextPath: String)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.