[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297

roadSurfer · 2023-11-24T15:47:31Z

This reverts the change from MENFORCER-364 as this led to a regressions with symbolic links.

The fundamental issue is that there is no clean way to deal with case-sensitivity as OSs can have multuple filesystems mounted that follow different rules. Thus the simple file.exists() is, despite the limitations, probably best. Those requiring more stringent checks writing their own handling.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MENFORCER-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MENFORCER-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

…and did not adequately deal with mixed filesystems.

slawekjaranowski · 2023-11-24T18:21:08Z

Ok, the simple file.exists() should be enough in normal usage ...

I only think - if we don't break and special cases ... but agree more complicated check should be done optionally or by next rule

By the way we can mention in documentation a way how file existence is checked to be clear.

roadSurfer · 2023-11-27T10:44:57Z

I have added some words to the affected documentation on case-sensitivity, as well as some explic testing of symbolic links.

Torbjorn-Svensson · 2023-11-27T15:20:30Z

Are all the white space changes in the .md files intended?
As far as I know, some of the trailing white spaces are needed in order to not join the lines.

roadSurfer · 2023-11-27T15:53:21Z

No, they are not intended. That will be my IDE doing its own thing, I'll use a different editor and update.

…

On Mon, 27 Nov 2023 at 15:20, Torbjorn-Svensson ***@***.***> wrote: Are all the white space changes in the .md files intended? As far as I know, some of the trailing white spaces are needed in order to not join the lines. — Reply to this email directly, view it on GitHub <#297 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJHMQU65AH2DOVYZVDQKTUTYGSVUVAVCNFSM6AAAAAA7ZJWO7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYGA2DCMRXGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

roadSurfer · 2023-11-27T16:44:21Z

That should be the whitepsace issues sorted. I also generated the site locally and it appeared OK to me.

elharo · 2025-01-08T14:02:42Z

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-sensitive rules when checking files. If you require checks that your


case-sensitivity

elharo · 2025-01-08T14:03:00Z

enforcer-rules/src/site/apt/requireFilesDontExist.apt.vm

-
+
+  The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your
+  filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider


delete "then you should"

elharo · 2025-01-08T14:03:21Z

enforcer-rules/src/site/apt/requireFilesDontExist.apt.vm

+
+  The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your
+  filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider
+  adding your own custom plugin that meets your specific needs.


your own --> a

elharo · 2025-01-08T14:03:31Z

enforcer-rules/src/site/apt/requireFilesExist.apt.vm

@@ -26,7 +26,10 @@
 Require Files Exist

  This rule checks that the specified list of files exist.
-
+
+  The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your


case-sensitivity

elharo · 2025-01-08T14:03:37Z

enforcer-rules/src/site/apt/requireFilesSize.apt.vm

@@ -26,7 +26,10 @@
 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-sensitive rules when checking files. If you require checks that your


case-sensitivity

elharo · 2025-01-08T14:03:52Z

...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java

-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.*;


project style does not use wildcard imports

Updated, and in TestRequireFilesExist as well

elharo · 2025-01-08T14:05:13Z

...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java

@@ -107,15 +106,58 @@ void testEmptyFileListAllowNull() {
    @Test
    void testFileDoesNotExist() throws EnforcerRuleException, IOException {


I don't understand this test. It checks that we create a file, delete it, and then that the file doesn't exist? Consider renamingi

This seems to just be a minor variation on the original test code, but I can change the name.
What do you think "testDeletedFileDetected"?
I'll update the symbolic link test to follow a similar format.

There is also "testFileDoesNotExistSatisfyAny", which is original code.

elharo · 2025-01-08T14:06:03Z

...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java

+
+            linkFile.delete();
+            rule.execute();
+        } finally {


can we use try with resources for this?

I don;'t think so, isn't that just for autoclosables?

Looking at other tests, it does not seem to be project standard to ensure tests delete files they create; I could just remove the try block entirely in that case.

I think the temporary folder rule should take care of that

elharo · 2025-01-08T14:06:47Z

...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java

+        rule.setFilesList(Collections.singletonList(linkFile));
+
+        try {
+            rule.execute();


no exception is a pass?

Correct. This whole class it basically the inverse of TestRequireFilesExist.

elharo · 2025-01-09T11:40:55Z

enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/files/RequireFilesExist.java

@@ -31,35 +30,11 @@ public final class RequireFilesExist extends AbstractRequireFiles {
    @Override
    boolean checkFile(File file) {
        // if we get here and the handle is null, treat it as a success
-        return file == null ? true : file.exists() && osIndependentNameMatch(file, true);
+        return file == null ? true : file.exists();


IMHO

return file == null || file.exists()

is clearer, but up to you

elharo · 2025-01-09T13:38:16Z

enforcer-rules/src/site/apt/requireFilesExist.apt.vm

@@ -26,7 +26,10 @@
 Require Files Exist

  This rule checks that the specified list of files exist.
-
+
+  The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your


case-sensitivity

elharo · 2025-01-09T13:38:21Z

enforcer-rules/src/site/apt/requireFilesSize.apt.vm

@@ -26,7 +26,10 @@
 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-sensitive rules when checking files. If you require checks that your


case-sensitivity

elharo · 2025-01-09T13:39:49Z

...cer-rules/src/test/java/org/apache/maven/enforcer/rules/files/TestRequireFilesDontExist.java

+
+            linkFile.delete();
+            rule.execute();
+        } finally {


I think the temporary folder rule should take care of that

roadSurfer added 2 commits November 24, 2023 15:38

Reverting MENFORCER-364 as the change was only partially implemented …

4568135

…and did not adequately deal with mixed filesystems.

Tidy-up from spotless. No functional changes.

c7ca861

slawekjaranowski and others added 3 commits November 24, 2023 19:57

Extend IT with example from issue

81b8027

Updated documentation with statement on filesystem case-sensitivity

0b45664

Added explicit testing of symbolic links

326ff7f

roadSurfer added 2 commits November 27, 2023 16:42

Maven Spotless fixes, no functional changes

b82306d

Fixed spurious whitespace changes

01570d4

elharo reviewed Jan 8, 2025

View reviewed changes

roadSurfer added 3 commits January 9, 2025 09:21

Merge branch 'master' into MENFORCER-390

e6cd78c

Language changes as requested

11dbb95

Code changes per PR comments

e0d8be0

elharo reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297

[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297

roadSurfer commented Nov 24, 2023 •

edited

Loading

slawekjaranowski commented Nov 24, 2023

roadSurfer commented Nov 27, 2023

Torbjorn-Svensson commented Nov 27, 2023

roadSurfer commented Nov 27, 2023 via email

roadSurfer commented Nov 27, 2023

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025 •

edited

Loading

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

roadSurfer Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 8, 2025

roadSurfer Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 9, 2025

elharo Jan 9, 2025



		The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your
		filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider

		@@ -107,15 +106,58 @@ void testEmptyFileListAllowNull() {
		@Test
		void testFileDoesNotExist() throws EnforcerRuleException, IOException {

[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297

Are you sure you want to change the base?

[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297

Conversation

roadSurfer commented Nov 24, 2023 • edited Loading

slawekjaranowski commented Nov 24, 2023

roadSurfer commented Nov 27, 2023

Torbjorn-Svensson commented Nov 27, 2023

roadSurfer commented Nov 27, 2023 via email

roadSurfer commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roadSurfer Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roadSurfer commented Nov 24, 2023 •

edited

Loading

roadSurfer Jan 9, 2025 •

edited

Loading