Polish "Avoid NPE when replacement property does not exist"

Closes gh-15394

Author: snicoll

spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReport.java

@@ -17,16 +17,13 @@
 package org.springframework.boot.context.properties.migrator;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import org.springframework.boot.configurationmetadata.ConfigurationMetadataProperty;
-import org.springframework.boot.configurationmetadata.Deprecation;
-import org.springframework.util.StringUtils;
 
 /**
  * Provides a properties migration report.
@@ -51,8 +48,7 @@ public String getWarningReport() {
 		StringBuilder report = new StringBuilder();
 		report.append(String.format("%nThe use of configuration keys that have been "
 				+ "renamed was found in the environment:%n%n"));
-		append(report, content, (metadata) -> "Replacement: "
-				+ metadata.getDeprecation().getReplacement());
+		append(report, content);
 		report.append(String.format("%n"));
 		report.append("Each configuration key has been temporarily mapped to its "
 				+ "replacement for your convenience. To silence this warning, please "
@@ -75,27 +71,14 @@ public String getErrorReport() {
 		StringBuilder report = new StringBuilder();
 		report.append(String.format("%nThe use of configuration keys that are no longer "
 				+ "supported was found in the environment:%n%n"));
-		append(report, content, this::determineReason);
+		append(report, content);
 		report.append(String.format("%n"));
 		report.append("Please refer to the migration guide or reference guide for "
 				+ "potential alternatives.");
 		report.append(String.format("%n"));
 		return report.toString();
 	}
 
-	private String determineReason(ConfigurationMetadataProperty metadata) {
-		Deprecation deprecation = metadata.getDeprecation();
-		if (StringUtils.hasText(deprecation.getShortReason())) {
-			return "Reason: " + deprecation.getShortReason();
-		}
-		if (StringUtils.hasText(deprecation.getReplacement())) {
-			return String.format(
-					"Reason: Replacement key '%s' uses an incompatible " + "target type",
-					deprecation.getReplacement());
-		}
-		return "Reason: none";
-	}
-
 	private Map<String, List<PropertyMigration>> getContent(
 			Function<LegacyProperties, List<PropertyMigration>> extractor) {
 		return this.content.entrySet().stream()
@@ -105,8 +88,7 @@ private Map<String, List<PropertyMigration>> getContent(
 	}
 
 	private void append(StringBuilder report,
-			Map<String, List<PropertyMigration>> content,
-			Function<ConfigurationMetadataProperty, String> deprecationMessage) {
+			Map<String, List<PropertyMigration>> content) {
 		content.forEach((name, properties) -> {
 			report.append(String.format("Property source '%s':%n", name));
 			properties.sort(PropertyMigration.COMPARATOR);
@@ -117,8 +99,7 @@ private void append(StringBuilder report,
 					report.append(
 							String.format("\t\tLine: %d%n", property.getLineNumber()));
 				}
-				report.append(
-						String.format("\t\t%s%n", deprecationMessage.apply(metadata)));
+				report.append(String.format("\t\t%s%n", property.determineReason()));
 			});
 			report.append(String.format("%n"));
 		});
@@ -127,36 +108,29 @@ private void append(StringBuilder report,
 	/**
 	 * Register a new property source.
 	 * @param name the name of the property source
-	 * @param renamed the properties that were renamed
-	 * @param unsupported the properties that are no longer supported
+	 * @param properties the {@link PropertyMigration} instances
 	 */
