[
https://issues.apache.org/jira/browse/MENFORCER-390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17922789#comment-17922789
]
ASF GitHub Bot commented on MENFORCER-390:
------------------------------------------
elharo commented on code in PR #297:
URL: https://github.com/apache/maven-enforcer/pull/297#discussion_r1937651407
##########
enforcer-rules/src/site/apt/requireFilesDontExist.apt.vm:
##########
@@ -26,7 +26,10 @@
Require Files Don't Exist
This rule checks that the specified list of files do not exist.
Review Comment:
delete "list of"
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java:
##########
@@ -105,19 +109,62 @@ void testEmptyFileListAllowNull() {
}
@Test
- void testFileDoesNotExist() throws EnforcerRuleException, IOException {
+ void testDeletedFileDetected() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
+ rule.setFilesList(Collections.singletonList(f));
+
+ // Check the file is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
+
f.delete();
assertFalse(f.exists());
- rule.setFilesList(Collections.singletonList(f));
+ // Rule should now pass as the file was deleted
+ rule.execute();
+ }
+
+ @Test
+ void testSymbolicLinkDeletedDetected() throws Exception {
+ File canonicalFile = File.createTempFile("canonical_", null,
temporaryFolder);
+ File linkFile = Files.createSymbolicLink(
+ Paths.get(temporaryFolder.getAbsolutePath(),
"symbolic.link"),
+ Paths.get(canonicalFile.getAbsolutePath()))
+ .toFile();
+
+ rule.setFilesList(Collections.singletonList(linkFile));
+ // Check the link is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
Review Comment:
assertNotNull
##########
enforcer-rules/src/site/apt/requireFilesExist.apt.vm:
##########
@@ -27,22 +27,25 @@ Require Files Exist
This rule checks that the specified list of files exist.
Review Comment:
ditto
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java:
##########
@@ -105,19 +109,62 @@ void testEmptyFileListAllowNull() {
}
@Test
- void testFileDoesNotExist() throws EnforcerRuleException, IOException {
+ void testDeletedFileDetected() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
+ rule.setFilesList(Collections.singletonList(f));
+
+ // Check the file is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
+
f.delete();
assertFalse(f.exists());
- rule.setFilesList(Collections.singletonList(f));
+ // Rule should now pass as the file was deleted
+ rule.execute();
+ }
+
+ @Test
+ void testSymbolicLinkDeletedDetected() throws Exception {
+ File canonicalFile = File.createTempFile("canonical_", null,
temporaryFolder);
+ File linkFile = Files.createSymbolicLink(
+ Paths.get(temporaryFolder.getAbsolutePath(),
"symbolic.link"),
+ Paths.get(canonicalFile.getAbsolutePath()))
+ .toFile();
+
+ rule.setFilesList(Collections.singletonList(linkFile));
+ // Check the link is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
+
+ linkFile.delete();
+
+ // Rule should now pass as the target was deleted
+ rule.execute();
+ }
+
+ @Test
+ void testSymbolicLinkTargetDeletedDetected() throws Exception {
+ File canonicalFile = File.createTempFile("canonical_", null,
temporaryFolder);
+ File linkFile = Files.createSymbolicLink(
+ Paths.get(temporaryFolder.getAbsolutePath(),
"symbolic.link"),
+ Paths.get(canonicalFile.getAbsolutePath()))
+ .toFile();
+
+ // Check the target is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
Review Comment:
assertNotNull
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java:
##########
@@ -107,12 +110,55 @@ void testEmptyFileListAllowNull() {
@Test
void testFileDoesNotExist() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
+ rule.setFilesList(Collections.singletonList(f));
+
+ // Check the file is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assertNotNull(e.getMessage());
+
f.delete();
assertFalse(f.exists());
Review Comment:
The wrong one was changed. Line 122 should be changed or better yet deleted.
The point of the test is not whether f.delete() succeeds. You're not testing
the delete method in java.io.File.
##########
enforcer-rules/src/site/apt/requireFilesDontExist.apt.vm:
##########
@@ -26,7 +26,10 @@
Require Files Don't Exist
This rule checks that the specified list of files do not exist.
-
+
+ The mounted filesystem(s) will dictate the case-sensitivity rules when
checking files. If checks are required that
Review Comment:
delete "mounted" and consider getting rid of the parentheses or just
changing this to "The filesystem" as is every file is on a single filesystem.
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java:
##########
@@ -105,19 +109,62 @@ void testEmptyFileListAllowNull() {
}
@Test
- void testFileDoesNotExist() throws EnforcerRuleException, IOException {
+ void testDeletedFileDetected() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
+ rule.setFilesList(Collections.singletonList(f));
+
+ // Check the file is detected as being present
+ EnforcerRuleException e = assertThrows(EnforcerRuleException.class,
rule::execute);
+ assumeFalse(e.getMessage() == null);
Review Comment:
This should be an assertNotNull
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesExist.java:
##########
@@ -50,12 +56,34 @@ void testFileExists() throws Exception {
}
@Test
- void testFileOsIndependentExists() {
- rule.setFilesList(Collections.singletonList(new File("POM.xml")));
+ void testSymbolicLinkExists() throws Exception {
+ File canonicalFile = File.createTempFile("canonical_", null,
temporaryFolder);
+ File linkFile = Files.createSymbolicLink(
+ Paths.get(temporaryFolder.getAbsolutePath(),
"symbolic.link"),
+ Paths.get(canonicalFile.getAbsolutePath()))
+ .toFile();
- EnforcerRuleException e = assertThrows(EnforcerRuleException.class, ()
-> rule.execute());
+ rule.setFilesList(Collections.singletonList(linkFile));
- assertNotNull(e.getMessage());
+ rule.execute();
+ }
+
+ @Test
+ void testSymbolicLinkTargetDoesNotExist() throws Exception {
+ File canonicalFile = File.createTempFile("canonical_", null,
temporaryFolder);
+ File linkFile = Files.createSymbolicLink(
+ Paths.get(temporaryFolder.getAbsolutePath(),
"symbolic.link"),
+ Paths.get(canonicalFile.getAbsolutePath()))
+ .toFile();
+ canonicalFile.delete();
+ rule.setFilesList(Collections.singletonList(linkFile));
+
+ try {
+ rule.execute();
+ fail("Should have received an exception");
+ } catch (Exception e) {
Review Comment:
which exception should be thrown here? Be specific.
##########
enforcer-rules/src/site/apt/requireFilesSize.apt.vm:
##########
@@ -27,26 +27,29 @@ Require File Size
This rule checks that the specified list of files exist and are within the
specified size range.
+ The mounted filesystem(s) will dictate the case-sensitivity rules when
checking files. If you require checks that your
Review Comment:
ditto
> "requireFilesExist" no longer handles non-canonical paths
> ---------------------------------------------------------
>
> Key: MENFORCER-390
> URL: https://issues.apache.org/jira/browse/MENFORCER-390
> Project: Maven Enforcer Plugin
> Issue Type: Bug
> Components: requireFilesExist, Standard Rules
> Affects Versions: 3.0.0
> Reporter: Gene Smith
> Priority: Major
>
> With the commit to resolve MENFORCER-364, the rule "requireFileExists" checks
> that the canonical path of a file is the same as the absolute path.
> But not all absolute paths are canonical.
> * absolute paths can involve symbolic links
> * and they are allowed to have parts which are relative
> ** {{/../}}
> ** {{/./}}
> And when it fails to handle a path, it can report that a file does not exist,
> even though the local system will resolve the path.
> A blunt solution might be three separate rules:
> * requireFileExists
> ** the provided path must resolve to a file (which may be a directory or
> link)
> * requireCanonicalFileExists
> ** the provided path must exist as a canonical file
> * requireCasesenstiveFileExists
> ** the provided path must file a file
> ** the file name must have the same case (upper//lower) as the
> ** the parts of the path from the file up must have the same case until they
> go through a symbolic link
> I have used the "nio" package to handle some of stuff before. I will add a
> comment with some java code I would start with. Since the outcome here is
> very dependent on the use case you pick, the java will be "meta code" with
> ??? where you have to know the use case to know the outcome.
> but basically, with "nio" you can march up a path checking for symbolic links
> and such.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
