From c556d2b58f90bac11d2d87a4f16b27e4592d80a2 Mon Sep 17 00:00:00 2001 From: Fahim Farook Date: Mon, 4 Jun 2018 00:06:13 +0530 Subject: [PATCH 1/2] Fix caching issues with map property sources Update `SpringIterableConfigurationPropertySource` so that the cache key from a `MapPropertySource` is invalidated when the map contents changes. Prior to this commit, the actual keys of the map were used as the key. This meant that if the underlying map changed, they key wouldn't be invalidated because it ultimately pointed to the same object instance. See gh-13344 --- ...ngIterableConfigurationPropertySource.java | 4 +--- .../LoggingApplicationListenerTests.java | 14 +++++++++++++- ...rableConfigurationPropertySourceTests.java | 19 +++++++++++++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index 04ee92ff8b..f4292fdf54 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -142,9 +142,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope } private Object getCacheKey() { - if (getPropertySource() instanceof MapPropertySource) { - return ((MapPropertySource) getPropertySource()).getSource().keySet(); - } + // gh-13344 return getPropertySource().getPropertyNames(); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java index f05cb1f5ec..276fd8ac68 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java @@ -76,6 +76,7 @@ import static org.hamcrest.Matchers.not; * @author Andy Wilkinson * @author Stephane Nicoll * @author Ben Hale + * @author Fahim Farook */ @RunWith(ModifiedClassPathRunner.class) @ClassPathExclusions("log4j*.jar") @@ -500,12 +501,23 @@ public class LoggingApplicationListenerTests { @Test public void environmentPropertiesIgnoreUnresolvablePlaceholders() { // gh-7719 + TestPropertySourceUtils.addInlinedPropertiesToEnvironment(this.context, + "logging.pattern.console=console ${doesnotexist}"); + this.initializer.initialize(this.context.getEnvironment(), + this.context.getClassLoader()); + assertThat(System.getProperty(LoggingSystemProperties.CONSOLE_LOG_PATTERN)) + .isEqualTo("console ${doesnotexist}"); + } + + @Test + public void environmentPropertiesResolvePlaceholders() { TestPropertySourceUtils.addInlinedPropertiesToEnvironment(this.context, "logging.pattern.console=console ${pid}"); this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader()); assertThat(System.getProperty(LoggingSystemProperties.CONSOLE_LOG_PATTERN)) - .isEqualTo("console ${pid}"); + .isEqualTo(this.context.getEnvironment() + .getProperty("logging.pattern.console")); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java index fd9a9d59b0..abd59f3c2b 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java @@ -37,6 +37,7 @@ import static org.mockito.Mockito.mock; * * @author Phillip Webb * @author Madhura Bhave + * @author Fahim Farook */ public class SpringIterableConfigurationPropertySourceTests { @@ -157,6 +158,24 @@ public class SpringIterableConfigurationPropertySourceTests { .isEqualTo(ConfigurationPropertyState.ABSENT); } + @SuppressWarnings("unchecked") + @Test + public void propertySourceChangeReflects() { + // gh-13344 + final Map source = new LinkedHashMap<>(); + source.put("key1", "value1"); + source.put("key2", "value2"); + final EnumerablePropertySource propertySource = new MapPropertySource("test", + source); + final SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( + propertySource, DefaultPropertyMapper.INSTANCE); + assertThat(adapter.stream().count()).isEqualTo(2); + + ((Map) adapter.getPropertySource().getSource()).put("key3", + "value3"); + assertThat(adapter.stream().count()).isEqualTo(3); + } + /** * Test {@link PropertySource} that's also an {@link OriginLookup}. */ From dc1c459cde74e73b621358ddf30c62110372595a Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 6 Jun 2018 14:55:20 -0700 Subject: [PATCH 2/2] Polish "Fix caching issues with map property sources" Refine the property source cache key fix so that a copy of the key is only taken when the values change. This allows us to retain the previous performance optimization of not creating unnecessary string arrays. Closes gh-13344 --- ...ngIterableConfigurationPropertySource.java | 55 ++++++++++++++++--- ...rableConfigurationPropertySourceTests.java | 20 +++---- 2 files changed, 56 insertions(+), 19 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index f4292fdf54..f300fc045d 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -18,8 +18,10 @@ package org.springframework.boot.context.properties.source; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Set; import java.util.stream.Stream; import org.springframework.core.env.EnumerablePropertySource; @@ -129,7 +131,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope } private Cache getCache() { - Object cacheKey = getCacheKey(); + CacheKey cacheKey = CacheKey.get(getPropertySource()); if (cacheKey == null) { return null; } @@ -137,15 +139,10 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope return this.cache; } this.cache = new Cache(); - this.cacheKey = cacheKey; + this.cacheKey = cacheKey.copy(); return this.cache; } - private Object getCacheKey() { - // gh-13344 - return getPropertySource().getPropertyNames(); - } - @Override protected EnumerablePropertySource getPropertySource() { return (EnumerablePropertySource) super.getPropertySource(); @@ -175,4 +172,48 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope } + private static final class CacheKey { + + private Object key; + + private CacheKey(Object key) { + this.key = key; + } + + public CacheKey copy() { + return new CacheKey(copyKey(this.key)); + } + + private Object copyKey(Object key) { + if (key instanceof Set) { + return new HashSet((Set) key); + } + return ((String[]) key).clone(); + } + + @Override + public int hashCode() { + return this.key.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + return ObjectUtils.nullSafeEquals(this.key, ((CacheKey) obj).key); + } + + public static CacheKey get(EnumerablePropertySource source) { + if (source instanceof MapPropertySource) { + return new CacheKey(((MapPropertySource) source).getSource().keySet()); + } + return new CacheKey(source.getPropertyNames()); + } + + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java index abd59f3c2b..f75fe647f4 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java @@ -158,21 +158,17 @@ public class SpringIterableConfigurationPropertySourceTests { .isEqualTo(ConfigurationPropertyState.ABSENT); } - @SuppressWarnings("unchecked") @Test - public void propertySourceChangeReflects() { + public void propertySourceKeyDataChangeInvalidatesCache() { // gh-13344 - final Map source = new LinkedHashMap<>(); - source.put("key1", "value1"); - source.put("key2", "value2"); - final EnumerablePropertySource propertySource = new MapPropertySource("test", - source); - final SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( - propertySource, DefaultPropertyMapper.INSTANCE); + Map map = new LinkedHashMap<>(); + map.put("key1", "value1"); + map.put("key2", "value2"); + EnumerablePropertySource source = new MapPropertySource("test", map); + SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( + source, DefaultPropertyMapper.INSTANCE); assertThat(adapter.stream().count()).isEqualTo(2); - - ((Map) adapter.getPropertySource().getSource()).put("key3", - "value3"); + map.put("key3", "value3"); assertThat(adapter.stream().count()).isEqualTo(3); }