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,50 +1334,41 @@ 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, 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 project The project to build from.
+     * @param model   The root model
      * @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);
+    private static Map<File, Model> getChildModels(Model model, Log logger) throws IOException {
+        Map<File, Model> result = new LinkedHashMap<>();
+        Map<File, Model> childResults = new LinkedHashMap<>();
 
-        File baseDir = path.length() > 0 ? new File(project.getBasedir(), path) : project.getBasedir();
-        removeMissingChildModules(logger, baseDir, childModules);
+        File baseDir = model.getPomFile().getParentFile();
 
-        childModules.stream()
+        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, logger));
                     } catch (IOException e) {
-                        logger.debug("Could not parse " + pomFile.getPath(), e);
+                        logger.error("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/docs/moduleC/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>
+
+    <parent>
+        <groupId>default-group</groupId>
+        <artifactId>default-artifact</artifactId>
+        <version>1.0-SNAPSHOT</version>
+        <relativePath>../..</relativePath>
+    </parent>
+
+    <artifactId>moduleC</artifactId>
+    <version>1.0.0-SNAPSHOT</version>
+    <packaging>pom</packaging>
+
+</project>

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,17 @@
+<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>
+        <module>docs/moduleC</module>
+    </modules>
+
+</project>

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

@@ -0,0 +1,5 @@
+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' )
+assert new File( basedir, "docs/moduleC/pom.xml" ).text.contains( 'TEST' )