Copilot commented on code in PR #641:
URL: https://github.com/apache/maven-war-plugin/pull/641#discussion_r3666937037
##########
src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java:
##########
@@ -934,4 +942,44 @@ 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
+ when(mavenSession.getStartTime()).thenReturn(new Date());
Review Comment:
`when(mavenSession.getStartTime())...` will fail at runtime with Mockito’s
`NotAMockException` unless `mavenSession` is actually a Mockito mock/spy.
Prefer setting the start time on the real session/request object (if available
in this test harness), or explicitly inject/replace the mojo’s session with a
mock session instance that the mojo actually uses.
##########
src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java:
##########
@@ -934,4 +942,44 @@ 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
+ when(mavenSession.getStartTime()).thenReturn(new Date());
+
+ // 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
+ File webAppDirectory = mojo.getWebappDirectory();
+ File libDir = new File(webAppDirectory, "WEB-INF/lib");
+ libDir.mkdirs();
+ File providedJar = new File(libDir, "derbyLocale_cs-10.14.2.0.jar");
+ providedJar.createNewFile();
+ // Set timestamp to something in the past (before session start)
+ providedJar.setLastModified(0L);
+
+ // Run the mojo
+ 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");
+
+ // Cleanup
+ providedJar.delete();
+ libDir.delete();
+ }
Review Comment:
This test writes into the configured `webappDirectory` and performs
best-effort cleanup without assertions; `libDir.delete()` will fail if other
files are present (likely after `mojo.execute()`), leaving state behind and
causing cross-test interference/flakiness. Use a per-test temp directory (or
ensure the configured `webappDirectory` is unique), and perform recursive
cleanup in a `finally` block (or assert delete results) so the test doesn’t
leave artifacts behind.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -682,6 +703,43 @@ 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)) {
+ try {
+ String type = artifact.getType();
+ if ("jar".equals(type)
+ || "ejb".equals(type)
+ || "ejb-client".equals(type)
+ || "test-jar".equals(type)
+ || "bundle".equals(type)
+ || "par".equals(type)) {
+ String classifier = artifact.getClassifier();
+ if (classifier != null &&
!classifier.trim().isEmpty()) {
+
fileNames.add(MappingUtils.evaluateFileNameMapping(
+
MappingUtils.DEFAULT_FILE_NAME_MAPPING_CLASSIFIER, artifact));
+ } else {
+
fileNames.add(MappingUtils.evaluateFileNameMapping(
+
MappingUtils.DEFAULT_FILE_NAME_MAPPING, artifact));
+ }
Review Comment:
This computes “expected” runtime filenames using
`MappingUtils.DEFAULT_FILE_NAME_MAPPING*`, which can diverge from the actual
filenames the WAR plugin produces when `outputFileNameMapping` (or similar
configuration) is set. That mismatch can cause true WAR-managed runtime jars to
be misclassified as “placed by another plugin” and never marked
outdated/cleaned up. Use the same filename-mapping configuration/mechanism that
the WAR plugin uses when copying dependencies into `WEB-INF/lib` so the
comparison is consistent.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -682,6 +703,43 @@ 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)) {
+ try {
+ String type = artifact.getType();
+ if ("jar".equals(type)
+ || "ejb".equals(type)
+ || "ejb-client".equals(type)
+ || "test-jar".equals(type)
+ || "bundle".equals(type)
+ || "par".equals(type)) {
Review Comment:
The hard-coded list of artifact `type`s duplicates packaging rules and is
likely to drift from the WAR plugin’s real inclusion logic over time, making
the “outdated” detection inconsistent. Consider reusing the existing
artifact-selection logic used for populating `WEB-INF/lib` (or centralizing the
allowed-types decision into a shared helper/constant) so this stays in sync.
--
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]
