Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import javax.inject.Named;

import java.io.File;
import java.io.IOException;

/**
* The Class RequireFilesExist.
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO

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

is clearer, but up to you

}

@Override
String getErrorMsg() {
return "Some required files are missing:" + System.lineSeparator();
}

/**
* OSes like Windows are case insensitive, so this method will compare the file path with the actual path. A simple
* {@link File#exists()} is not enough for such OS.
*
* @param file the file to verify
* @param defaultValue value to return in case an IO exception occurs, should never happen as the file already
* exists
* @return
*/
private boolean osIndependentNameMatch(File file, boolean defaultValue) {
try {
File absFile;
if (!file.isAbsolute()) {
absFile = new File(new File(".").getCanonicalFile(), file.getPath());
} else {
absFile = file;
}

return absFile.toURI().equals(absFile.getCanonicalFile().toURI());
} catch (IOException e) {
return defaultValue;
}
}
}
5 changes: 4 additions & 1 deletion enforcer-rules/src/site/apt/requireFilesDontExist.apt.vm
Original file line number Diff line number Diff line change
Expand Up @@ -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
filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) consider adding a custom
plugin that meets the specific needs.

The following parameters are supported by this rule:

Expand Down
5 changes: 4 additions & 1 deletion enforcer-rules/src/site/apt/requireFilesExist.apt.vm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case-sensitivity

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.

The following parameters are supported by this rule:

Expand Down
5 changes: 4 additions & 1 deletion enforcer-rules/src/site/apt/requireFilesSize.apt.vm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case-sensitivity

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.

The following parameters are supported by this rule:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;

Expand All @@ -29,6 +31,7 @@

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

Expand Down Expand Up @@ -107,15 +110,69 @@ void testEmptyFileListAllowNull() {
@Test
void testFileDoesNotExist() throws EnforcerRuleException, IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@roadSurfer roadSurfer Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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());

rule.setFilesList(Collections.singletonList(f));

// Rule should now pass as the file was deleted
rule.execute();
}

@Test
void testSymbolicLinkDoesNotExist() throws Exception {
File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder);
File linkFile = Files.createSymbolicLink(
Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"),
Paths.get(canonicalFile.getAbsolutePath()))
.toFile();

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

linkFile.delete();

// Rule should now pass as the link was deleted
rule.execute();
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use try with resources for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the temporary folder rule should take care of that

if (linkFile.exists()) {
linkFile.delete();
}
canonicalFile.delete();
}
}

@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();

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

canonicalFile.delete();
rule.setFilesList(Collections.singletonList(linkFile));

try {
// Rule should now pass as the target was deleted
rule.execute();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no exception is a pass?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} finally {
linkFile.delete();
}
}

@Test
void testFileDoesNotExistSatisfyAny() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,20 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;

import org.apache.maven.enforcer.rule.api.EnforcerRuleException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

/**
* Test the "require files exist" rule.
Expand All @@ -50,12 +56,40 @@ void testFileExists() throws Exception {
}

@Test
void testFileOsIndependentExists() {
rule.setFilesList(Collections.singletonList(new File("POM.xml")));

EnforcerRuleException e = assertThrows(EnforcerRuleException.class, () -> rule.execute());
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();
rule.setFilesList(Collections.singletonList(linkFile));

try {
rule.execute();
} finally {
linkFile.delete();
canonicalFile.delete();
}
}

assertNotNull(e.getMessage());
@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 get an exception");
} catch (Exception e) {
assertNotNull(e.getMessage());
} finally {
linkFile.delete();
}
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;

Expand Down Expand Up @@ -180,6 +182,52 @@ void testRequireFilesSizeSatisfyAny() throws EnforcerRuleException, IOException
rule.execute();
}

@Test
void testSymbolicLinkTooSmall() 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(Arrays.asList(linkFile));
rule.setMinsize(10);
try {
rule.execute();
fail("Should get exception");
} catch (EnforcerRuleException e) {
assertNotNull(e.getMessage());
} finally {
linkFile.delete();
canonicalFile.delete();
}
}

@Test
void testSymbolicLinkTooBig() throws Exception {
File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder);
try (BufferedWriter out = new BufferedWriter(new FileWriter(canonicalFile))) {
out.write("123456789101112131415");
}
assertTrue(canonicalFile.length() > 10);
File linkFile = Files.createSymbolicLink(
Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"),
Paths.get(canonicalFile.getAbsolutePath()))
.toFile();

rule.setFilesList(Arrays.asList(linkFile));
rule.setMaxsize(10);
try {
rule.execute();
fail("Should get exception");
} catch (EnforcerRuleException e) {
assertNotNull(e.getMessage());
} finally {
linkFile.delete();
canonicalFile.delete();
}
}

/**
* Test id.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ under the License.
<requireFilesExist>
<files>
<file>pom.xml</file>
<file>../require-files-exist/pom.xml</file>
<file>${project.basedir}/../require-files-exist/pom.xml</file>
</files>
</requireFilesExist>
</rules>
Expand Down
Loading