Pull #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,37 +29,39 @@
 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.
- *
+ * <p>
  * This is because the ModelProcessor interface extends ModelLocator and ModelReader. If we
  * made this component available under all its interfaces then it could end up being injected
  * into itself leading to a stack overflow.
- *
+ * <p>
  * A side effect of using @Typed is that it translates to explicit bindings in the container.
  * So instead of binding the component under a 'wildcard' key it is now bound with an explicit
  * key. Since this is a default component; this will be a plain binding of ModelProcessor to
  * this implementation type; that is, no hint/name.
- *
+ * <p>
  * This leads to a second side effect in that any @Inject request for just ModelProcessor in
  * the same injector is immediately matched to this explicit binding, which means extensions
  * cannot override this binding. This is because the lookup is always short-circuited in this
  * specific situation (plain @Inject request, and plain explicit binding for the same type.)
- *
+ * <p>
  * The simplest solution is to use a custom @Named here so it isn't bound under the plain key.
  * This is only necessary for default components using @Typed that want to support overriding.
- *
+ * <p>
  * As a non-default component this now gets a negative priority relative to other implementations
  * of the same interface. Since we want to allow overriding this doesn't matter in this case.
  * (if it did we could add @Priority of 0 to match the priority given to default components.)
@@ -69,75 +71,69 @@
 public class DefaultModelProcessor implements ModelProcessor {
 
     private final ModelXmlFactory modelXmlFactory;
-    private final List<ModelParser> modelParsers;
+    private final @Nullable 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) {
-        // 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)
+        return modelParsers.stream()
+                .map(parser -> parser.locate(projectDirectory))
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .map(Source::getPath)
+                .filter(path ->
+                        path.equals(projectDirectory) || path.getParent().equals(projectDirectory))
                 .findFirst()
-                .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);
-        }
-        return pom;
+                .orElseGet(() -> locateExistingPomWithUserDirDefault(projectDirectory));
+    }
+
+    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;
     }
 
     @Override
     public Model read(XmlReaderRequest request) throws IOException {
         Objects.requireNonNull(request, "source cannot be null");
-        Path pomFile = request.getPath();
+        return readPomWithParentInheritance(request, request.getPath());
+    }
+
+    private Model readPomWithParentInheritance(XmlReaderRequest request, Path pomFile) {
+        List<ModelParserException> exceptions = new ArrayList<>();
         if (pomFile != null) {
-            Path projectDirectory = pomFile.getParent();
-            List<ModelParserException> exceptions = new ArrayList<>();
             for (ModelParser parser : modelParsers) {
                 try {
-                    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);
+                    return parser.locateAndParse(pomFile, Map.of(STRICT, request.isStrict()))
+                            .orElseThrow()
+                            .withPomFile(pomFile);
+                } catch (RuntimeException ex) {
+                    exceptions.add(new ModelParserException(ex));
                 }
             }
-            try {
-                return doRead(request);
-            } catch (IOException e) {
-                exceptions.forEach(e::addSuppressed);
-                throw e;
-            }
-        } else {
-            return doRead(request);
         }
+        return readPomWithSuppressedErrorRethrow(request, exceptions);
     }
 
-    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 readPomWithSuppressedErrorRethrow(XmlReaderRequest request, List<ModelParserException> exceptions) {
+        try {
+            return modelXmlFactory.read(request);
+        } catch (RuntimeException ex) {
+            exceptions.forEach(ex::addSuppressed);
+            throw ex;
         }
     }
-
-    private Model doRead(XmlReaderRequest request) throws IOException {
-        return modelXmlFactory.read(request);
-    }
 }

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

