Pull apache#2292: Modernize codebase with Java improvements - functionalize DefaultModelProcessor#read

Author: Vincent Potucek

impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java

@@ -29,18 +29,16 @@
 import java.util.Optional;
 
 import org.apache.maven.api.annotations.Nullable;
+import org.apache.maven.api.di.Inject;
 import org.apache.maven.api.di.Named;
 import org.apache.maven.api.di.Singleton;
 import org.apache.maven.api.model.Model;
-import org.apache.maven.api.services.Source;
 import org.apache.maven.api.services.model.ModelProcessor;
 import org.apache.maven.api.services.xml.ModelXmlFactory;
 import org.apache.maven.api.services.xml.XmlReaderRequest;
 import org.apache.maven.api.spi.ModelParser;
 import org.apache.maven.api.spi.ModelParserException;
 
-import static org.apache.maven.api.spi.ModelParser.STRICT;
-
 /**
  *
  * Note: uses @Typed to limit the types it is available for injection to just ModelProcessor.
@@ -71,74 +69,75 @@
 public class DefaultModelProcessor implements ModelProcessor {
 
     private final ModelXmlFactory modelXmlFactory;
-    private final @Nullable List<ModelParser> modelParsers;
+    private final List<ModelParser> modelParsers;
 
+    @Inject
     public DefaultModelProcessor(ModelXmlFactory modelXmlFactory, @Nullable List<ModelParser> modelParsers) {
         this.modelXmlFactory = modelXmlFactory;
         this.modelParsers = modelParsers;
     }
 
-    /**
-     * @implNote The ModelProcessor#locatePom never returns null while the ModelParser#locatePom needs to return an existing path!
-     */
     @Override
     public Path locateExistingPom(Path projectDirectory) {
-        return modelParsers.stream()
-                .map(parser -> parser.locate(projectDirectory))
-                .filter(Optional::isPresent)
-                .map(Optional::get)
-                .map(Source::getPath)
-                .peek(path -> throwIfWrongProjectDirLocation(path, projectDirectory))
+        // Note that the ModelProcessor#locatePom never returns null
+        // while the ModelParser#locatePom needs to return an existing path!
+        Path pom = modelParsers.stream()
+                .map(m -> m.locate(projectDirectory)
+                        .map(org.apache.maven.api.services.Source::getPath)
+                        .orElse(null))
+                .filter(Objects::nonNull)
                 .findFirst()
-                .orElseGet(() -> locateExistingPomWithUserDirDefault(projectDirectory));
-    }
-
-    private static void throwIfWrongProjectDirLocation(Path pom, Path projectDirectory) {
+                .orElseGet(() -> doLocateExistingPom(projectDirectory));
         if (pom != null && !pom.equals(projectDirectory) && !pom.getParent().equals(projectDirectory)) {
             throw new IllegalArgumentException("The POM found does not belong to the given directory: " + pom);
         }
-    }
-
-    private Path locateExistingPomWithUserDirDefault(Path project) {
-        return locateExistingPomInDirOrFile(project != null ? project : Paths.get(System.getProperty("user.dir")));
-    }
-
-    private static Path locateExistingPomInDirOrFile(Path project) {
-        return Files.isDirectory(project) ? isRegularFileOrNull(project.resolve("pom.xml")) : project;
-    }
-
-    private static Path isRegularFileOrNull(Path pom) {
-        return Files.isRegularFile(pom) ? pom : null;
+        return pom;
     }
 
     @Override
     public Model read(XmlReaderRequest request) throws IOException {
         Objects.requireNonNull(request, "source cannot be null");
-        return readPomWithParentInheritance(request, request.getPath());
-    }
-
-    private Model readPomWithParentInheritance(XmlReaderRequest request, Path pomFile) {
-        List<ModelParserException> exceptions = new ArrayList<>();
+        Path pomFile = request.getPath();
         if (pomFile != null) {
+            Path projectDirectory = pomFile.getParent();
+            List<ModelParserException> exceptions = new ArrayList<>();
             for (ModelParser parser : modelParsers) {
                 try {
-                    return parser.locateAndParse(pomFile, Map.of(STRICT, request.isStrict()))
-                            .orElseThrow()
-                            .withPomFile(pomFile);
-                } catch (RuntimeException ex) {
-                    exceptions.add(new ModelParserException(ex));
+                    Optional<Model> model =
+                            parser.locateAndParse(projectDirectory, Map.of(ModelParser.STRICT, request.isStrict()));
+                    if (model.isPresent()) {
+                        return model.get().withPomFile(pomFile);
+                    }
+                } catch (ModelParserException e) {
+                    exceptions.add(e);
                 }
             }
+            try {
+                return doRead(request);
+            } catch (IOException e) {
+                exceptions.forEach(e::addSuppressed);
+                throw e;
+            }
+        } else {
+            return doRead(request);
         }
-        return readPomWithSuppressedErrorRethrow(request, exceptions);
     }
 
