From 2f98c5efc269bc7fc4afc1e09b408fa7213662e0 Mon Sep 17 00:00:00 2001 From: Thomas Yu Date: Mon, 26 Oct 2026 15:43:28 -0700 Subject: [PATCH 1/3] [SYNPY-1941] Preserve data_file_handle_id when file handle is withheld Synapse omits the file handle from the entity bundle when the caller lacks DOWNLOAD permission (e.g. unmet access requirements). _handle_file_entity still called _fill_from_file_handle() with an empty FileHandle, which overwrote the entity's dataFileHandleId with None. This caused File.get() followed by store() (e.g. moving a file) to fail with "Cannot store file". Only copy file handle fields onto the entity when a file handle was returned. Co-Authored-By: Claude Opus 5.5 --- synapseclient/api/entity_factory.py | 7 ++++- .../models/async/unit_test_file_async.py | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/synapseclient/api/entity_factory.py b/synapseclient/api/entity_factory.py index db3472641..be9207601 100644 --- a/synapseclient/api/entity_factory.py +++ b/synapseclient/api/entity_factory.py @@ -259,7 +259,12 @@ async def _handle_file_entity( entity_instance.file_handle = FileHandle().fill_from_dict( synapse_instance=file_handle ) - entity_instance._fill_from_file_handle() + # Synapse omits the file handle from the bundle when the caller lacks DOWNLOAD + # permission (e.g. unmet access requirements). Only copy file handle fields onto + # the entity when one was returned so that `dataFileHandleId` from the entity + # is preserved. + if file_handle: + entity_instance._fill_from_file_handle() if download_file: if file_handle: diff --git a/tests/unit/synapseclient/models/async/unit_test_file_async.py b/tests/unit/synapseclient/models/async/unit_test_file_async.py index 1d7478fc3..878a7b835 100644 --- a/tests/unit/synapseclient/models/async/unit_test_file_async.py +++ b/tests/unit/synapseclient/models/async/unit_test_file_async.py @@ -797,6 +797,32 @@ async def test_get_with_id(self) -> None: assert result.file_handle.is_preview assert result.file_handle.external_url == FILE_HANDLE_EXTERNAL_URL + async def test_get_with_unmet_access_requirement(self) -> None: + # GIVEN an example file + file = File(id=SYN_123, download_file=False) + + # AND an entity bundle where Synapse withheld the file handle because the + # caller has unmet access requirements + bundle = self.get_example_rest_api_file_output() + bundle["fileHandles"] = [] + bundle["restrictionInformation"] = {"hasUnmetAccessRequirement": True} + + # WHEN I get the example file + with patch( + "synapseclient.api.entity_factory.get_entity_id_bundle2", + new_callable=AsyncMock, + return_value=bundle, + ): + result = await file.get_async(synapse_client=self.syn) + + # THEN the data_file_handle_id from the entity should be preserved + assert result.id == SYN_123 + assert result.data_file_handle_id == DATA_FILE_HANDLE_ID + + # AND the file handle should have no metadata filled in + assert result.file_handle.id is None + assert result.content_md5 is None + async def test_get_with_path(self) -> None: # GIVEN an example file file = File(path=PATH, description=MODIFIED_DESCRIPTION) From db80264951f31b4a76c503428d9dae15edba0b7c Mon Sep 17 00:00:00 2001 From: Thomas Yu Date: Mon, 26 Oct 2026 15:59:25 -0700 Subject: [PATCH 2/3] [SYNPY-1941] Move file handle guard into _fill_from_file_handle `if self.file_handle:` is always true for a FileHandle dataclass instance, so an empty placeholder FileHandle (left when Synapse withholds the file handle due to missing DOWNLOAD permission) overwrote data_file_handle_id, content_type, content_size, and external_url with None. Guard on `file_handle.id` in File and RecordSet `_fill_from_file_handle` so every caller is protected, and revert the now-redundant guard in entity_factory._handle_file_entity. Add RecordSet regression test. Co-Authored-By: Claude Opus 5.5 --- synapseclient/api/entity_factory.py | 7 +---- synapseclient/models/file.py | 10 +++++-- synapseclient/models/recordset.py | 10 +++++-- .../models/async/unit_test_recordset_async.py | 27 +++++++++++++++++++ 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/synapseclient/api/entity_factory.py b/synapseclient/api/entity_factory.py index be9207601..db3472641 100644 --- a/synapseclient/api/entity_factory.py +++ b/synapseclient/api/entity_factory.py @@ -259,12 +259,7 @@ async def _handle_file_entity( entity_instance.file_handle = FileHandle().fill_from_dict( synapse_instance=file_handle ) - # Synapse omits the file handle from the bundle when the caller lacks DOWNLOAD - # permission (e.g. unmet access requirements). Only copy file handle fields onto - # the entity when one was returned so that `dataFileHandleId` from the entity - # is preserved. - if file_handle: - entity_instance._fill_from_file_handle() + entity_instance._fill_from_file_handle() if download_file: if file_handle: diff --git a/synapseclient/models/file.py b/synapseclient/models/file.py index c21645fab..fcdfbe595 100644 --- a/synapseclient/models/file.py +++ b/synapseclient/models/file.py @@ -600,8 +600,14 @@ def _set_last_persistent_instance(self) -> None: ) def _fill_from_file_handle(self) -> None: - """Fill the file object from the file handle.""" - if self.file_handle: + """Fill the file object from the file handle. + + This is a no-op when the file handle has no `id`. Synapse withholds the file + handle from callers without DOWNLOAD permission (e.g. unmet access + requirements), which leaves an empty placeholder `FileHandle` that must not + overwrite the `data_file_handle_id` returned on the entity. + """ + if self.file_handle and self.file_handle.id: self.data_file_handle_id = self.file_handle.id self.content_type = self.file_handle.content_type self.content_size = self.file_handle.content_size diff --git a/synapseclient/models/recordset.py b/synapseclient/models/recordset.py index d8324f5c1..02730c0f8 100644 --- a/synapseclient/models/recordset.py +++ b/synapseclient/models/recordset.py @@ -788,8 +788,14 @@ def _set_last_persistent_instance(self) -> None: ) def _fill_from_file_handle(self) -> None: - """Fill the file object from the file handle.""" - if self.file_handle: + """Fill the file object from the file handle. + + This is a no-op when the file handle has no `id`. Synapse withholds the file + handle from callers without DOWNLOAD permission (e.g. unmet access + requirements), which leaves an empty placeholder `FileHandle` that must not + overwrite the `data_file_handle_id` returned on the entity. + """ + if self.file_handle and self.file_handle.id: self.data_file_handle_id = self.file_handle.id self.content_type = self.file_handle.content_type self.content_size = self.file_handle.content_size diff --git a/tests/unit/synapseclient/models/async/unit_test_recordset_async.py b/tests/unit/synapseclient/models/async/unit_test_recordset_async.py index 0b9e577e7..8e4fc1d2f 100644 --- a/tests/unit/synapseclient/models/async/unit_test_recordset_async.py +++ b/tests/unit/synapseclient/models/async/unit_test_recordset_async.py @@ -571,6 +571,33 @@ async def side_effect(**kwargs): assert result.id == SYN_123 assert result.name == RECORD_SET_NAME + async def test_get_async_with_unmet_access_requirement(self) -> None: + # GIVEN a RecordSet with an id + record_set = RecordSet(id=SYN_123, download_file=False) + + # AND an entity bundle where Synapse withheld the file handle because the + # caller has unmet access requirements + bundle = { + "entity": _get_record_set_entity_response(), + "fileHandles": [], + "restrictionInformation": {"hasUnmetAccessRequirement": True}, + } + + # WHEN I call get_async + with patch( + "synapseclient.api.entity_factory.get_entity_id_bundle2", + new_callable=AsyncMock, + return_value=bundle, + ): + result = await record_set.get_async(synapse_client=self.syn) + + # THEN the data_file_handle_id from the entity should be preserved + assert result.id == SYN_123 + assert result.data_file_handle_id == DATA_FILE_HANDLE_ID + + # AND the file handle should have no metadata filled in + assert result.file_handle.id is None + async def test_get_async_with_path(self) -> None: # GIVEN a RecordSet with a path (looking up by local file) record_set = RecordSet(path=PATH) From d6222aa574d480d8013e1600f28c7fa308b5a700 Mon Sep 17 00:00:00 2001 From: Thomas Yu Date: Mon, 26 Oct 2026 16:02:55 -0700 Subject: [PATCH 3/3] [SYNPY-1941] Revert to guarding in _handle_file_entity Restore the guard in entity_factory._handle_file_entity and revert the _fill_from_file_handle changes in File and RecordSet. _handle_file_entity is the only caller that can pass an empty placeholder FileHandle and it handles both File and RecordSet, so a single guard there is the smaller fix. Keep both File and RecordSet regression tests. Co-Authored-By: Claude Opus 5.5 --- synapseclient/api/entity_factory.py | 7 ++++++- synapseclient/models/file.py | 10 ++-------- synapseclient/models/recordset.py | 10 ++-------- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/synapseclient/api/entity_factory.py b/synapseclient/api/entity_factory.py index db3472641..be9207601 100644 --- a/synapseclient/api/entity_factory.py +++ b/synapseclient/api/entity_factory.py @@ -259,7 +259,12 @@ async def _handle_file_entity( entity_instance.file_handle = FileHandle().fill_from_dict( synapse_instance=file_handle ) - entity_instance._fill_from_file_handle() + # Synapse omits the file handle from the bundle when the caller lacks DOWNLOAD + # permission (e.g. unmet access requirements). Only copy file handle fields onto + # the entity when one was returned so that `dataFileHandleId` from the entity + # is preserved. + if file_handle: + entity_instance._fill_from_file_handle() if download_file: if file_handle: diff --git a/synapseclient/models/file.py b/synapseclient/models/file.py index fcdfbe595..c21645fab 100644 --- a/synapseclient/models/file.py +++ b/synapseclient/models/file.py @@ -600,14 +600,8 @@ def _set_last_persistent_instance(self) -> None: ) def _fill_from_file_handle(self) -> None: - """Fill the file object from the file handle. - - This is a no-op when the file handle has no `id`. Synapse withholds the file - handle from callers without DOWNLOAD permission (e.g. unmet access - requirements), which leaves an empty placeholder `FileHandle` that must not - overwrite the `data_file_handle_id` returned on the entity. - """ - if self.file_handle and self.file_handle.id: + """Fill the file object from the file handle.""" + if self.file_handle: self.data_file_handle_id = self.file_handle.id self.content_type = self.file_handle.content_type self.content_size = self.file_handle.content_size diff --git a/synapseclient/models/recordset.py b/synapseclient/models/recordset.py index 02730c0f8..d8324f5c1 100644 --- a/synapseclient/models/recordset.py +++ b/synapseclient/models/recordset.py @@ -788,14 +788,8 @@ def _set_last_persistent_instance(self) -> None: ) def _fill_from_file_handle(self) -> None: - """Fill the file object from the file handle. - - This is a no-op when the file handle has no `id`. Synapse withholds the file - handle from callers without DOWNLOAD permission (e.g. unmet access - requirements), which leaves an empty placeholder `FileHandle` that must not - overwrite the `data_file_handle_id` returned on the entity. - """ - if self.file_handle and self.file_handle.id: + """Fill the file object from the file handle.""" + if self.file_handle: self.data_file_handle_id = self.file_handle.id self.content_type = self.file_handle.content_type self.content_size = self.file_handle.content_size