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

@@ -42,24 +42,24 @@
 /**
  *
  * 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.)
@@ -77,10 +77,12 @@ public DefaultModelProcessor(ModelXmlFactory modelXmlFactory, @Nullable List<Mod
         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)
@@ -97,32 +99,32 @@ public Path locateExistingPom(Path projectDirectory) {
     @Override
     public Model read(XmlReaderRequest request) throws IOException {
         Objects.requireNonNull(request, "source cannot be null");
-        Path pomFile = request.getPath();
+        return readOnSelfOrParent(request, request.getPath());
+    }
+
+    private Model readOnSelfOrParent(XmlReaderRequest request, Path pomFile) {
         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()));
+                    Optional<Model> model = locateAndParseParent(request, pomFile, parser);
                     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;
-            }
+            return readFromFactory(request);
         } else {
-            return doRead(request);
+            return readFromFactory(request);
         }
     }
 
+    private static Optional<Model> locateAndParseParent(XmlReaderRequest request, Path pomFile, ModelParser parser) {
+        return parser.locateAndParse(pomFile.getParent(), Map.of(ModelParser.STRICT, request.isStrict()));
+    }
+
     private Path doLocateExistingPom(Path project) {
         if (project == null) {
             project = Paths.get(System.getProperty("user.dir"));
@@ -137,7 +139,7 @@ private Path doLocateExistingPom(Path project) {
         }
     }
 
-    private Model doRead(XmlReaderRequest request) throws IOException {
+    private Model readFromFactory(XmlReaderRequest request) {
         return modelXmlFactory.read(request);
     }
 }

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

@@ -0,0 +1,139 @@
+package org.apache.maven.impl.model;
+
+import org.apache.maven.api.model.Model;
+import org.apache.maven.api.services.Source;
+import org.apache.maven.api.services.xml.Location;
+import org.apache.maven.api.services.xml.ModelXmlFactory;
+import org.apache.maven.api.services.xml.XmlReaderException;
+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.Test;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.*;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.Mockito.*;
+
+class DefaultModelProcessorTest {
+
+    @Test
+    void read_withValidParser_shouldReturnModel() 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));
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(factory, List.of(parser));
+        Model result = processor.read(request);
+
+        assertNotNull(result);
+        assertEquals(model, result);
+    }
+
+    @Test
+    void read_allParsersFail_shouldFallbackToFactory() throws Exception {
+        ModelXmlFactory factory = mock(ModelXmlFactory.class);
+        ModelParser parser = mock(ModelParser.class);
+        XmlReaderRequest request = mock(XmlReaderRequest.class);
+        Model fallbackModel = mock(Model.class);
+        Path path = Path.of("project/pom.xml");
+
+        when(request.getPath()).thenReturn(path);
+        when(request.isStrict()).thenReturn(false);
+        when(parser.locateAndParse(any(), any()))
+                .thenThrow(new ModelParserException("parse fail"));
+
+        when(factory.read(request)).thenReturn(fallbackModel);
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(factory, List.of(parser));
+        Model result = processor.read(request);
+
+        assertNotNull(result);
+        assertEquals(fallbackModel, result);
+    }
+
+    @Test
+    void read_nullPomPath_shouldUseFactoryDirectly() 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);
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(factory, List.of());
+        Model result = processor.read(request);
+
+        assertNotNull(result);
+        assertEquals(model, result);
+    }
+
+    @Test
+    void locateExistingPom_withParsers_shouldReturnFirstValid() {
+        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));
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of(parser));
+        Path result = processor.locateExistingPom(Path.of("project"));
+
+        assertEquals(expectedPom, result);
+    }
+
+    @Test
+    void locateExistingPom_parserReturnsPathOutsideProject_shouldThrow() {
+        Path unexpectedPom = Path.of("other/pom.xml");
+        Source mockSource = mock(Source.class);
+        when(mockSource.getPath()).thenReturn(unexpectedPom);
+
+        ModelParser parser = mock(ModelParser.class);
+        when(parser.locate(any())).thenReturn(Optional.of(mockSource));
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of(parser));
+        IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () ->
+                processor.locateExistingPom(Path.of("project")));
+
+        assertTrue(ex.getMessage().contains("does not belong to the given directory"));
+    }
+
+    @Test
+    void locateExistingPom_fallbackWithValidPom_shouldReturnPom() throws Exception {
+        Path projectDir = Files.createTempDirectory("testproject");
+        Path pomFile = projectDir.resolve("pom.xml");
+        Files.createFile(pomFile);
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of());
+        Path result = processor.locateExistingPom(projectDir);
+
+        assertEquals(pomFile, result);
+
+        // Clean up
+        Files.deleteIfExists(pomFile);
+        Files.deleteIfExists(projectDir);
+    }
+
+    @Test
+    void locateExistingPom_fallbackWithFileAsPath_shouldReturnThatFile() throws Exception {
+        Path tempPom = Files.createTempFile("pom", ".xml");
+
+        DefaultModelProcessor processor = new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of());
+        Path result = processor.locateExistingPom(tempPom);
+
+        assertEquals(tempPom, result);
+
+        Files.deleteIfExists(tempPom);
+    }
+}