mattcasters commented on code in PR #8971:
URL: https://github.com/apache/hop/pull/8971#discussion_r4300289375
##########
plugins/transforms/zipfile/src/main/java/org/apache/hop/pipeline/transforms/zipfile/ZipFile.java:
##########
@@ -336,158 +337,160 @@ private void addFilenameToResult() {
}
}
- private File getFile(final String filename) {
- try {
- URI uri = new URI(filename);
- return new File(uri);
- } catch (URISyntaxException | IllegalArgumentException ex) {
- // Not a file:// URI but a plain path such as /tmp/archive.zip or
C:\archive.zip
- }
- return new File(filename);
- }
-
private void zipFile() throws HopException {
- String localrealZipfilename = HopVfs.getFilename(data.zipFile);
+ // Update an existing zip by writing the new version next to it and moving
it over the
+ // original once complete. If anything fails, the original zip is left
untouched.
boolean updateZip = false;
-
- byte[] buffer = null;
- OutputStream dest = null;
- BufferedOutputStream buff = null;
- ZipOutputStream out = null;
- InputStream in = null;
+ boolean moving = false;
+ FileObject output = data.zipFile;
CountingInputStream countingIn = null;
CountingOutputStream countingOut = null;
- ZipInputStream zin = null;
- ZipEntry entry = null;
- File tempFile = null;
- HashSet<String> fileSet = new HashSet<>();
try {
-
- updateZip = (data.zipFile.exists() && meta.isOverwriteZipEntry());
-
+ updateZip = data.zipFile.exists() && meta.isOverwriteZipEntry();
if (updateZip) {
- // the Zipfile exists
- // and we weed to update entries
- // Let's create a temp file
- File fileZip = getFile(localrealZipfilename);
- // Create the temporary file next to the zip file, not in the shared
system temp folder: the
- // rename below then stays on the same file system and no other user
can claim the name.
- tempFile =
- File.createTempFile(fileZip.getName(), null,
fileZip.getAbsoluteFile().getParentFile());
- // delete it, otherwise we cannot rename existing zip to it.
- tempFile.delete();
-
- updateZip = fileZip.renameTo(tempFile);
+ output = createTemporaryZipFile(data.zipFile);
}
- // Prepare Zip File
- buffer = new byte[18024];
- countingOut =
- new
CountingOutputStream(HopVfs.getOutputStream(localrealZipfilename, false,
variables));
- dest = countingOut;
- buff = new BufferedOutputStream(dest);
- out = new ZipOutputStream(buff);
-
- if (updateZip) {
- // User want to append files to existing Zip file
- // The idea is to rename the existing zip file to a temporary file
- // and then adds all entries in the existing zip along with the new
files,
- // excluding the zip entries that have the same name as one of the new
files.
-
- zin = new ZipInputStream(new FileInputStream(tempFile));
- entry = zin.getNextEntry();
+ String entryName = getEntryName();
+ byte[] buffer = new byte[18024];
- while (entry != null) {
- String name = entry.getName();
+ try (OutputStream dest = HopVfs.getOutputStream(output, false)) {
+ countingOut = new CountingOutputStream(dest);
+ try (ZipOutputStream out = new ZipOutputStream(new
BufferedOutputStream(countingOut))) {
+ if (updateZip) {
+ // Copy the existing entries, except the one the source file
replaces
+ copyZipEntries(data.zipFile, out, entryName, buffer);
+ }
- if (!fileSet.contains(name)) {
+ // Set the method
+ out.setMethod(ZipOutputStream.DEFLATED);
+ out.setLevel(Deflater.BEST_COMPRESSION);
- // Add ZIP entry to output stream.
- out.putNextEntry(new ZipEntry(name));
- // Transfer bytes from the ZIP file to the output file
+ // Add ZIP entry to output stream.
+ //
+ out.putNextEntry(new ZipEntry(entryName));
+ try (InputStream in = HopVfs.getInputStream(data.sourceFile)) {
+ countingIn = new CountingInputStream(in);
int len;
- while ((len = zin.read(buffer)) > 0) {
+ while ((len = countingIn.read(buffer)) > 0) {
out.write(buffer, 0, len);
}
-
- fileSet.add(name);
}
- entry = zin.getNextEntry();
+ out.closeEntry();
}
- // Close the streams
- zin.close();
}
- // Set the method
- out.setMethod(ZipOutputStream.DEFLATED);
- out.setLevel(Deflater.BEST_COMPRESSION);
-
- // Associate a file input stream for the current file
- countingIn = new
CountingInputStream(HopVfs.getInputStream(data.sourceFile));
- in = countingIn;
+ if (updateZip) {
+ moving = true;
+ HopVfs.moveFile(output, data.zipFile);
+ }
+ } catch (Exception e) {
+ if (updateZip && output != data.zipFile) {
+ discardTemporaryZipFile(output, data.zipFile, moving);
+ }
+ throw new HopException(BaseMessages.getString(PKG,
"ZipFile.ErrorCreatingZip"), e);
+ } finally {
+ if (countingIn != null) {
+ dataVolumeIn = (dataVolumeIn != null ? dataVolumeIn : 0L) +
countingIn.getCount();
+ }
+ if (countingOut != null) {
+ dataVolumeOut = (dataVolumeOut != null ? dataVolumeOut : 0L) +
countingOut.getCount();
+ }
+ }
+ }
- // Add ZIP entry to output stream.
- //
- String relativeName = data.sourceFile.getName().getBaseName();
+ /** The name of the source file in the zip. */
+ private String getEntryName() throws HopException, FileSystemException {
+ FileName sourceName = data.sourceFile.getName();
+ if (!meta.isKeepSourceFolder()) {
+ return sourceName.getBaseName();
+ }
+ if ("file".equals(sourceName.getScheme())) {
+ // Local files keep the names they always had: the full path, without
the base folder.
+ String relativeName = HopVfs.getFilename(data.sourceFile);
+ if (data.baseFolder != null) {
+ relativeName = relativeName.replace(data.baseFolder +
Const.FILE_SEPARATOR, "");
+ }
+ return relativeName;
+ }
+ // A VFS file. Don't put the scheme in the zip.
+ if (!Utils.isEmpty(data.baseFolder)) {
+ FileName baseName = HopVfs.getFileObject(data.baseFolder,
variables).getName();
+ if (baseName.isDescendent(sourceName)) {
+ // Relative to the base folder
+ return baseName.getRelativeName(sourceName);
+ }
+ }
+ return sourceName.getPath();
+ }
- if (meta.isKeepSourceFolder()) {
- // Get full filename
- relativeName = HopVfs.getFilename(data.sourceFile);
+ /** A new file in the folder of the zip file, on the same file system. */
+ private static FileObject createTemporaryZipFile(FileObject zipFile) throws
IOException {
+ FileObject temporaryFile;
+ do {
+ temporaryFile =
+ zipFile
+ .getParent()
+ .resolveFile(zipFile.getName().getBaseName() + "_" +
UUID.randomUUID() + ".tmp");
+ } while (temporaryFile.exists());
+ return temporaryFile;
+ }
- if (data.baseFolder != null) {
- // Remove base folder
- data.baseFolder += Const.FILE_SEPARATOR;
- relativeName = relativeName.replace(data.baseFolder, "");
+ private static void copyZipEntries(
+ FileObject zipFile, ZipOutputStream out, String skipEntryName, byte[]
buffer)
+ throws IOException {
+ // A zip can hold the same name twice. Keep the first one, a zip output
stream refuses both.
+ Set<String> copied = new HashSet<>();
+ try (ZipInputStream zin = new
ZipInputStream(HopVfs.getInputStream(zipFile))) {
+ ZipEntry entry;
+ while ((entry = zin.getNextEntry()) != null) {
+ String name = entry.getName();
+ if (!name.equals(skipEntryName) && copied.add(name)) {
Review Comment:
**[suggestion]** `copyZipEntries` only skips `skipEntryName`. With "Keep
source folder" enabled, a non-`file` source used to be stored as
`HopVfs.getFilename` (the full URI, for example `minio:///bucket/in/src.txt`).
Creating that zip on VFS already worked; only the update failed. This change
stores `FileName.getPath()` or a path relative to the base folder, so an update
adds the new name and leaves the old URI entry in the archive. "Overwrite zip
entry" therefore does not replace the file in archives this option already
wrote.
**Suggestion:** When keep-source-folder is on and the scheme is not `file`,
also skip `HopVfs.getFilename(data.sourceFile)`. Add a unit test that starts
from a zip whose only entry is that URI and checks the update leaves just the
new path.
##########
plugins/transforms/zipfile/src/main/java/org/apache/hop/pipeline/transforms/zipfile/ZipFile.java:
##########
@@ -336,158 +337,160 @@ private void addFilenameToResult() {
}
}
- private File getFile(final String filename) {
- try {
- URI uri = new URI(filename);
- return new File(uri);
- } catch (URISyntaxException | IllegalArgumentException ex) {
- // Not a file:// URI but a plain path such as /tmp/archive.zip or
C:\archive.zip
- }
- return new File(filename);
- }
-
private void zipFile() throws HopException {
- String localrealZipfilename = HopVfs.getFilename(data.zipFile);
+ // Update an existing zip by writing the new version next to it and moving
it over the
+ // original once complete. If anything fails, the original zip is left
untouched.
boolean updateZip = false;
-
- byte[] buffer = null;
- OutputStream dest = null;
- BufferedOutputStream buff = null;
- ZipOutputStream out = null;
- InputStream in = null;
+ boolean moving = false;
+ FileObject output = data.zipFile;
CountingInputStream countingIn = null;
CountingOutputStream countingOut = null;
- ZipInputStream zin = null;
- ZipEntry entry = null;
- File tempFile = null;
- HashSet<String> fileSet = new HashSet<>();
try {
-
- updateZip = (data.zipFile.exists() && meta.isOverwriteZipEntry());
-
+ updateZip = data.zipFile.exists() && meta.isOverwriteZipEntry();
if (updateZip) {
- // the Zipfile exists
- // and we weed to update entries
- // Let's create a temp file
- File fileZip = getFile(localrealZipfilename);
- // Create the temporary file next to the zip file, not in the shared
system temp folder: the
- // rename below then stays on the same file system and no other user
can claim the name.
- tempFile =
- File.createTempFile(fileZip.getName(), null,
fileZip.getAbsoluteFile().getParentFile());
- // delete it, otherwise we cannot rename existing zip to it.
- tempFile.delete();
-
- updateZip = fileZip.renameTo(tempFile);
+ output = createTemporaryZipFile(data.zipFile);
}
- // Prepare Zip File
- buffer = new byte[18024];
- countingOut =
- new
CountingOutputStream(HopVfs.getOutputStream(localrealZipfilename, false,
variables));
- dest = countingOut;
- buff = new BufferedOutputStream(dest);
- out = new ZipOutputStream(buff);
-
- if (updateZip) {
- // User want to append files to existing Zip file
- // The idea is to rename the existing zip file to a temporary file
- // and then adds all entries in the existing zip along with the new
files,
- // excluding the zip entries that have the same name as one of the new
files.
-
- zin = new ZipInputStream(new FileInputStream(tempFile));
- entry = zin.getNextEntry();
+ String entryName = getEntryName();
+ byte[] buffer = new byte[18024];
- while (entry != null) {
- String name = entry.getName();
+ try (OutputStream dest = HopVfs.getOutputStream(output, false)) {
+ countingOut = new CountingOutputStream(dest);
+ try (ZipOutputStream out = new ZipOutputStream(new
BufferedOutputStream(countingOut))) {
+ if (updateZip) {
+ // Copy the existing entries, except the one the source file
replaces
+ copyZipEntries(data.zipFile, out, entryName, buffer);
+ }
- if (!fileSet.contains(name)) {
+ // Set the method
+ out.setMethod(ZipOutputStream.DEFLATED);
+ out.setLevel(Deflater.BEST_COMPRESSION);
- // Add ZIP entry to output stream.
- out.putNextEntry(new ZipEntry(name));
- // Transfer bytes from the ZIP file to the output file
+ // Add ZIP entry to output stream.
+ //
+ out.putNextEntry(new ZipEntry(entryName));
+ try (InputStream in = HopVfs.getInputStream(data.sourceFile)) {
+ countingIn = new CountingInputStream(in);
int len;
- while ((len = zin.read(buffer)) > 0) {
+ while ((len = countingIn.read(buffer)) > 0) {
out.write(buffer, 0, len);
}
-
- fileSet.add(name);
}
- entry = zin.getNextEntry();
+ out.closeEntry();
}
- // Close the streams
- zin.close();
}
- // Set the method
- out.setMethod(ZipOutputStream.DEFLATED);
- out.setLevel(Deflater.BEST_COMPRESSION);
-
- // Associate a file input stream for the current file
- countingIn = new
CountingInputStream(HopVfs.getInputStream(data.sourceFile));
- in = countingIn;
+ if (updateZip) {
+ moving = true;
+ HopVfs.moveFile(output, data.zipFile);
+ }
+ } catch (Exception e) {
+ if (updateZip && output != data.zipFile) {
+ discardTemporaryZipFile(output, data.zipFile, moving);
+ }
+ throw new HopException(BaseMessages.getString(PKG,
"ZipFile.ErrorCreatingZip"), e);
+ } finally {
+ if (countingIn != null) {
+ dataVolumeIn = (dataVolumeIn != null ? dataVolumeIn : 0L) +
countingIn.getCount();
+ }
+ if (countingOut != null) {
+ dataVolumeOut = (dataVolumeOut != null ? dataVolumeOut : 0L) +
countingOut.getCount();
+ }
+ }
+ }
- // Add ZIP entry to output stream.
- //
- String relativeName = data.sourceFile.getName().getBaseName();
+ /** The name of the source file in the zip. */
+ private String getEntryName() throws HopException, FileSystemException {
+ FileName sourceName = data.sourceFile.getName();
+ if (!meta.isKeepSourceFolder()) {
+ return sourceName.getBaseName();
+ }
+ if ("file".equals(sourceName.getScheme())) {
+ // Local files keep the names they always had: the full path, without
the base folder.
+ String relativeName = HopVfs.getFilename(data.sourceFile);
+ if (data.baseFolder != null) {
+ relativeName = relativeName.replace(data.baseFolder +
Const.FILE_SEPARATOR, "");
+ }
+ return relativeName;
+ }
+ // A VFS file. Don't put the scheme in the zip.
+ if (!Utils.isEmpty(data.baseFolder)) {
+ FileName baseName = HopVfs.getFileObject(data.baseFolder,
variables).getName();
+ if (baseName.isDescendent(sourceName)) {
+ // Relative to the base folder
+ return baseName.getRelativeName(sourceName);
+ }
+ }
+ return sourceName.getPath();
+ }
- if (meta.isKeepSourceFolder()) {
- // Get full filename
- relativeName = HopVfs.getFilename(data.sourceFile);
+ /** A new file in the folder of the zip file, on the same file system. */
+ private static FileObject createTemporaryZipFile(FileObject zipFile) throws
IOException {
+ FileObject temporaryFile;
+ do {
+ temporaryFile =
+ zipFile
+ .getParent()
+ .resolveFile(zipFile.getName().getBaseName() + "_" +
UUID.randomUUID() + ".tmp");
+ } while (temporaryFile.exists());
+ return temporaryFile;
+ }
- if (data.baseFolder != null) {
- // Remove base folder
- data.baseFolder += Const.FILE_SEPARATOR;
- relativeName = relativeName.replace(data.baseFolder, "");
+ private static void copyZipEntries(
+ FileObject zipFile, ZipOutputStream out, String skipEntryName, byte[]
buffer)
+ throws IOException {
+ // A zip can hold the same name twice. Keep the first one, a zip output
stream refuses both.
+ Set<String> copied = new HashSet<>();
+ try (ZipInputStream zin = new
ZipInputStream(HopVfs.getInputStream(zipFile))) {
+ ZipEntry entry;
+ while ((entry = zin.getNextEntry()) != null) {
+ String name = entry.getName();
+ if (!name.equals(skipEntryName) && copied.add(name)) {
+ out.putNextEntry(new ZipEntry(name));
+ int len;
+ while ((len = zin.read(buffer)) > 0) {
+ out.write(buffer, 0, len);
+ }
+ out.closeEntry();
}
}
- if (!fileSet.contains(relativeName)) {
- out.putNextEntry(new ZipEntry(relativeName));
+ }
+ }
- int len;
- while ((len = in.read(buffer)) > 0) {
- out.write(buffer, 0, len);
- }
- }
+ /**
+ * Remove the temporary zip file after a failure. When moving it over the
zip file failed after
+ * the zip file was already removed, the temporary file is the only copy
left: keep it.
+ */
+ private void discardTemporaryZipFile(
+ FileObject temporaryFile, FileObject zipFile, boolean moving) {
+ if (moving && !existsQuietly(zipFile)) {
Review Comment:
**[suggestion]** `discardTemporaryZipFile` deletes the temporary zip
whenever the destination still exists. `moving` is set only around
`HopVfs.moveFile`, and that helper is not atomic: `FileObject.moveTo` deletes
the destination before the rename, then on failure falls back to `copyFrom`,
which truncates the destination as soon as it opens. If that copy fails, the
destination exists but is a partial zip, and this method deletes the only
complete copy. `failedMoveWithTheZipStillThereRemovesTheTemporaryFile` does not
catch that, because it stubs `HopVfs.moveFile` to throw before the real
fallback runs. `ActionZipFile.discardTemporaryZipFile` (the `moving &&
!existsQuietly(zipFile)` check) has the same assumption.
**Suggestion:** Before the move, remember the destination size, or that it
was absent. On failure, delete the temp only when the destination is still
present and still that size. If it is missing or the size changed, keep the
temp and log where it is. Do this in both `ZipFile` and `ActionZipFile`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
