Merge pull request #97 from kjsanger/bug/recursive-meta-and-perms

Fix incorrect recursive behaviour of add/remove/supersede_permissions

src/partisan/irods.py

@@ -1481,72 +1481,87 @@ def supersede_metadata(
         return len(to_remove), len(to_add)
 
     @rods_type_check
-    def add_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int:
+    def add_permissions(self, *acs: AC, timeout=None, tries=1) -> int:
         """Add access controls to the item. Return the number of access
         controls added. If some argument access controls are already present,
         those arguments will be ignored.
 
+        This method handles only the permissions of this RodsItem. See the Collection
+        class for methods handling recursive operations.
+
         Args:
             *acs: Access controls.
-            recurse: Recursively add access control.
             timeout: Operation timeout in seconds.
             tries: Number of times to try the operation.
 
         Returns: int
         """
         current = self.acl()
         to_add = sorted(set(acs).difference(current))
-        log.debug("Adding to ACL", path=self, curr=current, arg=acs, add=to_add)
+        log.debug(
+            "Adding to ACL",
+            path=self,
+            curr=current,
+            arg=acs,
+            add=to_add,
+        )
 
         if to_add:
             item = self._to_dict()
             item[Baton.ACCESS] = to_add
             with client(self.pool) as c:
-                c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries)
+                c.set_permission(item, timeout=timeout, tries=tries)
 
         return len(to_add)
 
     @rods_type_check
-    def remove_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int:
+    def remove_permissions(self, *acs: AC, timeout=None, tries=1) -> int:
         """Remove access controls from the item. Return the number of access
         controls removed. If some argument access controls are not present, those
         arguments will be ignored.
 
+        This method handles only the permissions of this RodsItem. See the Collection
+        class for methods handling recursive operations.
+
         Args:
             *acs: Access controls.
-            recurse: Recursively add access control.
             timeout: Operation timeout in seconds.
             tries: Number of times to try the operation.
 
         Returns: int
         """
         current = self.acl()
         to_remove = sorted(set(current).intersection(acs))
-        log.debug("Removing from ACL", path=self, curr=current, arg=acs, rem=to_remove)
+        log.debug(
+            "Removing from ACL",
+            path=self,
+            curr=current,
+            arg=acs,
+            rem=to_remove,
+        )
 
         if to_remove:
             # In iRODS we "remove" permissions by setting them to NULL
-            for ac in to_remove:
-                ac.perm = Permission.NULL
+            to_null = [AC(ac.user, Permission.NULL, zone=ac.zone) for ac in to_remove]
 
             item = self._to_dict()
-            item[Baton.ACCESS] = to_remove
+            item[Baton.ACCESS] = to_null
             with client(self.pool) as c:
-                c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries)
+                c.set_permission(item, timeout=timeout, tries=tries)
 
         return len(to_remove)
 
     @rods_type_check
-    def supersede_permissions(
-        self, *acs: AC, recurse=False, timeout=None, tries=1
-    ) -> Tuple[int, int]:
+    def supersede_permissions(self, *acs: AC, timeout=None, tries=1) -> Tuple[int, int]:
         """Remove all access controls from the item, replacing them with the
         specified access controls. Return the numbers of access controls
         removed and added.
 
+        This method handles only the permissions of this RodsItem. See the Collection
+        class for methods handling recursive operations.
+
         Args:
             *acs: Access controls.
-            recurse: Recursively supersede access controls.
             timeout: Operation timeout in seconds.
             tries: Number of times to try the operation.
 
@@ -1560,21 +1575,20 @@ def supersede_permissions(
             log.debug("Removing from ACL", path=self, ac=to_remove)
 
             # In iRODS we "remove" permissions by setting them to NULL
-            for ac in to_remove:
-                ac.perm = Permission.NULL
+            to_null = [AC(ac.user, Permission.NULL, zone=ac.zone) for ac in to_remove]
 
             item = self._to_dict()
-            item[Baton.ACCESS] = to_remove
+            item[Baton.ACCESS] = to_null
             with client(self.pool) as c:
-                c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries)
+                c.set_permission(item, timeout=timeout, tries=tries)
 
         to_add = sorted(set(acs).difference(current))
         if to_add:
             log.debug("Adding to ACL", path=self, ac=to_add)
             item = self._to_dict()
             item[Baton.ACCESS] = to_add
             with client(self.pool) as c:
-                c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries)
+                c.set_permission(item, timeout=timeout, tries=tries)
 
         return len(to_remove), len(to_add)
 
@@ -2303,6 +2317,71 @@ def put(self, local_path: Union[Path, str], recurse=True, timeout=None, tries=1)
         """
         raise NotImplementedError()
 
