SpringBoot Short circuit checking of source already covered by ConfigurationPropertySources

(edited with updated description)

Currently ConfigurationPropertySources is added to the environment so that relaxed property resolution can work directly. The ConfigurationPropertySources acts as a facade over the existing property sources.

When making a call such as environment.getProperty("test"), if the property is found then ConfigurationPropertySources will return the result. If, however, the property is not found by ConfigurationPropertySources the PropertySourcesPropertyResolver continues to check all subsequent sources. This is a little inefficient for any source that has already been check via ConfigurationPropertySources.

---

Original report:

getProperty(String key, Class<T> targetValueType, boolean resolveNestedPlaceholders) in org.springframework.core.env.PropertySourcesPropertyResolver traverses all PropertySources, so why use ConfigurationPropertySourcesPropertySource to wrap these PropertySources collections, and ConfigurationPropertySourcesPropertySource is traversed as well? So it seems that after ConfigurationPropertySourcesPropertySource has been traversed, it seems meaningless that the remaining elements are traversed, because ConfigurationPropertySourcesPropertySource has been checked

Comment From: philwebb

I'm sorry but I don't understand what you're reporting. If you think you've found a bug can you please provide a sample application that shows the problem.

Comment From: brucelwl

@philwebb I want to know why to call method ConfigurationPropertySources.attach(environment); in method SpringApplication.prepareEnvironment(...) , It just contains the PropertySource collection and then puts it in environment.

So when invoking environment.getProperty("user.password"), query ConfigurationPropertySourcesPropertySource("configurationProperties") first, but if the attribute value cannot be found in configurationProperties, It will not be queried in the rest of the collection, because configurationProperties already contains the following set .

commandLineArgs
servletConfigInitParams
servletContextInitParams
systemProperties
systemEnvironment
random
applicationConfig: [classpath:/application.properties]

I just want to express that because ConfigurationPropertySources.attach(environment); is called, causes traversal lookups in PropertySourcesPropertyResolver.getProperty(...) to be redundant. So I want to know why to call ConfigurationPropertySources.attach(environment);

Comment From: philwebb

I've edited your comment to improve the formatting. You might want to check out this Mastering Markdown guide for future reference.

Comment From: philwebb

@brucelwl The issue tracker isn't the place for these kinds of discussions as It adds a lot of noise for everyone subscribed. We prefer to keep it for bugs or feature requests. Questions like this can be asked at stackoverflow.com or gitter.im.

Comment From: brucelwl

@philwebb I'm not just asking why, it's also a logical error, because it leads to a redundant loop traversal

Comment From: philwebb

@brucelwl I don't believe there's a logical error, but if you can provide a sample that shows what you mean I'm happy to take a look. The ConfigurationPropertySourcesPropertySource allows you to access Spring Boot properties using the standard environment. So for example, you can call environment.getProperty("customer.someExample") and it will find customer.some-example from the application.properties and CUSTOMER_SOMEEXAMPLE from the system environment. The PropertySourcesPropertyResolver class returns on the first non-null result.

Comment From: brucelwl

Please take a look at my demo, https://github.com/brucelwl/demo the code is very simple is to call environment.getProperty("user.password"), but you may need to add a breakpoint in PropertySourcesPropertyResolver.getProperty(...), at line 81. we can see that the collection this.propertySources contains the following elements: configurationProperties commandLineArgs servletConfigInitParams servletContextInitParams systemProperties systemEnvironment random applicationConfig: [classpath:/application.properties]

But we can see that the first PropertySource configurationProperties also contains the following collection elements: commandLineArgs servletConfigInitParams servletContextInitParams systemProperties systemEnvironment random applicationConfig: [classpath:/application.properties]

When using a loop to look up a key-value pair from each propertySource, if it can't be found from the first PropertySource configurationProperties, then the rest of the PropertySource will definitely not be found, so there is no need to continue looking

Comment From: philwebb

@brucelwl Thanks for the sample, I see what you're getting at now. The checking of subsequent property sources is only short circuited if the value is found. If the value is not found then we search the source again.

Unfortunately it's not that easy to fix this one. We might be able to improve things if we can change PropertySourcesPropertyResolver in Spring Framework to somehow be aware that a property source also covers other sources.

