gnodet commented on code in PR #13059:
URL: https://github.com/apache/maven/pull/13059#discussion_r3955003677
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -523,49 +526,81 @@ && isPropertyUsedByQuarkusBom(pomDocument, propertyName))
{
/**
* Upgrades a property value if it represents a plugin version below the
minimum.
+ * First checks the current POM's properties, then searches other POMs in
the project
+ * (e.g., parent POMs) if the property is not found locally.
*/
private boolean upgradePropertyVersion(
Document pomDocument,
+ Map<Path, Document> pomMap,
String propertyName,
PluginUpgradeInfo upgrade,
String sectionName,
UpgradeContext context) {
- Editor editor = new Editor(pomDocument);
- Element root = editor.root();
+ // First, try the current POM's properties
+ if (upgradePropertyInDocument(pomDocument, propertyName, upgrade,
sectionName, context)) {
+ return true;
+ }
+
+ // Property not found or not upgradable in current POM — search other
POMs in the project
+ for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
+ Document otherDoc = entry.getValue();
+ if (otherDoc == pomDocument) {
+ continue; // Skip the current POM, already checked
+ }
+ if (upgradePropertyInDocument(otherDoc, propertyName, upgrade,
sectionName, context)) {
+ return true;
+ }
+ }
+
+ // Property not found anywhere in the project
+ context.warning("Property " + propertyName + " not found in any
project POM properties");
Review Comment:
⚠️ **Spurious warning when property is already at target version.**
`upgradePropertyInDocument()` returns `false` for two distinct cases:
1. Property element not found → fall-through to search other POMs is correct
2. Property found but already at/above minimum → fall-through is **wrong**
In case 2, the loop finds nothing in sibling POMs (the property is in the
current POM, not there), falls through to this line, and emits `"Property X not
found in any project POM properties"` — which is factually wrong. The property
**was** found; it just didn't need upgrading.
**Concrete scenario:** Root POM has
`<exec.maven.version>3.5.0</exec.maven.version>` (already at target). Assembly
submodule uses `<version>${exec.maven.version}</version>`. User runs `mvnup`.
`upgradePropertyInDocument` on the current POM returns `false` (already at
min), the pomMap loop finds nothing, warning fires. User sees a confusing "not
found" message for a property that is perfectly defined.
Fix: track whether the property was *found* (regardless of upgrade outcome)
to suppress the false warning. Add an existence check before the search loop:
```suggestion
// First, try the current POM's properties
if (upgradePropertyInDocument(pomDocument, propertyName, upgrade,
sectionName, context)) {
return true;
}
// Check if property exists in the current POM but is already
at/above min (no upgrade needed).
// In that case, skip the cross-POM search and the warning — the
property IS defined.
Element currentRoot = pomDocument.root();
Element currentProps =
currentRoot.childElement(PROPERTIES).orElse(null);
if (currentProps != null &&
currentProps.childElement(propertyName).isPresent()) {
return false; // Found in current POM, no upgrade needed
}
// Property not in current POM — search other POMs in the project
(e.g., parent POM)
for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
Document otherDoc = entry.getValue();
if (otherDoc == pomDocument) {
continue; // Skip the current POM, already checked
}
if (upgradePropertyInDocument(otherDoc, propertyName, upgrade,
sectionName, context)) {
return true;
}
}
// Property not found anywhere in the project
context.warning("Property " + propertyName + " not found in any
project POM properties");
return false;
```
Also needs a test: single POM with
`<exec.maven.version>3.5.0</exec.maven.version>` + submodule using
`${exec.maven.version}` → no warning emitted, no modification.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -712,6 +712,106 @@ void shouldNotUpgradeWhenPropertyNotFound() throws
Exception {
assertTrue(result.success(), "Plugin upgrade should succeed");
// Note: POM might still be modified due to plugin management
additions
}
+
+ @Test
+ @DisplayName("should upgrade plugin with property version defined in
parent POM")
+ void shouldUpgradePluginWithPropertyVersionInParentPom() throws
Exception {
+ // Simulates hbase pattern: root POM defines
<exec.maven.version>3.1.0</exec.maven.version>
+ // and submodule uses <version>${exec.maven.version}</version>
+ String parentPomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>org.example</groupId>
+ <artifactId>parent</artifactId>
+ <version>1.0.0</version>
+ <packaging>pom</packaging>
+ <properties>
+ <exec.maven.version>3.1.0</exec.maven.version>
+ </properties>
+ <modules>
+ <module>assembly</module>
+ </modules>
+ </project>
+ """;
+
+ String submodulePomXml = """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <project xmlns="http://maven.apache.org/POM/4.0.0";>
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.example</groupId>
+ <artifactId>parent</artifactId>
+ <version>1.0.0</version>
+ </parent>
+ <artifactId>assembly</artifactId>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>exec-maven-plugin</artifactId>
+ <version>${exec.maven.version}</version>
+ </plugin>
+ </plugins>
+ </build>
+ </project>
+ """;
+
+ Path tempDir = Files.createTempDirectory("mvnup-test-");
+ try {
+ Files.createDirectories(tempDir.resolve(".mvn"));
+ Path parentPomPath = tempDir.resolve("pom.xml");
+ Files.writeString(parentPomPath, parentPomXml);
+ Path assemblyDir = tempDir.resolve("assembly");
+ Files.createDirectories(assemblyDir);
+ Path submodulePomPath = assemblyDir.resolve("pom.xml");
+ Files.writeString(submodulePomPath, submodulePomXml);
Review Comment:
💡 **Nit: dead filesystem I/O in this test.** `doApply(context, pomMap)`
reads `Document` objects from the `pomMap` values — it does not read files from
disk by path. The `Files.createDirectories(tempDir.resolve(".mvn"))` and
`Files.writeString(...)` calls are never consumed by the strategy in a
property-lookup scenario like this one. (The pre-existing tests that use a
remote parent `org.apache:apache:23` trigger effective-model resolution which
does need the filesystem — different situation.)
These calls are harmless but add confusion. The test could be simplified:
just construct `Document.of(xml)` and pass the in-memory map with any `Path`
key (e.g., `Paths.get("pom.xml")`), same as `jarPluginTargetShouldBe342()` does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