+    def add_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int:
+        """Add access controls to the collection. Return the number of access
+        controls added. If some argument access controls are already present,
+        those arguments will be ignored.
+
+        Args:
+            *acs: Access controls.
+            recurse: Recursively add access controls.
+            timeout: Operation timeout in seconds.
+            tries: Number of times to try the operation.
+
+        Returns: int
+        """
+        num_added = super().add_permissions(*acs, timeout=timeout, tries=tries)
+        if recurse:
+            for item in self.iter_contents(recurse=recurse):
+                num_added += item.add_permissions(*acs, timeout=timeout, tries=tries)
+        return num_added
+
+    def remove_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int:
+        """Remove access controls from the collection. Return the number of access
+        controls removed. If some argument access controls are not present, those
+        arguments will be ignored.
+
+        Args:
+            *acs: Access controls.
+            recurse: Recursively remove access controls.
+            timeout: Operation timeout in seconds.
+            tries: Number of times to try the operation.
+
+        Returns: int
+        """
+        num_removed = super().remove_permissions(*acs, timeout=timeout, tries=tries)
+        if recurse:
+            for item in self.iter_contents(recurse=recurse):
+                num_removed += item.remove_permissions(
+                    *acs, timeout=timeout, tries=tries
+                )
+        return num_removed
+
+    def supersede_permissions(
+        self, *acs: AC, recurse=False, timeout=None, tries=1
+    ) -> Tuple[int, int]:
+        """Remove all access controls from the collection, replacing them with the
+        specified access controls. Return the numbers of access controls
+        removed and added.
+
+        Args:
+            *acs: Access controls.
+            recurse: Recursively supersede access controls.
+            timeout: Operation timeout in seconds.
+            tries: Number of times to try the operation.
+
+        Returns: Tuple[int, int]
+        """
+        num_removed, num_added = super().supersede_permissions(
+            *acs, timeout=timeout, tries=tries
+        )
+        if recurse:
+            for item in self.iter_contents(recurse=recurse):
+                nr, na = item.supersede_permissions(*acs, timeout=timeout, tries=tries)
+                num_removed += nr
+                num_added += na
+        return num_removed, num_added
+
     def _list(self, **kwargs) -> List[dict]:
         with client(self.pool) as c:
             return c.list({Baton.COLL: self.path}, **kwargs)

tests/conftest.py

@@ -153,6 +153,24 @@ def annotated_collection(simple_collection):
         irm(simple_collection, force=True, recurse=True)
 
 
+@pytest.fixture(scope="function")
+def full_collection(tmp_path):
+    """A fixture providing a collection with some contents"""
+    root_path = PurePath("/testZone/home/irods/test")
+    rods_path = add_rods_path(root_path, tmp_path)
+
+    iput("./tests/data/recursive/", rods_path, recurse=True)
+    coll_path = rods_path / "recursive"
+
+    try:
+        add_test_groups()
+
+        yield coll_path
+    finally:
+        remove_test_groups()
+        irm(root_path, force=True, recurse=True)
+
+
 @pytest.fixture(scope="function")
 def simple_data_object(tmp_path):
     """A fixture providing a collection containing a single data object containing

tests/data/recursive/level1/level2/leaf1.txt

@@ -0,0 +1 @@
+e276ebe6-012e-11ee-b52e-c3459e3defec

tests/data/recursive/level1/level2/leaf2.txt

@@ -0,0 +1 @@
+d1dc40b0-012e-11ee-8774-af1e5dcfdab1