Resolves#1066: Fixed a possible NPE in interpolateVersion if a depden…

…dencyManagement entry has no version. (#1166)

Added a couple unit tests to interpolateVersion.
interpolateVersion does not mutate the dependency anymore but returns a modified copy if it needs to modify it.

versions-common/src/main/java/org/codehaus/mojo/versions/utils/MavenProjectUtils.java

@@ -32,13 +32,15 @@
 
 import static java.util.Collections.emptyList;
 import static java.util.Optional.ofNullable;
+import static org.apache.commons.lang3.StringUtils.startsWith;
 
 /**
  * Utility methods for extracting dependencies from a {@link org.apache.maven.project.MavenProject}
  */
 public class MavenProjectUtils {
     /**
      * Retrieves dependencies from the plugins section
+     *
      * @param project {@link MavenProject} instance
      * @return set of {@link Dependency} objects
      * or an empty set if none have been retrieveddependencies or an empty set if none have been retrieved
@@ -56,9 +58,10 @@ public static Set<Dependency> extractPluginDependenciesFromPluginsInPluginManage
 
     /**
      * Retrieves dependencies from plugin management
+     *
      * @param project {@link MavenProject} instance
      * @return set of {@link Dependency} objects
-     * or an empty set if none have been retrieveddependencies or an empty set if none have been retrieved
+     *         or an empty set if none have been retrieveddependencies or an empty set if none have been retrieved
      */
     public static Set<Dependency> extractDependenciesFromPlugins(MavenProject project) {
         return project.getBuildPlugins().stream()
@@ -71,14 +74,15 @@ public static Set<Dependency> extractDependenciesFromPlugins(MavenProject projec
      * Retrieves dependencies from the dependency management of the project
      * as well as its immediate parent project.
      *
-     * @param project {@link MavenProject} instance
+     * @param project                               {@link MavenProject} instance
      * @param processDependencyManagementTransitive if {@code true}, the original model will be considered
      *                                              instead of the interpolated model, which does not contain
      *                                              imported dependencies
-     * @param log {@link Log} instance (may not be null)
+     * @param log                                   {@link Log} instance (may not be null)
      * @return set of {@link Dependency} objects
      * @throws VersionRetrievalException thrown if version information retrieval fails
-     * or an empty set if none have been retrieveddependencies or an empty set if none have been retrieved
+     *                                   or an empty set if none have been retrieveddependencies or an empty set if none
+     *                                   have been retrieved
      */
     public static Set<Dependency> extractDependenciesFromDependencyManagement(
             MavenProject project, boolean processDependencyManagementTransitive, Log log)
@@ -119,8 +123,8 @@ public static Set<Dependency> extractDependenciesFromDependencyManagement(
                         throw new VersionRetrievalException(message);
                     }
                 } else {
-                    dependency = interpolateVersion(dependency, project);
-                    dependencyManagement.add(dependency);
+                    dependencyManagement.remove(dependency);
+                    dependencyManagement.add(interpolateVersion(dependency, project));
                 }
             }
         }
@@ -131,22 +135,24 @@ public static Set<Dependency> extractDependenciesFromDependencyManagement(
      * Attempts to interpolate the version from model properties.
      *
      * @param dependency the dependency
-     * @param project the maven project
+     * @param project    the maven project
      * @return the dependency with interpolated property (as far as possible)
      * @since 2.14.0
      */
     public static Dependency interpolateVersion(final Dependency dependency, final MavenProject project) {
-
         // resolve version from model properties if necessary (e.g. "${mycomponent.myversion}"
-        if (dependency.getVersion().startsWith("${")) {
-            final String resolvedVersion = project.getOriginalModel()
-                    .getProperties()
-                    .getProperty(dependency
-                            .getVersion()
-                            .substring(2, dependency.getVersion().length() - 1));
-            if (resolvedVersion != null && !resolvedVersion.isEmpty()) {
-                dependency.setVersion(resolvedVersion);
-            }
+        if (startsWith(dependency.getVersion(), "${")) {
+            return ofNullable(project.getOriginalModel()
+                            .getProperties()
+                            .getProperty(dependency
+                                    .getVersion()
+                                    .substring(2, dependency.getVersion().length() - 1)))
+                    .map(v -> {
+                        Dependency result = dependency.clone();
+                        result.setVersion(v);
+                        return result;
+                    })
+                    .orElse(dependency);
         }
         return dependency;
     }

versions-common/src/test/java/org/codehaus/mojo/versions/utils/MavenProjectUtilsTest.java

@@ -0,0 +1,88 @@
+package org.codehaus.mojo.versions.utils;
+
+import java.util.Properties;
+
+import org.apache.maven.model.Dependency;
+import org.apache.maven.model.Model;
+import org.apache.maven.project.MavenProject;
+import org.junit.jupiter.api.Test;
+
+import static org.codehaus.mojo.versions.utils.MavenProjectUtils.interpolateVersion;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.sameInstance;
+import static org.hamcrest.core.Is.is;
+
+public class MavenProjectUtilsTest {
+
+    @Test
+    public void testInterpolateVersion() {
+        Dependency dep = DependencyBuilder.newBuilder()
+                .withGroupId("groupA")
+                .withArtifactId("artifactA")
+                .withVersion("${param}")
+                .build();
+        MavenProject proj = new MavenProject() {
+            {
+                setOriginalModel(new Model() {
+                    {
+                        setProperties(new Properties() {
+                            {
+                                setProperty("param", "1.0.0");
+                            }
+                        });
+                    }
+                });
+            }
+        };
+        Dependency result = interpolateVersion(dep, proj);
+        assertThat(result.getVersion(), is("1.0.0"));
+    }
+
+    @Test
+    public void testImmutability() {
+        Dependency dep = DependencyBuilder.newBuilder()
+                .withGroupId("groupA")
+                .withArtifactId("artifactA")
+                .withVersion("${param}")
+                .build();
+        MavenProject proj = new MavenProject() {
+            {
+                setOriginalModel(new Model() {
+                    {
+                        setProperties(new Properties() {
+                            {
+                                setProperty("param", "1.0.0");
+                            }
+                        });
+                    }
+                });
+            }
+        };
+        assertThat(interpolateVersion(dep, proj), not(sameInstance(dep)));
+    }
+
+    @Test
+    public void testVersionlessDependency() {
+        Dependency dep = DependencyBuilder.newBuilder()
+                .withGroupId("groupA")
+                .withArtifactId("artifactA")
+                .build();
+        MavenProject proj = new MavenProject() {
+            {
+                setOriginalModel(new Model() {
+                    {
+                        setProperties(new Properties() {
+                            {
+                                setProperty("param", "1.0.0");
+                            }
+                        });
+                    }
+                });
+            }
+        };
+        Dependency result = interpolateVersion(dep, proj);
+        assertThat(result.getVersion(), nullValue());
+    }
+}

versions-maven-plugin/src/main/java/org/codehaus/mojo/versions/AbstractDependencyUpdatesReportMojo.java

@@ -33,6 +33,7 @@
 import org.apache.maven.artifact.versioning.ArtifactVersion;
 import org.apache.maven.doxia.sink.Sink;
 import org.apache.maven.model.Dependency;
+import org.apache.maven.model.DependencyManagement;
 import org.apache.maven.plugins.annotations.Parameter;
 import org.apache.maven.project.MavenProject;
 import org.apache.maven.reporting.MavenReportException;
@@ -47,6 +48,7 @@
 import org.eclipse.aether.RepositorySystem;
 
 import static java.util.Collections.emptyMap;
+import static java.util.Optional.ofNullable;
 import static org.codehaus.mojo.versions.utils.MavenProjectUtils.interpolateVersion;
 import static org.codehaus.mojo.versions.utils.MiscUtils.filter;
 
@@ -189,29 +191,29 @@ protected void handleDependencyManagementTransitive(
             MavenProject project, Set<Dependency> dependencyManagementCollector) throws MavenReportException {
         if (processDependencyManagementTransitive) {
             if (hasDependencyManagement(project)) {
-                for (Dependency dep : project.getDependencyManagement().getDependencies()) {
-                    getLog().debug("Dpmg: " + dep.getGroupId() + ":" + dep.getArtifactId() + ":" + dep.getVersion()
-                            + ":" + dep.getType() + ":" + dep.getScope());
+                if (getLog().isDebugEnabled()) {
+                    project.getDependencyManagement().getDependencies().forEach(dep -> getLog().debug(
+                                    "Dpmg: " + dep.getGroupId() + ":" + dep.getArtifactId() + ":" + dep.getVersion()
+                                            + ":" + dep.getType() + ":" + dep.getScope()));
                 }
                 dependencyManagementCollector.addAll(
                         project.getDependencyManagement().getDependencies());
             }
         } else {
-            if (project.getOriginalModel().getDependencyManagement() != null
-                    && project.getOriginalModel().getDependencyManagement().getDependencies() != null) {
-                // Using the original model to getModel the original dependencyManagement entries and
-                // not the interpolated model.
-                // TODO: I'm not 100% sure if this will work correctly in all cases.
-                for (Dependency dep :
-                        project.getOriginalModel().getDependencyManagement().getDependencies()) {
-                    dep = interpolateVersion(dep, project);
-
-                    getLog().debug("Original Dpmg: " + dep.getGroupId() + ":" + dep.getArtifactId() + ":"
-                            + dep.getVersion() + ":" + dep.getType() + ":" + dep.getScope());
-                }
-                dependencyManagementCollector.addAll(
-                        project.getOriginalModel().getDependencyManagement().getDependencies());
-            }
+            // Using the original model to getModel the original dependencyManagement entries and
+            // not the interpolated model.
+            // TODO: I'm not 100% sure if this will work correctly in all cases.
+            ofNullable(project.getOriginalModel().getDependencyManagement())
+                    .map(DependencyManagement::getDependencies)
+                    .ifPresent(list -> list.stream()
+                            .map(dep -> interpolateVersion(dep, project))
+                            .peek(dep -> {
+                                if (getLog().isDebugEnabled()) {
+                                    getLog().debug("Original Dpmg: " + dep.getGroupId() + ":" + dep.getArtifactId()
+                                            + ":" + dep.getVersion() + ":" + dep.getType() + ":" + dep.getScope());
+                                }
+                            })
+                            .forEach(dependencyManagementCollector::add));
         }
     }
 

versions-maven-plugin/src/test/java/org/codehaus/mojo/versions/DependencyUpdatesReportMojoTest.java

@@ -364,4 +364,15 @@ public void testResolvedVersionsWithoutTransitiveDependencyManagement()
         String output = os.toString();
         assertThat(output, Matchers.stringContainsInOrder("artifactA", "1.0.0", "artifactB", "1.2.3"));
     }
+
+    @Test
+    public void testVersionlessDependency() throws IOException, MavenReportException {
+        OutputStream os = new ByteArrayOutputStream();
+        SinkFactory sinkFactory = new Xhtml5SinkFactory();
+        new TestDependencyUpdatesReportMojo()
+                .withOriginalDependencyManagement(dependencyOf("artifactA", null))
+                .withProcessDependencyManagement(true)
+                .withProcessDependencyManagementTransitive(false)
+                .generate(sinkFactory.createSink(os), sinkFactory, Locale.getDefault());
+    }
 }