Skip to content

Commit

Permalink
Refine non-optional classpath location checking
Browse files Browse the repository at this point in the history 
Update `StandardConfigDataLocationResolver` to no longer check if
directories exist for classpath resources. Unfortunately checking for
the parent directory of a `ClassPathResource` isn't always possible
without resorting something similar to the
`PathMatchingResourcePatternResolver` which would add a lot of
complexity to the resolver.

In order to ensure that non-optional locations are always resolved,
the `ConfigDataEnvironment` now checks that all imported locations
have been loaded.

Closes gh-24143
  • Loading branch information
@philwebb
philwebb committed Dec 2, 2020
1 parent 01478a2 commit 3dc03ac
Show file tree
Hide file tree
Showing 9 changed files with 138 additions and 61 deletions.
30 changes: 30 additions & 0 deletions ...ing-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java
View file
Expand Up @@ -297,6 +297,7 @@ private ConfigDataEnvironmentContributors processWithProfiles(ConfigDataEnvironm
private void applyToEnvironment(ConfigDataEnvironmentContributors contributors,
ConfigDataActivationContext activationContext) {
checkForInvalidProperties(contributors);
checkMandatoryLocations(contributors, activationContext);
MutablePropertySources propertySources = this.environment.getPropertySources();
this.logger.trace("Applying config data environment contributions");
for (ConfigDataEnvironmentContributor contributor : contributors) {
Expand Down Expand Up @@ -327,4 +328,33 @@ private void checkForInvalidProperties(ConfigDataEnvironmentContributors contrib
}
}

private void checkMandatoryLocations(ConfigDataEnvironmentContributors contributors,
ConfigDataActivationContext activationContext) {
Set<ConfigDataLocation> mandatoryLocations = new LinkedHashSet<>();
for (ConfigDataEnvironmentContributor contributor : contributors) {
mandatoryLocations.addAll(getMandatoryImports(contributor));
}
for (ConfigDataEnvironmentContributor contributor : contributors) {
if (contributor.getLocation() != null) {
mandatoryLocations.remove(contributor.getLocation());
}
}
if (!mandatoryLocations.isEmpty()) {
for (ConfigDataLocation mandatoryLocation : mandatoryLocations) {
this.notFoundAction.handle(this.logger, new ConfigDataLocationNotFoundException(mandatoryLocation));
}
}
}

private Set<ConfigDataLocation> getMandatoryImports(ConfigDataEnvironmentContributor contributor) {
List<ConfigDataLocation> imports = contributor.getImports();
Set<ConfigDataLocation> mandatoryLocations = new LinkedHashSet<>(imports.size());
for (ConfigDataLocation location : imports) {
if (!location.isOptional()) {
mandatoryLocations.add(location);
}
}
return mandatoryLocations;
}

}
39 changes: 25 additions & 14 deletions ...c/main/java/org/springframework/boot/context/config/ConfigDataEnvironmentContributor.java
View file
Expand Up @@ -51,6 +51,8 @@
*/
class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironmentContributor> {

private final ConfigDataLocation location;

private final ConfigDataResource resource;

private final PropertySource<?> propertySource;
Expand All @@ -68,6 +70,7 @@ class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironment
/**
* Create a new {@link ConfigDataEnvironmentContributor} instance.
* @param kind the contributor kind
* @param location the location of this contributor
* @param resource the resource that contributed the data or {@code null}
* @param propertySource the property source for the data or {@code null}
* @param configurationPropertySource the configuration property source for the data
Expand All @@ -76,10 +79,12 @@ class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironment
* @param ignoreImports if import properties should be ignored
* @param children the children of this contributor at each {@link ImportPhase}
*/
ConfigDataEnvironmentContributor(Kind kind, ConfigDataResource resource, PropertySource<?> propertySource,
ConfigurationPropertySource configurationPropertySource, ConfigDataProperties properties,
boolean ignoreImports, Map<ImportPhase, List<ConfigDataEnvironmentContributor>> children) {
ConfigDataEnvironmentContributor(Kind kind, ConfigDataLocation location, ConfigDataResource resource,
PropertySource<?> propertySource, ConfigurationPropertySource configurationPropertySource,
ConfigDataProperties properties, boolean ignoreImports,
Map<ImportPhase, List<ConfigDataEnvironmentContributor>> children) {
this.kind = kind;
this.location = location;
this.resource = resource;
this.properties = properties;
this.propertySource = propertySource;
Expand All @@ -96,6 +101,10 @@ Kind getKind() {
return this.kind;
}

ConfigDataLocation getLocation() {
return this.location;
}

/**
* Return if this contributor is currently active.
* @param activationContext the activation context
Expand Down Expand Up @@ -191,8 +200,8 @@ ConfigDataEnvironmentContributor withBoundProperties(Binder binder) {
if (this.ignoreImports) {
properties = properties.withoutImports();
}
return new ConfigDataEnvironmentContributor(Kind.BOUND_IMPORT, this.resource, this.propertySource,
this.configurationPropertySource, properties, this.ignoreImports, null);
return new ConfigDataEnvironmentContributor(Kind.BOUND_IMPORT, this.location, this.resource,
this.propertySource, this.configurationPropertySource, properties, this.ignoreImports, null);
}

/**
Expand All @@ -206,7 +215,7 @@ ConfigDataEnvironmentContributor withChildren(ImportPhase importPhase,
List<ConfigDataEnvironmentContributor> children) {
Map<ImportPhase, List<ConfigDataEnvironmentContributor>> updatedChildren = new LinkedHashMap<>(this.children);
updatedChildren.put(importPhase, children);
return new ConfigDataEnvironmentContributor(this.kind, this.resource, this.propertySource,
return new ConfigDataEnvironmentContributor(this.kind, this.location, this.resource, this.propertySource,
this.configurationPropertySource, this.properties, this.ignoreImports, updatedChildren);
}

Expand All @@ -231,7 +240,7 @@ ConfigDataEnvironmentContributor withReplacement(ConfigDataEnvironmentContributo
}
updatedChildren.put(importPhase, Collections.unmodifiableList(updatedContributors));
});
return new ConfigDataEnvironmentContributor(this.kind, this.resource, this.propertySource,
return new ConfigDataEnvironmentContributor(this.kind, this.location, this.resource, this.propertySource,
this.configurationPropertySource, this.properties, this.ignoreImports, updatedChildren);
}

Expand All @@ -243,7 +252,7 @@ ConfigDataEnvironmentContributor withReplacement(ConfigDataEnvironmentContributo
static ConfigDataEnvironmentContributor of(List<ConfigDataEnvironmentContributor> contributors) {
Map<ImportPhase, List<ConfigDataEnvironmentContributor>> children = new LinkedHashMap<>();
children.put(ImportPhase.BEFORE_PROFILE_ACTIVATION, Collections.unmodifiableList(contributors));
return new ConfigDataEnvironmentContributor(Kind.ROOT, null, null, null, null, false, children);
return new ConfigDataEnvironmentContributor(Kind.ROOT, null, null, null, null, null, false, children);
}

/**
Expand All @@ -256,7 +265,8 @@ static ConfigDataEnvironmentContributor of(List<ConfigDataEnvironmentContributor
static ConfigDataEnvironmentContributor ofInitialImport(ConfigDataLocation initialImport) {
List<ConfigDataLocation> imports = Collections.singletonList(initialImport);
ConfigDataProperties properties = new ConfigDataProperties(imports, null);
return new ConfigDataEnvironmentContributor(Kind.INITIAL_IMPORT, null, null, null, properties, false, null);
return new ConfigDataEnvironmentContributor(Kind.INITIAL_IMPORT, null, null, null, null, properties, false,
null);
}

/**
Expand All @@ -267,25 +277,26 @@ static ConfigDataEnvironmentContributor ofInitialImport(ConfigDataLocation initi
* @return a new {@link ConfigDataEnvironmentContributor} instance
*/
static ConfigDataEnvironmentContributor ofExisting(PropertySource<?> propertySource) {
return new ConfigDataEnvironmentContributor(Kind.EXISTING, null, propertySource,
return new ConfigDataEnvironmentContributor(Kind.EXISTING, null, null, propertySource,
ConfigurationPropertySource.from(propertySource), null, false, null);
}

/**
* Factory method to create an {@link Kind#UNBOUND_IMPORT unbound import} contributor.
* This contributor has been actively imported from another contributor and may itself
* import further contributors later.
* @param resource the condig data resource
* @param location the location of this contributor
* @param resource the config data resource
* @param configData the config data
* @param propertySourceIndex the index of the property source that should be used
* @return a new {@link ConfigDataEnvironmentContributor} instance
*/
static ConfigDataEnvironmentContributor ofUnboundImport(ConfigDataResource resource, ConfigData configData,
int propertySourceIndex) {
static ConfigDataEnvironmentContributor ofUnboundImport(ConfigDataLocation location, ConfigDataResource resource,
ConfigData configData, int propertySourceIndex) {
PropertySource<?> propertySource = configData.getPropertySources().get(propertySourceIndex);
ConfigurationPropertySource configurationPropertySource = ConfigurationPropertySource.from(propertySource);
boolean ignoreImports = configData.getOptions().contains(ConfigData.Option.IGNORE_IMPORTS);
return new ConfigDataEnvironmentContributor(Kind.UNBOUND_IMPORT, resource, propertySource,
return new ConfigDataEnvironmentContributor(Kind.UNBOUND_IMPORT, location, resource, propertySource,
configurationPropertySource, null, ignoreImports, null);
}

Expand Down
25 changes: 19 additions & 6 deletions .../main/java/org/springframework/boot/context/config/ConfigDataEnvironmentContributors.java
View file
Expand Up @@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.logging.Log;
Expand Down Expand Up @@ -115,10 +116,9 @@ ConfigDataEnvironmentContributors withProcessedImports(ConfigDataImporter import
ConfigDataLoaderContext loaderContext = new ContributorDataLoaderContext(this);
List<ConfigDataLocation> imports = contributor.getImports();
this.logger.trace(LogMessage.format("Processing imports %s", imports));
Map<ConfigDataResource, ConfigData> imported = importer.resolveAndLoad(activationContext,
Map<ConfigDataResolutionResult, ConfigData> imported = importer.resolveAndLoad(activationContext,
locationResolverContext, loaderContext, imports);
this.logger.trace(LogMessage.of(() -> imported.isEmpty() ? "Nothing imported" : "Imported "
+ imported.size() + " resource " + ((imported.size() != 1) ? "s" : "") + imported.keySet()));
this.logger.trace(LogMessage.of(() -> getImportedMessage(imported.keySet())));
ConfigDataEnvironmentContributor contributorAndChildren = contributor.withChildren(importPhase,
asContributors(imported));
result = new ConfigDataEnvironmentContributors(this.logger, this.bootstrapContext,
Expand All @@ -127,6 +127,16 @@ ConfigDataEnvironmentContributors withProcessedImports(ConfigDataImporter import
}
}

private CharSequence getImportedMessage(Set<ConfigDataResolutionResult> results) {
if (results.isEmpty()) {
return "Nothing imported";
}
StringBuilder message = new StringBuilder();
message.append("Imported " + results.size() + " resource" + ((results.size() != 1) ? "s " : " "));
message.append(results.stream().map(ConfigDataResolutionResult::getResource).collect(Collectors.toList()));
return message;
}

protected final ConfigurableBootstrapContext getBootstrapContext() {
return this.bootstrapContext;
}
Expand All @@ -147,11 +157,14 @@ private boolean isActiveWithUnprocessedImports(ConfigDataActivationContext activ
return contributor.isActive(activationContext) && contributor.hasUnprocessedImports(importPhase);
}

private List<ConfigDataEnvironmentContributor> asContributors(Map<ConfigDataResource, ConfigData> imported) {
private List<ConfigDataEnvironmentContributor> asContributors(
Map<ConfigDataResolutionResult, ConfigData> imported) {
List<ConfigDataEnvironmentContributor> contributors = new ArrayList<>(imported.size() * 5);
imported.forEach((location, data) -> {
imported.forEach((resolutionResult, data) -> {
for (int i = data.getPropertySources().size() - 1; i >= 0; i--) {
contributors.add(ConfigDataEnvironmentContributor.ofUnboundImport(location, data, i));
ConfigDataLocation location = resolutionResult.getLocation();
ConfigDataResource resource = resolutionResult.getResource();
contributors.add(ConfigDataEnvironmentContributor.ofUnboundImport(location, resource, data, i));
}
});
return Collections.unmodifiableList(contributors);
Expand Down
8 changes: 4 additions & 4 deletions ...spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataImporter.java
View file
Expand Up @@ -73,7 +73,7 @@ class ConfigDataImporter {
* @param locations the locations to resolve
* @return a map of the loaded locations and data
*/
Map<ConfigDataResource, ConfigData> resolveAndLoad(ConfigDataActivationContext activationContext,
Map<ConfigDataResolutionResult, ConfigData> resolveAndLoad(ConfigDataActivationContext activationContext,
ConfigDataLocationResolverContext locationResolverContext, ConfigDataLoaderContext loaderContext,
List<ConfigDataLocation> locations) {
try {
Expand Down Expand Up @@ -106,9 +106,9 @@ private List<ConfigDataResolutionResult> resolve(ConfigDataLocationResolverConte
}
}

private Map<ConfigDataResource, ConfigData> load(ConfigDataLoaderContext loaderContext,
private Map<ConfigDataResolutionResult, ConfigData> load(ConfigDataLoaderContext loaderContext,
List<ConfigDataResolutionResult> candidates) throws IOException {
Map<ConfigDataResource, ConfigData> result = new LinkedHashMap<>();
Map<ConfigDataResolutionResult, ConfigData> result = new LinkedHashMap<>();
for (int i = candidates.size() - 1; i >= 0; i--) {
ConfigDataResolutionResult candidate = candidates.get(i);
ConfigDataLocation location = candidate.getLocation();
Expand All @@ -117,7 +117,7 @@ private Map<ConfigDataResource, ConfigData> load(ConfigDataLoaderContext loaderC
try {
ConfigData loaded = this.loaders.load(loaderContext, resource);
if (loaded != null) {
result.put(resource, loaded);
result.put(candidate, loaded);
}
}
catch (ConfigDataNotFoundException ex) {
Expand Down
7 changes: 5 additions & 2 deletions ...main/java/org/springframework/boot/context/config/StandardConfigDataLocationResolver.java
View file
Expand Up @@ -31,6 +31,7 @@
import org.springframework.boot.env.PropertySourceLoader;
import org.springframework.core.Ordered;
import org.springframework.core.env.Environment;
import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.Resource;
import org.springframework.core.io.ResourceLoader;
import org.springframework.core.io.support.SpringFactoriesLoader;
Expand Down Expand Up @@ -234,8 +235,10 @@ private void assertNonOptionalDirectories(Set<StandardConfigDataReference> refer

private void assertDirectoryExists(StandardConfigDataReference reference) {
Resource resource = this.resourceLoader.getResource(reference.getDirectory());
StandardConfigDataResource configDataResource = new StandardConfigDataResource(reference, resource);
ConfigDataResourceNotFoundException.throwIfDoesNotExist(configDataResource, resource);
if (!(resource instanceof ClassPathResource)) {
StandardConfigDataResource configDataResource = new StandardConfigDataResource(reference, resource);
ConfigDataResourceNotFoundException.throwIfDoesNotExist(configDataResource, resource);
}
}

private List<StandardConfigDataResource> resolve(StandardConfigDataReference reference) {
Expand Down
2 changes: 1 addition & 1 deletion ...mework/boot/context/config/ConfigDataEnvironmentContributorPlaceholdersResolverTests.java
View file
Expand Up @@ -122,7 +122,7 @@ static class TestConfigDataEnvironmentContributor extends ConfigDataEnvironmentC
private final boolean active;

protected TestConfigDataEnvironmentContributor(PropertySource<?> propertySource, boolean active) {
super(Kind.ROOT, null, propertySource, null, null, false, null);
super(Kind.ROOT, null, null, propertySource, null, null, false, null);
this.active = active;
}

Expand Down
30 changes: 15 additions & 15 deletions ...t/java/org/springframework/boot/context/config/ConfigDataEnvironmentContributorTests.java
View file
Expand Up @@ -83,8 +83,8 @@ void isActiveWhenPropertiesIsNotActiveReturnsFalse() {
void getLocationReturnsLocation() {
ConfigData configData = new ConfigData(Collections.singleton(new MockPropertySource()));
ConfigDataResource resource = mock(ConfigDataResource.class);
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(resource,
configData, 0);
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(TEST_LOCATION,
resource, configData, 0);
assertThat(contributor.getResource()).isSameAs(resource);
}

Expand All @@ -100,7 +100,7 @@ void getConfigurationPropertySourceReturnsAdaptedPropertySource() {
MockPropertySource propertySource = new MockPropertySource();
propertySource.setProperty("spring", "boot");
ConfigData configData = new ConfigData(Collections.singleton(propertySource));
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(null,
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(null, null,
configData, 0);
assertThat(contributor.getConfigurationPropertySource()
.getConfigurationProperty(ConfigurationPropertyName.of("spring")).getValue()).isEqualTo("boot");
Expand Down Expand Up @@ -279,14 +279,14 @@ void ofExistingCreatesExistingContributor() {

@Test
void ofUnboundImportCreatesImportedContributor() {
TestResource location = new TestResource("test");
TestResource resource = new TestResource("test");
MockPropertySource propertySource = new MockPropertySource();
propertySource.setProperty("spring.config.import", "test");
ConfigData configData = new ConfigData(Collections.singleton(propertySource));
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(location,
configData, 0);
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(TEST_LOCATION,
resource, configData, 0);
assertThat(contributor.getKind()).isEqualTo(Kind.UNBOUND_IMPORT);
assertThat(contributor.getResource()).isSameAs(location);
assertThat(contributor.getResource()).isSameAs(resource);
assertThat(contributor.getImports()).isEmpty();
assertThat(contributor.isActive(this.activationContext)).isTrue();
assertThat(contributor.getPropertySource()).isEqualTo(propertySource);
Expand All @@ -296,13 +296,13 @@ void ofUnboundImportCreatesImportedContributor() {

@Test
void bindCreatesImportedContributor() {
TestResource location = new TestResource("test");
TestResource resource = new TestResource("test");
MockPropertySource propertySource = new MockPropertySource();
propertySource.setProperty("spring.config.import", "test");
ConfigData configData = new ConfigData(Collections.singleton(propertySource));
ConfigDataEnvironmentContributor contributor = createBoundContributor(location, configData, 0);
ConfigDataEnvironmentContributor contributor = createBoundContributor(resource, configData, 0);
assertThat(contributor.getKind()).isEqualTo(Kind.BOUND_IMPORT);
assertThat(contributor.getResource()).isSameAs(location);
assertThat(contributor.getResource()).isSameAs(resource);
assertThat(contributor.getImports()).containsExactly(TEST_LOCATION);
assertThat(contributor.isActive(this.activationContext)).isTrue();
assertThat(contributor.getPropertySource()).isEqualTo(propertySource);
Expand All @@ -312,13 +312,13 @@ void bindCreatesImportedContributor() {

@Test
void bindWhenConfigDataHasIgnoreImportsOptionsCreatesImportedContributorWithoutImports() {
TestResource location = new TestResource("test");
TestResource resource = new TestResource("test");
MockPropertySource propertySource = new MockPropertySource();
propertySource.setProperty("spring.config.import", "test");
ConfigData configData = new ConfigData(Collections.singleton(propertySource), ConfigData.Option.IGNORE_IMPORTS);
ConfigDataEnvironmentContributor contributor = createBoundContributor(location, configData, 0);
ConfigDataEnvironmentContributor contributor = createBoundContributor(resource, configData, 0);
assertThat(contributor.getKind()).isEqualTo(Kind.BOUND_IMPORT);
assertThat(contributor.getResource()).isSameAs(location);
assertThat(contributor.getResource()).isSameAs(resource);
assertThat(contributor.getImports()).isEmpty();
assertThat(contributor.isActive(this.activationContext)).isTrue();
assertThat(contributor.getPropertySource()).isEqualTo(propertySource);
Expand All @@ -341,8 +341,8 @@ private ConfigDataEnvironmentContributor createBoundContributor(String location)

private ConfigDataEnvironmentContributor createBoundContributor(ConfigDataResource resource, ConfigData configData,
int propertySourceIndex) {
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(resource,
configData, propertySourceIndex);
ConfigDataEnvironmentContributor contributor = ConfigDataEnvironmentContributor.ofUnboundImport(TEST_LOCATION,
resource, configData, propertySourceIndex);
Binder binder = new Binder(contributor.getConfigurationPropertySource());
return contributor.withBoundProperties(binder);
}
Expand Down

0 comments on commit 3dc03ac

Please sign in to comment.