[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

  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

  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

Done. Apols.

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

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

  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

Done. Apols.

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

    }

    @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.

yes, renaming is a good idea

Please give me a format of the new method name you'd prefer and I will ensure that all test...Exists and test...DoesNotExist methods are updated in all test classes.
Such a change will affect TestRequireFilesSize, TestRequireFileChecksum TestRequireFilesDontExist, and TestRequireFilesExist; this means touching classes that are unrelated to the bug fix in order to ensure naming consistency.

To be clear, I disagree with this but it is not my project and so I will follow your direction.

Don't change other methods not related to this PR. keep the PR focused.

But that means this TestRequireFilesDontExist class will be following a different naming convention to the other classes. I don't follow why that is a good idea, I merely followed the convention that was already in place.

Better 9 wrong and 1 right than 10 wrong. That's the fundamental principle.

What should the new names be?

testFileDoesNotExist -> testDeletedFileDetected

testSymbolicLinkDoesNotExist -> testSymbolicLinDeletedDetected

testSymbolicLinkTargetDoesNotExist -> testSymbolicLinkTargetDeletedDetected

testFileDoesNotExistSatisfyAny -> testDeletedFileDetectedSatisfyAny

testSymbolicLinkDeletedDetected (spelling) but otherwise sounds good

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

    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

Old habits die hard. :-)

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

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

  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

  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

elharo · 2025-01-10T12:51:13Z

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

+
        f.delete();

        assertFalse(f.exists());


I think my confusion is that this is the only assert. It should be changed to an assumeFalse or removed.

This is a quick negative check to ensure the file is detected and the test does not pass by fluke. It is consistent with checks in the other tests (throws an exception with a message).

And I am sorry, but I do no understand how assumeFalse is helpful here. If the file is not detected as being present and an exception thrown with a message, then that should be a hard failure as the rule is not working according to spec.

I can see a mix of checkin/not-checking for an unexpected exception, I can add and explicit check for one and add a fail if you would like? I don't think it would be appropriate to update every test though, just one directly required by this fix.

Test setup failure does not mean the test failed. It means the test wasn't run. That's the difference between assume and assert. JUnit reports these two cases differently.

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.

elharo · 2025-01-10T12:51:31Z

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

    }

    @Test
    void testFileDoesNotExist() throws EnforcerRuleException, IOException {


yes, renaming is a good idea

elharo · 2025-01-31T17:31:39Z

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


 Require Files Don't Exist

  This rule checks that the specified list of files do not exist.


delete "list of"

elharo · 2025-01-31T17:32:47Z

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

  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


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.

elharo · 2025-01-31T17:33:00Z

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


 Require Files Exist

  This rule checks that the specified list of files exist.


elharo · 2025-01-31T17:33:18Z

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


  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


elharo · 2025-01-31T17:34:18Z

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

+
+        // Check the file is detected as being present
+        EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute);
+        assumeFalse(e.getMessage() == null);


This should be an assertNotNull

Akready sorted based on earlier comment

elharo · 2025-01-31T17:34:55Z

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

+
        f.delete();

        assertFalse(f.exists());


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.

elharo · 2025-01-31T17:35:47Z

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

+        rule.setFilesList(Collections.singletonList(linkFile));
+        // Check the link is detected as being present
+        EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute);
+        assumeFalse(e.getMessage() == null);


assertNotNull

elharo · 2025-01-31T17:36:00Z

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

+
+        // Check the target is detected as being present
+        EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute);
+        assumeFalse(e.getMessage() == null);


assertNotNull

elharo · 2025-01-31T17:36:38Z

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

+        try {
+            rule.execute();
+            fail("Should have received an exception");
+        } catch (Exception e) {


which exception should be thrown here? Be specific.

jira-importer · 2025-06-14T14:13:25Z

Resolve #501

jira-importer · 2025-06-14T14:29:10Z

Resolve #501

github-actions · 2025-08-17T04:43:35Z

This pull request is stale because it has been waiting for feedback for 60 days. Remove the stale label or comment on this PR, or it will be automatically closed in 30 days.

github-actions · 2025-09-17T04:37:11Z

This pull request has been closed because no response was received within 90 days.

jemischi · 2026-01-13T03:13:54Z

Please finish this patch-request. :)

roadSurfer · 2026-01-13T06:47:07Z

Please finish this patch-request. :)

As far as I am aware, I made good all the required changes. I do not believe it is for me to mark things as "Resolved" but for the person who raised the issue or another team member.

@slawekjaranowski - Who are you awaiting feedback from? I do not believe it to be me.

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

roadSurfer added 2 commits January 10, 2025 09:35

Merge branch 'apache:master' into MENFORCER-390

e6baf8f

Further updates per PR comments

14449e1

elharo reviewed Jan 10, 2025

View reviewed changes

roadSurfer added 2 commits January 31, 2025 15:15

Merge branch 'apache:master' into MENFORCER-390

b164816

Further updates per PR comments

61211d9

elharo reviewed Jan 31, 2025

View reviewed changes

roadSurfer added 2 commits February 3, 2025 16:49

Further updates per PR comments

4c62cb6

Further updates per PR comments

86dc7ee

slawekjaranowski added the waiting-for-feedback Waiting for 90 days until issues or pull request will be closed label Jun 17, 2025

github-actions bot added the Stale label Aug 17, 2025

github-actions bot closed this Sep 17, 2025

github-actions bot removed waiting-for-feedback Waiting for 90 days until issues or pull request will be closed Stale labels Jan 13, 2026

elharo reopened this Jan 13, 2026

		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



		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


		Require Files Don't Exist

		This rule checks that the specified list of files do not exist.


		Require Files Exist

		This rule checks that the specified list of files exist.

[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

Uh oh!

Conversation

roadSurfer commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slawekjaranowski commented Nov 24, 2023

Uh oh!

roadSurfer commented Nov 27, 2023

Uh oh!

Torbjorn-Svensson commented Nov 27, 2023

Uh oh!

roadSurfer commented Nov 27, 2023 via email

Uh oh!

roadSurfer commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roadSurfer Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roadSurfer Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roadSurfer commented Nov 24, 2023 •

edited

Loading

roadSurfer Jan 9, 2025 •

edited

Loading

roadSurfer Jan 29, 2025 •

edited

Loading