Copilot commented on code in PR #4149:
URL: https://github.com/apache/gobblin/pull/4149#discussion_r2476707914
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. The correct order is
`Assert.assertEquals(expected, actual)`. This should be
`Assert.assertEquals(existingAcls, fs.getAclEntries(testPath))`.
```suggestion
Assert.assertEquals(existingAcls, fs.getAclEntries(testPath));
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
+ }
+ }
+
+ /**
+ * Test that setPathPermission works correctly for files (not just
directories).
+ */
+ @Test
+ public void testSetPathPermissionForFile() throws Exception {
+ Path testPath = new Path(testTempPath, "testFile.txt");
+ fs.create(testPath).close();
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs on file
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Replace with new ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("-rw-r--r--"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were replaced
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, newAcls);
+ }
+
+ /**
+ * Test that setPathPermission handles multiple ACL entries correctly.
+ */
+ @Test
+ public void testSetPathPermissionWithMultipleAclEntries() throws Exception {
+ Path testPath = new Path(testTempPath, "testMultipleAcls");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Create multiple ACL entries
+ List<AclEntry> multipleAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:alice:rwx", true),
+ AclEntry.parseAclEntry("user:bob:r-x", true),
+ AclEntry.parseAclEntry("user:charlie:rw-", true),
+ AclEntry.parseAclEntry("group:developers:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), multipleAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify all ACLs were set
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 4);
+ Assert.assertEquals(resultAcls, multipleAcls);
+ }
+
+ /**
+ * Test that setPathPermission works with directory execute bit handling.
+ * Directories without owner execute bit get it added automatically.
+ */
+ @Test
+ public void testSetPathPermissionWithDirectoryExecuteBit() throws Exception {
+ Path testPath = new Path(testTempPath, "testDirExecuteBit");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rw-", true)
+ );
+ // Directory permission without owner execute bit
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drw-rw-rw-"), acls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were set even with execute bit handling
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, acls);
+ }
+
+ /**
+ * Test that setPathPermission correctly handles the scenario where a
directory
+ * has existing ACLs that need to be completely replaced.
+ */
+ @Test
+ public void testSetPathPermissionReplacesAllExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testCompleteReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set multiple existing ACLs
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:user1:rwx", true),
+ AclEntry.parseAclEntry("user:user2:r-x", true),
+ AclEntry.parseAclEntry("group:group1:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+
+ // Replace with completely different ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:differentuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify complete replacement
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ // Verify none of the old ACLs exist
+ for (AclEntry oldAcl : existingAcls) {
+ Assert.assertFalse(resultAcls.contains(oldAcl));
+ }
+ }
+
+ /**
+ * Test that permissions are still set correctly when ACL operations are
performed.
+ */
+ @Test
+ public void testSetPathPermissionSetsPermissionsCorrectly() throws Exception
{
+ Path testPath = new Path(testTempPath, "testPermissions");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ FsPermission permission = FsPermission.valueOf("drwxr-x---");
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
permission, acls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were set
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ Assert.assertNotNull(fs.getAclEntries(testPath));
+
+ // Verify permissions were also set (LocalFileSystem adds execute bit for
directories)
+ FileStatus updatedStatus = fs.getFileStatus(testPath);
+
Assert.assertTrue(updatedStatus.getPermission().getUserAction().implies(FsAction.EXECUTE));
+ }
+
+ /**
+ * Test that when destination has ACLs but source has no ACLs (empty list),
+ * all ACLs are removed from destination when removeExistingAcls=true.
+ * This ensures destination matches source exactly (no ACLs).
+ */
+ @Test
+ public void testSetPathPermissionRemovesDestinationAclsWhenSourceHasNoAcls()
throws Exception {
+ Path testPath = new Path(testTempPath, "testEmptySourceAcls");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Destination has existing ACLs
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:user1:rwx", true),
+ AclEntry.parseAclEntry("user:user2:r-x", true),
+ AclEntry.parseAclEntry("group:group1:rw-", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. The correct order is
`Assert.assertEquals(expected, actual)`. This should be
`Assert.assertEquals(existingAcls, fs.getAclEntries(testPath))`.
```suggestion
Assert.assertEquals(existingAcls, fs.getAclEntries(testPath));
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
+ }
+ }
+
+ /**
+ * Test that setPathPermission works correctly for files (not just
directories).
+ */
+ @Test
+ public void testSetPathPermissionForFile() throws Exception {
+ Path testPath = new Path(testTempPath, "testFile.txt");
+ fs.create(testPath).close();
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs on file
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Replace with new ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("-rw-r--r--"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were replaced
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, newAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. This should be
`Assert.assertEquals(newAcls, resultAcls)`.
```suggestion
Assert.assertEquals(newAcls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
+ }
+ }
+
+ /**
+ * Test that setPathPermission works correctly for files (not just
directories).
+ */
+ @Test
+ public void testSetPathPermissionForFile() throws Exception {
+ Path testPath = new Path(testTempPath, "testFile.txt");
+ fs.create(testPath).close();
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs on file
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Replace with new ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("-rw-r--r--"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were replaced
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, newAcls);
+ }
+
+ /**
+ * Test that setPathPermission handles multiple ACL entries correctly.
+ */
+ @Test
+ public void testSetPathPermissionWithMultipleAclEntries() throws Exception {
+ Path testPath = new Path(testTempPath, "testMultipleAcls");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Create multiple ACL entries
+ List<AclEntry> multipleAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:alice:rwx", true),
+ AclEntry.parseAclEntry("user:bob:r-x", true),
+ AclEntry.parseAclEntry("user:charlie:rw-", true),
+ AclEntry.parseAclEntry("group:developers:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), multipleAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify all ACLs were set
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 4);
+ Assert.assertEquals(resultAcls, multipleAcls);
+ }
+
+ /**
+ * Test that setPathPermission works with directory execute bit handling.
+ * Directories without owner execute bit get it added automatically.
+ */
+ @Test
+ public void testSetPathPermissionWithDirectoryExecuteBit() throws Exception {
+ Path testPath = new Path(testTempPath, "testDirExecuteBit");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rw-", true)
+ );
+ // Directory permission without owner execute bit
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drw-rw-rw-"), acls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were set even with execute bit handling
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, acls);
+ }
+
+ /**
+ * Test that setPathPermission correctly handles the scenario where a
directory
+ * has existing ACLs that need to be completely replaced.
+ */
+ @Test
+ public void testSetPathPermissionReplacesAllExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testCompleteReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set multiple existing ACLs
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:user1:rwx", true),
+ AclEntry.parseAclEntry("user:user2:r-x", true),
+ AclEntry.parseAclEntry("group:group1:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+
+ // Replace with completely different ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:differentuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify complete replacement
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. Line 905 should be
`Assert.assertEquals(1, resultAcls.size())` and line 906 should be
`Assert.assertEquals(newAcls, resultAcls)`.
```suggestion
Assert.assertEquals(1, resultAcls.size());
Assert.assertEquals(newAcls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. Line 715 should be
`Assert.assertEquals(1, resultAcls.size())` and line 716 should be
`Assert.assertEquals(newAcls, resultAcls)`.
```suggestion
Assert.assertEquals(1, resultAcls.size());
Assert.assertEquals(newAcls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. This should be
`Assert.assertEquals(initialAcls, resultAcls)`.
```suggestion
Assert.assertEquals(initialAcls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
+ }
+ }
+
+ /**
+ * Test that setPathPermission works correctly for files (not just
directories).
+ */
+ @Test
+ public void testSetPathPermissionForFile() throws Exception {
+ Path testPath = new Path(testTempPath, "testFile.txt");
+ fs.create(testPath).close();
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs on file
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Replace with new ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("-rw-r--r--"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were replaced
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, newAcls);
+ }
+
+ /**
+ * Test that setPathPermission handles multiple ACL entries correctly.
+ */
+ @Test
+ public void testSetPathPermissionWithMultipleAclEntries() throws Exception {
+ Path testPath = new Path(testTempPath, "testMultipleAcls");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Create multiple ACL entries
+ List<AclEntry> multipleAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:alice:rwx", true),
+ AclEntry.parseAclEntry("user:bob:r-x", true),
+ AclEntry.parseAclEntry("user:charlie:rw-", true),
+ AclEntry.parseAclEntry("group:developers:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), multipleAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify all ACLs were set
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 4);
+ Assert.assertEquals(resultAcls, multipleAcls);
+ }
+
+ /**
+ * Test that setPathPermission works with directory execute bit handling.
+ * Directories without owner execute bit get it added automatically.
+ */
+ @Test
+ public void testSetPathPermissionWithDirectoryExecuteBit() throws Exception {
+ Path testPath = new Path(testTempPath, "testDirExecuteBit");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rw-", true)
+ );
+ // Directory permission without owner execute bit
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drw-rw-rw-"), acls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were set even with execute bit handling
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, acls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. This should be `Assert.assertEquals(acls,
resultAcls)`.
```suggestion
Assert.assertEquals(acls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. This should be `Assert.assertEquals(acls,
resultAcls)`.
```suggestion
Assert.assertEquals(acls, resultAcls);
```
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -684,15 +684,331 @@ public void cleanup() {
}
/**
- * Created this class to support `setAcl` method for {@link LocalFileSystem}
for unit testing since LocalFileSystem
- * doesn't provide any implementation for `setAcl` method
+ * Test that setPathPermission with removeExistingAcls=true removes existing
ACLs before adding new ones.
+ */
+ @Test
+ public void testSetPathPermissionRemovesExistingAcls() throws Exception {
+ Path testPath = new Path(testTempPath, "testAclReplacement");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Simulate existing ACLs on target directory
+ List<AclEntry> existingAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true),
+ AclEntry.parseAclEntry("user:anotheruser:r-x", true)
+ );
+ fs.modifyAclEntries(testPath, existingAcls);
+ Assert.assertEquals(fs.getAclEntries(testPath), existingAcls);
+
+ // Set new ACLs with removeExistingAcls=true
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:rw-", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify old ACLs were removed and only new ACLs exist
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertEquals(resultAcls.size(), 1);
+ Assert.assertEquals(resultAcls, newAcls);
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ }
+
+ /**
+ * Test that setPathPermission with removeExistingAcls=false does NOT remove
existing ACLs.
+ * This tests the default behavior where ACLs are added without removal.
+ */
+ @Test
+ public void testSetPathPermissionDoesNotRemoveExistingAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testAclAddition");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:existinguser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Add new ACLs with removeExistingAcls=false
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r-x", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, false);
+
+ // Verify removeAcl was NOT called
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify new ACLs were added
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ // Should have old as well as new acls
+ initialAcls.addAll(newAcls);
+ Assert.assertEquals(resultAcls, initialAcls);
+ }
+
+ /**
+ * Test that setPathPermission with default parameter (no
removeExistingAcls) defaults to false.
+ */
+ @Test
+ public void testSetPathPermissionDefaultBehaviorDoesNotRemoveAcls() throws
Exception {
+ Path testPath = new Path(testTempPath, "testDefaultBehavior");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ List<AclEntry> acls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:testuser:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), acls);
+
+ // Call without removeExistingAcls parameter (should default to false)
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false);
+
+ // Verify removeAcl was NOT called (default is false)
+ Assert.assertFalse(fs.wasRemoveAclCalled(testPath));
+ // Verify ACLs were set (if filesystem supports it)
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ if (resultAcls != null) {
+ Assert.assertEquals(resultAcls, acls);
+ }
+ }
+
+ /**
+ * Test that setPathPermission works correctly for files (not just
directories).
+ */
+ @Test
+ public void testSetPathPermissionForFile() throws Exception {
+ Path testPath = new Path(testTempPath, "testFile.txt");
+ fs.create(testPath).close();
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Set initial ACLs on file
+ List<AclEntry> initialAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:olduser:rwx", true)
+ );
+ fs.modifyAclEntries(testPath, initialAcls);
+
+ // Replace with new ACLs
+ List<AclEntry> newAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:newuser:r--", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("-rw-r--r--"), newAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify ACLs were replaced
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls, newAcls);
+ }
+
+ /**
+ * Test that setPathPermission handles multiple ACL entries correctly.
+ */
+ @Test
+ public void testSetPathPermissionWithMultipleAclEntries() throws Exception {
+ Path testPath = new Path(testTempPath, "testMultipleAcls");
+ fs.mkdirs(testPath);
+ FileStatus fileStatus = fs.getFileStatus(testPath);
+ testPath = fileStatus.getPath();
+
+ // Create multiple ACL entries
+ List<AclEntry> multipleAcls = Lists.newArrayList(
+ AclEntry.parseAclEntry("user:alice:rwx", true),
+ AclEntry.parseAclEntry("user:bob:r-x", true),
+ AclEntry.parseAclEntry("user:charlie:rw-", true),
+ AclEntry.parseAclEntry("group:developers:rwx", true)
+ );
+ OwnerAndPermission ownerAndPermission = new OwnerAndPermission(null, null,
+ FsPermission.valueOf("drwxr-xr-x"), multipleAcls);
+
+ FileAwareInputStreamDataWriter.setPathPermission(fs, fileStatus,
ownerAndPermission, false, true);
+
+ // Verify all ACLs were set
+ Assert.assertTrue(fs.wasRemoveAclCalled(testPath));
+ List<AclEntry> resultAcls = fs.getAclEntries(testPath);
+ Assert.assertNotNull(resultAcls);
+ Assert.assertEquals(resultAcls.size(), 4);
+ Assert.assertEquals(resultAcls, multipleAcls);
Review Comment:
Use `Assert.assertEquals` with expected value as the first parameter and
actual value as the second parameter. Line 842 should be
`Assert.assertEquals(4, resultAcls.size())` and line 843 should be
`Assert.assertEquals(multipleAcls, resultAcls)`.
```suggestion
Assert.assertEquals(multipleAcls, resultAcls);
```
--
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]
