iloveeclipse commented on code in PR #634:
URL: https://github.com/apache/commons-compress/pull/634#discussion_r1926905629
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:
##########
@@ -931,7 +944,13 @@ public int read(final byte[] buffer, final int offset,
final int length) throws
} else if (current.entry.getMethod() ==
ZipMethod.UNSHRINKING.getCode() || current.entry.getMethod() ==
ZipMethod.IMPLODING.getCode()
|| current.entry.getMethod() ==
ZipMethod.ENHANCED_DEFLATED.getCode() || current.entry.getMethod() ==
ZipMethod.BZIP2.getCode()) {
read = current.inputStream.read(buffer, offset, length);
- } else {
+ } else if (current.entry.getMethod() == ZipMethod.ZSTD.getCode()) {
+ if (zstdInputStream == null) {
+ // Can happen if the ZSTDCompressorInputStream couldn't be
instantiated due to an exception
Review Comment:
This is probably the place where the stream could be opened for the first
time?
##########
src/test/java/org/apache/commons/compress/archivers/zip/ZstdSupportTest.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+package org.apache.commons.compress.archivers.zip;
+
+import static org.junit.Assert.*;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorInputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdUtils;
+import org.junit.jupiter.api.Test;
+
+public class ZstdSupportTest {
+
+ @Test
+ public void testZstdMethodInZipFile() throws IOException {
+ String zipFilePath = "/tmp/zstd_compressed.zip";
+ String zipContentFile = "Name.txt";
+ byte[] simpleText = "This is a Simple Test File".getBytes();
+
+ // Create the Zip File
+ {
+ ZipArchiveOutputStream zipOutputStream = new
ZipArchiveOutputStream(new File(zipFilePath));
Review Comment:
Should be in try/finally to close stream also if code fails in the middle.
##########
src/test/java/org/apache/commons/compress/archivers/zip/ZstdSupportTest.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+package org.apache.commons.compress.archivers.zip;
+
+import static org.junit.Assert.*;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorInputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdUtils;
+import org.junit.jupiter.api.Test;
+
+public class ZstdSupportTest {
+
+ @Test
+ public void testZstdMethodInZipFile() throws IOException {
+ String zipFilePath = "/tmp/zstd_compressed.zip";
Review Comment:
`/tmp/` is OS specific. Can we use
`java.nio.file.Files.createTempDirectory()` or `Files.createTempFile("",
".zip")` or something that is OS independent?
##########
src/test/java/org/apache/commons/compress/archivers/zip/ZstdSupportTest.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+package org.apache.commons.compress.archivers.zip;
+
+import static org.junit.Assert.*;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorInputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdUtils;
+import org.junit.jupiter.api.Test;
+
+public class ZstdSupportTest {
+
+ @Test
+ public void testZstdMethodInZipFile() throws IOException {
+ String zipFilePath = "/tmp/zstd_compressed.zip";
+ String zipContentFile = "Name.txt";
+ byte[] simpleText = "This is a Simple Test File".getBytes();
+
+ // Create the Zip File
+ {
+ ZipArchiveOutputStream zipOutputStream = new
ZipArchiveOutputStream(new File(zipFilePath));
+ ZipArchiveEntry archiveEntry = new ZipArchiveEntry(zipContentFile);
+ archiveEntry.setMethod(ZipMethod.ZSTD.getCode());
+ archiveEntry.setSize(simpleText.length);
+ zipOutputStream.putArchiveEntry(archiveEntry);
+ ZstdUtils.readAndCompressWrite(new
ByteArrayInputStream(simpleText), zipOutputStream);
+ zipOutputStream.closeArchiveEntry();
+ zipOutputStream.close();
+ }
+
+
+ // Read the Zip File
+ {
+ ZipFile zipFile = ZipFile.builder().setFile(new
File(zipFilePath)).get();
+
+ // Find the entry
+ ZipArchiveEntry entry = zipFile.getEntry(zipContentFile);
+
+ // Check the Zstd compression method
+ assertEquals(entry.getMethod(), ZipMethod.ZSTD.getCode());
+ InputStream inputStream = zipFile.getInputStream(entry);
+ assertTrue(inputStream instanceof ZstdCompressorInputStream);
+
+ long dataOffset = entry.getDataOffset();
+ int uncompressedSize = (int) entry.getSize();
+
+ assertEquals(simpleText.length, uncompressedSize);
+
+ byte[] uncompressedData = new byte[uncompressedSize];
+ inputStream.read(uncompressedData);
+ assertEquals(new String(simpleText), new String(uncompressedData));
+
+ zipFile.close();
Review Comment:
All close() operations should go to the "finally" blocks
##########
src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:
##########
@@ -134,4 +161,19 @@ public static void setCacheZstdAvailablity(final boolean
doCache) {
/** Private constructor to prevent instantiation of this utility class. */
private ZstdUtils() {
}
+
+ public static long readAndCompressWrite(InputStream content, OutputStream
out) throws IOException {
+ byte[] buf = new byte[8192];
+ SizeMeasurementOutputStream sizeMeasureingOutput = new
SizeMeasurementOutputStream(out);
+ try (ZstdCompressorOutputStream zstdStream = new
ZstdCompressorOutputStream(sizeMeasureingOutput)) {
+ int bytesRead;
+ while ((bytesRead = content.read(buf)) != -1) {
+ zstdStream.write(buf, 0, bytesRead);
+ zstdStream.flush();
+ }
Review Comment:
Please check indentation level here
##########
src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:
##########
@@ -134,4 +161,19 @@ public static void setCacheZstdAvailablity(final boolean
doCache) {
/** Private constructor to prevent instantiation of this utility class. */
private ZstdUtils() {
}
+
+ public static long readAndCompressWrite(InputStream content, OutputStream
out) throws IOException {
+ byte[] buf = new byte[8192];
Review Comment:
`8192` probably deserves a constant with appropriate name, like
`DEFAULT_CHUNK_SIZE`
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveInputStream.java:
##########
@@ -362,6 +365,12 @@ public ZipArchiveInputStream(final InputStream
inputStream, final String encodin
this.useUnicodeExtraFields = useUnicodeExtraFields;
this.allowStoredEntriesWithDataDescriptor =
allowStoredEntriesWithDataDescriptor;
this.skipSplitSig = skipSplitSig;
+ try {
+ zstdInputStream = new ZstdCompressorInputStream(in);
+ } catch (IOException e) {
+ e.printStackTrace();
Review Comment:
Is there a better way to **log** an exception? Or, may be better, to move
the zstd stream creation to the place where it will be required (init on
demand)?
##########
src/main/java/org/apache/commons/compress/compressors/zstandard/ZstdUtils.java:
##########
@@ -28,7 +31,31 @@
*/
public class ZstdUtils {
- enum CachedAvailability {
+ private static final class SizeMeasurementOutputStream extends
OutputStream {
+ long size = 0;
Review Comment:
`= 0`: there is no need to init fields to default values in Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
