[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r837628148
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -773,26 +773,64 @@ public void testCreateLocalTempFile() throws IOException {
public void testUnZip() throws Exception {
// make sa simple zip
final File simpleZip = new File(del, FILE);
-OutputStream os = new FileOutputStream(simpleZip);
-ZipOutputStream tos = new ZipOutputStream(os);
-try {
- ZipEntry ze = new ZipEntry("foo");
- byte[] data = "some-content".getBytes("UTF-8");
- ze.setSize(data.length);
- tos.putNextEntry(ze);
- tos.write(data);
- tos.closeEntry();
+try (OutputStream os = new FileOutputStream(simpleZip);
+ ZipArchiveOutputStream tos = new ZipArchiveOutputStream(os)) {
+ List ZipArchiveList = new ArrayList<>(7);
+ int count = 0;
+ // create 7 files to verify permissions
+ for (int i = 0; i < 7; i++) {
+ZipArchiveList.add(new ZipArchiveEntry("foo_" + i));
+ZipArchiveEntry archiveEntry = ZipArchiveList.get(i);
+archiveEntry.setUnixMode(count += 0100);
+byte[] data = "some-content".getBytes("UTF-8");
+archiveEntry.setSize(data.length);
+tos.putArchiveEntry(archiveEntry);
+tos.write(data);
+ }
+ tos.closeArchiveEntry();
tos.flush();
tos.finish();
-} finally {
- tos.close();
}
-
+
// successfully unzip it into an existing dir:
FileUtil.unZip(simpleZip, tmp);
// check result:
-Verify.exists(new File(tmp, "foo"));
-assertEquals(12, new File(tmp, "foo").length());
+assertTrue(new File(tmp, "foo_0").exists());
Review comment:
can you assign each of these to a variable and reuse in the assertions?
there's a lot of duplication
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r835272119
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -677,6 +683,31 @@ public static void unZip(InputStream inputStream, File
toDir)
}
}
+ public static Set permissionsFromMode(int mode) {
Review comment:
1. can you add some javadocs to say that these only deal with
user/group/other and not suid
2. can you make package scoped so it can be called in tests, but not by
applications (so we don't have to maintain consistent behaviour forever)
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -677,6 +683,31 @@ public static void unZip(InputStream inputStream, File
toDir)
}
}
+ public static Set permissionsFromMode(int mode) {
+EnumSet permissions =
+EnumSet.noneOf(PosixFilePermission.class);
+addPermissions(permissions, "OTHERS", (long) mode);
+addPermissions(permissions, "GROUP", (long) mode >> 3);
+addPermissions(permissions, "OWNER", (long) mode >> 6);
+return permissions;
+ }
+
+ /** Assign the original permissions to the file */
Review comment:
1. add params to javadoc and staqte that only bottom 7 bits are used
2. make package private
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -677,6 +683,31 @@ public static void unZip(InputStream inputStream, File
toDir)
}
}
+ public static Set permissionsFromMode(int mode) {
+EnumSet permissions =
+EnumSet.noneOf(PosixFilePermission.class);
+addPermissions(permissions, "OTHERS", (long) mode);
+addPermissions(permissions, "GROUP", (long) mode >> 3);
+addPermissions(permissions, "OWNER", (long) mode >> 6);
+return permissions;
+ }
+
+ /** Assign the original permissions to the file */
+ public static void addPermissions(
+ Set permissions,
+ String prefix,
+ Long mode) {
Review comment:
nit, should be int
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r832391092
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -706,35 +706,40 @@ public void testCreateLocalTempFile() throws IOException {
public void testUnZip() throws IOException {
// make sa simple zip
final File simpleZip = new File(del, FILE);
-OutputStream os = new FileOutputStream(simpleZip);
-ZipOutputStream tos = new ZipOutputStream(os);
-try {
- ZipEntry ze = new ZipEntry("foo");
- byte[] data = "some-content".getBytes("UTF-8");
- ze.setSize(data.length);
- tos.putNextEntry(ze);
- tos.write(data);
- tos.closeEntry();
- tos.flush();
- tos.finish();
-} finally {
- tos.close();
-}
-
-// successfully unzip it into an existing dir:
-FileUtil.unZip(simpleZip, tmp);
-// check result:
-assertTrue(new File(tmp, "foo").exists());
-assertEquals(12, new File(tmp, "foo").length());
-
-final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog");
-regularFile.createNewFile();
-assertTrue(regularFile.exists());
-try {
- FileUtil.unZip(simpleZip, regularFile);
- assertTrue("An IOException expected.", false);
-} catch (IOException ioe) {
- // okay
+try (OutputStream os = new FileOutputStream(simpleZip);
+ ZipArchiveOutputStream tos = new ZipArchiveOutputStream(os)) {
+ try {
+ZipArchiveEntry ze = new ZipArchiveEntry("foo");
+ze.setUnixMode(0555);
+byte[] data = "some-content".getBytes("UTF-8");
+ze.setSize(data.length);
+tos.putArchiveEntry(ze);
+tos.write(data);
+tos.closeArchiveEntry();
+tos.flush();
+tos.finish();
+ } finally {
+tos.close();
Review comment:
this close will be automatic. if you make the reference on L710
```
ZipArchiveOutputStream tos = new ZipArchiveOutputStream(new
FileOutputStream(simpleZip))
```
then everything will be lined up for automatic close
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -706,35 +706,40 @@ public void testCreateLocalTempFile() throws IOException {
public void testUnZip() throws IOException {
// make sa simple zip
final File simpleZip = new File(del, FILE);
-OutputStream os = new FileOutputStream(simpleZip);
-ZipOutputStream tos = new ZipOutputStream(os);
-try {
- ZipEntry ze = new ZipEntry("foo");
- byte[] data = "some-content".getBytes("UTF-8");
- ze.setSize(data.length);
- tos.putNextEntry(ze);
- tos.write(data);
- tos.closeEntry();
- tos.flush();
- tos.finish();
-} finally {
- tos.close();
-}
-
-// successfully unzip it into an existing dir:
-FileUtil.unZip(simpleZip, tmp);
-// check result:
-assertTrue(new File(tmp, "foo").exists());
-assertEquals(12, new File(tmp, "foo").length());
-
-final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog");
-regularFile.createNewFile();
-assertTrue(regularFile.exists());
-try {
- FileUtil.unZip(simpleZip, regularFile);
- assertTrue("An IOException expected.", false);
-} catch (IOException ioe) {
- // okay
+try (OutputStream os = new FileOutputStream(simpleZip);
+ ZipArchiveOutputStream tos = new ZipArchiveOutputStream(os)) {
+ try {
+ZipArchiveEntry ze = new ZipArchiveEntry("foo");
+ze.setUnixMode(0555);
+byte[] data = "some-content".getBytes("UTF-8");
+ze.setSize(data.length);
+tos.putArchiveEntry(ze);
+tos.write(data);
+tos.closeArchiveEntry();
+tos.flush();
+tos.finish();
+ } finally {
+tos.close();
+ }
+
+ // successfully unzip it into an existing dir:
+ FileUtil.unZip(simpleZip, tmp);
+ // check result:
+ assertTrue(new File(tmp, "foo").exists());
+ assertEquals(12, new File(tmp, "foo").length());
+ assertTrue("file lacks execute permissions", new File(tmp,
"foo").canExecute());
+ assertFalse("file has write permissions", new File(tmp,
"foo").canWrite());
+ assertTrue("file lacks read permissions", new File(tmp,
"foo").canRead());
+
+ final File regularFile = new File(tmp,
"QuickBrownFoxJumpsOverTheLazyDog");
+ regularFile.createNewFile();
+ assertTrue(regularFile.exists());
+ try {
Review comment:
use LambdaTestUtils.intercept() here. e.g.
```java
LambdaTestUtils.intercept(IOException.class, () ->
FileUtil.unZip(simpleZip, regularFile));
```
--
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.
T
[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r832236126
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -667,6 +670,9 @@ public static void unZip(InputStream inputStream, File
toDir)
if (!file.setLastModified(entry.getTime())) {
numOfFailedLastModifiedSet++;
}
+ if (entry.getPlatform() == ZipArchiveEntry.PLATFORM_UNIX) {
+Files.setPosixFilePermissions(file.toPath(),
permissionsFromMode(entry.getUnixMode()));
Review comment:
ok
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r823134783
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -726,7 +727,10 @@ public void testUnZip() throws IOException {
// check result:
assertTrue(new File(tmp, "foo").exists());
assertEquals(12, new File(tmp, "foo").length());
-
+assertTrue(new File(tmp, "foo").canExecute());
Review comment:
can you add ext to the asserts for permissons, e.g
```java
assertTrue("file lacks execute permissions", new File(tmp,
"foo").canExecute());
```
imagine a yetus test run failed? what do you want in the test reports. a
line number or a message?
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -706,15 +706,16 @@ public void testCreateLocalTempFile() throws IOException {
public void testUnZip() throws IOException {
// make sa simple zip
final File simpleZip = new File(del, FILE);
-OutputStream os = new FileOutputStream(simpleZip);
-ZipOutputStream tos = new ZipOutputStream(os);
+OutputStream os = new FileOutputStream(simpleZip);
+ZipArchiveOutputStream tos = new ZipArchiveOutputStream(os);
Review comment:
if you move this to try with resources, the close will be automatic, but
I'm not worried about this, as its test code and it keeps the patch smaller
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…
steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r815939653
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -36,8 +36,11 @@
import java.nio.charset.CharsetEncoder;
import java.nio.charset.StandardCharsets;
import java.nio.file.AccessDeniedException;
+import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.FileSystems;
import java.nio.file.Files;
+
+import java.util.*;
Review comment:
no .* expansions, just add any new imports into the existing ones. and
tell your IDE to stop combining imports into .* for future hadoop prs. please:
imports are backport pain
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -667,6 +670,9 @@ public static void unZip(InputStream inputStream, File
toDir)
if (!file.setLastModified(entry.getTime())) {
numOfFailedLastModifiedSet++;
}
+ if (entry.getPlatform() == ZipArchiveEntry.PLATFORM_UNIX) {
+Files.setPosixFilePermissions(file.toPath(),
permissionsFromMode(entry.getUnixMode()));
Review comment:
what if this fails?
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -726,7 +727,10 @@ public void testUnZip() throws IOException {
// check result:
assertTrue(new File(tmp, "foo").exists());
assertEquals(12, new File(tmp, "foo").length());
-
+assertTrue(new File(tmp, "foo").canExecute());
Review comment:
asserttrue tests need to say what is wrong when the assert fails. the
goal is to make a failed yetus/test run meaningful.
##
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##
@@ -726,7 +727,10 @@ public void testUnZip() throws IOException {
// check result:
assertTrue(new File(tmp, "foo").exists());
assertEquals(12, new File(tmp, "foo").length());
-
+assertTrue(new File(tmp, "foo").canExecute());
+assertTrue(new File(tmp, "foo").canRead());
+assertTrue(new File(tmp, "foo").canWrite());
Review comment:
1. there's no tests to verify that other/group permissions are picked
up. not sure if it is possible.
2. what happens if someone runs the test on windows?
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -676,6 +682,31 @@ public static void unZip(InputStream inputStream, File
toDir)
}
}
+ public static Set permissionsFromMode(Integer mode) {
Review comment:
is mode ever null?
##
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##
@@ -715,6 +746,9 @@ public static void unZip(File inFile, File unzipDir) throws
IOException {
}
} finally {
out.close();
+ if (entry.getPlatform() == ZipArchiveEntry.PLATFORM_UNIX) {
Review comment:
this is going to execute even if writing to out failed. not sure that is
ideal.
better to put it on a line after the try/finally block. so is only called if
the write succeeded.
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
