From c5382ccfd6144c5f1d9c6c633052f27bbcbd10bd Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 15 Dec 2017 14:13:41 +0000 Subject: [PATCH] Refactor ConfigurationPropertiesRebinder a bit It's not super expensive, but it all adds up, and rebinding beans that don't need to be rebound is wasteful. This change should improve things in three places: The ConfigurationPropertiesRebinder has always done a pre-emptive re-bind on startup, which was a bit of a blunt instrument, since all we really wanted as to rebind the beans in the parent context (to the current context's Environment). The rebinding always happened twice for every bean because we explicitly called into the ConfigurationPropertiesPostProcessor directly as well as indirectly through the initialization callbacks. Rebinding in response to an EnvironmentChangeEvent was not able to distinguish between events published locally and by child contexts. The child context always binds the parent's beans anyway, so you don't need to do it twice. --- ...onPropertiesRebinderAutoConfiguration.java | 27 +++++----- .../PropertySourceBootstrapConfiguration.java | 10 ++-- ...ironmentDecryptApplicationInitializer.java | 13 +++-- .../environment/EnvironmentChangeEvent.java | 9 +++- .../environment/EnvironmentManager.java | 4 +- .../ConfigurationPropertiesRebinder.java | 16 +++--- .../context/refresh/ContextRefresher.java | 12 +++-- ...ionPropertiesRebinderIntegrationTests.java | 49 +++++++++++++++---- .../cloud/logging/LoggingRebinderTests.java | 5 +- .../resources/application-config.properties | 1 + .../resources/bootstrap-config.properties | 2 + 11 files changed, 101 insertions(+), 47 deletions(-) create mode 100644 spring-cloud-context/src/test/resources/application-config.properties create mode 100644 spring-cloud-context/src/test/resources/bootstrap-config.properties diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java index fae23c21..65c09423 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java @@ -16,8 +16,6 @@ package org.springframework.cloud.autoconfigure; -import java.util.Collections; - import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -25,7 +23,6 @@ import org.springframework.boot.autoconfigure.condition.SearchStrategy; import org.springframework.boot.context.properties.ConfigurationBeanFactoryMetaData; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessorRegistrar; -import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.cloud.context.properties.ConfigurationPropertiesBeans; import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinder; import org.springframework.context.ApplicationContext; @@ -64,19 +61,27 @@ public class ConfigurationPropertiesRebinderAutoConfiguration @Bean @ConditionalOnMissingBean(search = SearchStrategy.CURRENT) public ConfigurationPropertiesRebinder configurationPropertiesRebinder( - ConfigurationPropertiesBeans beans, - ConfigurationPropertiesBindingPostProcessor binder) { + ConfigurationPropertiesBeans beans) { ConfigurationPropertiesRebinder rebinder = new ConfigurationPropertiesRebinder( - binder, beans); + beans); return rebinder; } @Override public void afterSingletonsInstantiated() { - // After all beans are initialized send a pre-emptive EnvironmentChangeEvent - // so that anything that needs to rebind gets a chance now (especially for - // beans in the parent context) - this.context.publishEvent( - new EnvironmentChangeEvent(Collections. emptySet())); + // After all beans are initialized explicitly rebind beans from the parent + // so that changes during the initialization of the current context are + // reflected. In particular this can be important when low level services like + // decryption are bootstrapped in the parent, but need to change their + // configuration before the child context is processed. + if (this.context.getParent() != null) { + // TODO: make this optional? (E.g. when creating child contexts that prefer to + // be isolated.) + ConfigurationPropertiesRebinder rebinder = context + .getBean(ConfigurationPropertiesRebinder.class); + for (String name : context.getParent().getBeanDefinitionNames()) { + rebinder.rebind(name); + } + } } } \ No newline at end of file diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java index fbbc9ed3..376f3efa 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/config/PropertySourceBootstrapConfiguration.java @@ -26,6 +26,7 @@ import java.util.TreeSet; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.bind.PropertySourcesPropertyValues; import org.springframework.boot.bind.RelaxedDataBinder; @@ -107,7 +108,7 @@ public class PropertySourceBootstrapConfiguration implements } insertPropertySources(propertySources, composite); reinitializeLoggingSystem(environment, logConfig, logFile); - setLogLevels(environment); + setLogLevels(applicationContext, environment); handleIncludedProfiles(environment); } } @@ -139,13 +140,14 @@ public class PropertySourceBootstrapConfiguration implements } } - private void setLogLevels(ConfigurableEnvironment environment) { + private void setLogLevels(ConfigurableApplicationContext applicationContext, + ConfigurableEnvironment environment) { LoggingRebinder rebinder = new LoggingRebinder(); rebinder.setEnvironment(environment); // We can't fire the event in the ApplicationContext here (too early), but we can // create our own listener and poke it (it doesn't need the key changes) - rebinder.onApplicationEvent( - new EnvironmentChangeEvent(Collections. emptySet())); + rebinder.onApplicationEvent(new EnvironmentChangeEvent(applicationContext, + Collections.emptySet())); } private void insertPropertySources(MutablePropertySources propertySources, diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java index 355c661b..ca5bbeeb 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java @@ -25,6 +25,7 @@ import java.util.regex.Pattern; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.cloud.bootstrap.BootstrapApplicationListener; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.context.ApplicationContext; @@ -96,8 +97,8 @@ public class EnvironmentDecryptApplicationInitializer implements if (!map.isEmpty()) { // We have some decrypted properties found.addAll(map.keySet()); - insert(applicationContext, - new SystemEnvironmentPropertySource(DECRYPTED_PROPERTY_SOURCE_NAME, map)); + insert(applicationContext, new SystemEnvironmentPropertySource( + DECRYPTED_PROPERTY_SOURCE_NAME, map)); } PropertySource bootstrap = propertySources .get(BootstrapApplicationListener.BOOTSTRAP_PROPERTY_SOURCE_NAME); @@ -115,7 +116,7 @@ public class EnvironmentDecryptApplicationInitializer implements // The parent is actually the bootstrap context, and it is fully // initialized, so we can fire an EnvironmentChangeEvent there to rebind // @ConfigurationProperties, in case they were encrypted. - parent.publishEvent(new EnvironmentChangeEvent(found)); + parent.publishEvent(new EnvironmentChangeEvent(parent, found)); } } @@ -214,8 +215,10 @@ public class EnvironmentDecryptApplicationInitializer implements if (COLLECTION_PROPERTY.matcher(key).matches()) { sourceHasDecryptedCollection = true; } - } else if (COLLECTION_PROPERTY.matcher(key).matches()){ - // put non-ecrypted properties so merging of index properties happens correctly + } + else if (COLLECTION_PROPERTY.matcher(key).matches()) { + // put non-ecrypted properties so merging of index properties + // happens correctly otherCollectionProperties.put(key, value); } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java index d0fb5e50..d58e24d8 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentChangeEvent.java @@ -32,10 +32,15 @@ public class EnvironmentChangeEvent extends ApplicationEvent { private Set keys; public EnvironmentChangeEvent(Set keys) { - super(keys); + // Backwards compatible constructor with less utility (practically no use at all) + this(keys, keys); + } + + public EnvironmentChangeEvent(Object context, Set keys) { + super(context); this.keys = keys; } - + /** * @return the keys */ diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java index d1addd47..12a89dd1 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/environment/EnvironmentManager.java @@ -68,7 +68,7 @@ public class EnvironmentManager implements ApplicationEventPublisherAware { Map result = new LinkedHashMap(map); if (!map.isEmpty()) { map.clear(); - publish(new EnvironmentChangeEvent(result.keySet())); + publish(new EnvironmentChangeEvent(publisher, result.keySet())); } return result; } @@ -88,7 +88,7 @@ public class EnvironmentManager implements ApplicationEventPublisherAware { if (!value.equals(environment.getProperty(name))) { map.put(name, value); - publish(new EnvironmentChangeEvent(Collections.singleton(name))); + publish(new EnvironmentChangeEvent(publisher, Collections.singleton(name))); } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java index 0b156443..e7d6693c 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java @@ -24,7 +24,6 @@ import org.springframework.aop.framework.Advised; import org.springframework.aop.support.AopUtils; import org.springframework.beans.BeansException; import org.springframework.boot.context.properties.ConfigurationProperties; -import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; import org.springframework.cloud.context.config.annotation.RefreshScope; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.context.ApplicationContext; @@ -55,16 +54,11 @@ public class ConfigurationPropertiesRebinder private ConfigurationPropertiesBeans beans; - private ConfigurationPropertiesBindingPostProcessor binder; - private ApplicationContext applicationContext; private Map errors = new ConcurrentHashMap<>(); - public ConfigurationPropertiesRebinder( - ConfigurationPropertiesBindingPostProcessor binder, - ConfigurationPropertiesBeans beans) { - this.binder = binder; + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { this.beans = beans; } @@ -102,7 +96,7 @@ public class ConfigurationPropertiesRebinder if (AopUtils.isCglibProxy(bean)) { bean = getTargetObject(bean); } - this.binder.postProcessBeforeInitialization(bean, name); + this.applicationContext.getAutowireCapableBeanFactory().destroyBean(bean); this.applicationContext.getAutowireCapableBeanFactory() .initializeBean(bean, name); return true; @@ -135,7 +129,11 @@ public class ConfigurationPropertiesRebinder @Override public void onApplicationEvent(EnvironmentChangeEvent event) { - rebind(); + if (this.applicationContext.equals(event.getSource()) + // Backwards compatible + || event.getKeys().equals(event.getSource())) { + rebind(); + } } } diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java index 6dd1f129..da405ac9 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/context/refresh/ContextRefresher.java @@ -34,9 +34,15 @@ public class ContextRefresher { private static final String REFRESH_ARGS_PROPERTY_SOURCE = "refreshArgs"; - private static final String[] DEFAULT_PROPERTY_SOURCES = new String[] { //order matters, cli args aren't first, things get messy + private static final String[] DEFAULT_PROPERTY_SOURCES = new String[] { // order + // matters, + // cli args + // aren't + // first, + // things get + // messy CommandLinePropertySource.COMMAND_LINE_PROPERTY_SOURCE_NAME, - "defaultProperties"}; + "defaultProperties" }; private Set standardSources = new HashSet<>( Arrays.asList(StandardEnvironment.SYSTEM_PROPERTIES_PROPERTY_SOURCE_NAME, @@ -59,7 +65,7 @@ public class ContextRefresher { addConfigFilesToEnvironment(); Set keys = changes(before, extract(this.context.getEnvironment().getPropertySources())).keySet(); - this.context.publishEvent(new EnvironmentChangeEvent(keys)); + this.context.publishEvent(new EnvironmentChangeEvent(context, keys)); this.scope.refreshAll(); return keys; } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java index 85ab65aa..fc7f3e86 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderIntegrationTests.java @@ -19,7 +19,9 @@ import javax.annotation.PostConstruct; import org.junit.Test; import org.junit.runner.RunWith; + import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -33,17 +35,22 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; import static org.junit.Assert.assertEquals; @RunWith(SpringRunner.class) @SpringBootTest(classes = TestConfiguration.class) +@ActiveProfiles("config") public class ConfigurationPropertiesRebinderIntegrationTests { @Autowired private TestProperties properties; + @Autowired + private ConfigProperties config; + @Autowired private ConfigurationPropertiesRebinder rebinder; @@ -54,44 +61,55 @@ public class ConfigurationPropertiesRebinderIntegrationTests { @DirtiesContext public void testSimpleProperties() throws Exception { assertEquals("Hello scope!", this.properties.getMessage()); - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...but don't refresh, so the bean stays the same: assertEquals("Hello scope!", this.properties.getMessage()); - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); + } + + @Test + @DirtiesContext + public void testRefreshInParent() throws Exception { + assertEquals("main", this.config.getName()); + // Change the dynamic property source... + EnvironmentTestUtils.addEnvironment(this.environment, "config.name=foo"); + // ...and then refresh, so the bean is re-initialized: + this.rebinder.rebind(); + assertEquals("foo", this.config.getName()); } @Test @DirtiesContext public void testRefresh() throws Exception { - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); assertEquals("Hello scope!", this.properties.getMessage()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...and then refresh, so the bean is re-initialized: this.rebinder.rebind(); assertEquals("Foo", this.properties.getMessage()); - assertEquals(3, this.properties.getCount()); + assertEquals(2, this.properties.getCount()); } @Test @DirtiesContext public void testRefreshByName() throws Exception { - assertEquals(2, this.properties.getCount()); + assertEquals(1, this.properties.getCount()); assertEquals("Hello scope!", this.properties.getMessage()); // Change the dynamic property source... EnvironmentTestUtils.addEnvironment(this.environment, "message:Foo"); // ...and then refresh, so the bean is re-initialized: this.rebinder.rebind("properties"); assertEquals("Foo", this.properties.getMessage()); - assertEquals(3, this.properties.getCount()); + assertEquals(2, this.properties.getCount()); } @Configuration @EnableConfigurationProperties @Import({ RefreshConfiguration.RebinderConfiguration.class, - PropertyPlaceholderAutoConfiguration.class }) + PropertyPlaceholderAutoConfiguration.class }) protected static class TestConfiguration { @Bean @@ -104,8 +122,8 @@ public class ConfigurationPropertiesRebinderIntegrationTests { // Hack out a protected inner class for testing protected static class RefreshConfiguration extends RefreshAutoConfiguration { @Configuration - protected static class RebinderConfiguration extends - ConfigurationPropertiesRebinderAutoConfiguration { + protected static class RebinderConfiguration + extends ConfigurationPropertiesRebinderAutoConfiguration { } } @@ -142,4 +160,17 @@ public class ConfigurationPropertiesRebinderIntegrationTests { } } + @ConfigurationProperties("config") + @ConditionalOnMissingBean(ConfigProperties.class) + public static class ConfigProperties { + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java index cb772bfb..646a29d8 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/logging/LoggingRebinderTests.java @@ -21,6 +21,7 @@ import org.junit.After; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import org.springframework.boot.logging.LogLevel; import org.springframework.boot.logging.LoggingSystem; import org.springframework.boot.test.util.EnvironmentTestUtils; @@ -52,7 +53,7 @@ public class LoggingRebinderTests { EnvironmentTestUtils.addEnvironment(environment, "logging.level.org.springframework.web=TRACE"); this.rebinder.setEnvironment(environment); - this.rebinder.onApplicationEvent(new EnvironmentChangeEvent( + this.rebinder.onApplicationEvent(new EnvironmentChangeEvent(environment, Collections.singleton("logging.level.org.springframework.web"))); assertTrue(this.logger.isTraceEnabled()); } @@ -64,7 +65,7 @@ public class LoggingRebinderTests { EnvironmentTestUtils.addEnvironment(environment, "logging.level.org.springframework.web=trace"); this.rebinder.setEnvironment(environment); - this.rebinder.onApplicationEvent(new EnvironmentChangeEvent( + this.rebinder.onApplicationEvent(new EnvironmentChangeEvent(environment, Collections.singleton("logging.level.org.springframework.web"))); assertTrue(this.logger.isTraceEnabled()); } diff --git a/spring-cloud-context/src/test/resources/application-config.properties b/spring-cloud-context/src/test/resources/application-config.properties new file mode 100644 index 00000000..d4408d3f --- /dev/null +++ b/spring-cloud-context/src/test/resources/application-config.properties @@ -0,0 +1 @@ +config.name: main diff --git a/spring-cloud-context/src/test/resources/bootstrap-config.properties b/spring-cloud-context/src/test/resources/bootstrap-config.properties new file mode 100644 index 00000000..d61d540a --- /dev/null +++ b/spring-cloud-context/src/test/resources/bootstrap-config.properties @@ -0,0 +1,2 @@ +spring.main.sources: org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderIntegrationTests.ConfigProperties +config.name: parent