spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.59k stars 40.56k forks source link

@ConfigurationProperties should have an option to fail fast for unresolvable properties #18816

Open anno1985 opened 4 years ago

anno1985 commented 4 years ago

When using placeholders in application.yaml (property files/externalised configuration) together with the @ConfigurationProperties annotation, there should be an option to have Spring fail fast at startup when a defined property is not found.

Example:

example.key: ${MY_ENV_VAR}
@ConfigurationProperties(prefix="example")
public class AcmeProperties {
  public String key;
  // Getters, setters, constructors omitted... 
}

Precondition: No environment variable MY_ENV_VAR defined.

Current behaviour: key above is populated with the verbatim String ${MY_ENV_VAR}

Expected/desired behaviour: An exception is thrown while the application is starting up.

Potential cause: Hardcoded flag in https://github.com/spring-projects/spring-boot/blob/v2.2.0.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/PropertySourcesPlaceholdersResolver.java#L51

Further information: https://stackoverflow.com/q/58622189/2018047

NB: Validating the String with @Validated after (failed) resolution is not the same as checking if resolution fails. Also, when no fail-fast is active, it might potentially be better to mirror System.getenv("MY_ENV_VAR")'s behaviour, and return null instead of the actual placeholder, ${MY_ENV_VAR}, verbatim.

philwebb commented 4 years ago

I believe we intentionally set ignoreUnresolvablePlaceholders to true to keep back compatibility with previous releases. I agree it would be nice to have a fail fast option.

philwebb commented 4 years ago

See also #8693 and #13202

philwebb commented 4 years ago

@mbhave also tracked down some internal discussion we had about this last time around:

From @wilkinsona

Is it by design that the 2.0 binder doesn’t ignore placeholder resolution failures? Someone on Gitter is complaining and I can’t remember if it’s intentional or not. Here’s a little app that starts fine with 1.5.x but fails with 2.0.x:

package com.example.demo;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.ConfigurableApplicationContext;

import com.example.demo.PlaceholderChangeApplication.ExampleProperties;

@SpringBootApplication
@EnableConfigurationProperties(ExampleProperties.class)
public class PlaceholderChangeApplication {

    public static void main(String[] args) {
        ConfigurableApplicationContext context =
                SpringApplication.run(PlaceholderChangeApplication.class,
                        "--com.example.alpha=Value with ${placeholder}");
        System.out.println(context.getBean(ExampleProperties.class).getAlpha());
    }

    @ConfigurationProperties(prefix="com.example")
    static class ExampleProperties {

        private String alpha;

        public String getAlpha() {
            return alpha;
        }

        public void setAlpha(String alpha) {
            this.alpha = alpha;
        }

    }

}

It works in 1.5.x because of this change that was in the early days of Spring Boot: https://github.com/spring-projects/spring-boot/commit/8922a6be4aa17495bc3303bc8898b797d2fc573f

philwebb commented 4 years ago

I think we intentionally rolled this back in 2.0 because the major upgrade was quite onerous already and we didn't want to add more friction.

camhashemi commented 4 years ago

we also desire this feature. it's easier to tell that a server won't start up if the process fails than if it goes into the current spring retry loop.

lacking this feature directly: are there any alternative paths to the same result?

tavin commented 4 years ago

lacking this feature directly: are there any alternative paths to the same result?

Yes:

    @Bean
    public static PropertySourcesPlaceholderConfigurer propertySourcesPlaceholderConfigurer() {
        return new PropertySourcesPlaceholderConfigurer();
    }

In other words, you can configure that bean yourself, instead of leaving it to spring boot, and by default PropertySourcesPlaceholderConfigurer does not ignore unresolvable properties.

philwebb commented 4 years ago

@tavin I'm afraid that only works for @Value annotations. We need to process application.properties before any @Bean definitions are loaded.

tavin commented 4 years ago

@tavin I'm afraid that only works for @Value annotations. We need to process application.properties before any @Bean definitions are loaded.

@philwebb Ah ok good to know, thanks. We're not using @ConfigurationProperties so it worked for our use case. This just happened to be the google result which seemed to fit my issue. I presume the fix for this will work for @Value usages as well?

keyzj commented 2 years ago

Ops, shooting up legs in production... Any updates on this? Maybe support API for configuration of this flag?

keyzj commented 2 years ago

Just not to be rude, i've come up with a workaround: you could use validation over your ConfigurationProperties and provide not valid default value for the required property. For example:

Configuration properties class:

@Data
@Component
@Validated
@ConfigurationProperties("my-config-props")
public class MyConfigProps {
    @NotBlank
    private String host;
}

application.yaml:

my-config-props:
  host: ${MY_CONFIG_PROPS_HOST:}
rfelgent commented 7 months ago

hi,

just a reminder that this problem still exists with Spring Boot 3.2.

There is no easy configuration flag for enabling/disabling exception-behaviour for not resolvable ENV during configuration property binding. This missing feature causes some headaches/work around on our code side when implementing fail-fast failure detection

@philwebb maybe I am thinking to "simple", but why not adding a System-Property as feature toggle ?

Something like (my suggestion)

        public PropertySourcesPlaceholdersResolver(Iterable<PropertySource<?>> sources, PropertyPlaceholderHelper helper) {
        this.sources = sources;
        this.helper = (helper != null) ? helper : new PropertyPlaceholderHelper(SystemPropertyUtils.PLACEHOLDER_PREFIX,
                SystemPropertyUtils.PLACEHOLDER_SUFFIX, SystemPropertyUtils.VALUE_SEPARATOR, 
                     Boolean.valueOf(System.getProperty("ignoreUnresolvablePlaceholders", "true"));
    }

instead of a hardcoded flag (current implementation)

        public PropertySourcesPlaceholdersResolver(Iterable<PropertySource<?>> sources, PropertyPlaceholderHelper helper) {
        this.sources = sources;
        this.helper = (helper != null) ? helper : new PropertyPlaceholderHelper(SystemPropertyUtils.PLACEHOLDER_PREFIX,
                SystemPropertyUtils.PLACEHOLDER_SUFFIX, SystemPropertyUtils.VALUE_SEPARATOR, true);
    }