Give SPOT for DefaultPluginXmlFactory leveraging OOP

Author: Vincent Potucek

api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlReaderRequest.java

@@ -78,6 +78,13 @@ interface Transformer {
         String transform(String source, String fieldName);
     }
 
+    default XmlReaderRequest assertReadable() {
+        if (getInputStream() == null && getReader() == null && getPath() == null && getURL() == null) {
+            throw new IllegalArgumentException("writer, outputStream or path must be non null");
+        }
+        return this;
+    }
+
     @Nonnull
     static XmlReaderRequestBuilder builder() {
         return new XmlReaderRequestBuilder();

api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlWriterRequest.java

@@ -55,6 +55,13 @@ static <T> XmlWriterRequestBuilder<T> builder() {
         return new XmlWriterRequestBuilder<>();
     }
 
+    default XmlWriterRequest<T> assertWritable() {
+        if (getWriter() == null && getOutputStream() == null && getPath() == null) {
+            throw new IllegalArgumentException("writer, outputStream or path must be non null");
+        }
+        return this;
+    }
+
     class XmlWriterRequestBuilder<T> {
         Path path;
         OutputStream outputStream;

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java

@@ -18,13 +18,10 @@
  */
 package org.apache.maven.impl;
 
-import java.io.InputStream;
-import java.io.OutputStream;
-import java.io.Reader;
-import java.io.Writer;
-import java.net.URL;
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
 import java.nio.file.Files;
-import java.nio.file.Path;
 
 import org.apache.maven.api.annotations.Nonnull;
 import org.apache.maven.api.di.Named;
@@ -38,65 +35,61 @@
 import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
 import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
 
-import static org.apache.maven.impl.ImplUtils.nonNull;
 import static org.apache.maven.impl.StaxLocation.getLocation;
 import static org.apache.maven.impl.StaxLocation.getMessage;
 
 @Named
 @Singleton
 public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
     @Override
     public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
-        nonNull(request, "request");
-        Path path = request.getPath();
-        URL url = request.getURL();
-        Reader reader = request.getReader();
-        InputStream inputStream = request.getInputStream();
-        if (path == null && url == null && reader == null && inputStream == null) {
-            throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
-        }
+        return read(request.assertReadable(), setAddDefaultEntities(request, new PluginDescriptorStaxReader()));
+    }
+
+    @Override
+    public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
+        write(request.assertWritable(), new PluginDescriptorStaxWriter());
+    }
+
+    private static PluginDescriptor read(XmlReaderRequest request, PluginDescriptorStaxReader reader) {
         try {
-            PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
-            xml.setAddDefaultEntities(request.isAddDefaultEntities());
-            if (inputStream != null) {
-                return xml.read(inputStream, request.isStrict());
-            } else if (reader != null) {
-                return xml.read(reader, request.isStrict());
-            } else if (path != null) {
-                try (InputStream is = Files.newInputStream(path)) {
-                    return xml.read(is, request.isStrict());
+            if (request.getInputStream() != null) {
+                return reader.read(request.getInputStream(), request.isStrict());
+            } else if (request.getReader() != null) {
+                return reader.read(request.getReader(), request.isStrict());
+            } else if (request.getPath() != null) {
+                try (var is = Files.newInputStream(request.getPath())) {
+                    return reader.read(is, request.isStrict());
                 }
             } else {
-                try (InputStream is = url.openStream()) {
-                    return xml.read(is, request.isStrict());
+                try (var is = request.getURL().openStream()) {
+                    return reader.read(is, request.isStrict());
                 }
             }
-        } catch (Exception e) {
+        } catch (IOException | XMLStreamException e) {
             throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
         }
     }
 
-    @Override
-    public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
-        nonNull(request, "request");
-        PluginDescriptor content = nonNull(request.getContent(), "content");
-        Path path = request.getPath();
-        OutputStream outputStream = request.getOutputStream();
-        Writer writer = request.getWriter();
-        if (writer == null && outputStream == null && path == null) {
-            throw new IllegalArgumentException("writer, outputStream or path must be non null");
-        }
+    private static PluginDescriptorStaxReader setAddDefaultEntities(
+            XmlReaderRequest request, PluginDescriptorStaxReader reader) {
+        reader.setAddDefaultEntities(request.isAddDefaultEntities());
+        return reader;
+    }
+
+    private static void write(XmlWriterRequest<PluginDescriptor> request, PluginDescriptorStaxWriter writer) {
         try {
-            if (writer != null) {
-                new PluginDescriptorStaxWriter().write(writer, content);
-            } else if (outputStream != null) {
-                new PluginDescriptorStaxWriter().write(outputStream, content);
+            if (request.getWriter() != null) {
+                writer.write(request.getWriter(), request.getContent());
+            } else if (request.getOutputStream() != null) {
+                writer.write(request.getOutputStream(), request.getContent());
             } else {
-                try (OutputStream os = Files.newOutputStream(path)) {
-                    new PluginDescriptorStaxWriter().write(os, content);
+                try (var os = Files.newOutputStream(request.getPath())) {
+                    writer.write(os, request.getContent());
                 }
             }
-        } catch (Exception e) {
+        } catch (XmlWriterException | XMLStreamException | IOException e) {
             throw new XmlWriterException("Unable to write plugin: " + getMessage(e), getLocation(e), e);
         }
     }

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultPluginXmlFactoryTest.java

@@ -46,6 +46,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 class DefaultPluginXmlFactoryReadWriteTest {
@@ -73,9 +74,9 @@ void readFromInputStreamParsesPluginDescriptorCorrectly() {
         PluginDescriptor descriptor = defaultPluginXmlFactory.read(XmlReaderRequest.builder()
                 .inputStream(new ByteArrayInputStream(SAMPLE_PLUGIN_XML.getBytes()))
                 .build());
-        assertThat(descriptor.getName()).isEqualTo(NAME);
-        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
         assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+        assertThat(descriptor.getName()).isEqualTo(NAME);
         assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
     }
 
@@ -160,18 +161,18 @@ void toXmlStringGeneratesValidXml() {
                 .artifactId("sample-plugin")
                 .version("1.0.0")
                 .build());
-        assertThat(xml).contains("<name>" + NAME + "</name>");
-        assertThat(xml).contains("<groupId>org.example</groupId>");
         assertThat(xml).contains("<artifactId>sample-plugin</artifactId>");
+        assertThat(xml).contains("<groupId>org.example</groupId>");
+        assertThat(xml).contains("<name>" + NAME + "</name>");
         assertThat(xml).contains("<version>1.0.0</version>");
     }
 
     @Test
     void staticFromXmlParsesValidXml() {
         PluginDescriptor descriptor = DefaultPluginXmlFactory.fromXml(SAMPLE_PLUGIN_XML);
-        assertThat(descriptor.getName()).isEqualTo(NAME);
-        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
         assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+        assertThat(descriptor.getName()).isEqualTo(NAME);
         assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
     }
 
@@ -184,22 +185,21 @@ void staticToXmlGeneratesValidXml() {
                 .version("1.0.0")
                 .build());
         assertThat(xml).contains("<name>" + NAME + "</name>");
-        assertThat(xml).contains("<name>" + NAME + "</name>");
         assertThat(xml).contains("<groupId>org.example</groupId>");
         assertThat(xml).contains("<artifactId>sample-plugin</artifactId>");
         assertThat(xml).contains("<version>1.0.0</version>");
     }
 
     @Test
     void writeWithFailingWriterThrowsXmlWriterException() {
-        String unableToWritePlugin = "Unable to write plugin" + randomUUID();
-        String ioEx = "ioEx" + randomUUID();
+        String unableToWritePlugin = "Unable to write plugin: " + NAME;
+        String ioEx = "ioEx: " + NAME;
         XmlWriterException exception = assertThatExceptionOfType(XmlWriterException.class)
                 .isThrownBy(() -> defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
                         .writer(new Writer() {
                             @Override
                             public void write(char[] cbuf, int off, int len) {
-                                throw new XmlWriterException(unableToWritePlugin, null, new IOException(ioEx));
+                                throw new XmlWriterException(NAME, null, new IOException(ioEx));
                             }
 
                             @Override
@@ -213,10 +213,10 @@ public void close() {}
                                 .build())
                         .build()))
                 .actual();
-        assertThat(exception.getMessage()).contains(unableToWritePlugin);
         assertThat(exception.getCause()).isInstanceOf(XmlWriterException.class);
         assertThat(exception.getCause().getCause().getMessage()).isEqualTo(ioEx);
-        assertThat(exception.getCause().getMessage()).isEqualTo(unableToWritePlugin);
+        assertThat(exception.getCause().getMessage()).isEqualTo(NAME);
+        assertThat(exception.getMessage()).isEqualTo(unableToWritePlugin);
     }
 
     @Test
@@ -239,8 +239,8 @@ void readMalformedXmlThrowsXmlReaderException() {
                         .inputStream(new ByteArrayInputStream("<plugin><name>Broken Plugin".getBytes()))
                         .build()))
                 .actual();
-        assertThat(exception.getMessage()).contains("Unable to read plugin");
         assertThat(exception.getCause()).isInstanceOf(WstxEOFException.class);
+        assertThat(exception.getMessage()).contains("Unable to read plugin");
     }
 
     @Test
@@ -258,19 +258,19 @@ void readFromUrlParsesPluginDescriptorCorrectly() throws Exception {
         PluginDescriptor descriptor = defaultPluginXmlFactory.read(XmlReaderRequest.builder()
                 .inputStream(xmlFile.toUri().toURL().openStream())
                 .build());
-        assertThat(descriptor.getName()).isEqualTo(NAME);
-        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
         assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+        assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+        assertThat(descriptor.getName()).isEqualTo(NAME);
         assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
     }
 
     @Test
     void testReadWithPath() throws Exception {
         Path tempPath = Files.createTempFile("plugin", ".xml");
         Files.writeString(tempPath, "<plugin/>");
-        XmlReaderRequest request = mock(XmlReaderRequest.class);
-        when(request.getPath()).thenReturn(tempPath);
+        XmlReaderRequest request = spy(XmlReaderRequest.class);
         when(request.getInputStream()).thenReturn(null);
+        when(request.getPath()).thenReturn(tempPath);
         when(request.getReader()).thenReturn(null);
         when(request.getURL()).thenReturn(null);
         when(request.isAddDefaultEntities()).thenReturn(false);
@@ -283,9 +283,9 @@ void testReadWithPath() throws Exception {
     void testReadWithPathUrlDefault() throws Exception {
         Path tempPath = Files.createTempFile("plugin", ".xml");
         Files.writeString(tempPath, "<plugin/>");
-        XmlReaderRequest request = mock(XmlReaderRequest.class);
-        when(request.getPath()).thenReturn(null);
+        XmlReaderRequest request = spy(XmlReaderRequest.class);
         when(request.getInputStream()).thenReturn(null);
+        when(request.getPath()).thenReturn(null);
         when(request.getReader()).thenReturn(null);
         when(request.getURL()).thenReturn(tempPath.toUri().toURL());
         when(request.isAddDefaultEntities()).thenReturn(false);