You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "kvr000 (via GitHub)" <gi...@apache.org> on 2023/12/27 19:41:11 UTC
[PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
kvr000 opened a new pull request, #455:
URL: https://github.com/apache/commons-compress/pull/455
Updates local file header metadata even for split archive, so it's readable by `zip -FF` utility.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451609748
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
Review Comment:
It looks like other methods in this file use `ZipLong.getValue(ByteBuffer)` to get this value. Should this call site do the same?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438716104
##########
src/main/java/org/apache/commons/compress/archivers/zip/SeekableChannelRandomAccessOutputStream.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation for SeekableByteChannel.
+ */
+class SeekableChannelRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final SeekableByteChannel channel;
+
+ private long position = 0L;
Review Comment:
I updated to use `channel.position()` instead.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1872201718
> @kvr000 Ping?
Thanks for your feedback, @garydgregory , and sorry for the delay. I'll take a look next few days.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1890754622
> Hello @kvr000 Thank you for your patience. Instead of adding yet another constructor, I've refactored the code to use a `ZipFile` builder (see `ZipFile.Builderr`). I think this is better than an options object builder. You can add and use the options you need if any for this PR to `ZipFile.Builder`. The options builder should no longer be needed so you can ignore the comments I made there. TY!
Hi Gary, thanks for your response.
The `ZipFile` builder certainly works too, in a way, it's a shortcut to pass parameters directly instead of via another object. The reason why I chose separate Options object is:
- It's pretty common pattern for complex initialization. It is actually used in SevenZFileOptions as well.
- The configuration is typically made once and then reused through the application. Having said that, to make it fully reusable, I should have probably passed the path separately, though `toBuilder().modify().build()` still works.
- It should be more reusable across different archivers, both readers and writers. Some would support only subset but generally, having single ZipOptions would allow sharing at least among ZipFile, ZipArchiveInputStream and ZipArchiveOutputStream.
I'm fine with the current option as well though. Please let me know whether the current choice is final and if so, I can adjust my code.
I'd still recommend passing full `Builder` to constructor instead of all arguments separately - it scales better and is less prone to incompatible changes.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438715949
##########
src/main/java/org/apache/commons/compress/utils/IOUtils.java:
##########
@@ -275,6 +277,49 @@ public static byte[] readRange(final ReadableByteChannel input, final int len) t
return output.toByteArray();
}
+ /**
+ * Writes full buffer to channel.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFully(SeekableByteChannel channel, ByteBuffer buf) throws IOException {
Review Comment:
The methods in `SeekableByteChannel` have `write` semantics to attempt to write and return as soon as the `write` is attempted. So I wanted to avoid naming conflict with those and be explicit that this writes full buffer or fails.
Other libraries use the same semantics, such as https://hadoop.apache.org/docs/stable/api/org/apache/hadoop/io/IOUtils.html#writeFully-java.nio.channels.FileChannel-java.nio.ByteBuffer-long-
The example also specifies the same order of parameters which in my opinion makes sense - it operates on object which comes as first parameter, not really in a sense of source of destination. I can amend this but it's inconsistent with other libraries.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439173423
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
The purpose is to provide append only `OutputStream` with possibility to write to arbitrary position. The `pread/pwrite` semantics is much more meaningful than `seek/tell+write/read` semantics in this case as the caller should not be able to change actual position of `OutputStream` stream. Apart from following the callers usage, this scales much better in concurrent environment (not necessarily the case here, more useful in `ZipFile` reader which already uses this approach).
I'm not sure I fully followed the second part. The meaning of `write()` - full or partial - should be defined in interface, and the implementations should behave consistently with the interface. I used `writeFully` name to avoid conflict with `FileChannel.write` which declares that it may be only partial write and such naming is also consistent with other 3rd-parties libraries. I agree the `At` can be removed from `writeFullyAt`, though in my opinion it's often better to be explicit with naming rather than relying on different method signature.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #455:
URL: https://github.com/apache/commons-compress/pull/455
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1437697139
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
+ this(file, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE);
+ }
+
+ public FileRandomAccessOutputStream(Path file, OpenOption... options) throws IOException {
+ this.channel = FileChannel.open(file, options);
+ }
+
+ public FileChannel channel() {
+ return channel;
+ }
+
+ @Override
+ public synchronized void write(byte[] b, int off, int len) throws IOException {
+ IOUtils.writeFully(this.channel, ByteBuffer.wrap(b, off, len));
+ position += len;
+ }
+
+ @Override
+ public synchronized long position() {
+ return position;
+ }
+
+ @Override
+ public void writeFullyAt(final byte[] b, final int off, final int len, long atPosition) throws IOException {
Review Comment:
Calling the API `write` should be enough, especially if Javadocs are comprehensive.
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
Review Comment:
Use `final` where you can.
##########
src/main/java/org/apache/commons/compress/utils/IOUtils.java:
##########
@@ -275,6 +277,49 @@ public static byte[] readRange(final ReadableByteChannel input, final int len) t
return output.toByteArray();
}
+ /**
+ * Writes full buffer to channel.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFully(SeekableByteChannel channel, ByteBuffer buf) throws IOException {
Review Comment:
`writeFully` -> `write` or is a "partial write" expected default behavior? That would be odd. Flip the arguments such that the source comes first and the destination second. IOW, "Writes from Foo to Bar", which happens to be the phrasing you use in the Javadoc ;-)
##########
src/main/java/org/apache/commons/compress/utils/IOUtils.java:
##########
@@ -275,6 +277,49 @@ public static byte[] readRange(final ReadableByteChannel input, final int len) t
return output.toByteArray();
}
+ /**
+ * Writes full buffer to channel.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFully(SeekableByteChannel channel, ByteBuffer buf) throws IOException {
+ while (buf.hasRemaining()) {
+ int remaining = buf.remaining();
+ int written = channel.write(buf);
+ if (written <= 0) {
+ throw new IOException("Failed to fully write: channel=" + channel + " length=" + remaining + " written=" + written);
+ }
+ }
+ }
+
+ /**
+ * Writes full buffer to channel at specified position.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @param position
+ * position to write at
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFullyAt(FileChannel channel, ByteBuffer buf, long position) throws IOException {
Review Comment:
See above. All new public and protected elements shoyuld have an `@since` Javadoc tag.
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
Review Comment:
This new class needs a unit test to prove this class behaves as an output stream and that all APIs do what they say.
##########
src/main/java/org/apache/commons/compress/archivers/zip/SeekableChannelRandomAccessOutputStream.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation for SeekableByteChannel.
+ */
+class SeekableChannelRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final SeekableByteChannel channel;
+
+ private long position = 0L;
Review Comment:
Don't initialize instance variables to their default values.
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
This new class needs a unit test to prove this class behaves as an output stream and that all APIs do what they say.
##########
src/main/java/org/apache/commons/compress/archivers/zip/SeekableChannelRandomAccessOutputStream.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation for SeekableByteChannel.
+ */
+class SeekableChannelRandomAccessOutputStream extends RandomAccessOutputStream {
Review Comment:
This new class needs a unit test to prove this class behaves as an output stream and that all APIs do what they say.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451609748
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
Review Comment:
It looks like other methods in this file use `ZipLong/ZipShort.getValue(ByteBuffer)` to get this value. Should this call site do the same?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1878589259
@kvr000
I will review this PR tomorrow. I appreciate your patience.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451528565
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java:
##########
@@ -0,0 +1,286 @@
+/*
+ * 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
+ *
+ * http://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 java.nio.channels.SeekableByteChannel;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.Objects;
+
+
+/**
+ * Zip open archive options. The class is constructed via builder.
+ */
+public class ZipOpenOptions {
+ private final Path path;
+
+ private final SeekableByteChannel channel;
+
+ private final Charset encoding;
+
+ private final boolean useUnicodeExtraFields;
+
+ private final boolean ignoreLocalFileHeader;
+
+ private final long maxNumberOfDisks;
+
+ /**
+ * Gets archive path.
+ *
+ * @return
+ * archive path
+ */
+ public Path path() {
+ return path;
+ }
+
+ /**
+ * Gets archive file channel.
+ *
+ * @return
+ * archive file channel
+ */
+ public SeekableByteChannel channel() {
+ return channel;
+ }
+
+ /**
+ * Gets archive encoding.
+ *
+ * @return
+ * archive encoding
+ */
+ public Charset encoding() {
+ return encoding;
+ }
+
+ /**
+ * Indicates whether to use unicode extra fields, default is true.
+ *
+ * @return
+ * indicates whether to use unicode extra fields
+ */
+ public boolean useUnicodeExtraFields() {
+ return useUnicodeExtraFields;
+ }
+
+ /**
+ * Indicates whether to ignore local headers, default is false.
+ *
+ * @return
+ * indicates whether to ignore local headers.
+ */
+ public boolean ignoreLocalFileHeader() {
+ return ignoreLocalFileHeader;
+ }
+
+ /**
+ * Gets max number of multi archive disks, default is 1 (no multi archive).
+ *
+ * @return
+ * max number of multi archive disks
+ */
+ public long maxNumberOfDisks() {
+ return maxNumberOfDisks;
+ }
+
+ /**
+ * Gets whether to close the channel on error during opening, default is true.
+ *
+ * @return
+ * whether to close the channel on error during opening
+ */
+ public boolean closeOnError() {
+ return closeOnError;
+ }
+
+ private final boolean closeOnError;
+
+ /**
+ * Creates new Builder.
+ *
+ * @return
+ * new freshly initialized Builder.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Converts the object to mutable builder.
+ *
+ * @return
+ * builder with same configuration.
+ */
+ public Builder toBuilder() {
+ return builder()
+ .path(path)
+ .channel(channel)
+ .encoding(encoding)
+ .useUnicodeExtraFields(useUnicodeExtraFields)
+ .ignoreLocalFileHeader(ignoreLocalFileHeader)
+ .maxNumberOfDisks(maxNumberOfDisks)
+ .closeOnError(closeOnError);
+ }
+
+ /**
+ * Creates new instance. The instance is not thread safe.
+ */
+ protected ZipOpenOptions(Builder builder) {
+ this.path = builder.path;
+ this.channel = builder.channel;
+ this.encoding = builder.encoding;
+ this.useUnicodeExtraFields = builder.useUnicodeExtraFields;
+ this.ignoreLocalFileHeader = builder.ignoreLocalFileHeader;
+ this.maxNumberOfDisks = builder.maxNumberOfDisks;
+ this.closeOnError = builder.closeOnError;
+ }
+
+ public static class Builder {
+
+ private Path path;
+
+ private SeekableByteChannel channel;
+
+ private Charset encoding = StandardCharsets.UTF_8;
+
+ private boolean useUnicodeExtraFields = true;
+
+ private boolean ignoreLocalFileHeader = false;
Review Comment:
Don't initialize instance variables to their default value.
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java:
##########
@@ -0,0 +1,286 @@
+/*
+ * 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
+ *
+ * http://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 java.nio.channels.SeekableByteChannel;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.Objects;
+
+
+/**
+ * Zip open archive options. The class is constructed via builder.
+ */
+public class ZipOpenOptions {
+ private final Path path;
+
+ private final SeekableByteChannel channel;
+
+ private final Charset encoding;
+
+ private final boolean useUnicodeExtraFields;
+
+ private final boolean ignoreLocalFileHeader;
+
+ private final long maxNumberOfDisks;
+
+ /**
+ * Gets archive path.
+ *
+ * @return
+ * archive path
+ */
+ public Path path() {
+ return path;
+ }
+
+ /**
+ * Gets archive file channel.
+ *
+ * @return
+ * archive file channel
+ */
+ public SeekableByteChannel channel() {
+ return channel;
+ }
+
+ /**
+ * Gets archive encoding.
+ *
+ * @return
+ * archive encoding
+ */
+ public Charset encoding() {
+ return encoding;
+ }
+
+ /**
+ * Indicates whether to use unicode extra fields, default is true.
+ *
+ * @return
+ * indicates whether to use unicode extra fields
+ */
+ public boolean useUnicodeExtraFields() {
+ return useUnicodeExtraFields;
+ }
+
+ /**
+ * Indicates whether to ignore local headers, default is false.
+ *
+ * @return
+ * indicates whether to ignore local headers.
+ */
+ public boolean ignoreLocalFileHeader() {
+ return ignoreLocalFileHeader;
+ }
+
+ /**
+ * Gets max number of multi archive disks, default is 1 (no multi archive).
+ *
+ * @return
+ * max number of multi archive disks
+ */
+ public long maxNumberOfDisks() {
+ return maxNumberOfDisks;
+ }
+
+ /**
+ * Gets whether to close the channel on error during opening, default is true.
+ *
+ * @return
+ * whether to close the channel on error during opening
+ */
+ public boolean closeOnError() {
+ return closeOnError;
+ }
+
+ private final boolean closeOnError;
+
+ /**
+ * Creates new Builder.
+ *
+ * @return
+ * new freshly initialized Builder.
+ */
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ /**
+ * Converts the object to mutable builder.
+ *
+ * @return
+ * builder with same configuration.
+ */
+ public Builder toBuilder() {
+ return builder()
+ .path(path)
+ .channel(channel)
+ .encoding(encoding)
+ .useUnicodeExtraFields(useUnicodeExtraFields)
+ .ignoreLocalFileHeader(ignoreLocalFileHeader)
+ .maxNumberOfDisks(maxNumberOfDisks)
+ .closeOnError(closeOnError);
+ }
+
+ /**
+ * Creates new instance. The instance is not thread safe.
+ */
+ protected ZipOpenOptions(Builder builder) {
+ this.path = builder.path;
+ this.channel = builder.channel;
+ this.encoding = builder.encoding;
+ this.useUnicodeExtraFields = builder.useUnicodeExtraFields;
+ this.ignoreLocalFileHeader = builder.ignoreLocalFileHeader;
+ this.maxNumberOfDisks = builder.maxNumberOfDisks;
+ this.closeOnError = builder.closeOnError;
+ }
+
+ public static class Builder {
+
+ private Path path;
+
+ private SeekableByteChannel channel;
+
+ private Charset encoding = StandardCharsets.UTF_8;
+
+ private boolean useUnicodeExtraFields = true;
+
+ private boolean ignoreLocalFileHeader = false;
+
+ private long maxNumberOfDisks = 1;
+
+ private boolean closeOnError = true;
+
+ /**
+ * Builds instance of ZipOpenOptions from configured values.
+ *
+ * @return
+ * ZipOpenOptions immutable instance.
+ */
+ public ZipOpenOptions build() {
+ return new ZipOpenOptions(this);
+ }
+
+ /**
+ * Sets archive path.
+ *
+ * @param path
+ * archive path
+ *
+ * @return this instance
+ */
+ public ZipOpenOptions.Builder path(Path path) {
Review Comment:
Use "set" as the prefix for all setter methods.
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipOpenOptions.java:
##########
@@ -0,0 +1,286 @@
+/*
+ * 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
+ *
+ * http://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 java.nio.channels.SeekableByteChannel;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.Objects;
+
+
+/**
+ * Zip open archive options. The class is constructed via builder.
+ */
Review Comment:
Javadoc: Add `@since 1.26.0` for all new public and protected elements.
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -435,6 +444,39 @@ public static void closeQuietly(final ZipFile zipFile) {
private long firstLocalFileHeaderOffset;
+
+ public ZipFile(ZipOpenOptions options) throws IOException {
Review Comment:
This class already has 15 (!) constructors, I want to avoid even more constructor creep. Instead of a builder of options, Commons IO provides the ability to provide builders for IO classes based on various IO elements like File, Path, and streams. Let's refactor this class like this example of another class with an exploding constructor count: [FileWriterWithEncoding](https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/output/FileWriterWithEncoding.java)
I'll work on that...
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451610610
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -435,6 +444,39 @@ public static void closeQuietly(final ZipFile zipFile) {
private long firstLocalFileHeaderOffset;
+
+ public ZipFile(ZipOpenOptions options) throws IOException {
Review Comment:
The rework is in git master.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439024770
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java:
##########
@@ -42,14 +48,20 @@ final class ZipSplitOutputStream extends OutputStream {
private static final long ZIP_SEGMENT_MIN_SIZE = 64 * 1024L;
private static final long ZIP_SEGMENT_MAX_SIZE = 4294967295L;
- private OutputStream outputStream;
+ private FileChannel currentChannel;
+ private FileRandomAccessOutputStream outputStream;
private Path zipFile;
private final long splitSize;
+ private long totalPosition;
private int currentSplitSegmentIndex;
private long currentSplitSegmentBytesWritten;
private boolean finished;
private final byte[] singleByte = new byte[1];
+ private List<Long> diskToPosition = new ArrayList<>();
+
+ private TreeMap<Long, Path> positionToFiles = new TreeMap<>();
Review Comment:
Is there going to be an issue here on case-insensitive file systems? Say, Windows, macOS?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1890776693
> > Hello @kvr000 Thank you for your patience. Instead of adding yet another constructor, I've refactored the code to use a `ZipFile` builder (see `ZipFile.Builderr`). I think this is better than an options object builder. You can add and use the options you need if any for this PR to `ZipFile.Builder`. The options builder should no longer be needed so you can ignore the comments I made there. TY!
>
> Hi Gary, thanks for your response.
>
> The `ZipFile` builder certainly works too, in a way, it's a shortcut to pass parameters directly instead of via another object. The reason why I chose separate Options object is:
>
> * It's pretty common pattern for complex initialization. It is actually used in SevenZFileOptions as well.
>
> * The configuration is typically made once and then reused through the application. Having said that, to make it fully reusable, I should have probably passed the path separately, though `toBuilder().modify().build()` still works.
>
> * It should be more reusable across different archivers, both readers and writers. Some would support only subset but generally, having single ZipOptions would allow sharing at least among ZipFile, ZipArchiveInputStream and ZipArchiveOutputStream.
>
>
> I'm fine with the current option as well though. Please let me know whether the current choice is final and if so, I can adjust my code.
>
> I'd still recommend passing full `Builder` to constructor instead of all arguments separately - it scales better and is less prone to incompatible changes.
Hi @kvr000
I think the best part forward is for you to rebase on git master and adapt to the new Builder. TY!
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451640542
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
Review Comment:
There isn't `ZipLong.getValue(ByteBuffer)` method, only `byte[]` overloads. It may be worth getting array if we read and parse bigger structure but that's not the case here.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1870621558
@kvr000
-1: This PR breaks binary compatibility.
Run 'mvn' before you push.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1872616449
> * Big picture: I am considering adding some random IO classes to Apache Commons IO like Apache PDFBox has, see https://github.com/apache/pdfbox/tree/trunk/io/src/main/java/org/apache/pdfbox/io
That totally makes sense, each library provides its own IOUtils currently. I'd suggest few things:
1. FileChannel is typically more meaningful than SeekableByteChannel. Even here in ZipFile and related, the SeekableByteChannel is used in a sense of FileChannel. I provided fix for this a while ago which allowed lock-free reading from multiple ZipFile entries in parallel but there is the same work in other parts.
2. The JDK unfortunately misses interface for position_read and position_write methods which makes custom implementations more tricky. I was addressing this issue in https://github.com/dryuf/dryuf-bigio (FlatChannel), you may consider this for inspiration when working on commons-io stuff.
3. The PdfBox in my opinion is quite complicated and may suffer from diamond inheritance problem. The above may be more straightforward and more efficient.
> * In light of this, it is good that the random IO classes you've added are package-protected. This will let us possibly replace this implementation with this very code ported to Commons IO or with code from PDFBox ported to Commons IO. We do not want Commons Compress to depend on PDFBox, especially since PDFBox already depends on Commons IO.
> * To reduce the new public API surface, please move the new IOUtils method into package-protected place in the `zip` package.
That was done.
> * I'll study all of this some more and see what is generic enough to add and reuse from Commons IO.
It may require more work to make it more generic. For now the package private is probably better choice.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1872200144
@kvr000
Ping?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438716377
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
Review Comment:
Right, added.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438723007
##########
src/main/java/org/apache/commons/compress/archivers/zip/SeekableChannelRandomAccessOutputStream.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation for SeekableByteChannel.
+ */
+class SeekableChannelRandomAccessOutputStream extends RandomAccessOutputStream {
Review Comment:
Added.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438724538
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
Added, just the method which actually is provided.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439022632
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
+ this(file, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE);
+ }
+
+ public FileRandomAccessOutputStream(Path file, OpenOption... options) throws IOException {
+ this.channel = FileChannel.open(file, options);
+ }
+
+ public FileChannel channel() {
+ return channel;
+ }
+
+ @Override
+ public synchronized void write(byte[] b, int off, int len) throws IOException {
+ IOUtils.writeFully(this.channel, ByteBuffer.wrap(b, off, len));
+ position += len;
+ }
+
+ @Override
+ public synchronized long position() {
+ return position;
+ }
+
+ @Override
+ public void writeFullyAt(final byte[] b, final int off, final int len, long atPosition) throws IOException {
Review Comment:
@kvr000
The example you use does not use the "At" in the method name postfix, which feels awkward to me, and we do not see it in the JRE. The fact that the method writes its argument at a position is implied by the fact that the position is passed in. I would not use "At" in the method name or parameter name.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439023886
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
Hello @kvr000 and Happy New Year!
The API design does not feel quite right to me. A `SeekableByteChannel` has `position()` and `position(long)`, and `RandomAccessFile` has `getFilePointer()` and `seek(long)`. In both cases, there is one API for "where am I?" and another for "go to this location". I think we should have the same here. This avoids adding a "position" argument everytime a new API is needed in this method. It also could let the implementation decide if a write is "full" or not. Avoiding the need for "fully" APIs in the first place. Thoughts?
@elharo, any thoughts here?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439173423
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
The purpose is to provide append only `OutputStream` with possibility to write to arbitrary position. The `pread/pwrite` semantics is much more meaningful than `seek/tell` semantics in this case as the caller should not be able to change actual position of `OutputStream` stream. Apart from following the callers usage, this scales much better in concurrent environment (not necessarily the case here, more useful in `ZipFile` reader which already uses this approach).
I'm not sure I fully followed the second part. The meaning of `write()` - full or partial - should be defined in interface, and the implementations should behave consistently with the interface. I used `writeFully` name to avoid conflict with `FileChannel.write` which declares that it may be only partial write and such naming is also consistent with other 3rd-parties libraries. I agree the `At` can be removed from `writeFullyAt`, though in my opinion it's often better to be explicit with naming rather than relying on different method signature.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439179899
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
+ this(file, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE);
+ }
+
+ public FileRandomAccessOutputStream(Path file, OpenOption... options) throws IOException {
+ this.channel = FileChannel.open(file, options);
+ }
+
+ public FileChannel channel() {
+ return channel;
+ }
+
+ @Override
+ public synchronized void write(byte[] b, int off, int len) throws IOException {
+ IOUtils.writeFully(this.channel, ByteBuffer.wrap(b, off, len));
+ position += len;
+ }
+
+ @Override
+ public synchronized long position() {
+ return position;
+ }
+
+ @Override
+ public void writeFullyAt(final byte[] b, final int off, final int len, long atPosition) throws IOException {
Review Comment:
Answered in later comment. It's possible to remove `At`, though I prefer more explicit names rather than different method signatures.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439173423
##########
src/main/java/org/apache/commons/compress/archivers/zip/RandomAccessOutputStream.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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
+ *
+ * http://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 java.io.IOException;
+import java.io.OutputStream;
+
+
+/**
+ * Abstraction over OutputStream which also allows random access writes.
+ */
+abstract class RandomAccessOutputStream extends OutputStream {
Review Comment:
Happy New Year, @garydgregory !
The purpose is to provide append only `OutputStream` with possibility to write to arbitrary position. The `pread/pwrite` semantics is much more meaningful than `seek/tell+write/read` semantics in this case as the caller should not be able to change actual position of `OutputStream` stream. Apart from following the callers usage, this scales much better in concurrent environment (not necessarily the case here, more useful in `ZipFile` reader which already uses this approach).
I'm not sure I fully followed the second part. The meaning of `write()` - full or partial - should be defined in interface, and the implementations should behave consistently with the interface. I used `writeFully` name to avoid conflict with `FileChannel.write` which declares that it may be only partial write and such naming is also consistent with other 3rd-parties libraries. I agree the `At` can be removed from `writeFullyAt`, though in my opinion it's often better to be explicit with naming rather than relying on different method signature.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1439174615
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipSplitOutputStream.java:
##########
@@ -42,14 +48,20 @@ final class ZipSplitOutputStream extends OutputStream {
private static final long ZIP_SEGMENT_MIN_SIZE = 64 * 1024L;
private static final long ZIP_SEGMENT_MAX_SIZE = 4294967295L;
- private OutputStream outputStream;
+ private FileChannel currentChannel;
+ private FileRandomAccessOutputStream outputStream;
private Path zipFile;
private final long splitSize;
+ private long totalPosition;
private int currentSplitSegmentIndex;
private long currentSplitSegmentBytesWritten;
private boolean finished;
private final byte[] singleByte = new byte[1];
+ private List<Long> diskToPosition = new ArrayList<>();
+
+ private TreeMap<Long, Path> positionToFiles = new TreeMap<>();
Review Comment:
Not here, the name created and name re-opened for `writeFullyAt` will be the same.
The issue may be in the reader but it's rather the opposite - it may fail on case-sensitive filesystem if the names are not consistent. Perhaps we can check both `.z01` and `.Z01` extensions.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1870605766
## [Codecov](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `18 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`d8dbdf8`)](https://app.codecov.io/gh/apache/commons-compress/commit/d8dbdf8807e470da80252863a7c2aff3d3317d7e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 80.39% compared to head [(`58b96ff`)](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 80.54%.
> Report is 2 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ava/org/apache/commons/compress/utils/IOUtils.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvdXRpbHMvSU9VdGlscy5qYXZh) | 71.42% | [3 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ss/archivers/zip/FileRandomAccessOutputStream.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9GaWxlUmFuZG9tQWNjZXNzT3V0cHV0U3RyZWFtLmphdmE=) | 85.71% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...mpress/archivers/zip/RandomAccessOutputStream.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9SYW5kb21BY2Nlc3NPdXRwdXRTdHJlYW0uamF2YQ==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...compress/archivers/zip/ZipArchiveOutputStream.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9aaXBBcmNoaXZlT3V0cHV0U3RyZWFtLmphdmE=) | 93.10% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/commons/compress/archivers/zip/ZipFile.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9aaXBGaWxlLmphdmE=) | 96.82% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...s/compress/archivers/zip/ZipSplitOutputStream.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9aaXBTcGxpdE91dHB1dFN0cmVhbS5qYXZh) | 93.93% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...s/zip/SeekableChannelRandomAccessOutputStream.java](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9TZWVrYWJsZUNoYW5uZWxSYW5kb21BY2Nlc3NPdXRwdXRTdHJlYW0uamF2YQ==) | 93.33% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #455 +/- ##
============================================
+ Coverage 80.39% 80.54% +0.15%
- Complexity 6724 6768 +44
============================================
Files 343 346 +3
Lines 24729 24837 +108
Branches 4000 4013 +13
============================================
+ Hits 19880 20005 +125
+ Misses 3323 3313 -10
+ Partials 1526 1519 -7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-compress/pull/455?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438716334
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
+ this(file, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE);
+ }
+
+ public FileRandomAccessOutputStream(Path file, OpenOption... options) throws IOException {
+ this.channel = FileChannel.open(file, options);
+ }
+
+ public FileChannel channel() {
+ return channel;
+ }
+
+ @Override
+ public synchronized void write(byte[] b, int off, int len) throws IOException {
+ IOUtils.writeFully(this.channel, ByteBuffer.wrap(b, off, len));
+ position += len;
+ }
+
+ @Override
+ public synchronized long position() {
+ return position;
+ }
+
+ @Override
+ public void writeFullyAt(final byte[] b, final int off, final int len, long atPosition) throws IOException {
Review Comment:
See the comment later - the default semantics on `SeekableByteChannel` is to write part of the buffer, not necessarily full. It's consistent with naming convention in other libraries such as https://hadoop.apache.org/docs/stable/api/org/apache/hadoop/io/IOUtils.html#writeFully-java.nio.channels.FileChannel-java.nio.ByteBuffer-long-
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on PR #455:
URL: https://github.com/apache/commons-compress/pull/455#issuecomment-1881714547
> @kvr000 I will review this PR tomorrow. I appreciate your patience.
Sure, take your time.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438729448
##########
src/main/java/org/apache/commons/compress/archivers/zip/FileRandomAccessOutputStream.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.OpenOption;
+import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation based on Path file.
+ */
+class FileRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final FileChannel channel;
+
+ private long position = 0L;
+
+ public FileRandomAccessOutputStream(Path file) throws IOException {
Review Comment:
I added `final` to few places, hope I didn't miss something.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451640900
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
+ }
+ if (numberOfFiles > Math.min(options.maxNumberOfDisks(), Integer.MAX_VALUE)) {
+ throw new IOException("Too many files in zip archive, max=" +
Review Comment:
Correct, the message was misleading. I corrected to `Too many disks...` .
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451640542
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
Review Comment:
There isn't `ZipLong.getValue(ByteBuffer)` method, only `byte[]` overloads.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438716104
##########
src/main/java/org/apache/commons/compress/archivers/zip/SeekableChannelRandomAccessOutputStream.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ *
+ * http://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 org.apache.commons.compress.utils.IOUtils;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+
+
+/**
+ * {@link RandomAccessOutputStream} implementation for SeekableByteChannel.
+ */
+class SeekableChannelRandomAccessOutputStream extends RandomAccessOutputStream {
+
+ private final SeekableByteChannel channel;
+
+ private long position = 0L;
Review Comment:
I wanted to be explicit here that the position starts at `0`, despite it technically gets `0` by JDK.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "kvr000 (via GitHub)" <gi...@apache.org>.
kvr000 commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1438714929
##########
src/main/java/org/apache/commons/compress/utils/IOUtils.java:
##########
@@ -275,6 +277,49 @@ public static byte[] readRange(final ReadableByteChannel input, final int len) t
return output.toByteArray();
}
+ /**
+ * Writes full buffer to channel.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFully(SeekableByteChannel channel, ByteBuffer buf) throws IOException {
+ while (buf.hasRemaining()) {
+ int remaining = buf.remaining();
+ int written = channel.write(buf);
+ if (written <= 0) {
+ throw new IOException("Failed to fully write: channel=" + channel + " length=" + remaining + " written=" + written);
+ }
+ }
+ }
+
+ /**
+ * Writes full buffer to channel at specified position.
+ *
+ * @param channel
+ * channel to write to
+ * @param buf
+ * buffer to write
+ * @param position
+ * position to write at
+ * @throws IOException
+ * when writing fails
+ */
+ public static void writeFullyAt(FileChannel channel, ByteBuffer buf, long position) throws IOException {
Review Comment:
Moved to package private ZipIoUtil as requested above.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] COMPRESS-653: Fix split archive updating incorrect file [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #455:
URL: https://github.com/apache/commons-compress/pull/455#discussion_r1451603745
##########
src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java:
##########
@@ -684,6 +715,53 @@ public ZipFile(final String name, final String encoding) throws IOException {
this(new File(name).toPath(), encoding, true);
}
+ private static SeekableByteChannel openZipArchive(Path path, ZipOpenOptions options) throws IOException {
+ FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
+ List<FileChannel> channels = new ArrayList<>();
+ try {
+ boolean is64 = positionAtEndOfCentralDirectoryRecord(channel);
+ long numberOfFiles;
+ if (is64) {
+ channel.position(channel.position() + ZipConstants.WORD + ZipConstants.WORD + ZipConstants.DWORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.WORD);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = buf.getInt() & 0xffffffffL;
+ } else {
+ channel.position(channel.position() + ZipConstants.WORD);
+ ByteBuffer buf = ByteBuffer.allocate(ZipConstants.SHORT);
+ buf.order(ByteOrder.LITTLE_ENDIAN);
+ IOUtils.readFully(channel, buf);
+ buf.flip();
+ numberOfFiles = (buf.getShort() & 0xffff) + 1;
+ }
+ if (numberOfFiles > Math.min(options.maxNumberOfDisks(), Integer.MAX_VALUE)) {
+ throw new IOException("Too many files in zip archive, max=" +
Review Comment:
Hello @kvr000
Either the option name or the error message is wrong: are we creating a bound on files in an archive or a count of volumes ("drives")? Or is there something I am missing?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org