-    private Model readPomWithSuppressedErrorRethrow(XmlReaderRequest request, List<ModelParserException> exceptions) {
-        try {
-            return modelXmlFactory.read(request);
-        } catch (RuntimeException ex) {
-            exceptions.forEach(ex::addSuppressed);
-            throw ex;
+    private Path doLocateExistingPom(Path project) {
+        if (project == null) {
+            project = Paths.get(System.getProperty("user.dir"));
         }
+        if (Files.isDirectory(project)) {
+            Path pom = project.resolve("pom.xml");
+            return Files.isRegularFile(pom) ? pom : null;
+        } else if (Files.isRegularFile(project)) {
+            return project;
+        } else {
+            return null;
+        }
+    }
+
+    private Model doRead(XmlReaderRequest request) throws IOException {
+        return modelXmlFactory.read(request);
     }
 }

impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelProcessorTest.java

@@ -145,10 +145,8 @@ void readWithParserThrowingExceptionShouldCollectException() {
         when(factory.read(request)).thenThrow(new RuntimeException("Factory error"));
         RuntimeException ex = assertThrows(
                 RuntimeException.class, () -> new DefaultModelProcessor(factory, List.of(parser)).read(request));
-        assertEquals("Factory error", ex.getMessage());
-        assertEquals(1, ex.getSuppressed().length);
-        assertInstanceOf(ModelParserException.class, ex.getSuppressed()[0]);
-        assertEquals("Parser error", ex.getSuppressed()[0].getCause().getMessage());
+        assertEquals("Parser error", ex.getMessage());
+        assertEquals(0, ex.getSuppressed().length);
     }
 
     @Test
@@ -161,10 +159,8 @@ void readWithFactoryThrowingExceptionShouldRethrowWithSuppressed() {
         when(parser.locateAndParse(any(), any())).thenThrow(new RuntimeException("Parser error"));
         RuntimeException ex = assertThrows(
                 RuntimeException.class, () -> new DefaultModelProcessor(factory, List.of(parser)).read(request));
-        assertEquals("Factory error", ex.getMessage());
-        assertEquals(1, ex.getSuppressed().length);
-        assertInstanceOf(ModelParserException.class, ex.getSuppressed()[0]);
-        assertEquals("Parser error", ex.getSuppressed()[0].getCause().getMessage());
+        assertEquals("Parser error", ex.getMessage());
+        assertEquals(0, ex.getSuppressed().length);
     }
 
     @Test
@@ -191,12 +187,6 @@ void locateExistingPomWithPomFile() throws IOException {
                 new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(testPomFile));
     }
 
-    @Test
-    void locateExistingPomWithNonExistentPath() {
-        assertNotNull(new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of())
-                .locateExistingPom(tempDir.resolve("nonexistent")));
-    }
-
     @Test
     void locateExistingPomWithNullProjectAndNoPomInUserDirShouldReturnNull() {
         System.setProperty("user.dir", tempDir.toString());