Inject property if it is defined in the external parent POM

...if needed to upgrade a dependency.

Co-authored-by: Tomas Hofman <[email protected]>

integration-tests/src/test/java/org/wildfly/channelplugin/it/UpgradeComponentsMojoIT.java

@@ -136,7 +136,7 @@ void override_property_test_case(MavenExecutionResult result) {
         Model model = result.getMavenProjectResult().getModel();
         DependencyModel dependencyModel = new DependencyModel(model);
 
-        // verify version property has been overriden
+        // verify version property has been overridden
         assertThat(model.getProperties().getProperty("undertow.version"))
                 .usingComparator(VersionMatcher.COMPARATOR).isEqualTo("2.2.5.Final-Overridden");
         // dependency still referencing the property
@@ -170,4 +170,27 @@ void override_dependency_test_case(MavenExecutionResult result) {
                     assertThat(o.get().getVersion()).isEqualTo("2.2.5.Final-Overridden");
                 });
     }
+
+
+    /**
+     * Tests injecting properties defined in external parent pom.
+     */
+    @MavenGoal("${project.groupId}:wildfly-channel-maven-plugin:${project.version}:upgrade")
+    @SystemProperty(value = "manifestFile", content = "manifest.yaml")
+    @MavenTest
+    void external_properties_test_case(MavenExecutionResult result) {
+        assertThat(result).isSuccessful();
+
+        Model model = result.getMavenProjectResult().getModel();
+        DependencyModel dependencyModel = new DependencyModel(model);
+
+        // verify version property has been overridden
+        assertThat(model.getProperties().get("version.clean.plugin")).isEqualTo("3.3.2");
+        // verify dependency still referencing the property
+        assertThat(dependencyModel.getDependency("org.apache.maven.plugins", "maven-clean-plugin", "pom", null))
+                .satisfies(o -> {
+                    assertThat(o).isPresent();
+                    assertThat(o.get().getVersion()).isEqualTo("${version.clean.plugin}");
+                });
+    }
 }

integration-tests/src/test/resources-its/org/wildfly/channelplugin/it/UpgradeComponentsMojoIT/external_properties_test_case/manifest.yaml

@@ -0,0 +1,6 @@
+schemaVersion: 1.0.0
+name: Sample Manifest
+streams:
+  - groupId: org.apache.maven.plugins
+    artifactId: maven-clean-plugin
+    version: "3.3.2"

integration-tests/src/test/resources-its/org/wildfly/channelplugin/it/UpgradeComponentsMojoIT/external_properties_test_case/pom.xml

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <parent>
+        <groupId>org.jboss</groupId>
+        <artifactId>jboss-parent</artifactId>
+        <version>40</version>
+    </parent>
+
+    <groupId>org.wildfly</groupId>
+    <artifactId>test-project</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+    <properties>
+        <maven.compiler.source>11</maven.compiler.source>
+        <maven.compiler.target>11</maven.compiler.target>
+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+    </properties>
+
+
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-clean-plugin</artifactId>
+            <version>${version.clean.plugin}</version>
+            <type>pom</type>
+        </dependency>
+    </dependencies>
+
+</project>

plugin/src/main/java/org/wildfly/channelplugin/UpgradeComponentsMojo.java

@@ -196,6 +196,14 @@ public class UpgradeComponentsMojo extends AbstractMojo {
     @Parameter(property = "ignoreTestDependencies", defaultValue = "true")
     boolean ignoreTestDependencies;
 
+    /**
+     * When a dependency is defined with version string referencing a property, and that property is defined in a parent
+     * pom outside the project, the property would be injected into a pom where the dependency is defined, if this
+     * parameter is set to true (default).
+     */
+    @Parameter(property = "injectExternalProperties", defaultValue = "true")
+    boolean injectExternalProperties;
+
     @Inject
     DependencyGraphBuilder dependencyGraphBuilder;
 
@@ -333,6 +341,9 @@ private void processModule(Project pmeProject, PomManipulator manipulator)
             String newVersion = upgrade.getRight();
             Dependency locatedDependency = upgrade.getLeft();
 
+            @SuppressWarnings("UnnecessaryLocalVariable")
+            Dependency d = locatedDependency;
+
             if (overriddenDependencies.contains(locatedDependency)) {
                 // if there was a hard version override, the dependency is not processed again
                 continue;
@@ -348,27 +359,32 @@ private void processModule(Project pmeProject, PomManipulator manipulator)
                 }
 
                 Pair<Project, String> projectProperty = followProperties(pmeProject, versionPropertyName);
+                if (projectProperty == null) {
+                    Pair<String, String> externalProperty = resolveExternalProperty(mavenProject, versionPropertyName);
+                    if (externalProperty != null) {
+                        projectProperty = Pair.of(null, externalProperty.getLeft());
+                    }
+                }
 
                 if (projectProperty == null) {
-                    Dependency d = locatedDependency;
                     ChannelPluginLogger.LOGGER.errorf(
-                            "Unable to upgrade %s:%s:%s to '%s', can't locate property '%s' in POM file %s",
+                            "Unable to upgrade %s:%s:%s to '%s', can't locate property '%s' in the project",
                             d.getGroupId(), d.getArtifactId(), d.getVersion(), newVersion,
-                            versionPropertyName, pmeProject.getPom().getPath());
+                            versionPropertyName);
                     continue;
                 }
 
                 Project targetProject = projectProperty.getLeft();
                 String targetPropertyName = projectProperty.getRight();
