Take care with order of property sources in /refresh endpoint

It's kind of a corner case, but not a very small corner: if a /refresh
leads to a change in a key that was overridden from its default value (e.g.
n a profile-specific config file) then the key is not identified as changed.
The change is still applied.

This commit fixes the key computation and adds a couple of tests.

Fixes gh-73

spring-cloud-context/src/main/java/org/springframework/cloud/endpoint/RefreshEndpoint.java

@@ -16,13 +16,16 @@
 
 package org.springframework.cloud.endpoint;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import org.springframework.boot.Banner.Mode;
 import org.springframework.boot.actuate.endpoint.AbstractEndpoint;
 import org.springframework.boot.builder.SpringApplicationBuilder;
 import org.springframework.boot.context.properties.ConfigurationProperties;
@@ -49,12 +52,12 @@ import org.springframework.web.context.support.StandardServletEnvironment;
 @ManagedResource
 public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 
-	private Set<String> standardSources = new HashSet<String>(Arrays.asList(
-			StandardEnvironment.SYSTEM_PROPERTIES_PROPERTY_SOURCE_NAME,
-			StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME,
-			StandardServletEnvironment.JNDI_PROPERTY_SOURCE_NAME,
-			StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME,
-			StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME));
+	private Set<String> standardSources = new HashSet<String>(
+			Arrays.asList(StandardEnvironment.SYSTEM_PROPERTIES_PROPERTY_SOURCE_NAME,
+					StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME,
+					StandardServletEnvironment.JNDI_PROPERTY_SOURCE_NAME,
+					StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME,
+					StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME));
 
 	private ConfigurableApplicationContext context;
 
