Resolves mojohaus#848: Fixing module resolution with nonstandard file…

…names

versions-common/src/main/java/org/codehaus/mojo/versions/api/PomHelper.java

@@ -106,7 +106,9 @@ public static Model getRawModel(MavenProject project) throws IOException {
     public static Model getRawModel(File moduleProjectFile) throws IOException {
         try (Reader reader =
                 new BufferedReader(new InputStreamReader(Files.newInputStream(moduleProjectFile.toPath())))) {
-            return getRawModel(reader);
+            Model result = getRawModel(reader);
+            result.setPomFile(moduleProjectFile);
+            return result;
         }
     }
 
@@ -1202,36 +1204,6 @@ public static void debugModules(Log logger, String message, Collection<String> m
         }
     }
 
-    /**
-     * Modifies the collection of child modules removing those which cannot be found relative to the parent.
-     *
-     * @param logger       The logger to log to.
-     * @param basedir      the project basedir.
-     * @param childModules the child modules.
-     */
-    public static void removeMissingChildModules(Log logger, File basedir, Collection<String> childModules) {
-        logger.debug("Removing child modules which are missing...");
-        Iterator<String> i = childModules.iterator();
-        while (i.hasNext()) {
-            String modulePath = i.next();
-            File moduleFile = new File(basedir, modulePath);
-
-            if (moduleFile.isDirectory() && new File(moduleFile, "pom.xml").isFile()) {
-                // it's a directory that exists
-                continue;
-            }
-
-            if (moduleFile.isFile()) {
-                // it's the pom.xml file directly referenced and it exists.
-                continue;
-            }
-
-            logger.debug("Removing missing child module " + modulePath);
-            i.remove();
-        }
-        debugModules(logger, "After removing missing", childModules);
-    }
-
     /**
      * Extracts the version from a raw model, interpolating from the parent if necessary.
      *
@@ -1362,48 +1334,39 @@ public static ProjectBuildingRequest createProjectBuilderRequest(
      * @return A map of raw models keyed by path relative to the project's basedir.
      * @throws IOException if things go wrong.
      */
