Copilot commented on code in PR #618:
URL: https://github.com/apache/maven-war-plugin/pull/618#discussion_r3651168479
##########
src/main/java/org/apache/maven/plugins/war/packaging/WarProjectPackagingTask.java:
##########
@@ -79,9 +79,13 @@ public void performPackaging(WarPackagingContext context)
throws MojoExecutionEx
// Prepare the INF directories
File webinfDir = new File(context.getWebappDirectory(), WEB_INF_PATH);
- webinfDir.mkdirs();
+ if (!webinfDir.exists() && !webinfDir.mkdirs()) {
+ throw new MojoExecutionException("Failed to create directory: " +
webinfDir.getAbsolutePath());
+ }
Review Comment:
The current check uses exists(), so it will silently accept the case where
WEB-INF already exists but is a regular file (not a directory). That will
likely fail later with a less clear error. Prefer using isDirectory() and only
attempting mkdirs() when it isn’t a directory (also avoids false failures if
mkdirs() returns false but the directory now exists).
This issue also appears on line 85 of the same file.
##########
src/main/java/org/apache/maven/plugins/war/packaging/AbstractWarPackagingTask.java:
##########
@@ -254,7 +254,10 @@ protected boolean copyFilteredFile(
encoding = context.getResourceEncoding();
}
// fix for MWAR-36, ensures that the parent dir are created
first
- targetFile.getParentFile().mkdirs();
+ File parentDir = targetFile.getParentFile();
+ if (!parentDir.exists() && !parentDir.mkdirs()) {
+ throw new MojoExecutionException("Failed to create
directory: " + parentDir.getAbsolutePath());
+ }
Review Comment:
If the computed parent path already exists as a regular file (not a
directory), parentDir.exists() is true and this code won’t fail early; the copy
will then fail later with a less actionable error. Use isDirectory() (and
re-check after mkdirs()) so the exception is thrown at the actual point of
failure.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -459,7 +459,10 @@ protected String[] getDependentWarIncludes() {
* @throws MojoFailureException in case of failure
*/
public void buildExplodedWebapp(File webapplicationDirectory) throws
MojoExecutionException, MojoFailureException {
- webapplicationDirectory.mkdirs();
+ if (!webapplicationDirectory.exists() &&
!webapplicationDirectory.mkdirs()) {
+ throw new MojoExecutionException(
+ "Failed to create directory: " +
webapplicationDirectory.getAbsolutePath());
+ }
Review Comment:
Using exists() here can miss the case where the target path already exists
but is not a directory. Switching to isDirectory() ensures we fail fast with
the intended MojoExecutionException in that scenario.
##########
src/main/java/org/apache/maven/plugins/war/packaging/ClassesPackagingTask.java:
##########
@@ -52,8 +52,8 @@ public ClassesPackagingTask(Overlay currentProjectOverlay) {
@Override
public void performPackaging(WarPackagingContext context) throws
MojoExecutionException {
final File webappClassesDirectory = new
File(context.getWebappDirectory(), CLASSES_PATH);
- if (!webappClassesDirectory.exists()) {
- webappClassesDirectory.mkdirs();
+ if (!webappClassesDirectory.exists() &&
!webappClassesDirectory.mkdirs()) {
+ throw new MojoExecutionException("Failed to create directory: " +
webappClassesDirectory.getAbsolutePath());
}
Review Comment:
This directory-creation check should use isDirectory() rather than exists(),
otherwise a pre-existing file at WEB-INF/classes will be treated as OK and will
fail later during copying. The condensed isDirectory()/mkdirs()/isDirectory()
pattern also avoids edge cases where mkdirs() returns false but the directory
now exists.
##########
src/main/java/org/apache/maven/plugins/war/packaging/WarProjectPackagingTask.java:
##########
@@ -79,9 +79,13 @@ public void performPackaging(WarPackagingContext context)
throws MojoExecutionEx
// Prepare the INF directories
File webinfDir = new File(context.getWebappDirectory(), WEB_INF_PATH);
- webinfDir.mkdirs();
+ if (!webinfDir.exists() && !webinfDir.mkdirs()) {
+ throw new MojoExecutionException("Failed to create directory: " +
webinfDir.getAbsolutePath());
Review Comment:
The new behavior (throwing MojoExecutionException when required directories
cannot be created) isn’t covered by existing unit tests. This is feasible to
test deterministically by creating a regular file where a directory is expected
(e.g., create a file named WEB-INF under the webappDirectory) and asserting the
mojo fails with MojoExecutionException.
##########
src/main/java/org/apache/maven/plugins/war/packaging/WarProjectPackagingTask.java:
##########
@@ -79,9 +79,13 @@ public void performPackaging(WarPackagingContext context)
throws MojoExecutionEx
// Prepare the INF directories
File webinfDir = new File(context.getWebappDirectory(), WEB_INF_PATH);
- webinfDir.mkdirs();
+ if (!webinfDir.exists() && !webinfDir.mkdirs()) {
+ throw new MojoExecutionException("Failed to create directory: " +
webinfDir.getAbsolutePath());
Review Comment:
PR description/issue #590 call out mkdirs() return values being ignored
across WAR assembly. There is still at least one unchecked mkdirs() in
OverlayPackagingTask#getOverlayTempDirectory (result.mkdirs()), so the change
doesn’t fully address the described problem area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
