Another round of code review

Author: rmaucher

src/main/java/org/apache/tomcat/maven/common/deployer/TomcatManager.java

@@ -183,15 +183,6 @@ public String getUserName() {
         return username;
     }
 
-    /**
-     * Gets the password to use when authenticating with Tomcat manager.
-     *
-     * @return the password to use when authenticating with Tomcat manager
-     */
-    public String getPassword() {
-        return password;
-    }
-
     /**
      * Gets the URL encoding charset to use when communicating with Tomcat manager.
      *
@@ -288,7 +279,7 @@ public TomcatManagerResponse deploy(String path, URL war, boolean update, String
      * Deploys the specified WAR as a HTTP PUT to the specified context path.
      *
      * @param path the webapp context path to deploy to
-     * @param war  an input stream to the WAR to deploy
+     * @param war  the WAR file to deploy
      * 
      * @return the Tomcat manager response
      * 
@@ -304,7 +295,7 @@ public TomcatManagerResponse deploy(String path, File war) throws TomcatManagerE
      * already exists.
      *
      * @param path   the webapp context path to deploy to
-     * @param war    an input stream to the WAR to deploy
+     * @param war  the WAR file to deploy
      * @param update whether to first undeploy the webapp if it already exists
      * 
      * @return the Tomcat manager response
@@ -322,7 +313,7 @@ public TomcatManagerResponse deploy(String path, File war, boolean update)
      * already exists and using the specified tag name.
      *
      * @param path   the webapp context path to deploy to
-     * @param war    an input stream to the WAR to deploy
+     * @param war  the WAR file to deploy
      * @param update whether to first undeploy the webapp if it already exists
      * @param tag    the tag name to use
      * 
@@ -341,7 +332,7 @@ public TomcatManagerResponse deploy(String path, File war, boolean update, Strin
      * already exists and using the specified tag name.
      *
      * @param path   the webapp context path to deploy to
-     * @param war    an input stream to the WAR to deploy
+     * @param war  the WAR file to deploy
      * @param update whether to first undeploy the webapp if it already exists
      * @param tag    the tag name to use
      * @param length the size of the war deployed
@@ -677,7 +668,11 @@ private TomcatManagerResponse deployImpl(String path, URL config, URL war, File
      */
     protected TomcatManagerResponse invoke(String path, File data, long length)
             throws TomcatManagerException, IOException {
-        HttpURLConnection connection = openConnection(url + path);
+        String urlString = url.toString();
+        if (urlString.endsWith("/") && path.startsWith("/")) {
+            urlString = urlString.substring(0, urlString.length() - 1);
+        }
+        HttpURLConnection connection = openConnection(urlString + path);
 
         if (data == null) {
             connection.setRequestMethod("GET");
@@ -719,8 +714,6 @@ protected TomcatManagerResponse invoke(String path, File data, long length)
                     }
                 }
                 transferSucceeded(completed, startTime);
-            } finally {
-                System.out.println();
             }
         }
 
@@ -732,8 +725,13 @@ protected TomcatManagerResponse invoke(String path, File data, long length)
             } catch (IOException e) {
                 is = connection.getErrorStream();
             }
-            return new TomcatManagerResponse(statusCode, connection.getResponseMessage(),
-                    is != null ? IOUtils.toString(is, StandardCharsets.UTF_8) : "");
+            String body = "";
+            if (is != null) {
+                try (InputStream urlIs = is) {
+                    body = IOUtils.toString(is, StandardCharsets.UTF_8);
+                }
+            }
+            return new TomcatManagerResponse(statusCode, connection.getResponseMessage(), body);
         } finally {
             connection.disconnect();
         }

src/main/java/org/apache/tomcat/maven/common/run/ClassLoaderEntriesCalculatorResult.java