-    public static Map<String, Model> getReactorModels(MavenProject project, Log logger) throws IOException {
-        Map<String, Model> result = new LinkedHashMap<>();
+    public static Map<File, Model> getChildModels(MavenProject project, Log logger) throws IOException {
+        Map<File, Model> result = new LinkedHashMap<>();
         final Model model = getRawModel(project);
-        final String path = "";
-        result.put(path, model);
-        result.putAll(getReactorModels(path, model, project, logger));
+        result.put(project.getFile(), model);
+        result.putAll(getChildModels(model, project, logger));
         return result;
     }
 
     /**
      * Builds a sub-map of raw models keyed by module path.
      *
-     * @param path    The relative path to base the sub-map on.
-     * @param model   The model at the relative path.
+     * @param model   The root model
      * @param project The project to build from.
      * @param logger  The logger for logging.
      * @return A map of raw models keyed by path relative to the project's basedir.
      * @throws IOException if things go wrong.
      */
-    private static Map<String, Model> getReactorModels(String path, Model model, MavenProject project, Log logger)
-            throws IOException {
-        Map<String, Model> result = new LinkedHashMap<>();
-        Map<String, Model> childResults = new LinkedHashMap<>();
-        Set<String> childModules = getAllChildModules(model, logger);
-
-        File baseDir = path.length() > 0 ? new File(project.getBasedir(), path) : project.getBasedir();
-        removeMissingChildModules(logger, baseDir, childModules);
+    private static Map<File, Model> getChildModels(Model model, MavenProject project, Log logger) throws IOException {
+        Map<File, Model> result = new LinkedHashMap<>();
+        Map<File, Model> childResults = new LinkedHashMap<>();
 
-        childModules.stream()
+        File baseDir = model.getPomFile().getParentFile();
+        getAllChildModules(model, logger).parallelStream()
                 .map(moduleName -> new File(baseDir, moduleName))
+                .map(file -> file.isFile() ? file : new File(file, "pom.xml"))
                 .filter(File::exists)
-                .forEach(moduleFile -> {
-                    File pomFile = moduleFile.isDirectory() ? new File(moduleFile, "/pom.xml") : moduleFile;
-                    String modulePath = (!path.isEmpty() && !path.endsWith("/") ? path + "/" : path)
-                            + pomFile.getParentFile().getName();
-
+                .forEach(pomFile -> {
                     try {
                         // the aim of this goal is to fix problems when the project cannot be parsed by Maven,
                         // so we have to work with the raw model and not the interpolated parsed model from maven
                         Model moduleModel = getRawModel(pomFile);
-                        result.put(modulePath, moduleModel);
-                        childResults.putAll(getReactorModels(modulePath, moduleModel, project, logger));
+                        result.put(pomFile, moduleModel);
+                        childResults.putAll(getChildModels(moduleModel, project, logger));
                     } catch (IOException e) {
                         logger.debug("Could not parse " + pomFile.getPath(), e);
                     }
@@ -1421,10 +1384,10 @@ private static Map<String, Model> getReactorModels(String path, Model model, Mav
      * @param artifactId The artifactId of the parent.
      * @return a map of models that have a specified groupId and artifactId as parent keyed by path.
      */
-    public static Map<String, Model> getChildModels(Map<String, Model> reactor, String groupId, String artifactId) {
-        final Map<String, Model> result = new LinkedHashMap<>();
-        for (Map.Entry<String, Model> entry : reactor.entrySet()) {
-            final String path = entry.getKey();
+    public static Map<File, Model> getChildModels(Map<File, Model> reactor, String groupId, String artifactId) {
+        final Map<File, Model> result = new LinkedHashMap<>();
+        for (Map.Entry<File, Model> entry : reactor.entrySet()) {
+            final File path = entry.getKey();
             final Model model = entry.getValue();
             final Parent parent = model.getParent();
             if (parent != null && groupId.equals(parent.getGroupId()) && artifactId.equals(parent.getArtifactId())) {
@@ -1442,7 +1405,7 @@ public static Map<String, Model> getChildModels(Map<String, Model> reactor, Stri
      * @param artifactId The artifactId to match.
      * @return The model or <code>null</code> if the model was not in the reactor.
      */
-    public static Model getModel(Map<String, Model> reactor, String groupId, String artifactId) {
+    public static Model getModel(Map<File, Model> reactor, String groupId, String artifactId) {
         return reactor.values().stream()
                 .filter(model -> (groupId == null || groupId.equals(getGroupId(model)))
                         && artifactId.equals(getArtifactId(model)))
@@ -1459,8 +1422,7 @@ public static Model getModel(Map<String, Model> reactor, String groupId, String
      * @param artifactId The artifactId to match.
      * @return The model entry or <code>null</code> if the model was not in the reactor.
      */
-    public static Map.Entry<String, Model> getModelEntry(
-            Map<String, Model> reactor, String groupId, String artifactId) {
+    public static Map.Entry<File, Model> getModelEntry(Map<File, Model> reactor, String groupId, String artifactId) {
         return reactor.entrySet().stream()
                 .filter(e -> (groupId == null || groupId.equals(PomHelper.getGroupId(e.getValue())))
                         && artifactId.equals(PomHelper.getArtifactId(e.getValue())))
@@ -1475,7 +1437,7 @@ public static Map.Entry<String, Model> getModelEntry(
      * @param model   The model.
      * @return The number of parents of this model in the reactor.
      */
-    public static int getReactorParentCount(Map<String, Model> reactor, Model model) {
+    public static int getReactorParentCount(Map<File, Model> reactor, Model model) {
         if (model.getParent() == null) {
             return 0;
         }

versions-common/src/main/java/org/codehaus/mojo/versions/api/VersionDetails.java

@@ -405,6 +405,8 @@ ArtifactVersion[] getAllUpdates(Optional<Segment> updateScope, boolean includeSn
      *
      * @param scope most major segment where updates are allowed Optional.empty() for no restriction
      * @return {@linkplain Restriction} object based on the arguments
+     * @throws InvalidSegmentException if the requested segment is outside the bounds (less than 1 or greater than
+     * the segment count)
      */
     Restriction restrictionFor(Optional<Segment> scope) throws InvalidSegmentException;
 

versions-common/src/main/java/org/codehaus/mojo/versions/api/VersionsHelper.java

@@ -217,6 +217,7 @@ Map<Plugin, PluginUpdatesDetails> lookupPluginsUpdates(Set<Plugin> plugins, bool
      * @param plugin The {@link Plugin} instance to look up.
      * @param allowSnapshots Include snapshots in the list of updates.
      * @return The plugin update details.
+     * @throws VersionRetrievalException thrown if version resolution fails
      * @since 1.0-beta-1
      */
     PluginUpdatesDetails lookupPluginUpdates(Plugin plugin, boolean allowSnapshots) throws VersionRetrievalException;

versions-common/src/main/java/org/codehaus/mojo/versions/ordering/ReactorDepthComparator.java

@@ -19,6 +19,7 @@
  * under the License.
  */
 
+import java.io.File;
 import java.util.Comparator;
 import java.util.Map;
 
@@ -31,14 +32,14 @@
  * @author Stephen Connolly
  * @since 15-Sep-2010 14:54:42
  */
-public class ReactorDepthComparator implements Comparator<String> {
-    private final Map<String, Model> reactor;
+public class ReactorDepthComparator implements Comparator<File> {
+    private final Map<File, Model> reactor;
 
-    public ReactorDepthComparator(Map<String, Model> reactor) {
+    public ReactorDepthComparator(Map<File, Model> reactor) {
         this.reactor = reactor;
     }
 
-    public int compare(String o1, String o2) {
+    public int compare(File o1, File o2) {
         final Model m1 = reactor.get(o1);
         final Model m2 = reactor.get(o2);
         final int d1 = PomHelper.getReactorParentCount(reactor, m1);

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

@@ -75,7 +75,9 @@ public static Set<Dependency> extractDependenciesFromPlugins(MavenProject projec
      * @param processDependencyManagementTransitive if {@code true}, the original model will be considered
      *                                              instead of the interpolated model, which does not contain
      *                                              imported dependencies
+     * @param {@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
      */
     public static Set<Dependency> extractDependenciesFromDependencyManagement(

versions-common/src/test/java/org/codehaus/mojo/versions/api/PomHelperTest.java

@@ -25,7 +25,6 @@
 import java.io.File;
 import java.io.StringReader;
 import java.net.URL;
-import java.util.Map;
 
 import org.apache.maven.model.Model;
 import org.apache.maven.model.io.xpp3.MavenXpp3Reader;
@@ -218,7 +217,6 @@ public void testSetProjectValueNewValueNonEmptyParent() throws XMLStreamExceptio
     public void testIssue505ChildModules() throws Exception {
         MavenProject project =
                 mojoRule.readMavenProject(new File("src/test/resources/org/codehaus/mojo/versions/api/issue-505"));
-        Map<String, Model> reactorModels = PomHelper.getReactorModels(project, new SystemStreamLog());
-        assertThat(reactorModels.keySet(), hasSize(3));
+        assertThat(PomHelper.getChildModels(project, new SystemStreamLog()).entrySet(), hasSize(3));
     }
 }

versions-maven-plugin/src/it/it-set-issue-848/invoker.properties

@@ -0,0 +1,2 @@
+invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:set
+invoker.mavenOpts = -DnewVersion=TEST

versions-maven-plugin/src/it/it-set-issue-848/moduleA/moduleA1.xml

@@ -0,0 +1,15 @@
+<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>default-group</groupId>
+        <artifactId>default-artifact</artifactId>
+        <version>1.0-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>moduleA1</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+</project>

versions-maven-plugin/src/it/it-set-issue-848/moduleA/moduleA2.xml

@@ -0,0 +1,15 @@
+<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>default-group</groupId>
+        <artifactId>default-artifact</artifactId>
+        <version>1.0-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>moduleA2</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+</project>

versions-maven-plugin/src/it/it-set-issue-848/moduleB/pom.xml

@@ -0,0 +1,15 @@
+<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>default-group</groupId>
+        <artifactId>default-artifact</artifactId>
+        <version>1.0-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>moduleB</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+</project>

versions-maven-plugin/src/it/it-set-issue-848/pom.xml

@@ -0,0 +1,16 @@
+<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>
+
+    <groupId>default-group</groupId>
+    <artifactId>default-artifact</artifactId>
+    <version>1.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+    <modules>
+        <module>moduleA/moduleA1.xml</module>
+        <module>moduleA/moduleA2.xml</module>
+        <module>moduleB/pom.xml</module>
+    </modules>
+
+</project>

versions-maven-plugin/src/it/it-set-issue-848/verify.groovy

@@ -0,0 +1,4 @@
+assert new File( basedir, "pom.xml" ).text.contains( 'TEST' )
+assert new File( basedir, "moduleA/moduleA1.xml" ).text.contains( 'TEST' )
+assert new File( basedir, "moduleA/moduleA2.xml" ).text.contains( 'TEST' )
+assert new File( basedir, "moduleB/pom.xml" ).text.contains( 'TEST' )

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

@@ -283,8 +283,7 @@ public void execute() throws MojoExecutionException, MojoFailureException {
         if (removeSnapshot && !nextSnapshot) {
             String version = getVersion();
             if (version.endsWith(SNAPSHOT)) {
-                String release = version.substring(0, version.indexOf(SNAPSHOT));
-                newVersion = release;
+                newVersion = version.substring(0, version.indexOf(SNAPSHOT));
                 getLog().info("SNAPSHOT found.  BEFORE " + version + "  --> AFTER: " + newVersion);
             }
         }
@@ -332,8 +331,8 @@ public void execute() throws MojoExecutionException, MojoFailureException {
                     : getProject();
 
             getLog().info("Local aggregation root: " + project.getBasedir());
-            Map<String, Model> reactorModels = PomHelper.getReactorModels(project, getLog());
-            final SortedMap<String, Model> reactor = new TreeMap<>(new ReactorDepthComparator(reactorModels));
+            Map<File, Model> reactorModels = PomHelper.getChildModels(project, getLog());
+            final SortedMap<File, Model> reactor = new TreeMap<>(new ReactorDepthComparator(reactorModels));
             reactor.putAll(reactorModels);
 
             // set of files to update
@@ -371,8 +370,8 @@ public void execute() throws MojoExecutionException, MojoFailureException {
                 reactor.values().parallelStream()
                         .map(m -> PomHelper.getModelEntry(reactor, PomHelper.getGroupId(m), PomHelper.getArtifactId(m)))
                         .filter(Objects::nonNull)
-                        .map(Map.Entry::getKey)
-                        .map(f -> getModuleProjectFile(project, f))
+                        .map(Map.Entry::getValue)
+                        .map(Model::getPomFile)
                         .forEach(files::add);
             }
 
@@ -421,7 +420,7 @@ private static String fixNullOrEmpty(String value, String defaultValue) {
 
     private void applyChange(
             MavenProject project,
-            SortedMap<String, Model> reactor,
+            SortedMap<File, Model> reactor,
             Set<File> files,
             String groupId,
             String artifactId,
@@ -432,14 +431,14 @@ private void applyChange(
         addChange(groupId, artifactId, oldVersion, newVersion);
         // now fake out the triggering change
 
-        Map.Entry<String, Model> current = PomHelper.getModelEntry(reactor, groupId, artifactId);
+        Map.Entry<File, Model> current = PomHelper.getModelEntry(reactor, groupId, artifactId);
         if (current != null) {
             current.getValue().setVersion(newVersion);
-            files.add(getModuleProjectFile(project, current.getKey()));
+            files.add(current.getValue().getPomFile());
         }
 
-        for (Map.Entry<String, Model> sourceEntry : reactor.entrySet()) {
-            final String sourcePath = sourceEntry.getKey();
+        for (Map.Entry<File, Model> sourceEntry : reactor.entrySet()) {
+            final File sourcePath = sourceEntry.getKey();
             final Model sourceModel = sourceEntry.getValue();
 
             getLog().debug(
@@ -463,12 +462,12 @@ private void applyChange(
                 continue;
             }
 
-            files.add(getModuleProjectFile(project, sourcePath));
+            files.add(sourceModel.getPomFile());
 
             getLog().debug("Looking for modules which use "
                     + ArtifactUtils.versionlessKey(sourceGroupId, sourceArtifactId) + " as their parent");
 
-            for (Map.Entry<String, Model> stringModelEntry : processAllModules
+            for (Map.Entry<File, Model> stringModelEntry : processAllModules
                     ? reactor.entrySet()
                     : PomHelper.getChildModels(reactor, sourceGroupId, sourceArtifactId)
                             .entrySet()) {
@@ -514,20 +513,6 @@ private void applyChange(
         }
     }
 
-    private static File getModuleProjectFile(MavenProject project, String relativePath) {
-        final File moduleDir = new File(project.getBasedir(), relativePath);
-        final File projectBaseDir = project.getBasedir();
-
-        if (projectBaseDir.equals(moduleDir)) {
-            return project.getFile();
-        } else if (moduleDir.isDirectory()) {
-            return new File(moduleDir, "pom.xml");
-        }
-        // i don't think this should ever happen... but just in case
-        // the module references the file-name
-        return moduleDir;
-    }
-
     /**
      * Updates the pom file.
      *