Don't report already migrated properties

Update `PropertiesMigrationReporter` so that already migrated properties
are not reported. Prior to this commit, if a deprecated property was
replaced by a property that could bind with the name relaxed name it
would be reported. For example: `test.someproperty` being replaced with
`test.some-property`.

In order to check the actual underlying property name, the
`PropertySourceOrigin` class has been updated so that it is always
returned, even if another `Origin` is available.

Fixes gh-35774
This commit is contained in:
Phillip Webb 2024-06-26 16:18:30 -07:00
parent 07442f8366
commit 962936370a
7 changed files with 172 additions and 84 deletions

View File

@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2024 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,11 +16,13 @@
package org.springframework.boot.context.properties.migrator;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;
@ -33,6 +35,7 @@ import org.springframework.boot.context.properties.source.ConfigurationPropertyS
import org.springframework.boot.context.properties.source.IterableConfigurationPropertySource;
import org.springframework.boot.env.OriginTrackedMapPropertySource;
import org.springframework.boot.origin.OriginTrackedValue;
import org.springframework.boot.origin.PropertySourceOrigin;
import org.springframework.core.env.ConfigurableEnvironment;
import org.springframework.core.env.PropertySource;
import org.springframework.util.LinkedMultiValueMap;
@ -64,7 +67,7 @@ class PropertiesMigrationReporter {
*/
PropertiesMigrationReport getReport() {
PropertiesMigrationReport report = new PropertiesMigrationReport();
Map<String, List<PropertyMigration>> properties = getMatchingProperties(
Map<String, List<PropertyMigration>> properties = getPropertySourceMigrations(
ConfigurationMetadataProperty::isDeprecated);
if (properties.isEmpty()) {
return report;
@ -78,6 +81,103 @@ class PropertiesMigrationReporter {
return report;
}
private Map<String, List<PropertyMigration>> getPropertySourceMigrations(
Predicate<ConfigurationMetadataProperty> filter) {
return getPropertySourceMigrations(this.allProperties.values().stream().filter(filter).toList());
}
private Map<String, List<PropertyMigration>> getPropertySourceMigrations(
List<ConfigurationMetadataProperty> metadataProperties) {
MultiValueMap<String, PropertyMigration> result = new LinkedMultiValueMap<>();
getPropertySourcesAsMap().forEach((propertySourceName, propertySource) -> {
for (ConfigurationMetadataProperty metadataProperty : metadataProperties) {
result.addAll(propertySourceName, getMigrations(propertySource, metadataProperty));
}
});
return result;
}
private Map<String, ConfigurationPropertySource> getPropertySourcesAsMap() {
Map<String, ConfigurationPropertySource> map = new LinkedHashMap<>();
for (ConfigurationPropertySource source : ConfigurationPropertySources.get(this.environment)) {
map.put(determinePropertySourceName(source), source);
}
return map;
}
private String determinePropertySourceName(ConfigurationPropertySource source) {
if (source.getUnderlyingSource() instanceof PropertySource<?> underlyingSource) {
return underlyingSource.getName();
}
return source.getUnderlyingSource().toString();
}
private List<PropertyMigration> getMigrations(ConfigurationPropertySource propertySource,
ConfigurationMetadataProperty metadataProperty) {
ConfigurationPropertyName propertyName = asConfigurationPropertyName(metadataProperty);
List<PropertyMigration> migrations = new ArrayList<>();
addMigration(propertySource, metadataProperty, propertyName, false, migrations);
if (isMapType(metadataProperty) && propertySource instanceof IterableConfigurationPropertySource iterable) {
iterable.stream()
.filter(propertyName::isAncestorOf)
.forEach((ancestorPropertyName) -> addMigration(propertySource, metadataProperty, ancestorPropertyName,
true, migrations));
}
return migrations;
}
private ConfigurationPropertyName asConfigurationPropertyName(ConfigurationMetadataProperty metadataProperty) {
return ConfigurationPropertyName.isValid(metadataProperty.getId())
? ConfigurationPropertyName.of(metadataProperty.getId())
: ConfigurationPropertyName.adapt(metadataProperty.getId(), '.');
}
private void addMigration(ConfigurationPropertySource propertySource,
ConfigurationMetadataProperty metadataProperty, ConfigurationPropertyName propertyName,
boolean mapMigration, List<PropertyMigration> migrations) {
ConfigurationProperty property = propertySource.getConfigurationProperty(propertyName);
if (property != null) {
ConfigurationMetadataProperty replacement = determineReplacementMetadata(metadataProperty);
if (replacement == null || !hasSameName(property, replacement)) {
migrations.add(new PropertyMigration(property, metadataProperty, replacement, mapMigration));
}
}
}
private boolean hasSameName(ConfigurationProperty property, ConfigurationMetadataProperty replacement) {
return (property.getOrigin() instanceof PropertySourceOrigin propertySourceOrigin)
&& Objects.equals(propertySourceOrigin.getPropertyName(), replacement.getName());
}
private ConfigurationMetadataProperty determineReplacementMetadata(ConfigurationMetadataProperty metadata) {
String replacementId = metadata.getDeprecation().getReplacement();
if (StringUtils.hasText(replacementId)) {
ConfigurationMetadataProperty replacement = this.allProperties.get(replacementId);
if (replacement != null) {
return replacement;
}
return detectMapValueReplacement(replacementId);
}
return null;
}
private ConfigurationMetadataProperty detectMapValueReplacement(String fullId) {
int lastDot = fullId.lastIndexOf('.');
if (lastDot == -1) {
return null;
}
ConfigurationMetadataProperty metadata = this.allProperties.get(fullId.substring(0, lastDot));
if (metadata != null && isMapType(metadata)) {
return metadata;
}
return null;
}
private boolean isMapType(ConfigurationMetadataProperty property) {
String type = property.getType();
return type != null && type.startsWith(Map.class.getName());
}
private PropertySource<?> mapPropertiesWithReplacement(PropertiesMigrationReport report, String name,
List<PropertyMigration> properties) {
report.add(name, properties);
@ -107,79 +207,6 @@ class PropertiesMigrationReporter {
}
}
private boolean isMapType(ConfigurationMetadataProperty property) {
String type = property.getType();
return type != null && type.startsWith(Map.class.getName());
}
private Map<String, List<PropertyMigration>> getMatchingProperties(
Predicate<ConfigurationMetadataProperty> filter) {
MultiValueMap<String, PropertyMigration> result = new LinkedMultiValueMap<>();
List<ConfigurationMetadataProperty> candidates = this.allProperties.values().stream().filter(filter).toList();
getPropertySourcesAsMap().forEach((propertySourceName, propertySource) -> candidates.forEach((metadata) -> {
ConfigurationPropertyName metadataName = ConfigurationPropertyName.isValid(metadata.getId())
? ConfigurationPropertyName.of(metadata.getId())
: ConfigurationPropertyName.adapt(metadata.getId(), '.');
// Direct match
ConfigurationProperty match = propertySource.getConfigurationProperty(metadataName);
if (match != null) {
result.add(propertySourceName,
new PropertyMigration(match, metadata, determineReplacementMetadata(metadata), false));
}
// Prefix match for maps
if (isMapType(metadata) && propertySource instanceof IterableConfigurationPropertySource) {
IterableConfigurationPropertySource iterableSource = (IterableConfigurationPropertySource) propertySource;
iterableSource.stream()
.filter(metadataName::isAncestorOf)
.map(propertySource::getConfigurationProperty)
.forEach((property) -> {
ConfigurationMetadataProperty replacement = determineReplacementMetadata(metadata);
result.add(propertySourceName, new PropertyMigration(property, metadata, replacement, true));
});
}
}));
return result;
}
private ConfigurationMetadataProperty determineReplacementMetadata(ConfigurationMetadataProperty metadata) {
String replacementId = metadata.getDeprecation().getReplacement();
if (StringUtils.hasText(replacementId)) {
ConfigurationMetadataProperty replacement = this.allProperties.get(replacementId);
if (replacement != null) {
return replacement;
}
return detectMapValueReplacement(replacementId);
}
return null;
}
private ConfigurationMetadataProperty detectMapValueReplacement(String fullId) {
int lastDot = fullId.lastIndexOf('.');
if (lastDot == -1) {
return null;
}
ConfigurationMetadataProperty metadata = this.allProperties.get(fullId.substring(0, lastDot));
if (metadata != null && isMapType(metadata)) {
return metadata;
}
return null;
}
private Map<String, ConfigurationPropertySource> getPropertySourcesAsMap() {
Map<String, ConfigurationPropertySource> map = new LinkedHashMap<>();
for (ConfigurationPropertySource source : ConfigurationPropertySources.get(this.environment)) {
map.put(determinePropertySourceName(source), source);
}
return map;
}
private String determinePropertySourceName(ConfigurationPropertySource source) {
if (source.getUnderlyingSource() instanceof PropertySource) {
return ((PropertySource<?>) source.getUnderlyingSource()).getName();
}
return source.getUnderlyingSource().toString();
}
/**
* {@link PropertySource} used to track accessed properties to protect against
* circular references.

View File

@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2024 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.
@ -25,6 +25,7 @@ import org.springframework.boot.configurationmetadata.Deprecation;
import org.springframework.boot.context.properties.source.ConfigurationProperty;
import org.springframework.boot.context.properties.source.ConfigurationPropertyName;
import org.springframework.boot.origin.Origin;
import org.springframework.boot.origin.PropertySourceOrigin;
import org.springframework.boot.origin.TextResourceOrigin;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
@ -67,6 +68,9 @@ class PropertyMigration {
private static Integer determineLineNumber(ConfigurationProperty property) {
Origin origin = property.getOrigin();
if (origin instanceof PropertySourceOrigin propertySourceOrigin) {
origin = propertySourceOrigin.getOrigin();
}
if (origin instanceof TextResourceOrigin textOrigin) {
if (textOrigin.getLocation() != null) {
return textOrigin.getLocation().getLine() + 1;

View File

@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2024 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.
@ -31,6 +31,7 @@ import org.springframework.boot.configurationmetadata.SimpleConfigurationMetadat
import org.springframework.boot.env.PropertiesPropertySourceLoader;
import org.springframework.boot.origin.Origin;
import org.springframework.boot.origin.OriginLookup;
import org.springframework.boot.origin.PropertySourceOrigin;
import org.springframework.core.env.ConfigurableEnvironment;
import org.springframework.core.env.MapPropertySource;
import org.springframework.core.env.MutablePropertySources;
@ -87,6 +88,13 @@ class PropertiesMigrationReporterTests {
assertThat(report).doesNotContain("wrong.one");
}
@Test
void warningReportReplacedWithSameRelaxedName() throws IOException {
this.environment.getPropertySources().addFirst(loadPropertySource("test", "config/config-relaxed.properties"));
String report = createWarningReport(loadRepository("metadata/sample-metadata.json"));
assertThat(report).isNull();
}
@Test
void errorReport() throws IOException {
this.environment.getPropertySources()
@ -232,7 +240,11 @@ class PropertiesMigrationReporterTests {
assertThat(propertySource.getProperty(name)).isEqualTo(value);
if (origin != null) {
assertThat(propertySource).isInstanceOf(OriginLookup.class);
assertThat(((OriginLookup<Object>) propertySource).getOrigin(name)).isEqualTo(origin);
Origin actualOrigin = ((OriginLookup<Object>) propertySource).getOrigin(name);
if (actualOrigin instanceof PropertySourceOrigin propertySourceOrigin) {
actualOrigin = propertySourceOrigin.getOrigin();
}
assertThat(actualOrigin).isEqualTo(origin);
}
}

View File

@ -54,6 +54,17 @@
{
"name": "custom.the-map-replacement",
"type": "java.util.Map<java.lang.String,java.lang.String>"
},
{
"name": "relaxed.thisthat-theother",
"type": "java.lang.String",
"deprecation": {
"replacement": "relaxed.this-that-the-other"
}
},
{
"name": "relaxed.this-that-the-other",
"type": "java.lang.String"
}
]
}

View File

@ -1,5 +1,5 @@
/*
* Copyright 2012-2019 the original author or authors.
* Copyright 2012-2024 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.
@ -25,22 +25,36 @@ import org.springframework.util.Assert;
* @author Phillip Webb
* @since 2.0.0
*/
public class PropertySourceOrigin implements Origin {
public class PropertySourceOrigin implements Origin, OriginProvider {
private final PropertySource<?> propertySource;
private final String propertyName;
private final Origin origin;
/**
* Create a new {@link PropertySourceOrigin} instance.
* @param propertySource the property source
* @param propertyName the name from the property source
*/
public PropertySourceOrigin(PropertySource<?> propertySource, String propertyName) {
this(propertySource, propertyName, null);
}
/**
* Create a new {@link PropertySourceOrigin} instance.
* @param propertySource the property source
* @param propertyName the name from the property source
* @param origin the actual origin for the source if known
* @since 3.2.8
*/
public PropertySourceOrigin(PropertySource<?> propertySource, String propertyName, Origin origin) {
Assert.notNull(propertySource, "PropertySource must not be null");
Assert.hasLength(propertyName, "PropertyName must not be empty");
this.propertySource = propertySource;
this.propertyName = propertyName;
this.origin = origin;
}
/**
@ -60,9 +74,25 @@ public class PropertySourceOrigin implements Origin {
return this.propertyName;
}
/**
* Return the actual origin for the source if known.
* @return the actual source origin
* @since 3.2.8
*/
@Override
public Origin getOrigin() {
return this.origin;
}
@Override
public Origin getParent() {
return (this.origin != null) ? this.origin.getParent() : null;
}
@Override
public String toString() {
return "\"" + this.propertyName + "\" from property source \"" + this.propertySource.getName() + "\"";
return (this.origin != null) ? this.origin.toString()
: "\"" + this.propertyName + "\" from property source \"" + this.propertySource.getName() + "\"";
}
/**
@ -75,7 +105,8 @@ public class PropertySourceOrigin implements Origin {
*/
public static Origin get(PropertySource<?> propertySource, String name) {
Origin origin = OriginLookup.getOrigin(propertySource, name);
return (origin != null) ? origin : new PropertySourceOrigin(propertySource, name);
return (origin instanceof PropertySourceOrigin) ? origin
: new PropertySourceOrigin(propertySource, name, origin);
}
}

View File

@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2024 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.
@ -84,7 +84,9 @@ class PropertySourceOriginTests {
withSettings().extraInterfaces(OriginLookup.class));
OriginLookup<String> originCapablePropertySource = (OriginLookup<String>) propertySource;
given(originCapablePropertySource.getOrigin("foo")).willReturn(origin);
assertThat(PropertySourceOrigin.get(propertySource, "foo")).isSameAs(origin);
Origin actual = PropertySourceOrigin.get(propertySource, "foo");
assertThat(actual).hasToString(origin.toString());
assertThat(((PropertySourceOrigin) actual).getOrigin()).isSameAs(origin);
}
@Test