+
                 if (isIgnoredProperty(targetPropertyName)) {
-                    getLog().info(String.format("Ignoring property '%s' (ignored prefix)", targetPropertyName));
+                    getLog().info(String.format("Ignoring property '%s'", targetPropertyName));
                     continue;
                 }
 
                 if (upgradedProperties.containsKey(projectProperty)) {
                     if (!upgradedProperties.get(projectProperty).equals(newVersion)) {
                         // property has already been changed to different value
-                        Dependency d = locatedDependency;
                         String propertyName = projectProperty.getRight();
                         String currentPropertyValue = upgradedProperties.get(projectProperty);
                         if (inlineVersionOnConflict) {
@@ -391,10 +407,25 @@ private void processModule(Project pmeProject, PomManipulator manipulator)
 
                 // get manipulator for the module where the target property is located
                 upgradedProperties.put(projectProperty, newVersion);
-                PomManipulator targetManipulator = manipulators.get(
-                        Pair.of(targetProject.getGroupId(), targetProject.getArtifactId()));
-                targetManipulator.overrideProperty(targetPropertyName, newVersion);
-            } else { // dependency version is inlined in version element, can be directly overriden
+
+                if (targetProject != null) {
+                    // property has been located in some project module
+                    // => override the located property in the module where it has been located
+                    PomManipulator targetManipulator = manipulators.get(
+                            Pair.of(targetProject.getGroupId(), targetProject.getArtifactId()));
+                    targetManipulator.overrideProperty(targetPropertyName, newVersion);
+                } else if (injectExternalProperties) {
+                    // property has been located in external parent pom
+                    // => inject the property into current module
+                    PomManipulator targetManipulator = manipulators.get(
+                            Pair.of(pmeProject.getGroupId(), pmeProject.getArtifactId()));
+                    targetManipulator.injectProperty(targetPropertyName, newVersion);
+                } else {
+                    getLog().warn(String.format("Can't upgrade %s:%s:%s to %s, property %s is not defined in the " +
+                                    "scope of the project (consider enabling the injectExternalProperties parameter).",
+                            d.getGroupId(), d.getArtifactId(), d.getVersion(), newVersion, targetPropertyName));
+                }
+            } else { // dependency version is inlined in version element, can be directly overwritten
                 manipulator.overrideDependencyVersion(toArtifactRef(locatedDependency), newVersion);
             }
         }
@@ -566,13 +597,22 @@ private List<Pair<Dependency, String>> findDependenciesToUpgrade(
                 continue;
             }
             if (artifactRef.getVersionString() == null) {
-                getLog().warn("Resolved dependency has null version: " + artifactRef);
+                // this is not expected to happen
+                getLog().error("Resolved dependency has null version: " + artifactRef);
                 continue;
             }
             if (VersionUtils.isProperty(artifactRef.getVersionString())) {
-                // didn't manage to resolve dependency version
-                getLog().warn("Resolved dependency has version with property: " + artifactRef);
-                continue;
+                // hack: PME doesn't seem to resolve properties from external parent poms
+                Pair<String, String> externalProperty = resolveExternalProperty(mavenProject,
+                        VersionUtils.extractPropertyName(artifactRef.getVersionString()));
+                if (externalProperty != null) {
+                    artifactRef = new SimpleArtifactRef(artifactRef.getGroupId(), artifactRef.getArtifactId(),
+                            externalProperty.getRight(), artifactRef.getType(), artifactRef.getClassifier());
+                } else {
+                    // didn't manage to resolve dependency version, this is not expected to happen
+                    getLog().error("Resolved dependency has version with property: " + artifactRef);
+                    continue;
+                }
             }
             if ("test".equals(dependency.getScope()) && ignoreTestDependencies) {
                 getLog().info("Skipping dependency (ignored scope): "
@@ -612,10 +652,10 @@ private void initChannel() throws MojoExecutionException {
             throw new MojoExecutionException("Exactly one of [channelFile, channelGAV, manifestFile, manifestGAV] has to be given.");
         }
 
-        if ((StringUtils.isNotBlank(manifestFile) || StringUtils.isNotBlank(manifestGAV)) && remoteRepositories.isEmpty()) {
-            // Do not enforce this for now, repositories are also read from project pom.xml currently.
-            //throw new MojoExecutionException("The remoteRepositories property is mandatory when manifest is given.");
-        }
+        // Do not enforce this for now, repositories are also read from project pom.xml currently.
+        /*if ((StringUtils.isNotBlank(manifestFile) || StringUtils.isNotBlank(manifestGAV)) && remoteRepositories.isEmpty()) {
+            throw new MojoExecutionException("The remoteRepositories property is mandatory when manifest is given.");
+        }*/
 
         try {
             if (StringUtils.isNotBlank(channelFile)) {
@@ -838,4 +878,32 @@ static Pair<Project, String> followProperties(Project pmeProject, String propert
         }
     }
 
+    /**
+     * Resolves a property from external parent pom. If given property references another property, this method
+     * tries to traverse the property chain.
+     *
+     * @return pair [property, value], where the property is the last property name in the traversal chain
+     */
+    static Pair<String, String> resolveExternalProperty(MavenProject mavenProject, String propertyName) {
+        if (mavenProject == null) {
+            return null;
+        }
+        Properties properties = mavenProject.getModel().getProperties();
+        if (!properties.containsKey(propertyName)) {
+            return resolveExternalProperty(mavenProject.getParent(), propertyName);
+        } else {
+            // property is defined in this module
+            String propertyValue = (String) properties.get(propertyName);
+            if (VersionUtils.isProperty(propertyValue)) {
+                // the property value is also a property reference -> follow the chain
+                String newPropertyName = VersionUtils.extractPropertyName(propertyValue);
+                Pair<String, String> targetProperty = resolveExternalProperty(mavenProject, newPropertyName);
+                if (targetProperty != null) {
+                    return targetProperty;
+                }
+            }
+            return Pair.of(propertyName, propertyValue);
+        }
+    }
+
 }

plugin/src/main/java/org/wildfly/channelplugin/manipulation/PomManipulator.java

@@ -95,6 +95,10 @@ public void injectRepository(String id, String url) throws XMLStreamException {
         }
     }
 
+    public void injectProperty(String key, String version) throws XMLStreamException {
+        injectProperty(eventReader, key, version);
+    }
+
     /**
      * Writes the updated POM file.
      */
@@ -116,15 +120,15 @@ private void assertOpen() {
     }
 
     /**
-     * This method attempts to inject new depenendency into at the end of the dependencyManagement section.
+     * This method attempts to inject new dependency into at the end of the dependencyManagement section.
      * <p>
      * The dependencyManagement section must be already present in the POM.
      */
     static void injectManagedDependency(ModifiedPomXMLEventReader eventReader, ArtifactRef dependency,
             Collection<ProjectRef> exclusions) throws XMLStreamException {
         eventReader.rewind();
 
-        Stack<String> stack = new Stack<String>();
+        Stack<String> stack = new Stack<>();
         String path = "";
 
         while (eventReader.hasNext()) {
@@ -148,6 +152,38 @@ static void injectManagedDependency(ModifiedPomXMLEventReader eventReader, Artif
         }
     }
 
+    /**
+     * This method attempts to inject new property at the end of the properties section.
+     * <p>
+     * The properties section must be already present in the POM.
+     */
+    static void injectProperty(ModifiedPomXMLEventReader eventReader, String key, String version) throws XMLStreamException {
+        eventReader.rewind();
+
+        Stack<String> stack = new Stack<>();
+        String path = "";
+
+        while (eventReader.hasNext()) {
+            XMLEvent event = eventReader.nextEvent();
+            if (event.isStartElement()) {
+                stack.push(path);
+                path = path + "/" + event.asStartElement().getName().getLocalPart();
+            } else if (event.isEndElement()) {
+                if (event.asEndElement().getName().getLocalPart().equals("properties")
+                        && path.equals("/project/properties")) {
+                    eventReader.mark(0);
+                    eventReader.replaceMark(0, String.format("    <%s>%s</%s>\n", key, version, key)
+                            + "    </properties>"
+                    );
+                    eventReader.clearMark(0);
+                    break;
+                }
+
+                path = stack.pop();
+            }
+        }
+    }
+
     private static String composeDependencyElementString(ArtifactRef artifact, Collection<ProjectRef> exclusions) {
         StringBuilder sb = new StringBuilder();
         sb.append("    <dependency>\n");