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

Make @ConfigurationProperties available in conditions #2282

Open
hekonsek opened this issue Jan 5, 2015 · 17 comments
Open

Make @ConfigurationProperties available in conditions #2282

hekonsek opened this issue Jan 5, 2015 · 17 comments
Labels
type: enhancement A general enhancement

Comments

@hekonsek
Copy link
Contributor

hekonsek commented Jan 5, 2015

Hi,

There is a general issue with @ConfigurationProperties and beans/classes conditions not working well together. Basically @ConfigurationProperties properties are instantiated after conditions are evaluated. As a result, conditions can't see those properties.

For example creating @ConfigurationProperties with some default true value...

@ConfigurationProperties(prefix = "foo")
public class MyConfigurationProperties {

  boolean bar = true;

  // getters & setters

...

}

...and using it in @ConditionalOnProperty("foo.bar") doesn't evaluate to true (because OnPropertyCondition#getMatchOutcome is executed before MyConfigurationProperties is processed by the ConfigurationClassPostProcessor, so PropertyResolver can't see foo.bar property yet).

The problem is not limited to @ConditionalOnProperty. It affects all conditions that relies on properties or on the other beans referring @ConfigurationProperties.

Fix to that will be highly appreciated :) .

Cheers.

@snicoll
Copy link
Member

snicoll commented Jan 5, 2015

Well, the link is reverse. The Environment injects values into @ConfigurationProperties annotated bean, not the other way around. So there is no way (currently) that the default value of your bar property would update the Environment.

@hekonsek
Copy link
Contributor Author

hekonsek commented Jan 5, 2015

Is there a chance to respect @ConfigurationProperties default values? Default values are pretty common in @ConfigurationProperties instances, so this is quite surprising for end-users that @ConditionalOnProperty("foo.bar") (and PropertyResolver in general) can't see them.

BTW The another painful scenario is when we create condition and use bean referring @ConfigurationProperties instance...

class MyBean {
  @Autowired
  MyProperties properties;

  void readFoo(){
    assertEquals("fooValue", properties.getFoo());
  }
}

If I access MyBean#readFoo using ConditionContext's bean factory, the value of foo property will not be properly evaluated (because environment has not been bound to the MyProperties at the moment of the condition evaluation).

In general with these issues in place, using @Value("${foo:default}") instead of @ConfigurationProperties sounds more predictable and intuitive.

Do you think that this kind of binding corner cases could be addressed? I (and not only me I guess) will highly appreciate it.

@philwebb
Copy link
Member

philwebb commented Jan 5, 2015

It's probably going to be pretty hard to make @ConditionalOnProperty look at the default values of the @ConfigurationProperties. As you said, currently the bind occurs after the bean has been created.

Have you seen the matchIfMissing and havingValue attributes of @ConditionalOnProperty. We use those internally quite a bit to do something similar to your first example:

@ConfigurationProperties(prefix = "foo")
@ConditionalOnProperty(prefix="foo", name="bar", matchIfMissing=true)
public class MyConfigurationProperties {

    private boolean bar = true;

    // getters & setters

}

@hekonsek
Copy link
Contributor Author

hekonsek commented Jan 5, 2015

Hi Phil,

Yes, actually I use matchIfMissing as the workaround for cases like this. :) However this is a bit ugly, as I really need to duplicate default value (keep in mind that matchIfMissing=true is really the duplication of the default @ConfigurationProperty like bar = true). And If I change default value in @ConfigurationPropeties I need to remember to change matchIfMissing in the appropriate bean/config. This workaround also works only for booleans - default String value won't work:

  @ConditionalOnProperty(prefix="foo", name="bar", havingValue="defaultStringValue")

My main concern here is that currently it is counter-intuitive that @ConditionalOnProperty or bean accessed via ConditionContext don't respect @ConfigurationProperties contract. I personally can live with that, but this is issue that will confuse some users or even sneak some hard-to-spot bugs into their production environments. I love Spring Boot and I recommend it to all the users/customers I work with, so I just try to predict some issues that may occur :) .

Can you consider keeping this issue open? I understand that this can be a tricky beast but I believe that it is worth some investigation in the future.

Laters!

@philwebb
Copy link
Member

philwebb commented Jan 6, 2015

Yeah, lets keep this open and take a look when the 1.2 bugs die down. It might be possible to update @ConditionalOnProperty to direct inspect the class that it's on to see if it is a @ConfigurationProperties. I'm not very keen to couple those classes though.

@snicoll
Copy link
Member

snicoll commented Jan 6, 2015

you could also read the metadata at runtime. We're already doing that for the configprops endpoint.

@hekonsek
Copy link
Contributor Author

hekonsek commented Jan 8, 2015

Thanks guys.

@snicoll
Copy link
Member

snicoll commented Jan 10, 2018

@longfeizheng could you please stop pasting the same comment in several issues? If you have a question, please ask on StackOverflow or join us on Gitter.

@spring-projects spring-projects deleted a comment from longfeizheng Jan 10, 2018
@philwebb philwebb added this to the Icebox milestone Mar 22, 2018
@wilkinsona wilkinsona changed the title @ConfigurationProperties are not available in conditions Make @ConfigurationProperties available in conditions Aug 29, 2019
@jamesmehorter
Copy link

jamesmehorter commented Jan 16, 2020

👋 Hi all, Seems I may have just stumbled into this issue as well? Please correct me if I'm mistaken

// application.properties
foo.bar=raboof

// Config.kt
@ConfigurationProperties(prefix = "foo")
data class Config {
    var bar: String? = null
}

// Bean.kt
@Bean
@Conditional(HasBar::class)
fun something( ... ) {
    ...
}

// HasBar.kt
@EnableConfigurationProperties(Config::class)
class HasBar(
    //val config: Config // "No argument provided for a required parameter"
) : Condition {
    override fun matches(context: ConditionContext, ...) {
        val config = context.beanFactory?.getBean('Config') as Config
        println(config) // outputs Config[bar=null]

        println(context.environment.getProperty("foo.bar")) // outputs raboof
        ...
    }
}

@wilkinsona
Copy link
Member

@jamesmehorter You can't safely retrieve beans from the context during condition evaluation as they are called too early in the application context's lifecycle.

@tokrug
Copy link

tokrug commented Dec 14, 2020

While @ConfigurationProperties are not available during @Conditional annotations execution you can still convert properties into POJOs with the use of Binder class. This means that properties will be bound to configuration classes mutliple times during application startup but it's a relatively small price to pay for being able to execute any logic on mapped properties.

Just create a custom conditional implementation and use this line.

Object bean = Binder.get(context.getEnvironment())
    .bind("your.properties.prefix", YourConfigurationPropertiesClass.class).orElse(null);

Here's a gist with full solution.

// EDIT: updated the link to gist

@michaelzangl
Copy link

@tokrug The gist does not work any more. Do you still have it somewhere?

@tokrug
Copy link

tokrug commented Jul 23, 2021

@michaelzangl I have updated the link in the original comment.

@albertocavalcante
Copy link

Is this use case being considered to be first-class supported in Spring Boot? Thanks

@philwebb
Copy link
Member

@albertocavalcante The issue is open an in the "General Backlog" milestone which means we're still considering it. Having said that, this we have a number of other tasks that are consuming a lot of our bandwidth so we're very unlikely to get to this one anytime soon.

@rajjaiswalsaumya
Copy link

@philwebb Can you please revisit this and consider this one?

@philwebb
Copy link
Member

@rajjaiswalsaumya I'm afraid the comment just above yours still stands. This isn't a particularly high priority compared to the other work we have on.

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

No branches or pull requests

9 participants