Merge pull request #16717 from dreis2211

* pr/16717: Polish "Optimize CacheKey handling for immutable sources" Optimize CacheKey handling for immutable sources Closes gh-16717
spring-projects · Jun 4, 2019 · 05ad955 · 05ad955
2 parents 071410c + 3bfc923
commit 05ad955
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 7 deletions.
diff --git a/...ngframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/...ngframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java
@@ -25,9 +25,11 @@
 import java.util.Set;
 import java.util.stream.Stream;
 
+import org.springframework.boot.env.OriginTrackedMapPropertySource;
 import org.springframework.core.env.EnumerablePropertySource;
 import org.springframework.core.env.MapPropertySource;
 import org.springframework.core.env.PropertySource;
+import org.springframework.core.env.StandardEnvironment;
 import org.springframework.core.env.SystemEnvironmentPropertySource;
 import org.springframework.util.ObjectUtils;
 
@@ -190,13 +192,19 @@ public void setMappings(PropertyMapping[] mappings) {
 
 	private static final class CacheKey {
 
+		private static final CacheKey IMMUTABLE_PROPERTY_SOURCE = new CacheKey(
+				new Object[0]);
+
 		private final Object key;
 
 		private CacheKey(Object key) {
 			this.key = key;
 		}
 
 		public CacheKey copy() {
+			if (this == IMMUTABLE_PROPERTY_SOURCE) {
+				return IMMUTABLE_PROPERTY_SOURCE;
+			}
 			return new CacheKey(copyKey(this.key));
 		}
 
@@ -215,7 +223,8 @@ public boolean equals(Object obj) {
 			if (obj == null || getClass() != obj.getClass()) {
 				return false;
 			}
-			return ObjectUtils.nullSafeEquals(this.key, ((CacheKey) obj).key);
+			CacheKey otherCacheKey = (CacheKey) obj;
+			return ObjectUtils.nullSafeEquals(this.key, otherCacheKey.key);
 		}
 
 		@Override
@@ -224,12 +233,27 @@ public int hashCode() {
 		}
 
 		public static CacheKey get(EnumerablePropertySource<?> source) {
+			if (isImmutable(source)) {
+				return IMMUTABLE_PROPERTY_SOURCE;
+			}
 			if (source instanceof MapPropertySource) {
-				return new CacheKey(((MapPropertySource) source).getSource().keySet());
+				MapPropertySource mapPropertySource = (MapPropertySource) source;
+				return new CacheKey(mapPropertySource.getSource().keySet());
 			}
 			return new CacheKey(source.getPropertyNames());
 		}
 
+		private static boolean isImmutable(EnumerablePropertySource<?> source) {
+			if (source instanceof OriginTrackedMapPropertySource) {
+				return ((OriginTrackedMapPropertySource) source).isImmutable();
+			}
+			if (StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME
+					.equals(source.getName())) {
+				return source.getSource() == System.getenv();
+			}
+			return false;
+		}
+
 	}
 
 }
diff --git a/...pring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java b/...pring-boot/src/main/java/org/springframework/boot/env/OriginTrackedMapPropertySource.java
@@ -22,6 +22,7 @@
 import org.springframework.boot.origin.OriginLookup;
 import org.springframework.boot.origin.OriginTrackedValue;
 import org.springframework.core.env.MapPropertySource;
+import org.springframework.core.env.PropertySource;
 
 /**
  * {@link OriginLookup} backed by a {@link Map} containing {@link OriginTrackedValue
@@ -35,9 +36,28 @@
 public final class OriginTrackedMapPropertySource extends MapPropertySource
 		implements OriginLookup<String> {
 
-	@SuppressWarnings({ "unchecked", "rawtypes" })
+	private final boolean immutable;
+
+	/**
+	 * Create a new {@link OriginTrackedMapPropertySource} instance.
+	 * @param name the property source name
+	 * @param source the underlying map source
+	 */
+	@SuppressWarnings("rawtypes")
 	public OriginTrackedMapPropertySource(String name, Map source) {
+		this(name, source, false);
+	}
+
+	/**
+	 * Create a new {@link OriginTrackedMapPropertySource} instance.
+	 * @param name the property source name
+	 * @param source the underlying map source
+	 * @param immutable if the underlying source is immutable and guaranteed not to change
+	 */
+	@SuppressWarnings({ "unchecked", "rawtypes" })
+	public OriginTrackedMapPropertySource(String name, Map source, boolean immutable) {
 		super(name, source);
+		this.immutable = immutable;
 	}
 
 	@Override