@@ -63,7 +63,7 @@ public ClassLoaderEntriesCalculatorResult(List<String> classPathEntries, List<Fi
      * @return the classpath entries
      */
     public List<String> getClassPathEntries() {
-        return classPathEntries;
+        return Collections.unmodifiableList(classPathEntries);
     }
 
     /**
@@ -79,7 +79,7 @@ public void setClassPathEntries(List<String> classPathEntries) {
      * @return the temporary directories
      */
     public List<File> getTmpDirectories() {
-        return tmpDirectories;
+        return Collections.unmodifiableList(tmpDirectories);
     }
 
     /**
@@ -95,6 +95,6 @@ public void setTmpDirectories(List<File> tmpDirectories) {
      * @return the build directories
      */
     public List<String> getBuildDirectories() {
-        return buildDirectories;
+        return Collections.unmodifiableList(buildDirectories);
     }
 }

src/main/java/org/apache/tomcat/maven/common/run/DefaultClassLoaderEntriesCalculator.java

@@ -110,10 +110,11 @@ public ClassLoaderEntriesCalculatorResult calculateClassPathEntries(ClassLoaderE
                     // install/package phase
                     // so artifact.getFile is a file not a directory and not added when iterate on
                     // project.classPathElements
-                    if (!isInProjectReferences(artifact, request.getMavenProject()) || artifact.getFile().isFile()) {
-                        String fileName = artifact.getGroupId() + "-" + artifact.getFile().getName();
+                    File artifactFile = artifact.getFile();
+                    if (artifactFile != null && (!isInProjectReferences(artifact, request.getMavenProject()) || artifactFile.isFile())) {
+                        String fileName = artifact.getGroupId() + "-" + artifactFile.getName();
                         if (!fileInClassLoaderEntries.contains(fileName)) {
-                            classLoaderEntries.add(artifact.getFile().toURI().toString());
+                            classLoaderEntries.add(artifactFile.toURI().toString());
                             fileInClassLoaderEntries.add(fileName);
                         }
                     } else {
@@ -133,7 +134,7 @@ public ClassLoaderEntriesCalculatorResult calculateClassPathEntries(ClassLoaderE
                     if (existed) {
                         // check timestamp to see if artifact is newer than extracted directory
                         long dirLastMod = tmpDir.lastModified();
-                        long warLastMod = artifact.getFile().lastModified();
+                        long warLastMod = artifact.getFile() != null ? artifact.getFile().lastModified() : 0L;
 
                         if (warLastMod == 0L || warLastMod > dirLastMod) {
                             request.getLog()

src/main/java/org/apache/tomcat/maven/common/run/EmbeddedRegistry.java

@@ -83,8 +83,9 @@ public synchronized boolean register(final Object container) {
      * registry.
      *
      * @param log the log to write possible shutdown exceptions to
-     * 
-     * @throws Exception the first exception which occurred will be rethrown
+     *
+     * @throws Exception the first exception that occurred during shutdown. Additional exceptions are attached as
+     *                       suppressed exceptions.
      */
     public synchronized void shutdownAll(final Log log) throws Exception {
         Exception firstException = null;

src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractCatalinaMojo.java

@@ -199,9 +199,7 @@ protected synchronized TomcatManager getManagerInternal() throws MojoExecutionEx
             // if userName/password are defined in the mojo or the cli they override
             if (this.username != null && !this.username.isEmpty()) {
                 userName = this.username;
-                if (this.password != null) {
-                    password = this.password;
-                }
+                password = this.password != null ? this.password : DEFAULT_PASSWORD;
             }
 
             manager = new TomcatManager(url, userName, password, charset, settings.isInteractiveMode());

src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/SessionsMojo.java

@@ -47,9 +47,7 @@ public SessionsMojo() {
     @Override
     protected void invokeManager() throws MojoExecutionException, TomcatManagerException, IOException {
         getLog().info(messagesProvider.getMessage("SessionsMojo.listSessions", getURL()));
-
         String responseBody = getManager().getSessions(getPath()).getHttpResponseBody();
-
         log(responseBody);
     }
 }

src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/UndeployMojo.java

@@ -40,6 +40,7 @@ public class UndeployMojo extends AbstractWarCatalinaMojo {
     public UndeployMojo() {
         // default constructor
     }
+
     // ----------------------------------------------------------------------
     // Mojo Parameters
     // ----------------------------------------------------------------------
@@ -62,18 +63,13 @@ protected void invokeManager() throws MojoExecutionException, TomcatManagerExcep
         getLog().info(messagesProvider.getMessage("UndeployMojo.undeployingApp", getDeployedURL()));
 
         try {
-
             TomcatManagerResponse tomcatResponse = getManager().undeploy(getPath());
-
             checkTomcatResponse(tomcatResponse);
-
             log(tomcatResponse.getHttpResponseBody());
-
-        } catch (TomcatManagerException e) {
+        } catch (MojoExecutionException e) {
             if (failOnError) {
                 throw e;
             }
-
             getLog().warn(messagesProvider.getMessage("UndeployMojo.undeployError", e.getMessage()));
         }
     }

src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractExecWarMojo.java

@@ -590,24 +590,19 @@ protected String[] toStringArray(List<String> list) {
      * @throws ArchiveException if an archive error occurs
      */
     protected File addContextXmlToWar(File contextXmlFile, File warFile) throws IOException, ArchiveException {
-        ArchiveOutputStream<JarArchiveEntry> os = null;
-        OutputStream warOutputStream = null;
         File tmpWar = Files.createTempFile("tomcat", "war-exec").toFile();
         tmpWar.deleteOnExit();
 
-        try {
-            warOutputStream = new FileOutputStream(tmpWar);
-            os = new ArchiveStreamFactory().createArchiveOutputStream(ArchiveStreamFactory.JAR, warOutputStream);
+        try (InputStream is = new FileInputStream(contextXmlFile);
+                OutputStream warOutputStream = new FileOutputStream(tmpWar);
+                ArchiveOutputStream<JarArchiveEntry> os = new ArchiveStreamFactory().createArchiveOutputStream(ArchiveStreamFactory.JAR, warOutputStream)) {
             os.putArchiveEntry(new JarArchiveEntry("META-INF/context.xml"));
-            IOUtils.copy(new FileInputStream(contextXmlFile), os);
+            IOUtils.copy(is, os);
             os.closeArchiveEntry();
 
             JarFile jarFile = new JarFile(warFile);
             extractJarToArchive(jarFile, os, null);
             os.flush();
-        } finally {
-            IOUtils.closeQuietly(os);
-            IOUtils.closeQuietly(warOutputStream);
         }
         return tmpWar;
     }

src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractRunMojo.java

@@ -44,7 +44,6 @@
 import javax.xml.stream.XMLStreamReader;
 
 import org.apache.catalina.Context;
-import org.apache.catalina.Host;
 import org.apache.catalina.LifecycleException;
 import org.apache.catalina.Wrapper;
 import org.apache.catalina.connector.Connector;
@@ -154,6 +153,13 @@ protected AbstractRunMojo() {
     @Parameter(property = "maven.tomcat.address")
     private String address;
 
+    /**
+     * The AJP secret. Will be exposed as System props and session.executionProperties with key
+     * tomcat.maven.ajp.secret
+     */
+    @Parameter(property = "maven.tomcat.ajp.secret")
+    private String ajpSecret;
+
     /**
      * The AJP port to run the Tomcat server on. By default it's 0 this means won't be started. The ajp connector will
      * be started only for value > 0. Will be exposed as System props and session.executionProperties with key
@@ -257,8 +263,10 @@ protected AbstractRunMojo() {
      * To preserve backward compatibility it's false by default.
      *
      * @since 1.0
-     * 
-     * @deprecated use webapps instead
+     *
+     * @deprecated use {@link #webapps} instead. Example migration: Replace
+     *                 {@code <addContextWarDependencies>true</addContextWarDependencies>} with explicit
+     *                 {@code <webapps>} configuration entries.
      */
     @Parameter(property = "maven.tomcat.addContextWarDependencies", defaultValue = "false")
     private boolean addContextWarDependencies;
@@ -735,30 +743,33 @@ protected StandardContext parseContextFile(File file) throws MojoExecutionExcept
         try (FileInputStream fis = new FileInputStream(file)) {
             StandardContext standardContext = new StandardContext();
             XMLInputFactory factory = XMLInputFactory.newFactory();
-            factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
             factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
             factory.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
             factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
             XMLStreamReader reader = factory.createXMLStreamReader(fis);
 
-            int tag = reader.next();
-
-            while (true) {
-                if (tag == XMLStreamConstants.START_ELEMENT && "Context".equals(reader.getLocalName())) {
-                    String path = reader.getAttributeValue(null, "path");
-                    if (path != null && !path.isEmpty()) {
-                        standardContext.setPath(path);
+            try {
+                int tag = reader.next();
+
+                while (true) {
+                    if (tag == XMLStreamConstants.START_ELEMENT && "Context".equals(reader.getLocalName())) {
+                        String path = reader.getAttributeValue(null, "path");
+                        if (path != null && !path.isEmpty()) {
+                            standardContext.setPath(path);
+                        }
+
+                        String docBase = reader.getAttributeValue(null, "docBase");
+                        if (docBase != null && !docBase.isEmpty()) {
+                            standardContext.setDocBase(docBase);
+                        }
                     }
-
-                    String docBase = reader.getAttributeValue(null, "docBase");
-                    if (docBase != null && !docBase.isEmpty()) {
-                        standardContext.setDocBase(docBase);
+                    if (!reader.hasNext()) {
+                        break;
                     }
+                    tag = reader.next();
                 }
-                if (!reader.hasNext()) {
-                    break;
-                }
-                tag = reader.next();
+            } finally {
+                reader.close();
             }
 
             return standardContext;
@@ -1023,7 +1034,7 @@ private void startContainer() throws IOException, LifecycleException, MojoExecut
                 }
 
             }
-            createStaticContext(embeddedTomcat, ctx, embeddedTomcat.getHost());
+            createStaticContext(embeddedTomcat);
 
             Connector connector = new Connector(protocol);
             connector.setPort(port);
@@ -1130,7 +1141,11 @@ private void startContainer() throws IOException, LifecycleException, MojoExecut
                 if (address != null) {
                     ajpConnector.setProperty("address", address);
                 }
-                ajpConnector.setProperty("secretRequired", "false");
+                if (ajpSecret != null) {
+                    ajpConnector.setProperty("secret", ajpSecret);
+                } else {
+                    ajpConnector.setProperty("secretRequired", "false");
+                }
                 embeddedTomcat.getEngine().getService().addConnector(ajpConnector);
             }
 
@@ -1347,11 +1362,11 @@ private void addContextFromArtifact(Tomcat container, List<Context> contexts, Ar
         contexts.add(context);
     }
 
-    private void createStaticContext(final Tomcat container, Context context, Host host) {
+    private void createStaticContext(final Tomcat container) {
         if (staticContextDocbase != null) {
             Context staticContext = container.addContext(staticContextPath, staticContextDocbase);
             staticContext.setPrivileged(true);
-            Wrapper servlet = context.createWrapper();
+            Wrapper servlet = staticContext.createWrapper();
             servlet.setServletClass(DefaultServlet.class.getName());
             servlet.setName("staticContent");
             staticContext.addChild(servlet);

src/main/java/org/apache/tomcat/maven/runner/PasswordUtil.java

@@ -96,7 +96,7 @@ public static String deobfuscate(String s) {
             if (s.length() % 4 != 0) {
                 throw new IllegalArgumentException("Invalid obfuscated password: length must be a multiple of 4");
             }
-            byte[] b = new byte[s.length() / 2];
+            byte[] b = new byte[s.length() / 4];
             int l = 0;
             for (int i = 0; i < s.length(); i += 4) {
                 String x = s.substring(i, i + 4);