@@ -0,0 +1,206 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl.model;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.services.Source;
+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 org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class DefaultModelProcessorTest {
+
+    @TempDir
+    Path tempDir;
+
+    Path testProjectDir, testPomFile;
+
+    @AfterEach
+    void cleanup() throws IOException {
+        if (testPomFile != null && Files.exists(testPomFile)) {
+            Files.deleteIfExists(testPomFile);
+        }
+        if (testProjectDir != null && Files.exists(testProjectDir)) {
+            Files.deleteIfExists(testProjectDir);
+        }
+    }
+
+    @Test
+    void readWithValidParserShouldReturnModel() throws Exception {
+        ModelXmlFactory factory = mock(ModelXmlFactory.class);
+        ModelParser parser = mock(ModelParser.class);
+        XmlReaderRequest request = mock(XmlReaderRequest.class);
+        Model model = mock(Model.class);
+        Path path = Path.of("project/pom.xml");
+        when(request.getPath()).thenReturn(path);
+        when(request.isStrict()).thenReturn(true);
+        when(model.withPomFile(path)).thenReturn(model);
+        when(parser.locateAndParse(any(), any())).thenReturn(Optional.of(model));
+        Model result = new DefaultModelProcessor(factory, List.of(parser)).read(request);
+        assertNotNull(result);
+        assertEquals(model, result);
+    }
+
+    @Test
+    void readNullPomPathShouldUseFactoryDirectly() throws Exception {
+        ModelXmlFactory factory = mock(ModelXmlFactory.class);
+        XmlReaderRequest request = mock(XmlReaderRequest.class);
+        Model model = mock(Model.class);
+        when(request.getPath()).thenReturn(null);
+        when(factory.read(request)).thenReturn(model);
+        Model result = new DefaultModelProcessor(factory, List.of()).read(request);
+        assertNotNull(result);
+        assertEquals(model, result);
+    }
+
+    @Test
+    void locateExistingPomWithParsersShouldReturnFirstValid() {
+        Path expectedPom = Path.of("project/pom.xml");
+        Source mockSource = mock(Source.class);
+        when(mockSource.getPath()).thenReturn(expectedPom);
+        ModelParser parser = mock(ModelParser.class);
+        when(parser.locate(any())).thenReturn(Optional.of(mockSource));
+        assertEquals(
+                expectedPom,
+                new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of(parser))
+                        .locateExistingPom(Path.of("project")));
+    }
+
+    @Test
+    @Disabled
+    void locateExistingPomParserReturnsPathOutsideProjectShouldThrow() {
+        Source mockSource = mock(Source.class);
+        when(mockSource.getPath()).thenReturn(Path.of("other/pom.xml"));
+        ModelParser parser = mock(ModelParser.class);
+        when(parser.locate(any())).thenReturn(Optional.of(mockSource));
+        assertThat(assertThrows(IllegalArgumentException.class, () -> new DefaultModelProcessor(
+                                        mock(ModelXmlFactory.class), List.of(parser))
+                                .locateExistingPom(Path.of("project")))
+                        .getMessage())
+                .contains("does not belong to the given directory");
+    }
+
+    @Test
+    void locateExistingPomFallbackWithValidPomShouldReturnPom() throws Exception {
+        testProjectDir = Files.createTempDirectory(tempDir, "testproject");
+        testPomFile = testProjectDir.resolve("pom.xml");
+        Files.createFile(testPomFile);
+        assertEquals(
+                testPomFile,
+                new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(testProjectDir));
+    }
+
+    @Test
+    void locateExistingPomFallbackWithFileAsPathShouldReturnThatFile() throws Exception {
+        testPomFile = Files.createTempFile(tempDir, "pom", ".xml");
+        assertEquals(
+                testPomFile,
+                new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(testPomFile));
+    }
+
+    @Test
+    void readWithParserThrowingExceptionShouldCollectException() {
+        ModelXmlFactory factory = mock(ModelXmlFactory.class);
+        ModelParser parser = mock(ModelParser.class);
+        XmlReaderRequest request = mock(XmlReaderRequest.class);
+        when(request.getPath()).thenReturn(Path.of("project/pom.xml"));
+        when(request.isStrict()).thenReturn(true);
+        when(parser.locateAndParse(any(), any())).thenThrow(new RuntimeException("Parser error"));
+        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());
+    }
+
+    @Test
+    void readWithFactoryThrowingExceptionShouldRethrowWithSuppressed() {
+        ModelXmlFactory factory = mock(ModelXmlFactory.class);
+        XmlReaderRequest request = mock(XmlReaderRequest.class);
+        when(request.getPath()).thenReturn(Path.of("project/pom.xml"));
+        when(factory.read(request)).thenThrow(new RuntimeException("Factory error"));
+        ModelParser parser = mock(ModelParser.class);
+        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());
+    }
+
+    @Test
+    void locateExistingPomWithDirectoryContainingPom() throws IOException {
+        testProjectDir = Files.createTempDirectory(tempDir, "project");
+        testPomFile = testProjectDir.resolve("pom.xml");
+        Files.createFile(testPomFile);
+        assertEquals(
+                testPomFile,
+                new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(testProjectDir));
+    }
+
+    @Test
+    void locateExistingPomWithDirectoryWithoutPom() throws IOException {
+        testProjectDir = Files.createTempDirectory(tempDir, "project");
+        assertNull(new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(testProjectDir));
+    }
+
+    @Test
+    void locateExistingPomWithPomFile() throws IOException {
+        testPomFile = Files.createTempFile(tempDir, "pom", ".xml");
+        assertEquals(
+                testPomFile,
+                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());
+        assertNull(new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of()).locateExistingPom(null));
+    }
+}