Copilot commented on code in PR #27:
URL: https://github.com/apache/maven-war-plugin/pull/27#discussion_r3665310373
##########
src/main/java/org/apache/maven/plugins/war/packaging/AbstractWarPackagingTask.java:
##########
@@ -372,6 +372,10 @@ protected boolean copyFile( WarPackagingContext context,
File source, File desti
}
else
{
+ if ( context.isForceOverwriteResources() &&
destination.exists() && !destination.canWrite() )
+ {
+ destination.setWritable( true );
+ }
FileUtils.copyFile( source.getCanonicalFile(), destination );
// preserve timestamp
Review Comment:
`File#setWritable(true)` returns a boolean that is currently ignored; if it
fails (common on some platforms/filesystems), the subsequent copy can still
fail with an access error. Please check the return value and either throw a
clear exception (including the path) or fall back to a more robust permission
change (e.g., NIO `Files.setPosixFilePermissions` where supported). Also
consider whether permissions should be restored after the overwrite to avoid
permanently changing the destination’s read-only state.
##########
src/main/java/org/apache/maven/plugins/war/packaging/WarPackagingContext.java:
##########
@@ -258,4 +258,10 @@ public interface WarPackagingContext
* @since 3.3.0
*/
String getOutputTimestamp();
+
+ /**
+ * If files are modified, but the target is readonly, force update.
+ * @return forceOverwriteResources
+ */
+ boolean isForceOverwriteResources();
Review Comment:
Adding a new abstract method to a `public interface` is a
source/binary-breaking change for any external implementations. To preserve
compatibility, make this a `default` method returning `false` (or introduce a
new sub-interface) so existing implementers don’t break at compile-time/runtime.
##########
src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java:
##########
@@ -147,6 +147,65 @@ public void testSimpleExplodedWarWTargetPath()
expectedWebResourceFile.delete();
}
+ /**
+ * @throws Exception in case of an error.
+ */
+ public void testReadOnlyFileInDestinationInExplodedWar()
+ throws Exception
+ {
+ // setup test data
+ String testId = "SimpleExplodedWar";
+ MavenProjectBasicStub project = new MavenProjectBasicStub();
+ File webAppSource = createWebAppSource( testId );
+ File classesDir = createClassesDir( testId, false );
+ File webAppResource = new File( getTestDirectory(), testId +
"-resources" );
+ File webAppDirectory = new File( getTestDirectory(), testId );
+ File sampleResource = new File( webAppResource, "pix/panis_na.jpg" );
+ ResourceStub[] resources = new ResourceStub[] { new ResourceStub() };
+
+
+ createFile( sampleResource );
+
+ File conflictingFile = new File( webAppDirectory, "pix/panis_na.jpg" );
+ if (conflictingFile.exists() )
+ {
+ conflictingFile.delete();
+ }
+
+ createFile( conflictingFile );
+ conflictingFile.setWritable(false);
+ conflictingFile.setLastModified(System.currentTimeMillis()-100_000);
+
+ assertTrue( "sampeResource not found", sampleResource.exists() );
Review Comment:
Corrected spelling of 'sampeResource' to 'sampleResource' in the assertion
message.
##########
src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java:
##########
@@ -147,6 +147,65 @@ public void testSimpleExplodedWarWTargetPath()
expectedWebResourceFile.delete();
}
+ /**
+ * @throws Exception in case of an error.
+ */
+ public void testReadOnlyFileInDestinationInExplodedWar()
+ throws Exception
+ {
+ // setup test data
+ String testId = "SimpleExplodedWar";
+ MavenProjectBasicStub project = new MavenProjectBasicStub();
+ File webAppSource = createWebAppSource( testId );
+ File classesDir = createClassesDir( testId, false );
+ File webAppResource = new File( getTestDirectory(), testId +
"-resources" );
+ File webAppDirectory = new File( getTestDirectory(), testId );
+ File sampleResource = new File( webAppResource, "pix/panis_na.jpg" );
+ ResourceStub[] resources = new ResourceStub[] { new ResourceStub() };
+
+
+ createFile( sampleResource );
+
+ File conflictingFile = new File( webAppDirectory, "pix/panis_na.jpg" );
+ if (conflictingFile.exists() )
+ {
+ conflictingFile.delete();
+ }
+
+ createFile( conflictingFile );
+ conflictingFile.setWritable(false);
+ conflictingFile.setLastModified(System.currentTimeMillis()-100_000);
+
+ assertTrue( "sampeResource not found", sampleResource.exists() );
Review Comment:
This test currently asserts only file existence, which doesn’t prove that
the read-only destination file was actually overwritten (it would still exist
even if the overwrite failed). To make the test validate the behavior, write
distinct content (or a distinct last-modified) to `sampleResource` and
`conflictingFile`, run the mojo, and then assert that the destination’s content
(or last-modified) matches the source (and/or differs from the pre-existing
destination). Also, consider guarding the test with an assumption if the
platform/filesystem doesn’t support making files read-only via
`setWritable(false)`.
##########
src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java:
##########
@@ -146,6 +146,11 @@ public abstract class AbstractWarMojo
@Parameter
private Resource[] webResources;
+ /**
+ * If resources should be overwritten, even if they are read only
+ */
+ @Parameter( defaultValue = "false" )
+ boolean forceOverwriteResources;
Review Comment:
Maven plugin parameters are typically `private` to match the rest of the
Mojo’s encapsulation pattern and reduce accidental access/mutation. Consider
making this field `private` (and keep passing it through via the existing
wiring) and enhance the Javadoc with an `@since` tag and a clearer description
(e.g., explicitly stating this affects overwriting read-only files in the
exploded WAR output).
--
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]
