Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 14, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
827f8d6
Refactor delete_permissions logic to enforce include_container_conten…
BryanFauble May 14, 2025
65c4827
reorder args
BryanFauble May 14, 2025
d8d7343
Correct integration test
BryanFauble May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions synapseclient/models/mixins/access_control.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"].
Expand Down Expand Up @@ -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())
```
Expand All @@ -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."
)

Comment on lines +344 to +349
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a consideration from a ux perspective:
Had it been clarified that setting recurive=True and include_container_content=False wouldn't have deleted any of the access controls (but that it still iterates through the project and displays the folders), as I user I would have appreciated having the functionality to do a dry run and see everything that would have had access controls removed before actually doing so. @BryanFauble What are your thoughts on not halting execution and rather adjusting the messaging displayed in the case of this combination of parameters? Granted it doesn't look like files themselves are displayed in the current messaging?

[WARNING] Failed to delete ACL for entity syn66520312: 403 Client Error: Cannot restore inheritance for resource which has no parent.
[syn66520312:acc_control_test]: Syncing Project from Synapse.                                                                                                                                                                                         
Syncing from Synapse:   0%|                                                                                                                                                                                                | 0.00/1.00 [00:00<?, ?B/s]
[syn66520313:temp folder]: Syncing Folder from Synapse.                                                                                                                                                                                               
Syncing from Synapse:   0%|

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Failed to delete ACL for entity syn66520312: 403 Client Error: Cannot restore inheritance for resource which has no parent.

This message only shows because you cannot delete the ACL for projects.

Had it been clarified that setting recurive=True and include_container_content=False wouldn't have deleted any of the access controls (but that it still iterates through the project and displays the folders)

This was not clarified, and in the code it would have just skipped over these entries.

as I user I would have appreciated having the functionality to do a dry run and see everything that would have had access controls removed before actually doing so. @BryanFauble What are your thoughts on not halting execution and rather adjusting the messaging displayed in the case of this combination of parameters?

This is a great idea! I created this follow-up ticket https://sagebionetworks.jira.com/browse/SYNPY-1604 to accomplish this ask. We can do quite a bit to make this easier for folks to understand the ACL structure of their project and implications of running the delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that all makes sense, thanks! @BryanFauble

synced = await self._sync_container_structure(client)
if not synced:
return
Expand Down
37 changes: 26 additions & 11 deletions synapseclient/models/protocols/access_control_protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"].
Expand Down Expand Up @@ -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"]
)
```
"""
Expand Down
12 changes: 8 additions & 4 deletions tests/integration/synapseclient/models/async/test_permissions_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
14 changes: 9 additions & 5 deletions tests/integration/synapseclient/models/synchronous/test_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
38 changes: 20 additions & 18 deletions tests/unit/synapseclient/models/async/unit_test_permissions_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
)
36 changes: 18 additions & 18 deletions tests/unit/synapseclient/models/unit_test_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
)
Loading