Copilot commented on code in PR #642:
URL: https://github.com/apache/maven-war-plugin/pull/642#discussion_r3668383253
##########
src/main/java/org/apache/maven/plugins/war/packaging/ArtifactsPackagingTask.java:
##########
@@ -98,14 +98,10 @@ public void performPackaging(WarPackagingContext context)
throws MojoExecutionEx
copyFile(id, context, artifact.getFile(),
MODULES_PATH + targetFileName);
} else if ("xar".equals(type)) {
copyFile(id, context, artifact.getFile(),
EXTENSIONS_PATH + targetFileName);
- } else if ("jar".equals(type)
- || "ejb".equals(type)
- || "ejb-client".equals(type)
- || "test-jar".equals(type)
- || "bundle".equals(type)) {
- copyFile(id, context, artifact.getFile(), LIB_PATH
+ targetFileName);
- } else if ("par".equals(type)) {
- targetFileName = targetFileName.substring(0,
targetFileName.lastIndexOf('.')) + ".jar";
+ } else if (isLibraryType(type)) {
+ if ("par".equals(type)) {
+ targetFileName = targetFileName.substring(0,
targetFileName.lastIndexOf('.')) + ".jar";
+ }
copyFile(id, context, artifact.getFile(), LIB_PATH
+ targetFileName);
Review Comment:
`targetFileName.substring(0, targetFileName.lastIndexOf('.'))` will throw if
`targetFileName` has no dot (e.g., if a custom `outputFileNameMapping` omits an
extension). Please make the `.par`→`.jar` conversion robust by removing the
extension safely (or by checking `lastIndexOf('.') >= 0` before substring)
and/or by explicitly handling the “no extension” case.
##########
src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java:
##########
@@ -934,4 +942,47 @@ public void
testExplodedWarWithOutputFileNameMappingAndDuplicateDependencies(War
expectedEJBArtifact.delete();
expectedEJBDupArtifact.delete();
}
+
+ /**
+ * Test for MWAR-443: Files placed by maven-dependency-plugin in
WEB-INF/lib
+ * for provided-scope artifacts should not be deleted by the WAR plugin.
+ */
+ @InjectMojo(goal = "exploded", pom =
"src/test/resources/unit/warexplodedmojo/plugin-config.xml")
+ @MojoParameter(
+ name = "classesDirectory",
+ value =
"target/test-classes/unit/warexplodedmojo/SimpleExplodedWar-test-data/classes/")
+ @MojoParameter(
+ name = "warSourceDirectory",
+ value =
"target/test-classes/unit/warexplodedmojo/SimpleExplodedWar-test-data/source/")
+ @MojoParameter(name = "webappDirectory", value =
"target/test-classes/unit/warexplodedmojo/MWAR443Test")
+ @MojoParameter(name = "outdatedCheckPath", value = "WEB-INF/lib/")
+ @Test
+ public void
testProvidedScopeArtifactPlacedByDependencyPluginShouldNotBeDeleted(WarExplodedMojo
mojo)
+ throws Exception {
+ // Ensure session has a start time so the outdated resource detection
is active.
+ // Uses same pattern as WarExplodedMojoFilteringTest which also calls
+ // when(mavenSession.get...()).thenReturn(...)
+ when(mavenSession.getStartTime()).thenReturn(new Date());
+
+ File webAppDirectory = mojo.getWebappDirectory();
+ File libDir = new File(webAppDirectory, "WEB-INF/lib");
+ File providedJar = new File(libDir, "derbyLocale_cs-10.14.2.0.jar");
+ try {
+ // Setup: Create a file in WEB-INF/lib with an old timestamp,
+ // simulating a file placed by maven-dependency-plugin for a
provided-scope artifact
+ libDir.mkdirs();
+ providedJar.createNewFile();
+ // Set timestamp to something in the past (before session start)
+ providedJar.setLastModified(0L);
+
+ mojo.execute();
+
+ // The file should NOT be deleted - it was placed by another plugin
+ assertTrue(providedJar.exists(), "provided-scope artifact should
not be deleted by WAR plugin");
+ } finally {
+ // Cleanup
+ providedJar.delete();
+ libDir.delete();
+ }
Review Comment:
`libDir.delete()` will fail if `mojo.execute()` creates additional files
under `WEB-INF/lib` (which is likely), leaving test artifacts behind and
potentially causing test pollution across runs. Prefer cleaning up recursively
(or using a dedicated temporary webapp directory for this test) and/or
asserting deletions so failures are visible.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -682,6 +704,47 @@ protected boolean checkAllPathsForOutdated() {
return outdatedCheckPath.equals("/");
}
+ /**
+ * Returns the set of expected target filenames for runtime-scope
artifacts.
+ * Used to avoid marking files placed by other plugins (e.g.,
maven-dependency-plugin)
+ * as outdated and subsequently deleting them (MWAR-443).
+ */
+ private Set<String> getRuntimeArtifactFileNames() {
+ Set<String> fileNames = new HashSet<>();
+ ScopeArtifactFilter filter = new
ScopeArtifactFilter(Artifact.SCOPE_RUNTIME);
+ if (project.getArtifacts() != null) {
+ for (Artifact artifact : project.getArtifacts()) {
+ if (!artifact.isOptional()
+ && filter.include(artifact)
+ &&
AbstractWarPackagingTask.isLibraryType(artifact.getType())) {
+ try {
+ String targetFileName;
+ if (getOutputFileNameMapping() != null) {
+ targetFileName =
+
MappingUtils.evaluateFileNameMapping(getOutputFileNameMapping(), artifact);
+ } else {
+ String classifier = artifact.getClassifier();
+ if (classifier != null &&
!classifier.trim().isEmpty()) {
+ targetFileName =
MappingUtils.evaluateFileNameMapping(
+
MappingUtils.DEFAULT_FILE_NAME_MAPPING_CLASSIFIER, artifact);
+ } else {
+ targetFileName =
MappingUtils.evaluateFileNameMapping(
+
MappingUtils.DEFAULT_FILE_NAME_MAPPING, artifact);
+ }
+ }
+ if ("par".equals(artifact.getType())) {
+ targetFileName = targetFileName.substring(0,
targetFileName.lastIndexOf('.')) + ".jar";
+ }
+ fileNames.add(targetFileName);
Review Comment:
Same substring issue as in `ArtifactsPackagingTask`: if the evaluated
filename mapping yields a name without an extension, `lastIndexOf('.')` becomes
`-1` and this will throw. Since this code runs during outdated-resource
scanning, an exception here can disable/alter cleanup behavior. Please switch
to a safe “remove extension” approach or handle the `-1` case explicitly.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -665,7 +675,19 @@ public FileVisitResult visitFile(Path file,
BasicFileAttributes attrs) throws IO
.relativize(file)
.toString();
if (checkAllPathsForOutdated() ||
path.startsWith(outdatedCheckPath)) {
- outdatedResources.add(path);
+ // MWAR-443: For files under the artifact
lib directory,
+ // only mark as outdated if they match a
runtime-scope artifact
+ // that the WAR plugin would copy. This
prevents deleting files
+ // placed by other plugins (e.g.,
maven-dependency-plugin) for
+ // non-runtime-scope dependencies.
+ String normalizedPath = path.replace('\\',
'/');
+ if
(normalizedPath.startsWith("WEB-INF/lib/")
+ &&
!runtimeArtifactFileNames.contains(
+ file.toFile().getName())) {
+ // Skip: file was placed by another
plugin, not managed by WAR plugin
+ } else {
+ outdatedResources.add(path);
+ }
Review Comment:
This allowlist approach can prevent removal of *stale runtime jars* on
version change. Example: if `foo-1.0.jar` exists from a previous run and the
current runtime dependency is `foo-1.1.jar`, then `foo-1.0.jar` won’t be in
`runtimeArtifactFileNames` and will be skipped (so it survives), potentially
leaving multiple versions on the classpath. Consider adjusting the rule so
provided-scope (and other non-runtime) artifacts are preserved, while
unknown/non-current runtime libs can still be removed (e.g., by preserving
filenames for non-runtime project artifacts rather than only deleting known
runtime filenames, or by matching/removing stale files based on artifact
identity beyond the exact filename).
--
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]