Comment From: brucelwl

@philwebb I don't think it is necessary to add ConfigurationPropertySourcesPropertySource to MutablePropertySources,just need to be a static variable of ConfigurationPropertySources.this will solve the problem. of course, that's just my suggestion. here is my sample code:

public class MyConfigurationPropertySources {

    private static final String ATTACHED_PROPERTY_SOURCE_NAME = "configurationProperties";

    private static ConfigurationPropertySourcesPropertySource configurationSources;

    private MyConfigurationPropertySources() {
    }


    public static boolean isAttachedConfigurationPropertySource(PropertySource<?> propertySource) {
        return ATTACHED_PROPERTY_SOURCE_NAME.equals(propertySource.getName());
    }


    public static void attach(Environment environment) {
        Assert.isInstanceOf(ConfigurableEnvironment.class, environment);
        MutablePropertySources sources = ((ConfigurableEnvironment) environment).getPropertySources();
        //PropertySource<?> attached = sources.get(ATTACHED_PROPERTY_SOURCE_NAME);
        //if (attached != null && attached.getSource() != sources) {
        //    sources.remove(ATTACHED_PROPERTY_SOURCE_NAME);
        //    attached = null;
        //}
        //if (attached == null) {
        //    sources.addFirst(new ConfigurationPropertySourcesPropertySource(ATTACHED_PROPERTY_SOURCE_NAME,
        //            new SpringConfigurationPropertySources(sources)));
        //}

        configurationSources = new ConfigurationPropertySourcesPropertySource(ATTACHED_PROPERTY_SOURCE_NAME,
                new SpringConfigurationPropertySources(sources));
    }

    public static Iterable<ConfigurationPropertySource> get(Environment environment) {
        Assert.isInstanceOf(ConfigurableEnvironment.class, environment);
        MutablePropertySources sources = ((ConfigurableEnvironment) environment).getPropertySources();
        //ConfigurationPropertySourcesPropertySource attached = (ConfigurationPropertySourcesPropertySource) sources
        //        .get(ATTACHED_PROPERTY_SOURCE_NAME);
        ConfigurationPropertySourcesPropertySource attached = configurationSources;
        if (attached == null) {
            return from(sources);
        }
        return attached.getSource();
    }

    public static Iterable<ConfigurationPropertySource> from(PropertySource<?> source) {
        return Collections.singleton(SpringConfigurationPropertySource.from(source));
    }


    public static Iterable<ConfigurationPropertySource> from(Iterable<PropertySource<?>> sources) {
        return new SpringConfigurationPropertySources(sources);
    }

    private static Stream<PropertySource<?>> streamPropertySources(PropertySources sources) {
        return sources.stream().flatMap(org.springframework.boot.context.properties.source.MyConfigurationPropertySources::flatten)
                .filter(org.springframework.boot.context.properties.source.MyConfigurationPropertySources::isIncluded);
    }

    private static Stream<PropertySource<?>> flatten(PropertySource<?> source) {
        if (source.getSource() instanceof ConfigurableEnvironment) {
            return streamPropertySources(((ConfigurableEnvironment) source.getSource()).getPropertySources());
        }
        return Stream.of(source);
    }

    private static boolean isIncluded(PropertySource<?> source) {
        return !(source instanceof PropertySource.StubPropertySource)
                && !(source instanceof ConfigurationPropertySourcesPropertySource);
    }
}

Comment From: philwebb

@brucelwl A static won't work as an application can have more than one Environment.

Comment From: brucelwl

@philwebb Maybe we can save it by Map<Environment,ConfigurationPropertySourcesPropertySource> configurationSources

public class MyConfigurationPropertySources {

    private static final String ATTACHED_PROPERTY_SOURCE_NAME = "configurationProperties";

    private static Map<Environment,ConfigurationPropertySourcesPropertySource> allEnvironmentSources`;

    private MyConfigurationPropertySources() {
    }
   .....
}

Comment From: dngzs

Excuse me, have you fixed it?

Comment From: wilkinsona

No, not yet. This issue is still open and currently assigned to the general backlog.

Comment From: SeifMostafa

Hello, Any help?

Comment From: dngzs

Did you finally fix it with cache？

Comment From: philwebb

@dngzs The issue is still open