From 4cae2c9c94c273b36eab5e45df528cb4f40a0a1a Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 28 Dec 2018 18:19:12 +0100 Subject: [PATCH] Polish "Avoid NPE when replacement property does not exist" Closes gh-15394 --- .../migrator/PropertiesMigrationReport.java | 56 ++++-------- .../migrator/PropertiesMigrationReporter.java | 85 ++++++------------- .../migrator/PropertyMigration.java | 84 ++++++++++++++++-- .../PropertiesMigrationReporterTests.java | 2 +- 4 files changed, 120 insertions(+), 107 deletions(-) diff --git a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReport.java b/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReport.java index 41c9245905..b2550ac031 100644 --- a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReport.java +++ b/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReport.java @@ -17,7 +17,6 @@ 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; @@ -25,8 +24,6 @@ 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 @@ class PropertiesMigrationReport { 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,7 +71,7 @@ class PropertiesMigrationReport { 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."); @@ -83,19 +79,6 @@ class PropertiesMigrationReport { 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> getContent( Function> extractor) { return this.content.entrySet().stream() @@ -105,8 +88,7 @@ class PropertiesMigrationReport { } private void append(StringBuilder report, - Map> content, - Function deprecationMessage) { + Map> content) { content.forEach((name, properties) -> { report.append(String.format("Property source '%s':%n", name)); properties.sort(PropertyMigration.COMPARATOR); @@ -117,8 +99,7 @@ class PropertiesMigrationReport { 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 @@ class PropertiesMigrationReport { /** * 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 renamed, - List unsupported) { - this.content.put(name, new LegacyProperties(renamed, unsupported)); + void add(String name, List properties) { + this.content.put(name, new LegacyProperties(properties)); } private static class LegacyProperties { - private final List renamed; - - private final List unsupported; - - LegacyProperties(List renamed, - List unsupported) { - this.renamed = asNewList(renamed); - this.unsupported = asNewList(unsupported); - } + private final List properties; - private List asNewList(List source) { - return (source != null) ? new ArrayList<>(source) : Collections.emptyList(); + LegacyProperties(List properties) { + this.properties = new ArrayList<>(properties); } public List getRenamed() { - return this.renamed; + return this.properties.stream().filter(PropertyMigration::isCompatibleType) + .collect(Collectors.toList()); } public List getUnsupported() { - return this.unsupported; + return this.properties.stream() + .filter((property) -> !property.isCompatibleType()) + .collect(Collectors.toList()); } } diff --git a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java b/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java index dc4b7017d1..880c02fe6f 100644 --- a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java +++ b/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 @@ class PropertiesMigrationReporter { private PropertySource mapPropertiesWithReplacement( PropertiesMigrationReport report, String name, List properties) { - List renamed = new ArrayList<>(); - List unsupported = new ArrayList<>(); - properties.forEach((property) -> (isRenamed(property) ? renamed : unsupported) - .add(property)); - report.add(name, renamed, unsupported); + report.add(name, properties); + List renamed = properties.stream() + .filter(PropertyMigration::isCompatibleType).collect(Collectors.toList()); if (renamed.isEmpty()) { return null; } @@ -102,55 +98,6 @@ class PropertiesMigrationReporter { 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> getMatchingProperties( Predicate filter) { MultiValueMap result = new LinkedMultiValueMap<>(); @@ -162,14 +109,36 @@ class PropertiesMigrationReporter { .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 deprecatedFilter() { return (property) -> property.getDeprecation() != null && property.getDeprecation().getLevel() == Deprecation.Level.ERROR; diff --git a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java b/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java index d98a08d783..1089d475e1 100644 --- a/spring-boot-project/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java +++ b/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 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 @@ class PropertyMigration { 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 @@ class PropertyMigration { 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"; + } + } diff --git a/spring-boot-project/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java b/spring-boot-project/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java index 076f01af31..d6fb45c447 100644 --- a/spring-boot-project/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java +++ b/spring-boot-project/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java @@ -161,7 +161,7 @@ public class PropertiesMigrationReporterTests { 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"); }