-	void add(String name, List<PropertyMigration> renamed,
-			List<PropertyMigration> unsupported) {
-		this.content.put(name, new LegacyProperties(renamed, unsupported));
+	void add(String name, List<PropertyMigration> properties) {
+		this.content.put(name, new LegacyProperties(properties));
 	}
 
 	private static class LegacyProperties {
 
-		private final List<PropertyMigration> renamed;
-
-		private final List<PropertyMigration> unsupported;
-
-		LegacyProperties(List<PropertyMigration> renamed,
-				List<PropertyMigration> unsupported) {
-			this.renamed = asNewList(renamed);
-			this.unsupported = asNewList(unsupported);
-		}
+		private final List<PropertyMigration> properties;
 
-		private <T> List<T> asNewList(List<T> source) {
-			return (source != null) ? new ArrayList<>(source) : Collections.emptyList();
+		LegacyProperties(List<PropertyMigration> properties) {
+			this.properties = new ArrayList<>(properties);
 		}
 
 		public List<PropertyMigration> getRenamed() {
-			return this.renamed;
+			return this.properties.stream().filter(PropertyMigration::isCompatibleType)
+					.collect(Collectors.toList());
 		}
 
 		public List<PropertyMigration> getUnsupported() {
-			return this.unsupported;
+			return this.properties.stream()
+					.filter((property) -> !property.isCompatibleType())
+					.collect(Collectors.toList());
 		}
 
 	}

spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java

@@ -16,8 +16,6 @@
 
 package org.springframework.boot.context.properties.migrator;
 
-import java.time.Duration;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -83,11 +81,9 @@ public PropertiesMigrationReport getReport() {
 	private PropertySource<?> mapPropertiesWithReplacement(
 			PropertiesMigrationReport report, String name,
 			List<PropertyMigration> properties) {
-		List<PropertyMigration> renamed = new ArrayList<>();
-		List<PropertyMigration> unsupported = new ArrayList<>();
-		properties.forEach((property) -> (isRenamed(property) ? renamed : unsupported)
-				.add(property));
-		report.add(name, renamed, unsupported);
+		report.add(name, properties);
+		List<PropertyMigration> renamed = properties.stream()
+				.filter(PropertyMigration::isCompatibleType).collect(Collectors.toList());
 		if (renamed.isEmpty()) {
 			return null;
 		}
@@ -102,55 +98,6 @@ private PropertySource<?> mapPropertiesWithReplacement(
 		return new OriginTrackedMapPropertySource(target, content);
 	}
 
-	private boolean isRenamed(PropertyMigration property) {
-		ConfigurationMetadataProperty metadata = property.getMetadata();
-		String replacementId = metadata.getDeprecation().getReplacement();
-		if (StringUtils.hasText(replacementId)) {
-			ConfigurationMetadataProperty replacement = this.allProperties
-					.get(replacementId);
-			if (replacement != null) {
-				return isCompatibleType(metadata.getType(), replacement.getType());
-			}
-			return isCompatibleType(metadata.getType(),
-					detectMapValueReplacementType(replacementId));
-		}
-		return false;
-	}
-
-	private boolean isCompatibleType(String currentType, String replacementType) {
-		if (replacementType == null || currentType == null) {
-			return false;
-		}
-		if (replacementType.equals(currentType)) {
-			return true;
-		}
-		if (replacementType.equals(Duration.class.getName())
-				&& (currentType.equals(Long.class.getName())
-						|| currentType.equals(Integer.class.getName()))) {
-			return true;
-		}
-		return false;
-	}
-
-	private String detectMapValueReplacementType(String fullId) {
-		int lastDot = fullId.lastIndexOf('.');
-		if (lastDot != -1) {
-			ConfigurationMetadataProperty property = this.allProperties
-					.get(fullId.substring(0, lastDot));
-			if (property == null) {
-				return null;
-			}
-			String type = property.getType();
-			if (type != null && type.startsWith(Map.class.getName())) {
-				int lastComma = type.lastIndexOf(',');
-				if (lastComma != -1) {
-					return type.substring(lastComma + 1, type.length() - 1).trim();
-				}
-			}
-		}
-		return null;
-	}
-
 	private Map<String, List<PropertyMigration>> getMatchingProperties(
 			Predicate<ConfigurationMetadataProperty> filter) {
 		MultiValueMap<String, PropertyMigration> result = new LinkedMultiValueMap<>();
@@ -162,14 +109,36 @@ private Map<String, List<PropertyMigration>> getMatchingProperties(
 						.getConfigurationProperty(
 								ConfigurationPropertyName.of(metadata.getId()));
 				if (configurationProperty != null) {
-					result.add(name,
-							new PropertyMigration(metadata, configurationProperty));
+					result.add(name, new PropertyMigration(configurationProperty,
+							metadata, determineReplacementMetadata(metadata)));
 				}
 			});
 		});
 		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 this.allProperties.get(fullId.substring(0, lastDot));
+		}
+		return null;
+	}
+
 	private Predicate<ConfigurationMetadataProperty> deprecatedFilter() {
 		return (property) -> property.getDeprecation() != null
 				&& property.getDeprecation().getLevel() == Deprecation.Level.ERROR;

spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java

@@ -16,12 +16,16 @@
 
 package org.springframework.boot.context.properties.migrator;
 
+import java.time.Duration;
 import java.util.Comparator;
+import java.util.Map;
 
 import org.springframework.boot.configurationmetadata.ConfigurationMetadataProperty;
+import org.springframework.boot.configurationmetadata.Deprecation;
 import org.springframework.boot.context.properties.source.ConfigurationProperty;
 import org.springframework.boot.origin.Origin;
 import org.springframework.boot.origin.TextResourceOrigin;
+import org.springframework.util.StringUtils;
 
 /**
  * Description of a property migration.
@@ -33,17 +37,24 @@ class PropertyMigration {
 	public static final Comparator<PropertyMigration> COMPARATOR = Comparator
 			.comparing((property) -> property.getMetadata().getId());
 
-	private final ConfigurationMetadataProperty metadata;
-
 	private final ConfigurationProperty property;
 
 	private final Integer lineNumber;
 
-	PropertyMigration(ConfigurationMetadataProperty metadata,
-			ConfigurationProperty property) {
-		this.metadata = metadata;
+	private final ConfigurationMetadataProperty metadata;
+
+	private final ConfigurationMetadataProperty replacementMetadata;
+
+	private final boolean compatibleType;
+
+	PropertyMigration(ConfigurationProperty property,
+			ConfigurationMetadataProperty metadata,
+			ConfigurationMetadataProperty replacementMetadata) {
 		this.property = property;
 		this.lineNumber = determineLineNumber(property);
+		this.metadata = metadata;
+		this.replacementMetadata = replacementMetadata;
+		this.compatibleType = determineCompatibleType(metadata, replacementMetadata);
 	}
 
 	private static Integer determineLineNumber(ConfigurationProperty property) {
@@ -57,8 +68,37 @@ private static Integer determineLineNumber(ConfigurationProperty property) {
 		return null;
 	}
 
-	public ConfigurationMetadataProperty getMetadata() {
-		return this.metadata;
+	private static boolean determineCompatibleType(ConfigurationMetadataProperty metadata,
+			ConfigurationMetadataProperty replacementMetadata) {
+		String currentType = metadata.getType();
+		String replacementType = determineReplacementType(replacementMetadata);
+		if (replacementType == null || currentType == null) {
+			return false;
+		}
+		if (replacementType.equals(currentType)) {
+			return true;
+		}
+		if (replacementType.equals(Duration.class.getName())
+				&& (currentType.equals(Long.class.getName())
+						|| currentType.equals(Integer.class.getName()))) {
+			return true;
+		}
+		return false;
+	}
+
+	private static String determineReplacementType(
+			ConfigurationMetadataProperty replacementMetadata) {
+		if (replacementMetadata == null || replacementMetadata.getType() == null) {
+			return null;
+		}
+		String candidate = replacementMetadata.getType();
+		if (candidate.startsWith(Map.class.getName())) {
+			int lastComma = candidate.lastIndexOf(',');
+			if (lastComma != -1) {
+				return candidate.substring(lastComma + 1, candidate.length() - 1).trim();
+			}
+		}
+		return candidate;
 	}
 
 	public ConfigurationProperty getProperty() {
@@ -69,4 +109,34 @@ public Integer getLineNumber() {
 		return this.lineNumber;
 	}
 
+	public ConfigurationMetadataProperty getMetadata() {
+		return this.metadata;
+	}
+
+	public boolean isCompatibleType() {
+		return this.compatibleType;
+	}
+
+	public String determineReason() {
+		if (this.compatibleType) {
+			return "Replacement: " + this.metadata.getDeprecation().getReplacement();
+		}
+		Deprecation deprecation = this.metadata.getDeprecation();
+		if (StringUtils.hasText(deprecation.getShortReason())) {
+			return "Reason: " + deprecation.getShortReason();
+		}
+		if (StringUtils.hasText(deprecation.getReplacement())) {
+			if (this.replacementMetadata != null) {
+				return String.format(
+						"Reason: Replacement key '%s' uses an incompatible target type",
+						deprecation.getReplacement());
+			}
+			else {
+				return String.format("Reason: No metadata found for replacement key '%s'",
+						deprecation.getReplacement());
+			}
+		}
+		return "Reason: none";
+	}
+
 }

spring-boot-project/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java

@@ -161,7 +161,7 @@ public void invalidReplacementHandled() throws IOException {
 		assertThat(report).isNotNull();
 		assertThat(report).containsSubsequence("Property source 'first'",
 				"deprecated.six.test", "Line: 1", "Reason",
-				"Replacement key 'does.not.exist' uses an incompatible target type");
+				"No metadata found for replacement key 'does.not.exist'");
 		assertThat(report).doesNotContain("null");
 	}