Skip to content

Configuration properties changelog generator misses items that are removed without prior deprecation #45267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+41 −11
Closed

Configuration properties changelog generator misses items that are removed without prior deprecation #45267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions ...tor/src/main/java/org/springframework/boot/configurationmetadata/changelog/Changelog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2025 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.
Expand All @@ -21,6 +21,7 @@

import org.springframework.boot.configurationmetadata.ConfigurationMetadataProperty;
import org.springframework.boot.configurationmetadata.ConfigurationMetadataRepository;
import org.springframework.boot.configurationmetadata.Deprecation;

/**
* A changelog containing differences computed from two repositories of configuration
Expand All @@ -32,6 +33,7 @@
* @author Stephane Nicoll
* @author Andy Wilkinson
* @author Phillip Webb
* @author Yoobin Yoon
*/
record Changelog(String oldVersionNumber, String newVersionNumber, List<Difference> differences) {

Expand All @@ -48,14 +50,24 @@ static List<Difference> computeDifferences(ConfigurationMetadataRepository oldMe
String id = oldProperty.getId();
seenIds.add(id);
ConfigurationMetadataProperty newProperty = newMetadata.getAllProperties().get(id);
Difference difference = Difference.compute(oldProperty, newProperty);
if (difference != null) {
differences.add(difference);
if (newProperty == null) {
differences.add(new Difference(DifferenceType.DELETED, oldProperty, null));
}
else {
Difference difference = Difference.compute(oldProperty, newProperty);
if (difference != null) {
differences.add(difference);
}
Comment on lines +53 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't necessary as Difference.compute already handles the case where newProperty is null. Additionally, it checks that the old property isn't deprecated at error level before reporting a difference, something that's lost here.

I'll revert this part of the changes and add a test for this scenario (removing metadata for a property that's deprecated at error level) as part of merging this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your explanation about why that part of the change wasn't necessary and how the existing code already handles the case.

}
}
for (ConfigurationMetadataProperty newProperty : newMetadata.getAllProperties().values()) {
if ((!seenIds.contains(newProperty.getId())) && (!newProperty.isDeprecated())) {
differences.add(new Difference(DifferenceType.ADDED, null, newProperty));
if (!seenIds.contains(newProperty.getId())) {
if (newProperty.isDeprecated() && newProperty.getDeprecation().getLevel() == Deprecation.Level.ERROR) {
differences.add(new Difference(DifferenceType.DELETED, null, newProperty));
}
else if (!newProperty.isDeprecated()) {
differences.add(new Difference(DifferenceType.ADDED, null, newProperty));
}
}
}
return List.copyOf(differences);
Expand Down
11 changes: 7 additions & 4 deletions ...rc/test/java/org/springframework/boot/configurationmetadata/changelog/ChangelogTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2023 the original author or authors.
* Copyright 2012-2025 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.
Expand Down Expand Up @@ -29,6 +29,7 @@
*
* @author Stephane Nicoll
* @author Andy Wilkinson
* @author Yoobin Yoon
*/
class ChangelogTests {

Expand All @@ -38,7 +39,7 @@ void diffContainsDifferencesBetweenLeftAndRightInputs() {
assertThat(differences).isNotNull();
assertThat(differences.oldVersionNumber()).isEqualTo("1.0");
assertThat(differences.newVersionNumber()).isEqualTo("2.0");
assertThat(differences.differences()).hasSize(4);
assertThat(differences.differences()).hasSize(5);
List<Difference> added = differences.differences()
.stream()
.filter((difference) -> difference.type() == DifferenceType.ADDED)
Expand All @@ -49,10 +50,12 @@ void diffContainsDifferencesBetweenLeftAndRightInputs() {
.stream()
.filter((difference) -> difference.type() == DifferenceType.DELETED)
.toList();
assertThat(deleted).hasSize(2)
assertThat(deleted).hasSize(3)
.anySatisfy((entry) -> assertProperty(entry.oldProperty(), "test.delete", String.class, "delete"))
.anySatisfy(
(entry) -> assertProperty(entry.newProperty(), "test.delete.deprecated", String.class, "delete"));
(entry) -> assertProperty(entry.newProperty(), "test.delete.deprecated", String.class, "delete"))
.anySatisfy((entry) -> assertProperty(entry.newProperty(), "test.removed.directly", String.class,
"directlyRemoved"));
List<Difference> deprecated = differences.differences()
.stream()
.filter((difference) -> difference.type() == DifferenceType.DEPRECATED)
Expand Down
13 changes: 12 additions & 1 deletion ...spring-boot-configuration-metadata-changelog-generator/src/test/resources/sample-2.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@
"replacement": "test.add",
"reason": "it was just bad"
}
},
{
"name": "test.removed.directly",
"type": "java.lang.String",
"description": "Test property removed without prior deprecation.",
"defaultValue": "directlyRemoved",
"deprecation": {
"level": "error",
"replacement": "test.new.property",
"reason": "Removed in Upgrade 10"
}
}
]
}
}
4 changes: 4 additions & 0 deletions ...-boot-configuration-metadata-changelog-generator/src/test/resources/sample.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ _None_.
| `test.delete.deprecated`
| `test.add`
| it was just bad

| `test.removed.directly`
| `test.new.property`
| Removed in Upgrade 10
|======================