@@ -68,8 +71,8 @@ public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 
 	@ManagedOperation
 	public synchronized String[] refresh() {
-		Map<String, Object> before = extract(this.context.getEnvironment()
-				.getPropertySources());
+		Map<String, Object> before = extract(
+				this.context.getEnvironment().getPropertySources());
 		addConfigFilesToEnvironment();
 		Set<String> keys = changes(before,
 				extract(this.context.getEnvironment().getPropertySources())).keySet();
@@ -81,23 +84,33 @@ public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 	private void addConfigFilesToEnvironment() {
 		ConfigurableApplicationContext capture = null;
 		try {
-			StandardEnvironment environment = copyEnvironment(this.context.getEnvironment());
-			capture = new SpringApplicationBuilder(Empty.class).showBanner(false)
+			StandardEnvironment environment = copyEnvironment(
+					this.context.getEnvironment());
+			capture = new SpringApplicationBuilder(Empty.class).bannerMode(Mode.OFF)
 					.web(false).environment(environment).run();
 			MutablePropertySources target = this.context.getEnvironment()
 					.getPropertySources();
+			String targetName = null;
 			for (PropertySource<?> source : environment.getPropertySources()) {
 				String name = source.getName();
+				if (target.contains(name)) {
+					targetName = name;
+				}
 				if (!this.standardSources.contains(name)) {
 					if (target.contains(name)) {
 						target.replace(name, source);
 					}
 					else {
-						if (target.contains("defaultProperties")) {
-							target.addBefore("defaultProperties", source);
+						if (targetName != null) {
+							target.addAfter(targetName, source);
 						}
 						else {
-							target.addLast(source);
+							if (target.contains("defaultProperties")) {
+								target.addBefore("defaultProperties", source);
+							}
+							else {
+								target.addLast(source);
+							}
 						}
 					}
 				}
@@ -118,16 +131,15 @@ public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 		}
 	}
 
-	// Don't use ConfigurableEnvironment.merge() in case there are clashes with property source names
+	// Don't use ConfigurableEnvironment.merge() in case there are clashes with property
+	// source names
 	private StandardEnvironment copyEnvironment(ConfigurableEnvironment input) {
 		StandardEnvironment environment = new StandardEnvironment();
-		MutablePropertySources capturedPropertySources = environment
-				.getPropertySources();
+		MutablePropertySources capturedPropertySources = environment.getPropertySources();
 		for (PropertySource<?> source : capturedPropertySources) {
 			capturedPropertySources.remove(source.getName());
 		}
-		for (PropertySource<?> source : input
-				.getPropertySources()) {
+		for (PropertySource<?> source : input.getPropertySources()) {
 			capturedPropertySources.addLast(source);
 		}
 		environment.setActiveProfiles(input.getActiveProfiles());
@@ -171,9 +183,13 @@ public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 
 	private Map<String, Object> extract(MutablePropertySources propertySources) {
 		Map<String, Object> result = new HashMap<String, Object>();
-		for (PropertySource<?> parent : propertySources) {
-			if (!this.standardSources.contains(parent.getName())) {
-				extract(parent, result);
+		List<PropertySource<?>> sources = new ArrayList<PropertySource<?>>();
+		for (PropertySource<?> source : propertySources) {
+			sources.add(0, source);
+		}
+		for (PropertySource<?> source : sources) {
+			if (!this.standardSources.contains(source.getName())) {
+				extract(source, result);
 			}
 		}
 		return result;
@@ -182,8 +198,12 @@ public class RefreshEndpoint extends AbstractEndpoint<Collection<String>> {
 	private void extract(PropertySource<?> parent, Map<String, Object> result) {
 		if (parent instanceof CompositePropertySource) {
 			try {
+				List<PropertySource<?>> sources = new ArrayList<PropertySource<?>>();
 				for (PropertySource<?> source : ((CompositePropertySource) parent)
 						.getPropertySources()) {
+					sources.add(0, source);
+				}
+				for (PropertySource<?> source : sources) {
 					extract(source, result);
 				}
 			}

spring-cloud-context/src/test/java/org/springframework/cloud/endpoint/RefreshEndpointTests.java

@@ -16,17 +16,17 @@
 
 package org.springframework.cloud.endpoint;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
 import java.lang.reflect.Field;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
 import org.junit.After;
 import org.junit.Test;
+import org.springframework.boot.Banner.Mode;
 import org.springframework.boot.builder.SpringApplicationBuilder;
+import org.springframework.boot.test.EnvironmentTestUtils;
 import org.springframework.cloud.context.environment.EnvironmentChangeEvent;
 import org.springframework.cloud.context.scope.refresh.RefreshScope;
 import org.springframework.cloud.context.scope.refresh.RefreshScopeRefreshedEvent;
@@ -37,6 +37,9 @@ import org.springframework.context.event.EventListener;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.ReflectionUtils;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
 /**
  * @author Dave Syer
  *
@@ -47,15 +50,42 @@ public class RefreshEndpointTests {
 
 	@After
 	public void close() {
-		if (this.context!=null) {
+		if (this.context != null) {
 			this.context.close();
 		}
 	}
 
+	@Test
+	public void keysComputedWhenAdded() throws Exception {
+		this.context = new SpringApplicationBuilder(Empty.class).web(false)
+				.bannerMode(Mode.OFF).properties("spring.cloud.bootstrap.name:none")
+				.run();
+		RefreshScope scope = new RefreshScope();
+		scope.setApplicationContext(this.context);
+		EnvironmentTestUtils.addEnvironment(this.context, "spring.profiles.active=local");
+		RefreshEndpoint endpoint = new RefreshEndpoint(this.context, scope);
+		Collection<String> keys = endpoint.invoke();
+		assertTrue("Wrong keys: " + keys, keys.contains("added"));
+	}
+
+	@Test
+	public void keysComputedWhenOveridden() throws Exception {
+		this.context = new SpringApplicationBuilder(Empty.class).web(false)
+				.bannerMode(Mode.OFF).properties("spring.cloud.bootstrap.name:none")
+				.run();
+		RefreshScope scope = new RefreshScope();
+		scope.setApplicationContext(this.context);
+		EnvironmentTestUtils.addEnvironment(this.context,
+				"spring.profiles.active=override");
+		RefreshEndpoint endpoint = new RefreshEndpoint(this.context, scope);
+		Collection<String> keys = endpoint.invoke();
+		assertTrue("Wrong keys: " + keys, keys.contains("message"));
+	}
+
 	@Test
 	public void eventsPublishedInOrder() throws Exception {
-		this.context = new SpringApplicationBuilder(Empty.class)
-				.web(false).showBanner(false).run();
+		this.context = new SpringApplicationBuilder(Empty.class).web(false)
+				.bannerMode(Mode.OFF).run();
 		RefreshScope scope = new RefreshScope();
 		scope.setApplicationContext(this.context);
 		RefreshEndpoint endpoint = new RefreshEndpoint(this.context, scope);
@@ -69,7 +99,7 @@ public class RefreshEndpointTests {
 	@Test
 	public void shutdownHooksCleaned() {
 		ConfigurableApplicationContext context = new SpringApplicationBuilder(Empty.class)
-				.web(false).showBanner(false).run();
+				.web(false).bannerMode(Mode.OFF).run();
 		RefreshScope scope = new RefreshScope();
 		scope.setApplicationContext(context);
 		RefreshEndpoint endpoint = new RefreshEndpoint(context, scope);
@@ -92,10 +122,12 @@ public class RefreshEndpointTests {
 	@Configuration
 	protected static class Empty {
 		private List<ApplicationEvent> events = new ArrayList<ApplicationEvent>();
+
 		@EventListener(EnvironmentChangeEvent.class)
 		public void changed(EnvironmentChangeEvent event) {
 			this.events.add(event);
 		}
+
 		@EventListener(RefreshScopeRefreshedEvent.class)
 		public void refreshed(RefreshScopeRefreshedEvent event) {
 			this.events.add(event);

spring-cloud-context/src/test/resources/application-local.properties

@@ -0,0 +1 @@
+added: Hello added!

spring-cloud-context/src/test/resources/application-override.properties

@@ -0,0 +1 @@
+message: Hello override!