Skip to content

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

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

Closed
chengchen opened this issue Oct 21, 2019 · 17 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@chengchen
Copy link

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
Copy link
Member

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?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Oct 21, 2019
@chengchen
Copy link
Author

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 21, 2019
@snicoll
Copy link
Member

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 added the for: team-attention An issue we'd like other members of the team to review label Oct 21, 2019
@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 type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: feedback-provided Feedback has been provided labels Oct 23, 2019
@philwebb philwebb added this to the 2.2.x milestone Oct 23, 2019
@philwebb
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Oct 27, 2019
@derTobsch
Copy link

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
Copy link

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
Copy link
Member

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.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 30, 2019
@wilkinsona wilkinsona self-assigned this Nov 4, 2019
@wilkinsona
Copy link
Member

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
Copy link
Member

I've broken some of the smoke tests.

@wilkinsona wilkinsona reopened this Nov 4, 2019
@fkowal
Copy link

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
Copy link
Member

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
Copy link

steklopod commented Nov 8, 2019

My example in kotlin & Spring Boot 2.2.1:

@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)

@emersonf
Copy link

emersonf commented Nov 20, 2019

Are the 2.2 release notes correct?

The statement "[we] decided that we're no longer going to enable configuration property scanning by default" in this issue seems to contradict the statement "If you use @SpringBootApplication, scanning is enabled by default for the package that contains the @SpringBootApplication-annotated class." in the release notes. While pedantically this issue supersedes the 2.2 release notes if they're treated as 2.2.0 release notes, pragmatically the release notes will get far more eyeballs on them than this ticket.

@wilkinsona
Copy link
Member

Thanks, @emersonf. That was an oversight on our part. I've updated the release notes to hopefully clarify things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests