MFileZip fixes (#1345)

* Simplify code * Fix length and last modified for root zip file and add unit tests for this * Add isZipFile to MFile, to distinguish between directories and zip doriectories * Handle MFileZip writeToStream as MFileOS would * Allow zip extension to be case insensive * Close zip files in tests * Fix expected zip file length in tests * Rename variables and add name to temporary file * Add test for writeToStream with offset and size
Unidata · May 23, 2024 · fabaec0 · fabaec0
1 parent 004f75d
commit fabaec0
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 22 deletions.
diff --git a/cdm/core/src/main/java/thredds/inventory/MFile.java b/cdm/core/src/main/java/thredds/inventory/MFile.java
@@ -35,6 +35,15 @@ public interface MFile extends Comparable<MFile> {
 
   boolean isDirectory();
 
+  /**
+   * Check if this MFile is a zip file
+   * 
+   * @return true if the MFile is a zip file
+   */
+  default boolean isZipFile() {
+    return false;
+  }
+
   default boolean isReadable() {
     return true;
   }

diff --git a/cdm/zarr/src/main/java/thredds/inventory/zarr/MFileZip.java b/cdm/zarr/src/main/java/thredds/inventory/zarr/MFileZip.java
@@ -7,6 +7,7 @@
 
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.Locale;
 import javax.annotation.Nonnull;
 import thredds.filesystem.MFileOS;
 import thredds.inventory.MFile;
@@ -22,6 +23,7 @@
 import java.util.zip.ZipFile;
 import java.util.zip.ZipInputStream;
 import ucar.nc2.util.IO;
+import ucar.unidata.io.RandomAccessFile;
 
 /**
  * Implements thredds.inventory.MFile for ZipFiles and ZipEntries
@@ -101,23 +103,28 @@ private List<ZipEntry> getEntries() {
 
   @Override
   public long getLastModified() {
-    return this.entry == null ? 0 : this.entry.getLastModifiedTime().toMillis();
+    return this.entry == null ? rootPath.toFile().lastModified() : this.entry.getLastModifiedTime().toMillis();
   }
 
   @Override
   public long getLength() {
-    return this.entry == null ? 0 : this.entry.getSize();
+    return this.entry == null ? rootPath.toFile().length() : this.entry.getSize();
   }
 
   @Override
   public boolean isDirectory() {
     return leafEntries.size() > 1;
   }
 
+  @Override
+  public boolean isZipFile() {
+    return true;
+  }
+
   @Override
   public boolean isReadable() {
     // readable if root is readable
-    return Files.isReadable(Paths.get(root.getName()));
+    return Files.isReadable(rootPath);
   }
 
   @Override
@@ -162,16 +169,14 @@ public InputStream getInputStream() {
 
   @Override
   public void writeToStream(OutputStream outputStream) throws IOException {
-    for (ZipEntry entry : leafEntries) {
-      final File file = new File(entry.getName());
-      IO.copyFile(file, outputStream);
-    }
+    IO.copyFile(rootPath.toFile(), outputStream);
   }
 
   @Override
   public void writeToStream(OutputStream outputStream, long offset, long maxBytes) throws IOException {
-    throw new UnsupportedOperationException(
-        "Writing MFileZip with a byte range to stream not implemented. Filename: " + getName());
+    try (RandomAccessFile randomAccessFile = RandomAccessFile.acquire(rootPath.toString())) {
+      IO.copyRafB(randomAccessFile, offset, maxBytes, outputStream);
+    }
   }
 
   @Override
@@ -202,7 +207,7 @@ public String getProtocol() {
 
     @Override
     public boolean canProvide(String location) {
-      return location != null && location.contains(ext);
+      return location != null && location.toLowerCase(Locale.ROOT).contains(ext);
     }
 
     @Nonnull

diff --git a/cdm/zarr/src/test/java/thredds/inventory/zarr/TestMFileZip.java b/cdm/zarr/src/test/java/thredds/inventory/zarr/TestMFileZip.java
@@ -40,33 +40,81 @@ public static List<Integer[]> getTestParameters() {
     }
 
     @Parameterized.Parameter(0)
-    public int expectedSize;
+    public int entrySize;
 
     @Parameterized.Parameter(1)
-    public int expectedNumberOfFiles;
+    public int numberOfEntries;
 
     @Test
     public void shouldWriteZipToStream() throws IOException {
-      final ZipFile zipFile = createTemporaryZipFile(expectedSize, expectedNumberOfFiles);
-      final MFileZip mFile = new MFileZip(zipFile.getName());
+      try (ZipFile zipFile = createTemporaryZipFile("TestWriteZip", entrySize, numberOfEntries)) {
+        final MFileZip mFile = new MFileZip(zipFile.getName());
 
-      final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
-      mFile.writeToStream(outputStream);
-      assertThat(outputStream.size()).isEqualTo(expectedSize * expectedNumberOfFiles);
+        final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        mFile.writeToStream(outputStream);
+        assertThat(outputStream.size()).isEqualTo(mFile.getLength());
+      }
+    }
+
+    @Test
+    public void shouldWritePartialZipToStream() throws IOException {
+      try (ZipFile zipFile = createTemporaryZipFile("TestWritePartialZip", entrySize, numberOfEntries)) {
+        final MFileZip mFile = new MFileZip(zipFile.getName());
+        final int length = (int) mFile.getLength();
+
+        final int offset = 1;
+        final int maxBytes = 100;
+
+        final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+
+        final int startPosition = Math.min(offset, length);
+        final int endPosition = Math.min(offset + maxBytes, length);
+
+        mFile.writeToStream(outputStream, offset, maxBytes);
+        assertThat(outputStream.size()).isEqualTo(Math.max(0, endPosition - startPosition));
+      }
     }
   }
 
   public static class TestMFileZipNonParameterized {
     @Test
     public void shouldReturnTrueForExistingFile() throws IOException {
-      final ZipFile zipFile = createTemporaryZipFile(2, 0);
-      final MFileZip mFile = new MFileZip(zipFile.getName());
-      assertThat(mFile.exists()).isEqualTo(true);
+      try (ZipFile zipFile = createTemporaryZipFile("TestExists", 2, 0)) {
+        final MFileZip mFile = new MFileZip(zipFile.getName());
+        assertThat(mFile.exists()).isEqualTo(true);
+      }
+    }
+
+    @Test
+    public void shouldGetLastModified() throws IOException {
+      try (ZipFile zipFile = createTemporaryZipFile("TestLastModified", 20, 1)) {
+        final MFileZip mFile = new MFileZip(zipFile.getName());
+        assertThat(mFile.getLastModified()).isGreaterThan(0);
+        assertThat(mFile.getLastModified()).isEqualTo(new File(zipFile.getName()).lastModified());
+      }
+    }
+
+    @Test
+    public void shouldGetLengthForZipFile() throws IOException {
+      try (ZipFile zipFile = createTemporaryZipFile("TestLength", 30, 1)) {
+        final MFileZip mFile = new MFileZip(zipFile.getName());
+        assertThat(mFile.getLength()).isGreaterThan(30);
+        assertThat(mFile.getLength()).isEqualTo(new File(zipFile.getName()).length());
+      }
+    }
+
+    @Test
+    public void shouldProvideForZipExtensions() {
+      MFileZip.Provider provider = new MFileZip.Provider();
+      assertThat(provider.canProvide("foo.zip")).isTrue();
+      assertThat(provider.canProvide("foo.ZIP")).isTrue();
+      assertThat(provider.canProvide("foo.zip2")).isTrue();
+      assertThat(provider.canProvide("foo.txt")).isFalse();
     }
   }
 
-  private static ZipFile createTemporaryZipFile(int size, int numberOfFiles) throws IOException {
-    final File zipFile = tempFolder.newFile("TestMFileZip" + size + "-" + numberOfFiles + ".zip");
+  private static ZipFile createTemporaryZipFile(String name, int size, int numberOfFiles) throws IOException {
+    final File zipFile = tempFolder.newFile(name + "-" + size + "-" + numberOfFiles + ".zip");
 
     try (FileOutputStream fos = new FileOutputStream(zipFile.getPath());
         ZipOutputStream zipOS = new ZipOutputStream(fos)) {