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 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 || file.exists();
}

@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.
Copy link
Contributor

Choose a reason for hiding this comment

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

delete "list of"

Copy link
Author

Choose a reason for hiding this comment

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

Done



The mounted filesystem(s) will dictate the case-sensitivity rules when checking files. If checks are required that
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

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
19 changes: 11 additions & 8 deletions enforcer-rules/src/site/apt/requireFilesExist.apt.vm
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
~~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
~~ KIND, either express or implied. See the License for the
~~ specific language governing permissions and limitations
~~ under the License.
~~ under the License.

------
Require Files Exist
------
Expand All @@ -27,22 +27,25 @@ Require Files Exist

This rule checks that the specified list of files exist.
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.

Done


The mounted filesystem(s) will dictate the case-sensitivity 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.

The following parameters are supported by this rule:

* <<message>> - an optional message to the user if the rule fails.

* <<files>> - A list of files to check.

* <<allowNulls>> - If null files should be allowed. If allowed, they will be treated as if they do exist. Default is false.

* <<satisfyAny>> - Allows that one of files can make the rule pass, instead of all the files. Default is false.

[]


Sample Plugin Configuration:

+---+
<project>
[...]
Expand Down
23 changes: 13 additions & 10 deletions enforcer-rules/src/site/apt/requireFilesSize.apt.vm
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
~~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
~~ KIND, either express or implied. See the License for the
~~ specific language governing permissions and limitations
~~ under the License.
~~ under the License.

------
Require File Size
------
Expand All @@ -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
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.

Done

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:

* <<message>> - an optional message to the user if the rule fails.

* <<files>> - A list of files to check. If this list is empty, the main project artifact will be checked.

* <<maxsize>> - maximum size in bytes for this file.

* <<minsize>> - minimum size in bytes for this file.

* <<allowNulls>> - If null files should be allowed. If allowed, they will be treated as if they do exist. Default is false.

* <<satisfyAny>> - Allows that one of files can make the rule pass, instead of all the files. Default is false.

[]


Sample Plugin Configuration:

+---+
<project>
[...]
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,8 +31,10 @@

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;
import static org.junit.jupiter.api.Assumptions.assumeFalse;

/**
* Test the "require files don't exist" rule.
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an assertNotNull

Copy link
Author

Choose a reason for hiding this comment

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

Akready sorted based on earlier comment


f.delete();

assertFalse(f.exists());
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 my confusion is that this is the only assert. It should be changed to an assumeFalse or removed.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

assertNotNull

Copy link
Author

Choose a reason for hiding this comment

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

Done


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

assertNotNull

Copy link
Author

Choose a reason for hiding this comment

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

Done


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

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

@Test
void testFileDoesNotExistSatisfyAny() throws EnforcerRuleException, IOException {
void testDeletedFileDetectedSatisfyAny() throws EnforcerRuleException, IOException {
File f = File.createTempFile("junit", null, temporaryFolder);
f.delete();

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,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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

which exception should be thrown here? Be specific.

Copy link
Author

Choose a reason for hiding this comment

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

Done

assertNotNull(e.getMessage());
}
}

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

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

/**
* 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