Blazer-007 commented on code in PR #4117:
URL: https://github.com/apache/gobblin/pull/4117#discussion_r2099226373
##########
gobblin-data-management/src/main/java/org/apache/gobblin/util/commit/CreateDirectoryWithPermissionsCommitStep.java:
##########
@@ -75,7 +76,7 @@ public void execute() throws IOException {
try {
// Is a no-op if directory already exists, stops when it hits first
parent
// Sets the execute bit for USER in order to rename files to the
folder, so it should be reset after this step is completed
- HadoopUtils.ensureDirectoryExists(fs, path,
entry.getValue().listIterator(), throwOnError);
+ HadoopUtils.ensureDirectoryExists(fs, path,
entry.getValue().listIterator(), throwOnError, true);
Review Comment:
Can we add a single line comment saying directory is created with only
source acl similar to `// Sets the execute bit for USER in order to rename
files to the folder, so it should be reset after this step is completed`
written above ?
##########
gobblin-utility/src/test/java/org/apache/gobblin/util/HadoopUtilsTest.java:
##########
@@ -333,4 +344,166 @@ public void testMoveToTrash() throws IOException {
Assert.assertFalse(fs.exists(hadoopUtilsTestDir));
Assert.assertTrue(fs.exists(trashPath));
}
+
+ @Test
+ public void testEnsureDirectoryExistsWithAclPreservation() throws Exception {
+ final Path testDir = new Path(new Path(TEST_DIR_PATH),
"HadoopUtilsTestDir");
+ FileSystem fs = Mockito.mock(FileSystem.class);
+ Path targetDir = new Path(testDir, "target");
+
+ Mockito.when(fs.exists(targetDir)).thenReturn(false);
+ Mockito.when(fs.exists(targetDir.getParent())).thenReturn(true);
+
+ // Create ACL entries
+ List<AclEntry> aclEntries = Lists.newArrayList(
+ new AclEntry.Builder()
+ .setName("user1")
+ .setType(AclEntryType.USER)
+ .setScope(AclEntryScope.ACCESS)
+ .setPermission(FsAction.ALL)
+ .build(),
+ new AclEntry.Builder()
+ .setName("group1")
+ .setType(AclEntryType.GROUP)
+ .setScope(AclEntryScope.ACCESS)
+ .setPermission(FsAction.READ_EXECUTE)
+ .build()
+ );
+
+ // Create OwnerAndPermission with the ACLs
+ OwnerAndPermission ownerAndPermission =
getOwnerAndPermissionForAclEntries(aclEntries);
+
+ // Mock mkdirs to return true
+ Mockito.when(fs.mkdirs(targetDir)).thenReturn(true);
+ // Call ensureDirectoryExists with copyOnlySourceAclToDest=true
+ HadoopUtils.ensureDirectoryExists(fs, targetDir,
+ Collections.singletonList(ownerAndPermission).listIterator(),
+ true, true);
+ // Verify mkdirs was called
+ Mockito.verify(fs).mkdirs(targetDir);
+ Mockito.verify(fs).removeAcl(targetDir);
+ // Verify modifyAclEntries was called with correct ACLs
+ Mockito.verify(fs).modifyAclEntries(targetDir, aclEntries);
+ }
+
+ @Test
+ public void testEnsureDirectoryExistsWithExistingDirectory() throws
Exception {
+ final Path testDir = new Path(new Path(TEST_DIR_PATH),
TEST_CHILD_DIR_NAME);
+ FileSystem fs = Mockito.mock(FileSystem.class);
+ // Create target directory path
+ Path targetDir = new Path(testDir, "target");
+
+ Mockito.when(fs.exists(targetDir)).thenReturn(true);
+ Mockito.when(fs.exists(targetDir.getParent())).thenReturn(true);
Review Comment:
Can this be combined in one line ?
##########
gobblin-utility/src/test/java/org/apache/gobblin/util/HadoopUtilsTest.java:
##########
@@ -20,18 +20,25 @@
import java.io.IOException;
import java.net.URI;
import java.nio.file.AccessDeniedException;
+import java.util.Collections;
+import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.util.filesystem.OwnerAndPermission;
Review Comment:
import order
##########
gobblin-utility/src/main/java/org/apache/gobblin/util/HadoopUtils.java:
##########
@@ -761,18 +761,73 @@ public static void safeRenameRecursively(FileSystem
fileSystem, Path from, Path
}
}
+ /**
+ * Creates a directory with the given path and enforces the given owner and
permissions recursively all the way up to root,
+ * or until the list of owner and permissions is exhausted.
+ *
+ * This is a convenience method that delegates to {@link
#ensureDirectoryExists(FileSystem, Path, Iterator, boolean, boolean)}
+ * with copyOnlySourceAclToDest set to false, meaning it will preserve any
existing ACLs on the target directory.
+ *
+ * This method will:
+ * 1. Create the directory if it doesn't exist
+ * 2. Apply owner, group, and permission settings from the
OwnerAndPermission iterator
+ * 3. Apply ACL entries if present in the OwnerAndPermission
+ * 4. Recursively create and set permissions for parent directories if they
don't exist
+ *
+ * If any of the parent directories already exists, it will not overwrite
the existing permissions
+ * and this function will be a no-op for those directories.
+ *
+ * The method will add execute permission to the owner of each directory to
ensure proper access.
+ *
+ * @param fs The FileSystem instance to use for operations
+ * @param path The path of the directory to create
+ * @param ownerAndPermissionIterator Iterator containing OwnerAndPermission
objects for each level of the directory hierarchy.
Review Comment:
this looks duplicated can we just link the other function here directly
instead of copy pasting same java doc
`see {@link #ensureDirectoryExists(...)}`
--
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: dev-unsubscr...@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