@@ -58,4 +78,13 @@ public Origin getOrigin(String name) {
 		return null;
 	}
 
+	/**
+	 * Return {@code true} if this {@link PropertySource} is immutable and has contents
+	 * that will never change.
+	 * @return if the property source is read only
+	 */
+	public boolean isImmutable() {
+		return this.immutable;
+	}
+
 }
diff --git a/...pring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java b/...pring-boot/src/main/java/org/springframework/boot/env/PropertiesPropertySourceLoader.java
@@ -48,8 +48,8 @@ public List<PropertySource<?>> load(String name, Resource resource)
 		if (properties.isEmpty()) {
 			return Collections.emptyList();
 		}
-		return Collections
-				.singletonList(new OriginTrackedMapPropertySource(name, properties));
+		return Collections.singletonList(new OriginTrackedMapPropertySource(name,
+				Collections.unmodifiableMap(properties), true));
 	}
 
 	@SuppressWarnings({ "unchecked", "rawtypes" })

diff --git a/...ject/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java b/...ject/spring-boot/src/main/java/org/springframework/boot/env/YamlPropertySourceLoader.java
@@ -55,7 +55,7 @@ public List<PropertySource<?>> load(String name, Resource resource)
 		for (int i = 0; i < loaded.size(); i++) {
 			String documentNumber = (loaded.size() != 1) ? " (document #" + i + ")" : "";
 			propertySources.add(new OriginTrackedMapPropertySource(name + documentNumber,
-					loaded.get(i)));
+					Collections.unmodifiableMap(loaded.get(i)), true));
 		}
 		return propertySources;
 	}

diff --git a/...mework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/...mework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java
@@ -25,6 +25,7 @@
 
 import org.junit.Test;
 
+import org.springframework.boot.env.OriginTrackedMapPropertySource;
 import org.springframework.boot.origin.Origin;
 import org.springframework.boot.origin.OriginLookup;
 import org.springframework.core.env.EnumerablePropertySource;
@@ -160,7 +161,7 @@ public void containsDescendantOfShouldCheckSourceNames() {
 	}
 
 	@Test
-	public void propertySourceKeyDataChangeInvalidatesCache() {
+	public void simpleMapPropertySourceKeyDataChangeInvalidatesCache() {
 		// gh-13344
 		Map<String, Object> map = new LinkedHashMap<>();
 		map.put("key1", "value1");
@@ -184,10 +185,36 @@ public void concurrentModificationExceptionInvalidatesCache() {
 				source, DefaultPropertyMapper.INSTANCE);
 		assertThat(adapter.stream().count()).isEqualTo(2);
 		map.setThrowException(true);
+	}
+
+	public void originTrackedMapPropertySourceKeyAdditionInvalidatesCache() {
+		// gh-13344
+		Map<String, Object> map = new LinkedHashMap<>();
+		map.put("key1", "value1");
+		map.put("key2", "value2");
+		EnumerablePropertySource<?> source = new OriginTrackedMapPropertySource("test",
+				map);
+		SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource(
+				source, DefaultPropertyMapper.INSTANCE);
+		assertThat(adapter.stream().count()).isEqualTo(2);
 		map.put("key3", "value3");
 		assertThat(adapter.stream().count()).isEqualTo(3);
 	}
 
+	public void readOnlyOriginTrackedMapPropertySourceKeyAdditionDoesNotInvalidateCache() {
+		// gh-16717
+		Map<String, Object> map = new LinkedHashMap<>();
+		map.put("key1", "value1");
+		map.put("key2", "value2");
+		EnumerablePropertySource<?> source = new OriginTrackedMapPropertySource("test",
+				map, true);
+		SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource(
+				source, DefaultPropertyMapper.INSTANCE);
+		assertThat(adapter.stream().count()).isEqualTo(2);
+		map.put("key3", "value3");
+		assertThat(adapter.stream().count()).isEqualTo(2);
+	}
+
 	/**
 	 * Test {@link PropertySource} that's also an {@link OriginLookup}.
 	 */