Refactor delete_permissions logic to enforce include_container_content with recursive flag and update unit tests for validation

Author: BryanFauble

synapseclient/models/mixins/access_control.py

@@ -243,12 +243,15 @@ async def delete_permissions_async(
         Arguments:
             include_self: If True (default), delete the ACL of the current entity.
                 If False, skip deleting the ACL of the current entity and only process
-                children if recursive=True or include_container_content=True.
+                children if recursive=True and include_container_content=True.
             recursive: If True and the entity is a container (e.g., Project or Folder),
-                recursively delete ACLs from all child containers and their children.
+                recursively process child containers. Note that this must be used with
+                include_container_content=True to have any effect. Setting recursive=True
+                with include_container_content=False will raise a ValueError.
                 Only works on classes that support the `sync_from_synapse_async` method.
             include_container_content: If True, delete ACLs from contents directly within
-                containers (files and folders inside Projects/Folders). Defaults to False.
+                containers (files and folders inside Projects/Folders). This must be set to
+                True for recursive to have any effect. Defaults to False.
             target_entity_types: Specify which entity types to process when deleting ACLs.
                 Allowed values are "folder" and "file" (case-insensitive).
                 If None, defaults to ["folder", "file"].
@@ -292,25 +295,37 @@ async def main():
             syn.login()
 
             async def main():
-                # Delete permissions for this folder and all container entities (folders) within it recursively
-                await Folder(id="syn123").delete_permissions_async(recursive=True)
+                # Delete permissions for this folder only (does not affect children)
+                await Folder(id="syn123").delete_permissions_async()
 
-                # Delete permissions for all entities within this folder, but not the folder itself
+                # Delete permissions for all files and folders directly within this folder,
+                # but not the folder itself
                 await Folder(id="syn123").delete_permissions_async(
                     include_self=False,
                     include_container_content=True
                 )
 
+                # Delete permissions for all items in the entire hierarchy (folders and their files)
+                # Both recursive and include_container_content must be True
+                await Folder(id="syn123").delete_permissions_async(
+                    recursive=True,
+                    include_container_content=True
+                )
+
                 # Delete permissions only for folder entities within this folder recursively
+                # and their contents
                 await Folder(id="syn123").delete_permissions_async(
                     recursive=True,
+                    include_container_content=True,
                     target_entity_types=["folder"]
                 )
 
-                # Delete all permissions in the entire hierarchy
+                # Delete permissions only for files within this folder and all subfolders
                 await Folder(id="syn123").delete_permissions_async(
+                    include_self=False,
                     recursive=True,
-                    include_container_content=True
+                    include_container_content=True,
+                    target_entity_types=["file"]
                 )
             asyncio.run(main())
             ```
@@ -327,6 +342,12 @@ async def main():
 
         should_process_children = recursive or include_container_content
         if should_process_children and hasattr(self, "sync_from_synapse_async"):
+            if recursive and not include_container_content:
+                raise ValueError(
+                    "When recursive=True, include_container_content must also be True. "
+                    "Setting recursive=True with include_container_content=False has no effect."
+                )
+
             synced = await self._sync_container_structure(client)
             if not synced:
                 return

synapseclient/models/protocols/access_control_protocol.py

@@ -191,12 +191,15 @@ def delete_permissions(
         Arguments:
             include_self: If True (default), delete the ACL of the current entity.
                 If False, skip deleting the ACL of the current entity and only process
-                children if recursive=True or include_container_content=True.
+                children if recursive=True and include_container_content=True.
             recursive: If True and the entity is a container (e.g., Project or Folder),
-                recursively delete ACLs from all child containers and their children.
+                recursively process child containers. Note that this must be used with
+                include_container_content=True to have any effect. Setting recursive=True
+                with include_container_content=False will raise a ValueError.
                 Only works on classes that support the `sync_from_synapse_async` method.
             include_container_content: If True, delete ACLs from contents directly within
-                containers (files and folders inside Projects/Folders). Defaults to False.
+                containers (files and folders inside Projects/Folders). This must be set to
+                True for recursive to have any effect. Defaults to False.
             target_entity_types: Specify which entity types to process when deleting ACLs.
                 Allowed values are "folder" and "file" (case-insensitive).
                 If None, defaults to ["folder", "file"].
@@ -234,25 +237,38 @@ def delete_permissions(
             syn = Synapse()
             syn.login()
 
-            # Delete permissions for this folder and all container entities (folders) within it recursively
-            Folder(id="syn123").delete_permissions(recursive=True)
+            # Delete permissions for this folder only (does not affect children)
+            Folder(id="syn123").delete_permissions()
 
-            # Delete permissions for all entities within this folder, but not the folder itself
+            # Delete permissions for all files and folders directly within this folder,
+            # but not the folder itself
             Folder(id="syn123").delete_permissions(
                 include_self=False,
                 include_container_content=True
             )
 
+            # Delete permissions for all items in the entire hierarchy (folders and their files)
+            # Both recursive and include_container_content must be True
+            Folder(id="syn123").delete_permissions(
+                recursive=True,
+                include_container_content=True
+            )
+
             # Delete permissions only for folder entities within this folder recursively
+            # and their contents
             Folder(id="syn123").delete_permissions(
                 recursive=True,
+                include_container_content=True,
                 target_entity_types=["folder"]
             )
 
-            # Delete all permissions in the entire hierarchy
+            # Delete permissions only for files within this folder and all subfolders
+            # Both recursive and include_container_content must be True for this to work
             Folder(id="syn123").delete_permissions(
+                include_self=False,
                 recursive=True,
-                include_container_content=True
+                include_container_content=True,
+                target_entity_types=["file"]
             )
             ```
         """

tests/unit/synapseclient/models/async/unit_test_permissions_async.py

@@ -165,24 +165,6 @@ async def test_delete_permissions_folder_recursive(self):
             entity_id="syn123", synapse_client=self.synapse_client
         )
 
-    async def test_sync_failure_doesnt_process_children(self):
-        """Test that sync failure prevents processing children."""
-        # GIVEN a folder with an ID and mocked sync_from_synapse_async method that fails
-        folder = Folder(id="syn123")
-        folder.sync_from_synapse_async = AsyncMock(side_effect=Exception("Sync failed"))
-
-        # WHEN deleting permissions recursively
-        await folder.delete_permissions_async(recursive=True)
-
-        # THEN sync_from_synapse_async should be called
-        folder.sync_from_synapse_async.assert_called_once()
-
-        # AND delete_entity_acl should be called on the folder itself
-        self.mock_delete_acl.assert_called_once()
-
-        # AND a warning log message should be generated
-        self.synapse_client.logger.warning.assert_called_once()
-
     async def test_filter_by_entity_type_folder(self):
         """Test filtering deletion by folder entity type."""
         # GIVEN a folder with an ID and child folder and file
@@ -299,3 +281,23 @@ async def test_general_exception_during_delete(self):
 
         # AND a warning log message should be generated
         self.synapse_client.logger.warning.assert_called_once()
+
+    async def test_invalid_recursive_without_include_container_content(self):
+        """Test that setting recursive=True without include_container_content=True raises ValueError."""
+        # GIVEN a folder with an ID
+        folder = Folder(id="syn123")
+
+        # WHEN attempting to delete permissions with recursive=True but include_container_content=False
+        # THEN a ValueError should be raised
+        with pytest.raises(
+            ValueError,
+            match="When recursive=True, include_container_content must also be True",
+        ):
+            await folder.delete_permissions_async(
+                recursive=True, include_container_content=False
+            )
+
+        # AND the delete_entity_acl function should still be called for the folder itself
+        self.mock_delete_acl.assert_called_once_with(
+            entity_id="syn123", synapse_client=self.synapse_client
+        )

tests/unit/synapseclient/models/unit_test_permissions.py

@@ -163,24 +163,6 @@ async def test_delete_permissions_folder_recursive(self):
             entity_id="syn123", synapse_client=self.synapse_client
         )
 
-    async def test_sync_failure_doesnt_process_children(self):
-        """Test that sync failure prevents processing children."""
-        # GIVEN a folder with an ID and mocked sync_from_synapse_async method that fails
-        folder = Folder(id="syn123")
-        folder.sync_from_synapse_async = AsyncMock(side_effect=Exception("Sync failed"))
-
-        # WHEN deleting permissions recursively
-        folder.delete_permissions(recursive=True)
-
-        # THEN sync_from_synapse_async should be called
-        folder.sync_from_synapse_async.assert_called_once()
-
-        # AND delete_entity_acl should be called on the folder itself
-        self.mock_delete_acl.assert_called_once()
-
-        # AND a warning log message should be generated
-        self.synapse_client.logger.warning.assert_called_once()
-
     async def test_filter_by_entity_type_folder(self):
         """Test filtering deletion by folder entity type."""
         # GIVEN a folder with an ID and child folder and file
@@ -297,3 +279,21 @@ async def test_general_exception_during_delete(self):
 
         # AND a warning log message should be generated
         self.synapse_client.logger.warning.assert_called_once()
+
+    async def test_invalid_recursive_without_include_container_content(self):
+        """Test that setting recursive=True without include_container_content=True raises ValueError."""
+        # GIVEN a folder with an ID
+        folder = Folder(id="syn123")
+
+        # WHEN attempting to delete permissions with recursive=True but include_container_content=False
+        # THEN a ValueError should be raised
+        with pytest.raises(
+            ValueError,
+            match="When recursive=True, include_container_content must also be True",
+        ):
+            folder.delete_permissions(recursive=True, include_container_content=False)
+
+        # AND the delete_entity_acl function should still be called for the folder itself
+        self.mock_delete_acl.assert_called_once_with(
+            entity_id="syn123", synapse_client=self.synapse_client
+        )