Skip to content

Commit

Permalink
Detect bad properties in profile specific files
Browse files Browse the repository at this point in the history
Throw an `InvalidConfigDataPropertyException` if bad properties are
detected in profile specific files. The following properties will now
trigger an exception if used in a profile specific file:

	`spring.profiles.include`
	`spring.profiles.active`
	`spring.profiles.default`
	`spring.config.activate.on-profile`
	`spring.profiles`

Prior to this commit, profile based properties in a profile specific
file would be silently ignored, making them hard to find.

Fixes gh-24733
  • Loading branch information
philwebb committed Jan 12, 2021
1 parent 5ed2b11 commit b6cb9c0
Show file tree
Hide file tree
Showing 14 changed files with 165 additions and 84 deletions.
Expand Up @@ -34,7 +34,6 @@
import org.springframework.boot.context.properties.bind.Bindable;
import org.springframework.boot.context.properties.bind.Binder;
import org.springframework.boot.context.properties.bind.PlaceholdersResolver;
import org.springframework.boot.context.properties.source.ConfigurationPropertyName;
import org.springframework.boot.context.properties.source.ConfigurationPropertySource;
import org.springframework.boot.logging.DeferredLogFactory;
import org.springframework.core.env.ConfigurableEnvironment;
Expand Down Expand Up @@ -98,9 +97,6 @@ class ConfigDataEnvironment {

private static final ConfigDataLocation[] EMPTY_LOCATIONS = new ConfigDataLocation[0];

private static final ConfigurationPropertyName INCLUDE_PROFILES = ConfigurationPropertyName
.of(Profiles.INCLUDE_PROFILES_PROPERTY_NAME);

private static final Bindable<ConfigDataLocation[]> CONFIG_DATA_LOCATION_ARRAY = Bindable
.of(ConfigDataLocation[].class);

Expand Down Expand Up @@ -293,9 +289,9 @@ private Collection<? extends String> getIncludedProfiles(ConfigDataEnvironmentCo
continue;
}
Binder binder = new Binder(Collections.singleton(source), placeholdersResolver);
binder.bind(INCLUDE_PROFILES, STRING_LIST).ifBound((includes) -> {
binder.bind(Profiles.INCLUDE_PROFILES, STRING_LIST).ifBound((includes) -> {
if (!contributor.isActive(activationContext)) {
InactiveConfigDataAccessException.throwIfPropertyFound(contributor, INCLUDE_PROFILES);
InactiveConfigDataAccessException.throwIfPropertyFound(contributor, Profiles.INCLUDE_PROFILES);
}
result.addAll(includes);
});
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand Down Expand Up @@ -55,6 +55,8 @@ class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironment

private final ConfigDataResource resource;

private final boolean profileSpecific;

private final PropertySource<?> propertySource;

private final ConfigurationPropertySource configurationPropertySource;
Expand All @@ -72,6 +74,7 @@ class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironment
* @param kind the contributor kind
* @param location the location of this contributor
* @param resource the resource that contributed the data or {@code null}
* @param profileSpecific if the contributor is from a profile specific import
* @param propertySource the property source for the data or {@code null}
* @param configurationPropertySource the configuration property source for the data
* or {@code null}
Expand All @@ -80,12 +83,13 @@ class ConfigDataEnvironmentContributor implements Iterable<ConfigDataEnvironment
* @param children the children of this contributor at each {@link ImportPhase}
*/
ConfigDataEnvironmentContributor(Kind kind, ConfigDataLocation location, ConfigDataResource resource,
PropertySource<?> propertySource, ConfigurationPropertySource configurationPropertySource,
ConfigDataProperties properties, boolean ignoreImports,
Map<ImportPhase, List<ConfigDataEnvironmentContributor>> children) {
boolean profileSpecific, PropertySource<?> propertySource,
ConfigurationPropertySource configurationPropertySource, ConfigDataProperties properties,
boolean ignoreImports, Map<ImportPhase, List<ConfigDataEnvironmentContributor>> children) {
this.kind = kind;
this.location = location;
this.resource = resource;
this.profileSpecific = profileSpecific;
this.properties = properties;
this.propertySource = propertySource;
this.configurationPropertySource = configurationPropertySource;
Expand Down Expand Up @@ -122,6 +126,14 @@ ConfigDataResource getResource() {
return this.resource;
}

/**
* Return if the contributor is from a profile specific import.
* @return if the contributor is profile specific
*/
boolean isProfileSpecific() {
return this.profileSpecific;
}

/**
* Return the property source for this contributor.
* @return the property source or {@code null}
Expand Down Expand Up @@ -201,7 +213,8 @@ ConfigDataEnvironmentContributor withBoundProperties(Binder binder) {
properties = properties.withoutImports();
}
return new ConfigDataEnvironmentContributor(Kind.BOUND_IMPORT, this.location, this.resource,
this.propertySource, this.configurationPropertySource, properties, this.ignoreImports, null);
this.profileSpecific, this.propertySource, this.configurationPropertySource, properties,
this.ignoreImports, null);
}

/**
Expand All @@ -215,8 +228,9 @@ 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.location, this.resource, this.propertySource,
this.configurationPropertySource, this.properties, this.ignoreImports, updatedChildren);
return new ConfigDataEnvironmentContributor(this.kind, this.location, this.resource, this.profileSpecific,
this.propertySource, this.configurationPropertySource, this.properties, this.ignoreImports,
updatedChildren);
}

/**
Expand All @@ -240,8 +254,9 @@ ConfigDataEnvironmentContributor withReplacement(ConfigDataEnvironmentContributo
}
updatedChildren.put(importPhase, Collections.unmodifiableList(updatedContributors));
});
return new ConfigDataEnvironmentContributor(this.kind, this.location, this.resource, this.propertySource,
this.configurationPropertySource, this.properties, this.ignoreImports, updatedChildren);
return new ConfigDataEnvironmentContributor(this.kind, this.location, this.resource, this.profileSpecific,
this.propertySource, this.configurationPropertySource, this.properties, this.ignoreImports,
updatedChildren);
}

/**
Expand All @@ -252,7 +267,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, null, false, children);
return new ConfigDataEnvironmentContributor(Kind.ROOT, null, null, false, null, null, null, false, children);
}

/**
Expand All @@ -265,8 +280,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, null, properties, false,
null);
return new ConfigDataEnvironmentContributor(Kind.INITIAL_IMPORT, null, null, false, null, null, properties,
false, null);
}

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

Expand All @@ -287,26 +302,29 @@ static ConfigDataEnvironmentContributor ofExisting(PropertySource<?> propertySou
* import further contributors later.
* @param location the location of this contributor
* @param resource the config data resource
* @param profileSpecific if the contributor is from a profile specific import
* @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(ConfigDataLocation location, ConfigDataResource resource,
ConfigData configData, int propertySourceIndex) {
boolean profileSpecific, 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, location, resource, propertySource,
configurationPropertySource, null, ignoreImports, null);
return new ConfigDataEnvironmentContributor(Kind.UNBOUND_IMPORT, location, resource, profileSpecific,
propertySource, configurationPropertySource, null, ignoreImports, null);
}

/**
* Factory method to create an {@link Kind#EMPTY_LOCATION empty location} contributor.
* @param location the location of this contributor
* @param profileSpecific if the contributor is from a profile specific import
* @return a new {@link ConfigDataEnvironmentContributor} instance
*/
static ConfigDataEnvironmentContributor ofEmptyLocation(ConfigDataLocation location) {
return new ConfigDataEnvironmentContributor(Kind.EMPTY_LOCATION, location, null, null, null, null, true, null);
static ConfigDataEnvironmentContributor ofEmptyLocation(ConfigDataLocation location, boolean profileSpecific) {
return new ConfigDataEnvironmentContributor(Kind.EMPTY_LOCATION, location, null, profileSpecific, null, null,
null, true, null);
}

/**
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand Down Expand Up @@ -163,12 +163,14 @@ private List<ConfigDataEnvironmentContributor> asContributors(
imported.forEach((resolutionResult, data) -> {
ConfigDataLocation location = resolutionResult.getLocation();
ConfigDataResource resource = resolutionResult.getResource();
boolean profileSpecific = resolutionResult.isProfileSpecific();
if (data.getPropertySources().isEmpty()) {
contributors.add(ConfigDataEnvironmentContributor.ofEmptyLocation(location));
contributors.add(ConfigDataEnvironmentContributor.ofEmptyLocation(location, profileSpecific));
}
else {
for (int i = data.getPropertySources().size() - 1; i >= 0; i--) {
contributors.add(ConfigDataEnvironmentContributor.ofUnboundImport(location, resource, data, i));
contributors.add(ConfigDataEnvironmentContributor.ofUnboundImport(location, resource,
profileSpecific, data, i));
}
}
});
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand Down Expand Up @@ -111,21 +111,21 @@ List<ConfigDataResolutionResult> resolve(ConfigDataLocationResolverContext conte

private List<ConfigDataResolutionResult> resolve(ConfigDataLocationResolver<?> resolver,
ConfigDataLocationResolverContext context, ConfigDataLocation location, Profiles profiles) {
List<ConfigDataResolutionResult> resolved = resolve(location, () -> resolver.resolve(context, location));
List<ConfigDataResolutionResult> resolved = resolve(location, false, () -> resolver.resolve(context, location));
if (profiles == null) {
return resolved;
}
List<ConfigDataResolutionResult> profileSpecific = resolve(location,
List<ConfigDataResolutionResult> profileSpecific = resolve(location, true,
() -> resolver.resolveProfileSpecific(context, location, profiles));
return merge(resolved, profileSpecific);
}

private List<ConfigDataResolutionResult> resolve(ConfigDataLocation location,
private List<ConfigDataResolutionResult> resolve(ConfigDataLocation location, boolean profileSpecific,
Supplier<List<? extends ConfigDataResource>> resolveAction) {
List<ConfigDataResource> resources = nonNullList(resolveAction.get());
List<ConfigDataResolutionResult> resolved = new ArrayList<>(resources.size());
for (ConfigDataResource resource : resources) {
resolved.add(new ConfigDataResolutionResult(location, resource));
resolved.add(new ConfigDataResolutionResult(location, resource, profileSpecific));
}
return resolved;
}
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand Down Expand Up @@ -94,7 +94,7 @@ ConfigDataProperties withoutImports() {

ConfigDataProperties withLegacyProfiles(String[] legacyProfiles, ConfigurationProperty property) {
if (this.activate != null && !ObjectUtils.isEmpty(this.activate.onProfile)) {
throw new InvalidConfigDataPropertyException(property, NAME.append("activate.on-profile"), null);
throw new InvalidConfigDataPropertyException(property, false, NAME.append("activate.on-profile"), null);
}
return new ConfigDataProperties(this.imports, new Activate(this.activate.onCloudPlatform, legacyProfiles));
}
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand Down Expand Up @@ -28,9 +28,12 @@ class ConfigDataResolutionResult {

private final ConfigDataResource resource;

ConfigDataResolutionResult(ConfigDataLocation location, ConfigDataResource resource) {
private final boolean profileSpecific;

ConfigDataResolutionResult(ConfigDataLocation location, ConfigDataResource resource, boolean profileSpecific) {
this.location = location;
this.resource = resource;
this.profileSpecific = profileSpecific;
}

ConfigDataLocation getLocation() {
Expand All @@ -41,4 +44,8 @@ ConfigDataResource getResource() {
return this.resource;
}

boolean isProfileSpecific() {
return this.profileSpecific;
}

}
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2021 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.
Expand All @@ -18,13 +18,16 @@

import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;

import org.apache.commons.logging.Log;

import org.springframework.boot.context.properties.source.ConfigurationProperty;
import org.springframework.boot.context.properties.source.ConfigurationPropertyName;
import org.springframework.boot.context.properties.source.ConfigurationPropertySource;
import org.springframework.core.env.AbstractEnvironment;

/**
* Exception thrown if an invalid property is found when processing config data.
Expand All @@ -35,12 +38,23 @@
*/
public class InvalidConfigDataPropertyException extends ConfigDataException {

private static final Map<ConfigurationPropertyName, ConfigurationPropertyName> WARNING;
private static final Map<ConfigurationPropertyName, ConfigurationPropertyName> WARNINGS;
static {
Map<ConfigurationPropertyName, ConfigurationPropertyName> warning = new LinkedHashMap<>();
warning.put(ConfigurationPropertyName.of("spring.profiles"),
Map<ConfigurationPropertyName, ConfigurationPropertyName> warnings = new LinkedHashMap<>();
warnings.put(ConfigurationPropertyName.of("spring.profiles"),
ConfigurationPropertyName.of("spring.config.activate.on-profile"));
WARNING = Collections.unmodifiableMap(warning);
WARNINGS = Collections.unmodifiableMap(warnings);
}

private static final Set<ConfigurationPropertyName> PROFILE_SPECIFIC_ERRORS;
static {
Set<ConfigurationPropertyName> errors = new LinkedHashSet<>();
errors.add(Profiles.INCLUDE_PROFILES);
errors.add(ConfigurationPropertyName.of(AbstractEnvironment.ACTIVE_PROFILES_PROPERTY_NAME));
errors.add(ConfigurationPropertyName.of(AbstractEnvironment.DEFAULT_PROFILES_PROPERTY_NAME));
errors.add(ConfigurationPropertyName.of("spring.config.activate.on-profile"));
errors.add(ConfigurationPropertyName.of("spring.profiles"));
PROFILE_SPECIFIC_ERRORS = Collections.unmodifiableSet(errors);
}

private final ConfigurationProperty property;
Expand All @@ -49,9 +63,9 @@ public class InvalidConfigDataPropertyException extends ConfigDataException {

private final ConfigDataResource location;

InvalidConfigDataPropertyException(ConfigurationProperty property, ConfigurationPropertyName replacement,
ConfigDataResource location) {
super(getMessage(property, replacement, location), null);
InvalidConfigDataPropertyException(ConfigurationProperty property, boolean profileSpecific,
ConfigurationPropertyName replacement, ConfigDataResource location) {
super(getMessage(property, profileSpecific, replacement, location), null);
this.property = property;
this.replacement = replacement;
this.location = location;
Expand Down Expand Up @@ -94,24 +108,35 @@ public ConfigurationPropertyName getReplacement() {
static void throwOrWarn(Log logger, ConfigDataEnvironmentContributor contributor) {
ConfigurationPropertySource propertySource = contributor.getConfigurationPropertySource();
if (propertySource != null) {
WARNING.forEach((invalid, replacement) -> {
ConfigurationProperty property = propertySource.getConfigurationProperty(invalid);
WARNINGS.forEach((name, replacement) -> {
ConfigurationProperty property = propertySource.getConfigurationProperty(name);
if (property != null) {
logger.warn(getMessage(property, replacement, contributor.getResource()));
logger.warn(getMessage(property, false, replacement, contributor.getResource()));
}
});
if (contributor.isProfileSpecific()) {
PROFILE_SPECIFIC_ERRORS.forEach((name) -> {
ConfigurationProperty property = propertySource.getConfigurationProperty(name);
if (property != null) {
throw new InvalidConfigDataPropertyException(property, true, null, contributor.getResource());
}
});
}
}
}

private static String getMessage(ConfigurationProperty property, ConfigurationPropertyName replacement,
ConfigDataResource location) {
private static String getMessage(ConfigurationProperty property, boolean profileSpecific,
ConfigurationPropertyName replacement, ConfigDataResource location) {
StringBuilder message = new StringBuilder("Property '");
message.append(property.getName());
if (location != null) {
message.append("' imported from location '");
message.append(location);
}
message.append("' is invalid");
if (profileSpecific) {
message.append(" in a profile specific resource");
}
if (replacement != null) {
message.append(" and should be replaced with '");
message.append(replacement);
Expand Down

0 comments on commit b6cb9c0

Please sign in to comment.