From 522e3df394f9bb7c1001e17f1685d077f7d3e888 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 15 Mar 2018 16:09:48 -0700 Subject: [PATCH] Fix same name configuration properties adapting Update `SpringConfigurationPropertySources` adapter logic so that and exact instance match is used when checking if the cache can be used. Prior to this commit if a property source was replaced with one that had exactly the same name, the adapted version would not change. Fixes gh-12451 --- .../source/ConfigurationPropertySources.java | 3 +- .../SpringConfigurationPropertySources.java | 180 ++++++++---------- ...ringConfigurationPropertySourcesTests.java | 36 +++- 3 files changed, 109 insertions(+), 110 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySources.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySources.java index 002c9e084d..23ef8b3915 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySources.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/ConfigurationPropertySources.java @@ -124,7 +124,8 @@ public final class ConfigurationPropertySources { * This method will flatten any nested property sources and will filter all * {@link StubPropertySource stub property sources}. Updates to the underlying source, * identified by changes in the sources returned by its iterator, will be - * automatically tracked. + * automatically tracked. The underlying source should be thread safe, for example a + * {@link MutablePropertySources} * @param sources the Spring property sources to adapt * @return an {@link Iterable} containing newly adapted * {@link SpringConfigurationPropertySource} instances diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySources.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySources.java index c64b4ce6be..31be48dc0b 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySources.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySources.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,19 +16,20 @@ package org.springframework.boot.context.properties.source; -import java.util.ArrayList; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.Iterator; -import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import java.util.stream.StreamSupport; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.function.Function; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySource; import org.springframework.core.env.PropertySource.StubPropertySource; import org.springframework.util.Assert; -import org.springframework.util.ObjectUtils; +import org.springframework.util.ConcurrentReferenceHashMap; +import org.springframework.util.ConcurrentReferenceHashMap.ReferenceType; /** * Adapter to convert Spring's {@link MutablePropertySources} to @@ -41,9 +42,8 @@ class SpringConfigurationPropertySources private final Iterable> sources; - private volatile PropertySourcesKey lastKey; - - private volatile List adaptedSources; + private final Map, ConfigurationPropertySource> cache = new ConcurrentReferenceHashMap<>( + 16, ReferenceType.SOFT); SpringConfigurationPropertySources(Iterable> sources) { Assert.notNull(sources, "Sources must not be null"); @@ -52,107 +52,81 @@ class SpringConfigurationPropertySources @Override public Iterator iterator() { - checkForChanges(); - return this.adaptedSources.iterator(); + return new SourcesIterator(this.sources.iterator(), this::adapt); } - private void checkForChanges() { - PropertySourcesKey lastKey = this.lastKey; - PropertySourcesKey currentKey = new PropertySourcesKey(this.sources); - if (!currentKey.equals(lastKey)) { - onChange(this.sources); - this.lastKey = currentKey; - } - } - - private void onChange(Iterable> sources) { - this.adaptedSources = streamPropertySources(sources) - .map(SpringConfigurationPropertySource::from) - .collect(Collectors.toList()); - } - - private Stream> streamPropertySources( - Iterable> sources) { - return StreamSupport.stream(sources.spliterator(), false).flatMap(this::flatten) - .filter(this::isIncluded); - } - - private Stream> flatten(PropertySource source) { - if (source.getSource() instanceof ConfigurableEnvironment) { - return streamPropertySources( - ((ConfigurableEnvironment) source.getSource()).getPropertySources()); - } - return Stream.of(source); - } - - private boolean isIncluded(PropertySource source) { - return !(source instanceof StubPropertySource) - && !(source instanceof ConfigurationPropertySourcesPropertySource); - } - - private static class PropertySourcesKey { - - private final List keys = new ArrayList<>(); - - PropertySourcesKey(Iterable> sources) { - sources.forEach(this::addKey); - } - - private void addKey(PropertySource source) { - this.keys.add(new PropertySourceKey(source)); - } - - @Override - public int hashCode() { - return this.keys.hashCode(); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null || getClass() != obj.getClass()) { - return false; - } - return this.keys.equals(((PropertySourcesKey) obj).keys); - } - - } - - private static class PropertySourceKey { - - private final String name; - - private final Class type; - - PropertySourceKey(PropertySource source) { - this.name = source.getName(); - this.type = source.getClass(); - } - - @Override - public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ObjectUtils.nullSafeHashCode(this.name); - result = prime * result + ObjectUtils.nullSafeHashCode(this.type); + private ConfigurationPropertySource adapt(PropertySource source) { + ConfigurationPropertySource result = this.cache.get(source); + // Most PropertySource test quality only using the source name, we need to + // check the actual source hasn't also changed. + if (result != null && result.getUnderlyingSource() == source) { return result; } + result = SpringConfigurationPropertySource.from(source); + this.cache.put(source, result); + return result; + } + + private static class SourcesIterator + implements Iterator { + + private Deque>> iterators; + + private ConfigurationPropertySource next; + + private final Function, ConfigurationPropertySource> adapter; + + SourcesIterator(Iterator> iterator, + Function, ConfigurationPropertySource> adapter) { + this.iterators = new ArrayDeque<>(4); + this.iterators.push(iterator); + this.adapter = adapter; + } @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; + public boolean hasNext() { + return fetchNext() != null; + } + + @Override + public ConfigurationPropertySource next() { + ConfigurationPropertySource next = fetchNext(); + this.next = null; + if (next == null) { + throw new NoSuchElementException(); } - if (obj == null || getClass() != obj.getClass()) { - return false; + return next; + } + + private ConfigurationPropertySource fetchNext() { + if (this.next == null) { + if (this.iterators.isEmpty()) { + return null; + } + if (!this.iterators.peek().hasNext()) { + this.iterators.pop(); + return fetchNext(); + } + PropertySource candidate = this.iterators.peek().next(); + if (candidate.getSource() instanceof ConfigurableEnvironment) { + push((ConfigurableEnvironment) candidate.getSource()); + return fetchNext(); + } + if (isIgnored(candidate)) { + return fetchNext(); + } + this.next = this.adapter.apply(candidate); } - PropertySourceKey other = (PropertySourceKey) obj; - boolean result = true; - result = result && ObjectUtils.nullSafeEquals(this.name, other.name); - result = result && ObjectUtils.nullSafeEquals(this.type, other.type); - return result; + return this.next; + } + + private void push(ConfigurableEnvironment environment) { + this.iterators.push(environment.getPropertySources().iterator()); + } + + private boolean isIgnored(PropertySource candidate) { + return (candidate instanceof StubPropertySource + || candidate instanceof ConfigurationPropertySourcesPropertySource); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java index d753c78e84..c187c815b9 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java @@ -152,12 +152,36 @@ public class SpringConfigurationPropertySourcesTests { @Test public void shouldTrackChanges() { MutablePropertySources sources = new MutablePropertySources(); - sources.addLast( - new MapPropertySource("test1", Collections.singletonMap("a", "b"))); - assertThat(new SpringConfigurationPropertySources(sources).iterator()).hasSize(1); - sources.addLast( - new MapPropertySource("test2", Collections.singletonMap("b", "c"))); - assertThat(new SpringConfigurationPropertySources(sources).iterator()).hasSize(2); + SpringConfigurationPropertySources configurationSources = new SpringConfigurationPropertySources( + sources); + assertThat(configurationSources.iterator()).hasSize(0); + MapPropertySource source1 = new MapPropertySource("test1", + Collections.singletonMap("a", "b")); + sources.addLast(source1); + assertThat(configurationSources.iterator()).hasSize(1); + MapPropertySource source2 = new MapPropertySource("test2", + Collections.singletonMap("b", "c")); + sources.addLast(source2); + assertThat(configurationSources.iterator()).hasSize(2); + } + + @Test + public void shouldTrackWhenSourceHasIdenticalName() { + MutablePropertySources sources = new MutablePropertySources(); + SpringConfigurationPropertySources configurationSources = new SpringConfigurationPropertySources( + sources); + ConfigurationPropertyName name = ConfigurationPropertyName.of("a"); + MapPropertySource source1 = new MapPropertySource("test", + Collections.singletonMap("a", "s1")); + sources.addLast(source1); + assertThat(configurationSources.iterator().next().getConfigurationProperty(name) + .getValue()).isEqualTo("s1"); + MapPropertySource source2 = new MapPropertySource("test", + Collections.singletonMap("a", "s2")); + sources.remove("test"); + sources.addLast(source2); + assertThat(configurationSources.iterator().next().getConfigurationProperty(name) + .getValue()).isEqualTo("s2"); } }