Merge pull request #76 from cxcorp/zipper-path-separators

Improve StudentFileAwareZipper

Author: nygrenh

tmc-langs-framework/src/main/java/fi/helsinki/cs/tmc/langs/io/zip/StudentFileAwareZipper.java

@@ -16,31 +16,93 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 
+/**
+ * This {@link Zipper} implementation recursively zips the files of a directory.
+ * Only files considered to be student files by the specified {@link StudentFilePolicy}
+ * are included in the archive.
+ *
+ * <p>The {@link StudentFilePolicy} provided either via the
+ * {@link #StudentFileAwareZipper(StudentFilePolicy) constructor} or a
+ * {@link #setStudentFilePolicy(StudentFilePolicy) setter method} is used to determine whether
+ * a file or directory should be included in the archive.</p>
+ *
+ * <p>Individual directories can be excluded from archival by adding a file in the directory
+ * root with the name of {@code .tmcnosubmit}. For example, if the folder {@code sensitive/}
+ * should be excluded, the file {@code sensitive/.tmcnosubmit} should be created. The contents
+ * of the file are ignored.</p>
+ *
+ * <p>File system roots (e.g. {@code /} on *nix and {@code C:\} on Windows platforms) cannot
+ * be zipped, as this {@link Zipper} includes the specified {@code rootDirectory}
+ * in the zip file as a parent directory.</p>
+ */
 public final class StudentFileAwareZipper implements Zipper {
 
     private static final Logger log = LoggerFactory.getLogger(StudentFileAwareZipper.class);
+    // The zip standard mandates the forward slash "/" to be used as path separator
+    private static final char ZIP_SEPARATOR = '/';
+
     private StudentFilePolicy filePolicy;
 
+    /**
+     * Instantiates a new {@link StudentFileAwareZipper} without a {@link StudentFilePolicy}.
+     * The {@link #setStudentFilePolicy(StudentFilePolicy)} method can be used to set the policy.
+     */
     public StudentFileAwareZipper() {}
 
+    /**
+     * Instantiates a new {@link StudentFileAwareZipper} with the
+     * specified {@link StudentFilePolicy}.
+     *
+     * @param filePolicy Determines which files and directories are included in the archive.
+     * @see #setStudentFilePolicy(StudentFilePolicy)
+     */
     public StudentFileAwareZipper(StudentFilePolicy filePolicy) {
         this.filePolicy = filePolicy;
     }
 
+    /**
+     * Sets the {@link StudentFilePolicy} which determines which files and directories
+     * are included in the archive.
+     *
+     * @param studentFilePolicy Determines which files and directories are included in the archive.
+     * @see #StudentFileAwareZipper(StudentFilePolicy)
+     */
     @Override
     public void setStudentFilePolicy(StudentFilePolicy studentFilePolicy) {
         this.filePolicy = studentFilePolicy;
     }
 
+    /**
+     * Recursively zips all files and directories which are considered to be student files.
+     *
+     * @param rootDirectory The root directory of the files and directories to zip.
+     *                      Included in the archive. Cannot be a file system root.
+     * @return Byte array containing the bytes of the {@link ZipArchiveOutputStream}.
+     * @throws IOException if reading a file or directory fails.
+     * @throws IllegalArgumentException if attempting to zip a file system root.
+     * @see #setStudentFilePolicy(StudentFilePolicy)
+     */
     @Override
     public byte[] zip(Path rootDirectory) throws IOException {
         log.debug("Starting to zip {}", rootDirectory);
 
         if (!Files.exists(rootDirectory)) {
-            log.error("Attempted to zip nonexistent directory {}", rootDirectory);
+            log.error("Attempted to zip nonexistent directory \"{}\"", rootDirectory);
             throw new FileNotFoundException("Attempted to zip nonexistent directory");
         }
 
+        if (rootDirectory.toAbsolutePath().getNameCount() == 0) {
+            // getNameCount returns 0 if the path only represents a root component
+            log.error("Attempted to zip a root \"{}\"", rootDirectory);
+            throw new IllegalArgumentException("Filesystem root zipping is not supported");
+        }
+
+        if (filePolicy == null) {
+            log.error("Attepted to zip before setting the filePolicy");
+            throw new IllegalStateException(
+                    "The student file policy must be set before zipping files");
+        }
+
         ByteArrayOutputStream buffer = new ByteArrayOutputStream();
         try (ZipArchiveOutputStream zipStream = new ZipArchiveOutputStream(buffer)) {
             zipRecursively(rootDirectory, zipStream, rootDirectory);
@@ -109,13 +171,8 @@ private void writeToZip(Path currentPath, ZipArchiveOutputStream zipStream, Path
 
         log.trace("Writing {} to zip", currentPath);
 
-        String name = projectPath.getParent().relativize(currentPath).toString();
-
-        if (Files.isDirectory(currentPath)) {
-            log.trace("{} is a directory", currentPath);
-            // Must be "/", can not be replaces with File.separator
-            name += "/";
-        }
+        Path relativePath = projectPath.getParent().relativize(currentPath);
+        String name = relativePathToZipCompliantName(relativePath, Files.isDirectory(currentPath));
 
         ZipArchiveEntry entry = new ZipArchiveEntry(name);
         zipStream.putArchiveEntry(entry);
@@ -129,4 +186,25 @@ private void writeToZip(Path currentPath, ZipArchiveOutputStream zipStream, Path
         log.trace("Closing entry");
         zipStream.closeArchiveEntry();
     }
+
+    private static String relativePathToZipCompliantName(Path path, boolean isDirectory) {
+        log.trace("Generating zip-compliant filename from Path \"{}\", isDirectory: {}",
+                path, isDirectory);
+
+        StringBuilder sb = new StringBuilder();
+        for (Path part : path) {
+            sb.append(part);
+            sb.append(ZIP_SEPARATOR);
+        }
+
+        if (!isDirectory) {
+            // ZipArchiveEntry assumes the entry represents a directory if and only
+            // if the name ends with a forward slash "/". Remove the trailing slash
+            // because this wasn't a directory.
+            log.trace("Path wasn't a directory, removing trailing slash");
+            sb.deleteCharAt(sb.length() - 1);
+        }
+
+        return sb.toString();
+    }
 }

tmc-langs-framework/src/test/java/fi/helsinki/cs/tmc/langs/io/zip/StudentFileAwareZipperTest.java

@@ -5,6 +5,7 @@
 import static org.junit.Assert.fail;
 
 import fi.helsinki.cs.tmc.langs.io.EverythingIsStudentFileStudentFilePolicy;
+import fi.helsinki.cs.tmc.langs.io.StudentFilePolicy;
 import fi.helsinki.cs.tmc.langs.utils.TestUtils;
 
 import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
@@ -20,6 +21,7 @@
 import java.nio.file.FileVisitor;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Enumeration;
 import java.util.HashMap;
@@ -77,10 +79,23 @@ public FileVisitResult postVisitDirectory(Path path, IOException ex)
     }
 
     @Test(expected = FileNotFoundException.class)
-    public void zipperThrowsExceptionWhenUnzippingNonExistentFile() throws IOException {
+    public void zipperThrowsExceptionWhenZippingNonExistentFile() throws IOException {
         zipper.zip(TEST_ASSETS_DIR.resolve("noSuchDir"));
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void zipperThrowsExceptionWhenZippingRoot() throws IOException {
+        // platform-specific root
+        zipper.zip(Paths.get("/").toAbsolutePath());
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void zipperThrowsExceptionWhenZippingWithoutSettingPolicy() throws IOException {
+        Path existingPath = TestUtils.getPath(StudentFileAwareUnzipperTest.class,
+                "tmcnosubmit_test_case");
+        new StudentFileAwareZipper().zip(existingPath);
+    }
+
     @Test
     public void zipperCorrectlyZipsSingleFile() throws IOException {
 
@@ -100,7 +115,7 @@ public void zipperCorrectlyZipsSingleFile() throws IOException {
     @Test
     public void zipperCorrectlyZipsFolderWithFilesAndSubFolders() throws IOException {
         // Create empty dir that is not in git
-        Path emptyDir = (TEST_DIR.resolve("dir"));
+        Path emptyDir = TEST_DIR.resolve("dir");
         if (Files.notExists(emptyDir)) {
             Files.createDirectory(emptyDir);
         }
@@ -137,7 +152,43 @@ public void zipperDetectectsAndObeysTmcnosubmitFiles() throws IOException {
         expected.close();
         actual.close();
         Files.deleteIfExists(compressed);
+    }
+
+    @Test
+    public void zipperFollowsStudentPolicy() throws IOException {
+        Path uncompressed = TestUtils.getPath(StudentFileAwareUnzipperTest.class,
+                "zip_studentpolicy_test_case");
+
+        // Policy: zip every directory and file whose name starts with "include"
+        zipper.setStudentFilePolicy(new StudentFilePolicy() {
+            @Override
+            public boolean isStudentFile(Path path, Path projectRootPath) {
+                if (path.equals(projectRootPath)) {
+                    return true;
+                }
+                return path.getFileName().toString().startsWith("include");
+            }
+
+            @Override
+            public boolean mayDelete(Path file, Path projectRoot) {
+                return true;
+            }
+        });
+
+        byte[] zip = zipper.zip(uncompressed);
+        Path compressed = Files.createTempFile("testZip", ".zip");
+        Files.write(compressed, zip);
+
+        Path referenceZip = TEST_ASSETS_DIR.resolve("zip_studentpolicy_test_case.zip");
 
+        ZipFile expected = new ZipFile(referenceZip.toFile());
+        ZipFile actual = new ZipFile(compressed.toFile());
+
+        assertZipsEqualDecompressed(expected, actual);
+
+        expected.close();
+        actual.close();
+        Files.deleteIfExists(compressed);
     }
 
     private void assertZipsEqualDecompressed(ZipFile expected, ZipFile actual)

tmc-langs-framework/src/test/resources/zipTestResources/zip_studentpolicy_test_case.zip

@@ -0,0 +1 @@
+liirum laarum

tmc-langs-framework/src/test/resources/zip_studentpolicy_test_case/file.txt

@@ -0,0 +1 @@
+Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec venenatis ac ante id egestas. Praesent at enim id lorem egestas iaculis eu pharetra dolor. Nullam ullamcorper laoreet massa, sit amet sodales ex pharetra vitae. Aliquam id vehicula nunc, bibendum mattis orci. Nunc fringilla sem enim, a suscipit justo convallis id. Etiam maximus urna lorem, ac maximus nulla sagittis nec. Curabitur nec mauris finibus, fringilla mauris eu, elementum neque. Sed et sem tellus. Suspendisse eget ipsum vel odio mattis venenatis. Morbi egestas elementum felis eu aliquet.

tmc-langs-framework/src/test/resources/zip_studentpolicy_test_case/include_laarum/Sit.txt

@@ -0,0 +1 @@
+Suspendisse tincidunt vehicula sem. Cras sit amet enim nec arcu euismod tincidunt nec sed eros. Curabitur tempor enim sit amet accumsan dictum. Fusce tempus lorem eget odio imperdiet fringilla. Proin lobortis consequat finibus. Praesent in nunc metus. Fusce hendrerit lacus id orci vulputate, vitae consequat velit tempor. Sed iaculis ac eros id sollicitudin. Proin vitae feugiat justo. Nunc ac diam aliquet, lacinia nibh eget, commodo nibh. Mauris ipsum leo, finibus non risus non, egestas semper nulla. Sed sit amet maximus tortor, a rhoncus neque. Morbi eu venenatis eros. Etiam imperdiet viverra vestibulum. Mauris tristique rhoncus ante et porta. Etiam consequat a orci eu scelerisque.

tmc-langs-framework/src/test/resources/zip_studentpolicy_test_case/include_laarum/include_Dolor.txt

@@ -0,0 +1 @@
+In iaculis quam eu accumsan condimentum. Proin molestie lectus magna, ac rutrum ex elementum eget. Duis tincidunt ante ex. Suspendisse aliquam venenatis aliquam. Nullam viverra consequat tincidunt. Praesent ut odio ac arcu consequat hendrerit. Proin rutrum sapien convallis accumsan pharetra.

tmc-langs-framework/src/test/resources/zip_studentpolicy_test_case/include_laarum/include_Ipsum.txt

@@ -0,0 +1,2 @@
+Lorem;Ipsum
+Yes;No