From 962936370adcd2d3e073b2804b22e0b5202238e2 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 26 Jun 2024 16:18:30 -0700 Subject: [PATCH] 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 --- .../migrator/PropertiesMigrationReporter.java | 177 ++++++++++-------- .../migrator/PropertyMigration.java | 6 +- .../PropertiesMigrationReporterTests.java | 16 +- .../config/config-relaxed.properties | 1 + .../resources/metadata/sample-metadata.json | 11 ++ .../boot/origin/PropertySourceOrigin.java | 39 +++- .../origin/PropertySourceOriginTests.java | 6 +- 7 files changed, 172 insertions(+), 84 deletions(-) create mode 100644 spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties diff --git a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java index 85603a7bb0c..4be09608c1f 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporter.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/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.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> properties = getMatchingProperties( + Map> properties = getPropertySourceMigrations( ConfigurationMetadataProperty::isDeprecated); if (properties.isEmpty()) { return report; @@ -78,6 +81,103 @@ class PropertiesMigrationReporter { return report; } + private Map> getPropertySourceMigrations( + Predicate filter) { + return getPropertySourceMigrations(this.allProperties.values().stream().filter(filter).toList()); + } + + private Map> getPropertySourceMigrations( + List metadataProperties) { + MultiValueMap result = new LinkedMultiValueMap<>(); + getPropertySourcesAsMap().forEach((propertySourceName, propertySource) -> { + for (ConfigurationMetadataProperty metadataProperty : metadataProperties) { + result.addAll(propertySourceName, getMigrations(propertySource, metadataProperty)); + } + }); + return result; + } + + private Map getPropertySourcesAsMap() { + Map 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 getMigrations(ConfigurationPropertySource propertySource, + ConfigurationMetadataProperty metadataProperty) { + ConfigurationPropertyName propertyName = asConfigurationPropertyName(metadataProperty); + List 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 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 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> getMatchingProperties( - Predicate filter) { - MultiValueMap result = new LinkedMultiValueMap<>(); - List 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 getPropertySourcesAsMap() { - Map 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. diff --git a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java index 93646794698..564699c3cc7 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/main/java/org/springframework/boot/context/properties/migrator/PropertyMigration.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/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.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; diff --git a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java index 3de49fe3666..9149f967070 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/java/org/springframework/boot/context/properties/migrator/PropertiesMigrationReporterTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/java/org/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.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) propertySource).getOrigin(name)).isEqualTo(origin); + Origin actualOrigin = ((OriginLookup) propertySource).getOrigin(name); + if (actualOrigin instanceof PropertySourceOrigin propertySourceOrigin) { + actualOrigin = propertySourceOrigin.getOrigin(); + } + assertThat(actualOrigin).isEqualTo(origin); } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties new file mode 100644 index 00000000000..84e50f6b863 --- /dev/null +++ b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/config/config-relaxed.properties @@ -0,0 +1 @@ +relaxed.this-that-the-other=test diff --git a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/metadata/sample-metadata.json b/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/metadata/sample-metadata.json index 4c76e6cbaa8..fefdb392d6a 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-properties-migrator/src/test/resources/metadata/sample-metadata.json +++ b/spring-boot-project/spring-boot-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" + }, + { + "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/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/origin/PropertySourceOrigin.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/origin/PropertySourceOrigin.java index 48c7f4e16c2..6bb296889cd 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/origin/PropertySourceOrigin.java +++ b/spring-boot-project/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 @@ 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); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/origin/PropertySourceOriginTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/origin/PropertySourceOriginTests.java index a8647f6ca16..fc3dee39cd4 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/origin/PropertySourceOriginTests.java +++ b/spring-boot-project/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 @@ class PropertySourceOriginTests { withSettings().extraInterfaces(OriginLookup.class)); OriginLookup originCapablePropertySource = (OriginLookup) 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