This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/tomcat-maven-plugin.git
The following commit(s) were added to refs/heads/trunk by this push:
new 710931b Additional code review
710931b is described below
commit 710931b1e5e6c2c6949b7f952b26e898d1e8cee6
Author: remm <remm@meteor>
AuthorDate: Thu Jun 4 16:32:52 2026 +0200
Additional code review
---
.../maven/common/deployer/TomcatManager.java | 26 +++-----------
.../run/DefaultClassLoaderEntriesCalculator.java | 2 +-
.../tomcat/maven/common/run/EmbeddedRegistry.java | 41 ++++++++--------------
.../maven/plugin/tomcat/AbstractCatalinaMojo.java | 4 ++-
.../maven/plugin/tomcat/AbstractTomcatMojo.java | 1 +
.../plugin/tomcat/deploy/AbstractDeployMojo.java | 2 +-
.../plugin/tomcat/run/AbstractExecWarMojo.java | 4 +--
.../maven/plugin/tomcat/run/AbstractRunMojo.java | 17 +++++----
.../apache/tomcat/maven/runner/TomcatRunner.java | 21 ++++++-----
.../tomcat/maven/runner/TomcatRunnerCli.java | 14 ++++----
10 files changed, 55 insertions(+), 77 deletions(-)
diff --git
a/src/main/java/org/apache/tomcat/maven/common/deployer/TomcatManager.java
b/src/main/java/org/apache/tomcat/maven/common/deployer/TomcatManager.java
index 61702c3..8e76438 100644
--- a/src/main/java/org/apache/tomcat/maven/common/deployer/TomcatManager.java
+++ b/src/main/java/org/apache/tomcat/maven/common/deployer/TomcatManager.java
@@ -107,27 +107,6 @@ public class TomcatManager {
// Constructors
// ----------------------------------------------------------------------
- /**
- * Creates a Tomcat manager wrapper for the specified URL that uses a
username of <code>admin</code>, an empty
- * password and ISO-8859-1 URL encoding.
- *
- * @param url the full URL of the Tomcat manager instance to use
- */
- public TomcatManager(URL url) {
- this(url, "admin");
- }
-
- /**
- * Creates a Tomcat manager wrapper for the specified URL and username
that uses an empty password and ISO-8859-1
- * URL encoding.
- *
- * @param url the full URL of the Tomcat manager instance to use
- * @param username the username to use when authenticating with Tomcat
manager
- */
- public TomcatManager(URL url, String username) {
- this(url, username, "");
- }
-
/**
* Creates a Tomcat manager wrapper for the specified URL, username and
password that uses ISO-8859-1 URL encoding.
*
@@ -778,8 +757,11 @@ public class TomcatManager {
} catch (URISyntaxException e) {
throw new MalformedURLException(e.getMessage());
}
- java.net.Proxy netProxy = java.net.Proxy.NO_PROXY;
+ if (!"https".equalsIgnoreCase(url.getProtocol()) && log != null) {
+ log.warn("Sending credentials over non-HTTPS connection to " +
url.getHost());
+ }
+ java.net.Proxy netProxy = java.net.Proxy.NO_PROXY;
if (proxySettings != null) {
ProxyInfo proxyInfo = new ProxyInfo();
proxyInfo.setNonProxyHosts(proxySettings.getNonProxyHosts());
diff --git
a/src/main/java/org/apache/tomcat/maven/common/run/DefaultClassLoaderEntriesCalculator.java
b/src/main/java/org/apache/tomcat/maven/common/run/DefaultClassLoaderEntriesCalculator.java
index 5b8f974..cb60e6e 100644
---
a/src/main/java/org/apache/tomcat/maven/common/run/DefaultClassLoaderEntriesCalculator.java
+++
b/src/main/java/org/apache/tomcat/maven/common/run/DefaultClassLoaderEntriesCalculator.java
@@ -149,7 +149,7 @@ public class DefaultClassLoaderEntriesCalculator implements
ClassLoaderEntriesCa
}
}
- // All directories must be added, this is not for cleanup
+ // Add all directories for cleanup on shutdown
tmpDirectories.add(tmpDir);
try {
diff --git
a/src/main/java/org/apache/tomcat/maven/common/run/EmbeddedRegistry.java
b/src/main/java/org/apache/tomcat/maven/common/run/EmbeddedRegistry.java
index eb42212..1239c1e 100644
--- a/src/main/java/org/apache/tomcat/maven/common/run/EmbeddedRegistry.java
+++ b/src/main/java/org/apache/tomcat/maven/common/run/EmbeddedRegistry.java
@@ -20,7 +20,6 @@ package org.apache.tomcat.maven.common.run;
import org.apache.maven.plugin.logging.Log;
-import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Iterator;
import java.util.Set;
@@ -46,13 +45,17 @@ public final class EmbeddedRegistry {
* Don't instantiate - use the instance through {@link #getInstance()}.
*/
private EmbeddedRegistry() {
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- getInstance().shutdownAll(null);
- } catch (Exception e) {
- // ignore, the exception should already have been reported
- }
- }));
+ try {
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ try {
+ getInstance().shutdownAll(null);
+ } catch (Exception e) {
+ // ignore
+ }
+ }));
+ } catch (IllegalStateException e) {
+ // VM is already shutting down; skip hook registration
+ }
}
/**
@@ -91,29 +94,13 @@ public final class EmbeddedRegistry {
Method method = embedded.getClass().getMethod("stop");
method.invoke(embedded);
iterator.remove();
- } catch (NoSuchMethodException e) {
- if (firstException == null) {
- firstException = e;
- error(log, e, "no stop/destroy method in class " +
embedded.getClass().getName());
- } else {
- error(log, e, "Error while shutting down embedded
Tomcat.");
- }
- } catch (IllegalAccessException e) {
- if (firstException == null) {
- firstException = e;
- error(log, e,
- "IllegalAccessException for stop/destroy method in
class " + embedded.getClass().getName());
- } else {
- error(log, e, "Error while shutting down embedded
Tomcat.");
- }
- } catch (InvocationTargetException e) {
+ } catch (Exception e) {
if (firstException == null) {
firstException = e;
- error(log, e,
- "InvocationTargetException for stop/destroy method
in class " + embedded.getClass().getName());
} else {
- error(log, e, "Error while shutting down embedded
Tomcat.");
+ firstException.addSuppressed(e);
}
+ error(log, e, "Error while shutting down embedded Tomcat: " +
embedded.getClass().getName());
}
}
if (firstException != null) {
diff --git
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractCatalinaMojo.java
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractCatalinaMojo.java
index fcb4800..fd45694 100644
---
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractCatalinaMojo.java
+++
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractCatalinaMojo.java
@@ -199,7 +199,9 @@ public abstract class AbstractCatalinaMojo extends
AbstractTomcatMojo {
// if userName/password are defined in the mojo or the cli they
override
if (this.username != null && !this.username.isEmpty()) {
userName = this.username;
- password = this.password == null ? "" : this.password;
+ if (this.password != null) {
+ password = this.password;
+ }
}
manager = new TomcatManager(url, userName, password, charset,
settings.isInteractiveMode());
diff --git
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractTomcatMojo.java
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractTomcatMojo.java
index 252d7c6..352ff30 100644
---
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractTomcatMojo.java
+++
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/AbstractTomcatMojo.java
@@ -89,6 +89,7 @@ public abstract class AbstractTomcatMojo extends AbstractMojo
{
": " + tomcatResponse.getHttpResponseBody());
} else if (!tomcatResponse.getHttpResponseBody().startsWith("OK -")) {
// Tomcat Manager and Host Manager will always use "OK -" and
"FAIL -" prefixes for their text endpoints
+ // There is no other way to process the Tomcat manager responses,
and this has to be kept in sync with Tomcat
getLog().error(messagesProvider.getMessage("tomcatHttpBodyError",
tomcatResponse.getHttpResponseBody()));
throw new MojoExecutionException(
messagesProvider.getMessage("tomcatHttpBodyError",
tomcatResponse.getHttpResponseBody()));
diff --git
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/AbstractDeployMojo.java
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/AbstractDeployMojo.java
index 9ed3522..cadd93b 100644
---
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/AbstractDeployMojo.java
+++
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/deploy/AbstractDeployMojo.java
@@ -175,7 +175,7 @@ public abstract class AbstractDeployMojo extends
AbstractWarCatalinaMojo {
getLog().info(messagesProvider.getMessage("AbstractDeployMojo.deployingContext",
getDeployedURL()));
URL contextURL = getContextFile().toURI().toURL();
- TomcatManagerResponse tomcatResponse = getManager().deploy(getPath(),
contextURL, isUpdate(), getTag());
+ TomcatManagerResponse tomcatResponse =
getManager().deployContext(getPath(), contextURL, isUpdate(), getTag());
checkTomcatResponse(tomcatResponse);
log(tomcatResponse.getHttpResponseBody());
}
diff --git
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractExecWarMojo.java
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractExecWarMojo.java
index adbf048..37c33cc 100644
---
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractExecWarMojo.java
+++
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractExecWarMojo.java
@@ -192,7 +192,7 @@ public abstract class AbstractExecWarMojo extends
AbstractTomcatMojo {
* the type to use for the attached/generated artifact
*/
@Parameter(property = "maven.tomcat.exec.war.attachArtifactType",
defaultValue = "jar", required = true)
- protected String attachArtifactClassifierType;
+ protected String attachArtifactType;
/**
* to enable naming when starting tomcat
@@ -439,7 +439,7 @@ public abstract class AbstractExecWarMojo extends
AbstractTomcatMojo {
if (attachArtifact) {
// MavenProject project, String artifactType, String
artifactClassifier, File artifactFile
- projectHelper.attachArtifact(project,
attachArtifactClassifierType, attachArtifactClassifier,
+ projectHelper.attachArtifact(project, attachArtifactType,
attachArtifactClassifier,
execWarJar);
}
diff --git
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractRunMojo.java
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractRunMojo.java
index d584108..0b4090a 100644
---
a/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractRunMojo.java
+++
b/src/main/java/org/apache/tomcat/maven/plugin/tomcat/run/AbstractRunMojo.java
@@ -734,7 +734,12 @@ public abstract class AbstractRunMojo extends
AbstractTomcatMojo {
protected StandardContext parseContextFile(File file) throws
MojoExecutionException {
try (FileInputStream fis = new FileInputStream(file)) {
StandardContext standardContext = new StandardContext();
- XMLStreamReader reader =
XMLInputFactory.newFactory().createXMLStreamReader(fis);
+ 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();
@@ -1226,6 +1231,7 @@ public abstract class AbstractRunMojo extends
AbstractTomcatMojo {
/**
* Causes the current thread to wait indefinitely. This method does not
return.
+ * This allows Tomcat to run until it receives a command to shutdown the
JVM.
*/
private void waitIndefinitely() {
Object lock = new Object();
@@ -1313,11 +1319,11 @@ public abstract class AbstractRunMojo extends
AbstractTomcatMojo {
// Extract the module
unArchiver.extract();
} catch (NoSuchArchiverException | ArchiverException e) {
- getLog().error(e);
- return;
+ getLog().error("Failed to extract WAR: " + artifact.getFile(),
e);
+ throw new MojoExecutionException("Failed to extract WAR
artifact: " + artifact.getFile(), e);
}
}
- // TODO make that configurable ?
+
WebappLoader webappLoader = createWebappLoader();
Context context = null;
if (asWebApp) {
@@ -1339,7 +1345,6 @@ public abstract class AbstractRunMojo extends
AbstractTomcatMojo {
}
contexts.add(context);
- // container.getHost().addChild(context);
}
private void createStaticContext(final Tomcat container, Context context,
Host host) {
@@ -1351,8 +1356,6 @@ public abstract class AbstractRunMojo extends
AbstractTomcatMojo {
servlet.setName("staticContent");
staticContext.addChild(servlet);
staticContext.addServletMappingDecoded("/", "staticContent");
- // see https://issues.apache.org/jira/browse/MTOMCAT-238
- // host.addChild( staticContext );
}
}
diff --git a/src/main/java/org/apache/tomcat/maven/runner/TomcatRunner.java
b/src/main/java/org/apache/tomcat/maven/runner/TomcatRunner.java
index fd6f5f8..a475cd2 100644
--- a/src/main/java/org/apache/tomcat/maven/runner/TomcatRunner.java
+++ b/src/main/java/org/apache/tomcat/maven/runner/TomcatRunner.java
@@ -118,7 +118,7 @@ public class TomcatRunner {
public int ajpPort;
/**
- * AJP port number.
+ * AJP secret.
*/
public String ajpSecret;
@@ -222,14 +222,18 @@ public class TomcatRunner {
if (timestampFile.exists()) {
String timestampValue =
timestampProps.getProperty(TomcatRunner.ARCHIVE_GENERATION_TIMESTAMP_KEY);
if (timestampValue != null) {
- long timestamp = Long.parseLong(timestampValue);
- archiveTimestampChanged = Long.parseLong(
-
runtimeProperties.getProperty(TomcatRunner.ARCHIVE_GENERATION_TIMESTAMP_KEY)) >
timestamp;
-
- debugMessage("read timestamp from file " + timestampValue + ",
archiveTimestampChanged: " +
- archiveTimestampChanged);
+ try {
+ long timestamp = Long.parseLong(timestampValue);
+ long currentTimestamp = Long.parseLong(
+
runtimeProperties.getProperty(TomcatRunner.ARCHIVE_GENERATION_TIMESTAMP_KEY));
+ archiveTimestampChanged = currentTimestamp > timestamp;
+ debugMessage("read timestamp from file " + timestampValue
+ ", archiveTimestampChanged: " +
+ archiveTimestampChanged);
+ } catch (NumberFormatException e) {
+ debugMessage("ERROR: Invalid timestamp value, forcing
re-extraction: " + timestampValue);
+ archiveTimestampChanged = true;
+ }
}
-
}
codeSourceContextPath =
runtimeProperties.getProperty(CODE_SOURCE_CONTEXT_PATH);
@@ -671,7 +675,6 @@ public class TomcatRunner {
output.write(buffer, 0, n);
}
}
- // Ignore
}
/**
diff --git a/src/main/java/org/apache/tomcat/maven/runner/TomcatRunnerCli.java
b/src/main/java/org/apache/tomcat/maven/runner/TomcatRunnerCli.java
index 77990eb..b454bcb 100644
--- a/src/main/java/org/apache/tomcat/maven/runner/TomcatRunnerCli.java
+++ b/src/main/java/org/apache/tomcat/maven/runner/TomcatRunnerCli.java
@@ -82,10 +82,10 @@ public class TomcatRunnerCli {
static final Option CLIENT_AUTH = Option.builder().longOpt("clientAuth")
.desc("enable client authentication for https").get();
- static final Option KEY_ALIAS =
Option.builder().longOpt("keyAlias").hasArgs()
+ static final Option KEY_ALIAS =
Option.builder().longOpt("keyAlias").hasArg()
.argName("alias from keystore for ssl").get();
- static final Option OBFUSCATE =
Option.builder().longOpt("obfuscate").hasArgs().argName("password")
+ static final Option OBFUSCATE =
Option.builder().longOpt("obfuscate").hasArg().argName("password")
.desc("obfuscate the password and exit").get();
static final Option HTTP_PROTOCOL =
Option.builder().longOpt("httpProtocol").hasArg().argName("httpProtocol")
@@ -154,7 +154,6 @@ public class TomcatRunnerCli {
System.err.println("Invalid value for " +
HTTP_PORT.getArgName() + " '" +
port + "'. Must be an integer.");
System.exit(1);
- System.exit(1);
}
}
@@ -237,11 +236,12 @@ public class TomcatRunnerCli {
}
private static Properties buildStandaloneProperties() throws IOException {
- InputStream is = Thread.currentThread().getContextClassLoader()
- .getResourceAsStream(STAND_ALONE_PROPERTIES_FILENAME);
Properties properties = new Properties();
- if (is != null) {
- properties.load(is);
+ try (InputStream is = Thread.currentThread().getContextClassLoader()
+ .getResourceAsStream(STAND_ALONE_PROPERTIES_FILENAME)) {
+ if (is != null) {
+ properties.load(is);
+ }
}
return properties;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
