[SYNPY-1244] Clarify docstrings with delete_permissions function and recursive behavior (#1202)

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

Author: BryanFauble

synapseclient/models/mixins/access_control.py

@@ -219,8 +219,8 @@ async def main():
     async def delete_permissions_async(
         self,
         include_self: bool = True,
-        recursive: bool = False,
         include_container_content: bool = False,
+        recursive: bool = False,
         target_entity_types: Optional[List[str]] = None,
         *,
         synapse_client: Optional[Synapse] = None,
@@ -242,13 +242,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.
+                If False, skip deleting the ACL of the current entity.
+            include_container_content: If True, delete ACLs from contents directly within
+                containers (files and folders inside self). This must be set to
+                True for recursive to have any effect. Defaults to False.
             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.
             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 +294,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 +341,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

@@ -166,9 +166,9 @@ def set_permissions(
 
     def delete_permissions(
         self,
-        recursive: bool = False,
         include_self: bool = True,
         include_container_content: bool = False,
+        recursive: bool = False,
         target_entity_types: Optional[List[str]] = None,
         *,
         synapse_client: Optional[Synapse] = None,
@@ -190,13 +190,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.
+                If False, skip deleting the ACL of the current entity.
+            include_container_content: If True, delete ACLs from contents directly within
+                containers (files and folders inside self). This must be set to
+                True for recursive to have any effect. Defaults to False.
             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.
             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 +236,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/integration/synapseclient/models/async/test_permissions_async.py

@@ -736,17 +736,21 @@ async def test_delete_permissions_recursive(self, project_model: Project) -> Non
         await self._set_custom_permissions(file_3)
 
         # WHEN I delete permissions recursively but without container content
-        await top_level_folder.delete_permissions_async(
-            recursive=True, include_container_content=False
-        )
+        with pytest.raises(
+            ValueError,
+            match="When recursive=True, include_container_content must also be True",
+        ) as exc_info:
+            await top_level_folder.delete_permissions_async(
+                recursive=True, include_container_content=False
+            )
 
         # THEN the top_level_folder permissions should be deleted
         await self._verify_permissions_deleted(top_level_folder)
 
         # AND the folder_1 permissions should remain (because include_container_content=False)
         await self._verify_permissions_not_deleted(folder_1)
 
-        # BUT the file_3 permissions should remain (because include_container_content=False)
+        # AND the file_3 permissions should remain (because include_container_content=False)
         assert await self._verify_permissions_not_deleted(file_3)
 
     async def test_delete_permissions_recursive_with_container_content(

tests/integration/synapseclient/models/synchronous/test_permissions.py

@@ -727,18 +727,22 @@ async def test_delete_permissions_recursive(self, project_model: Project) -> Non
         await self._set_custom_permissions(folder_1)
         await self._set_custom_permissions(file_3)
 
-        # WHEN I delete permissions recursively but without container content
-        top_level_folder.delete_permissions(
-            recursive=True, include_container_content=False
-        )
+        with pytest.raises(
+            ValueError,
+            match="When recursive=True, include_container_content must also be True",
+        ) as exc_info:
+            # WHEN I delete permissions recursively but without container content
+            top_level_folder.delete_permissions(
+                recursive=True, include_container_content=False
+            )
 
         # THEN the top_level_folder permissions should be deleted
         await self._verify_permissions_deleted(top_level_folder)
 
         # AND the folder_1 permissions should remain (because include_container_content=False)
         await self._verify_permissions_not_deleted(folder_1)
 
-        # BUT the file_3 permissions should remain (because include_container_content=False)
+        # AND the file_3 permissions should remain (because include_container_content=False)
         assert await self._verify_permissions_not_deleted(file_3)
 
     async def test_delete_permissions_recursive_with_container_content(

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
+        )