[SYNPY-1674] Creating functions for additional Synapse class methods, find_entity_id, is_synapse_id, onweb, md5_query #1301
Conversation
…id, is_synapse_id, onweb, md5_query
There was a problem hiding this comment.
Pull request overview
This PR introduces factory-style standalone functions for utility operations in the Synapse client, following the established pattern from previous PRs. The changes expose four utility methods (find_entity_id, is_synapse_id, onweb, and md5_query) as standalone functions with both synchronous and asynchronous versions, while adding deprecation warnings to the corresponding legacy methods on the Synapse class.
Key changes include:
- Factory functions in
synapseclient.operations.utility_operationsmodule with comprehensive documentation - New API layer functions in
synapseclient.api.entity_servicesandsynapseclient.api.web_services - Deprecation warnings (version 4.11.0, removal in 5.0.0) for legacy
Synapseclass methods
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
synapseclient/operations/utility_operations.py |
Implements the main factory functions (find_entity_id, is_synapse_id, onweb, md5_query) and their async variants with comprehensive docstrings and examples |
synapseclient/api/entity_services.py |
Adds the is_synapse_id API function with error handling for 400/403/404 status codes |
synapseclient/api/web_services.py |
Implements open_entity_in_browser function to construct and open Synapse portal URLs |
synapseclient/operations/__init__.py |
Exports the new utility operation functions for public API |
synapseclient/api/__init__.py |
Exports new API layer functions (is_synapse_id, open_entity_in_browser) |
synapseclient/client.py |
Adds deprecation decorators to legacy methods (is_synapse_id, onweb, get, store, delete, md5Query, findEntityId) |
tests/unit/synapseclient/operations/unit_test_utility_operations.py |
Unit tests for wrapper functions ensuring proper delegation |
tests/unit/synapseclient/api/unit_test_entity_services.py |
Unit tests for get_child, is_synapse_id, and get_entities_by_md5 API functions |
tests/unit/synapseclient/api/unit_test_web_services.py |
Unit tests for open_entity_in_browser with various input types |
tests/integration/synapseclient/operations/synchronous/test_utility_operations.py |
Integration tests for synchronous versions of utility functions |
tests/integration/synapseclient/operations/async/test_utility_operations_async.py |
Integration tests for asynchronous versions of utility functions |
tests/unit/synapseclient/operations/__init__.py |
Module docstring for unit test package |
Comments suppressed due to low confidence (1)
synapseclient/client.py:1591
- The
onwebmethod in the legacySynapseclass does not return a value (returns None), but the newonwebfunction returns the URL that was opened. While this is an improvement in the API design, it represents a breaking change in behavior for users who might be expecting None as the return value. Consider documenting this change explicitly in the deprecation message or migration guide to avoid confusion.
def onweb(self, entity, subpageId=None):
"""Opens up a browser window to the entity page or wiki-subpage.
Arguments:
entity: Either an Entity or a Synapse ID
subpageId: (Optional) ID of one of the wiki's sub-pages
Returns:
None
"""
if isinstance(entity, str) and os.path.isfile(entity):
entity = self.get(entity, downloadFile=False)
synId = id_of(entity)
if subpageId is None:
webbrowser.open("%s#!Synapse:%s" % (self.portalEndpoint, synId))
else:
webbrowser.open(
"%s#!Wiki:%s/ENTITY/%s" % (self.portalEndpoint, synId, subpageId)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from synapseclient.api import get_entity | ||
|
|
||
| client = Synapse.get_client(synapse_client=synapse_client) | ||
|
|
||
| # If it's a file path, get the entity first | ||
| if isinstance(entity, str) and os.path.isfile(entity): | ||
| entity_dict = await get_entity(entity_id=entity, synapse_client=synapse_client) | ||
| synapse_id = entity_dict.get("id") | ||
| else: | ||
| synapse_id = id_of(entity) | ||
|
|
||
| # Construct the URL | ||
| portal_endpoint = client.portalEndpoint |
There was a problem hiding this comment.
When the entity parameter is a file path (line 74), the code calls get_entity(entity_id=entity, ...) where entity is still the file path string, not a Synapse ID. This will likely fail because get_entity expects a Synapse ID, not a file path. The legacy implementation (line 1583 in client.py) calls self.get(entity, downloadFile=False) which can handle file paths. Consider using a similar approach or clarifying the expected behavior for file path inputs.
| from synapseclient.api import get_entity | |
| client = Synapse.get_client(synapse_client=synapse_client) | |
| # If it's a file path, get the entity first | |
| if isinstance(entity, str) and os.path.isfile(entity): | |
| entity_dict = await get_entity(entity_id=entity, synapse_client=synapse_client) | |
| synapse_id = entity_dict.get("id") | |
| else: | |
| synapse_id = id_of(entity) | |
| # Construct the URL | |
| portal_endpoint = client.portalEndpoint | |
| client = Synapse.get_client(synapse_client=synapse_client) | |
| # If it's a file path, resolve it to an entity using the Synapse client | |
| if isinstance(entity, str) and os.path.isfile(entity): | |
| resolved_entity = client.get(entity, downloadFile=False) | |
| synapse_id = id_of(resolved_entity) | |
| else: | |
| synapse_id = id_of(entity) | |
| # Construct the URL | |
| portal_endpoint = client.portalEndpoint | |
| portal_endpoint = client.portalEndpoint |
| except (SynapseHTTPError, SynapseAuthenticationError) as err: | ||
| # Extract status code | ||
| status = None | ||
| if hasattr(err, "__context__") and hasattr(err.__context__, "response"): | ||
| status = err.__context__.response.status_code | ||
| elif hasattr(err, "response"): | ||
| status = err.response.status_code | ||
|
|
||
| if status in (400, 404): | ||
| return False | ||
| # Valid ID but user lacks permission or is not logged in | ||
| elif status == 403: | ||
| return True | ||
| raise |
There was a problem hiding this comment.
The error handling logic attempts to extract the status code from either err.response.status_code or err.__context__.response.status_code, but the order of checks may not properly handle all cases. In the original implementation (line 1556 of client.py), it uses or to combine both checks: err.__context__.response.status_code or err.response.status_code. The current implementation checks for the presence of attributes first, which is safer, but when status is None after both checks, the code will still attempt to check if status in (400, 404) which could lead to unexpected behavior. Consider adding an explicit check to ensure status is not None before the conditional checks, or re-raise the exception if status cannot be determined.
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| from typing import TYPE_CHECKING, List, Optional, Union | ||
|
|
||
| from synapseclient.core.async_utils import wrap_async_to_sync | ||
|
|
||
| if TYPE_CHECKING: | ||
| from synapseclient import Synapse | ||
|
|
||
|
|
||
| def find_entity_id( | ||
| name: str, | ||
| parent: Optional[Union[str, object]] = None, | ||
| *, | ||
| synapse_client: Optional["Synapse"] = None, | ||
| ) -> Optional[str]: |
There was a problem hiding this comment.
Import of 'Synapse' is not used.
| from typing import TYPE_CHECKING, List, Optional, Union | |
| from synapseclient.core.async_utils import wrap_async_to_sync | |
| if TYPE_CHECKING: | |
| from synapseclient import Synapse | |
| def find_entity_id( | |
| name: str, | |
| parent: Optional[Union[str, object]] = None, | |
| *, | |
| synapse_client: Optional["Synapse"] = None, | |
| ) -> Optional[str]: | |
| from typing import List, Optional, Union | |
| from synapseclient.core.async_utils import wrap_async_to_sync | |
| def find_entity_id( | |
| name: str, | |
| parent: Optional[Union[str, object]] = None, | |
| *, | |
| synapse_client: Optional["Synapse"] = None, | |
| ) -> Optional[str]: | |
| name: str, | |
| parent: Optional[Union[str, object]] = None, | |
| *, | |
| synapse_client: Optional["Synapse"] = None, | |
| ) -> Optional[str]: |
| if TYPE_CHECKING: | ||
| from synapseclient import Synapse |
There was a problem hiding this comment.
Import of 'Synapse' is not used.
| if TYPE_CHECKING: | |
| from synapseclient import Synapse |
Problem:
synapseclientlibrary requires users to instantiate theSynapseclass and call methods on it for common utility operations, which creates unnecessary friction and doesn't align with modern Python patterns.get/get_asyncand extended in the previous PR withstore/delete, the remaining utility methods from the Jira ticket need to be exposed as standalone functions.findEntityId,is_synapse_id,onweb, andmd5Query.Solution:
find_entity_id/find_entity_id_async: Find an entity by name and optional parent, returning the entity ID or None if not found. Supports both string IDs and entity objects for the parent parameter.is_synapse_id/is_synapse_id_async: Validate whether a Synapse ID string corresponds to an actual entity. Returns True for valid IDs (including those the user lacks permission to access), False for invalid/non-existent IDs.onweb/onweb_async: Open an entity's page in the default web browser. Supports optionalsubpage_idfor wiki subpages. Returns the URL that was opened.md5_query/md5_query_async: Find entities with attached files matching a given MD5 hash. Returns a list of entity header dictionaries.synapseclient.api.entity_services.is_synapse_id: Async implementation with proper error handling for 400/403/404 status codessynapseclient.api.web_services.open_entity_in_browser: Async implementation that constructs and opens Synapse portal URLsSynapseclass methods:is_synapse_id,onweb,get,store,delete,md5Query,findEntityIdTesting:
test_utility_operations_async.py: Tests forfind_entity_id_async(project lookup, file in parent, not found cases, parent object support),is_synapse_id_async(valid/invalid IDs, non-string input),md5_query_async(matching files, non-existent hashes),onweb_async(ID string, entity object, wiki subpage URLs)test_utility_operations.py: Synchronous equivalents of all async testsunit_test_entity_services.py: Tests forget_child(found/not found/project lookup),is_synapse_id(valid/invalid/403 permission errors/non-string),get_entities_by_md5(with/without results)unit_test_web_services.py: Tests foropen_entity_in_browser(Synapse ID, entity object, subpage, file path handling, custom portal endpoint)unit_test_utility_operations.py: Tests for all wrapper functions ensuring proper delegation to underlying API functions