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
spring-projects · Jun 26, 2024 · 9629363 · 9629363
1 parent 07442f8
commit 9629363
Show file tree

Hide file tree

Showing 7 changed files with 162 additions and 74 deletions.
diff --git a/...ava/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java b/...ava/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java
@@ -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.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,67 +81,72 @@ PropertiesMigrationReport getReport() {
 		return report;
 	}
 
-	private PropertySource<?> mapPropertiesWithReplacement(PropertiesMigrationReport report, String name,
-			List<PropertyMigration> properties) {
-		report.add(name, properties);
-		List<PropertyMigration> renamed = properties.stream().filter(PropertyMigration::isCompatibleType).toList();
-		if (renamed.isEmpty()) {
-			return null;
-		}
-		NameTrackingPropertySource nameTrackingPropertySource = new NameTrackingPropertySource();
-		this.environment.getPropertySources().addFirst(nameTrackingPropertySource);
-		try {
-			String target = "migrate-" + name;
-			Map<String, OriginTrackedValue> content = new LinkedHashMap<>();
-			for (PropertyMigration candidate : renamed) {
-				String newPropertyName = candidate.getNewPropertyName();
-				Object value = candidate.getProperty().getValue();
-				if (nameTrackingPropertySource.isPlaceholderThatAccessesName(value, newPropertyName)) {
-					continue;
-				}
-				OriginTrackedValue originTrackedValue = OriginTrackedValue.of(value,
-						candidate.getProperty().getOrigin());
-				content.put(newPropertyName, originTrackedValue);
+	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 new OriginTrackedMapPropertySource(target, content);
+		});
+		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);
 		}
-		finally {
-			this.environment.getPropertySources().remove(nameTrackingPropertySource.getName());
+		return map;
+	}
+
+	private String determinePropertySourceName(ConfigurationPropertySource source) {
+		if (source.getUnderlyingSource() instanceof PropertySource<?> underlyingSource) {
+			return underlyingSource.getName();
 		}
+		return source.getUnderlyingSource().toString();
 	}
 
-	private boolean isMapType(ConfigurationMetadataProperty property) {
-		String type = property.getType();
-		return type != null && type.startsWith(Map.class.getName());
+	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 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));
-					});
+	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));
 			}
-		}));
-		return result;
+		}
+	}
+
+	private boolean hasSameName(ConfigurationProperty property, ConfigurationMetadataProperty replacement) {
+		return (property.getOrigin() instanceof PropertySourceOrigin propertySourceOrigin)
+				&& Objects.equals(propertySourceOrigin.getPropertyName(), replacement.getName());
 	}
 
 	private ConfigurationMetadataProperty determineReplacementMetadata(ConfigurationMetadataProperty metadata) {
@@ -165,19 +173,38 @@ private ConfigurationMetadataProperty detectMapValueReplacement(String fullId) {
 		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 boolean isMapType(ConfigurationMetadataProperty property) {
+		String type = property.getType();
+		return type != null && type.startsWith(Map.class.getName());
 	}
 
-	private String determinePropertySourceName(ConfigurationPropertySource source) {
-		if (source.getUnderlyingSource() instanceof PropertySource) {
-			return ((PropertySource<?>) source.getUnderlyingSource()).getName();
+	private PropertySource<?> mapPropertiesWithReplacement(PropertiesMigrationReport report, String name,
+			List<PropertyMigration> properties) {
+		report.add(name, properties);
+		List<PropertyMigration> renamed = properties.stream().filter(PropertyMigration::isCompatibleType).toList();
+		if (renamed.isEmpty()) {
+			return null;
+		}
+		NameTrackingPropertySource nameTrackingPropertySource = new NameTrackingPropertySource();
+		this.environment.getPropertySources().addFirst(nameTrackingPropertySource);
+		try {
+			String target = "migrate-" + name;
+			Map<String, OriginTrackedValue> content = new LinkedHashMap<>();
+			for (PropertyMigration candidate : renamed) {
+				String newPropertyName = candidate.getNewPropertyName();
+				Object value = candidate.getProperty().getValue();
+				if (nameTrackingPropertySource.isPlaceholderThatAccessesName(value, newPropertyName)) {
+					continue;
+				}
+				OriginTrackedValue originTrackedValue = OriginTrackedValue.of(value,
+						candidate.getProperty().getOrigin());
+				content.put(newPropertyName, originTrackedValue);
+			}
+			return new OriginTrackedMapPropertySource(target, content);
+		}
+		finally {
+			this.environment.getPropertySources().remove(nameTrackingPropertySource.getName());
 		}
-		return source.getUnderlyingSource().toString();
 	}
 
 	/**

diff --git a/...src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java b/...src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java
@@ -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.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;

diff --git a/...rg/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java b/...rg/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java
@@ -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.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 @@ void warningReport() throws IOException {
 		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 @@ private void assertMappedProperty(PropertySource<?> propertySource, String name,
 		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);
 		}
 	}
 

diff --git a/...tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties b/...tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties
@@ -0,0 +1 @@
+relaxed.this-that-the-other=test
diff --git a/...ot-tools/spring-boot-properties-migrator/src/test/resources/metadata/sample-metadata.json b/...ot-tools/spring-boot-properties-migrator/src/test/resources/metadata/sample-metadata.json
@@ -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"
     }
   ]
 }
diff --git a/...oject/spring-boot/src/main/java/org/springframework/boot/origin/PropertySourceOrigin.java b/...oject/spring-boot/src/main/java/org/springframework/boot/origin/PropertySourceOrigin.java
@@ -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 @@
  * @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 String getPropertyName() {
 		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 String toString() {
 	 */
 	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);
 	}
 
 }
diff --git a/.../spring-boot/src/test/java/org/springframework/boot/origin/PropertySourceOriginTests.java b/.../spring-boot/src/test/java/org/springframework/boot/origin/PropertySourceOriginTests.java
@@ -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 @@ void getWhenPropertySourceSupportsOriginLookupShouldReturnOrigin() {
 				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