[SYNPY-1511] Correct missing synapse client propogation and caching (#…

…1125)

* Correct missing propogation and move when synapseClient instance is cached

synapseclient/client.py

@@ -2,7 +2,6 @@
 The `Synapse` object encapsulates a connection to the Synapse service and is used for building projects, uploading and
 retrieving data, and recording provenance of data analysis.
 """
-
 import asyncio
 import collections
 import collections.abc
@@ -278,6 +277,7 @@ def __init__(
         requests_session_async_synapse: httpx.AsyncClient = None,
         requests_session_storage: httpx.Client = None,
         asyncio_event_loop: asyncio.AbstractEventLoop = None,
+        cache_client: bool = True,
     ) -> "Synapse":
         """
         Initialize Synapse object
@@ -301,6 +301,9 @@ def __init__(
             asyncio_event_loop: The event loop that is going to be used while executing
                 this code. This is optional and only used when you are manually
                 specifying an async HTTPX client.
+            cache_client: Whether to cache the Synapse client object in the Synapse module. Defaults to True.
+                             When set to True anywhere a `Synapse` object is optional you do not need to pass an
+                             instance of `Synapse` to that function, method, or class.
 
         Raises:
             ValueError: Warn for non-boolean debug value.
@@ -412,6 +415,8 @@ def log_response(response: httpx.Response) -> None:
         self._parallel_file_transfer_semaphore = {}
         self.use_boto_sts_transfers = transfer_config["use_boto_sts"]
         self._parts_transfered_counter = 0
+        if cache_client:
+            Synapse.set_client(synapse_client=self)
 
     def _get_requests_session_async_synapse(
         self, asyncio_event_loop: asyncio.AbstractEventLoop
@@ -709,7 +714,6 @@ def login(
         email: str = None,
         silent: bool = False,
         authToken: str = None,
-        cache_client: bool = True,
     ) -> None:
         """
         Valid combinations of login() arguments:
@@ -730,9 +734,6 @@ def login(
             authToken:    A bearer authorization token, e.g. a
                 [personal access token](https://python-docs.synapse.org/tutorials/authentication/).
             silent:       Defaults to False.  Suppresses the "Welcome ...!" message.
-            cache_client: Whether to cache the Synapse client object in the Synapse module. Defaults to True.
-                             When set to True anywhere a `Synapse` object is optional you do not need to pass an
-                             instance of `Synapse` to that function, method, or class.
 
         Example: Logging in
             Using an auth token:
@@ -774,9 +775,6 @@ def login(
             display_name = self.credentials.displayname or self.credentials.username
             self.logger.info(f"Welcome, {display_name}!\n")
 
-        if cache_client:
-            Synapse.set_client(self)
-
     @deprecated(
         version="4.4.0",
         reason="To be removed in 5.0.0. "
@@ -1954,14 +1952,9 @@ def store(
                     upload_file_handle_async(
                         self,
                         parent_id_for_upload,
-                        (
-                            local_state["path"]
-                            if (
-                                synapseStore
-                                or local_state_fh.get("externalURL") is None
-                            )
-                            else local_state_fh.get("externalURL")
-                        ),
+                        local_state["path"]
+                        if (synapseStore or local_state_fh.get("externalURL") is None)
+                        else local_state_fh.get("externalURL"),
                         synapse_store=synapseStore,
                         md5=local_file_md5_hex or local_state_fh.get("contentMd5"),
                         file_size=local_state_fh.get("contentSize"),
@@ -3131,11 +3124,9 @@ def _convertProvenanceList(self, usedList: list, limitSearch: str = None) -> lis
         if usedList is None:
             return None
         usedList = [
-            (
-                self.get(target, limitSearch=limitSearch)
-                if (os.path.isfile(target) if isinstance(target, str) else False)
-                else target
-            )
+            self.get(target, limitSearch=limitSearch)
+            if (os.path.isfile(target) if isinstance(target, str) else False)
+            else target
             for target in usedList
         ]
         return usedList
@@ -4938,6 +4929,7 @@ def getWiki(self, owner, subpageId=None, version=None):
                     destination=os.path.join(
                         cache_dir, str(wiki.markdownFileHandleId) + ".md"
                     ),
+                    synapse_client=self,
                 ),
                 syn=self,
             )
@@ -5606,6 +5598,7 @@ def _queryTableCsv(
                 synapse_id=extract_synapse_id_from_query(query),
                 entity_type="TableEntity",
                 destination=os.path.join(download_dir, filename),
+                synapse_client=self,
             ),
             syn=self,
         )

synapseclient/core/download/download_async.py

@@ -314,7 +314,9 @@ async def download_file(self) -> None:
             syn=self._syn, url=url_info.url, debug=self._download_request.debug
         )
         self._progress_bar = get_or_create_download_progress_bar(
-            file_size=file_size, postfix=self._download_request.object_id
+            file_size=file_size,
+            postfix=self._download_request.object_id,
+            synapse_client=self._syn,
         )
         self._prep_file()
 

synapseclient/core/download/download_functions.py

@@ -166,6 +166,7 @@ async def download_file_entity(
                 synapse_id=object_id,
                 entity_type=object_type,
                 destination=download_path,
+                synapse_client=client,
             )
 
         if download_path is None or not os.path.exists(download_path):
@@ -274,6 +275,7 @@ async def download_file_entity_model(
                 synapse_id=object_id,
                 entity_type=object_type,
                 destination=download_path,
+                synapse_client=client,
             )
 
         if download_path is None or not os.path.exists(download_path):
@@ -416,7 +418,7 @@ async def download_by_file_handle(
                 )
 
                 progress_bar = get_or_create_download_progress_bar(
-                    file_size=1, postfix=synapse_id
+                    file_size=1, postfix=synapse_id, synapse_client=syn
                 )
                 loop = asyncio.get_running_loop()
                 downloaded_path = await loop.run_in_executor(
@@ -440,7 +442,7 @@ async def download_by_file_handle(
                 and concrete_type == concrete_types.S3_FILE_HANDLE
             ):
                 progress_bar = get_or_create_download_progress_bar(
-                    file_size=1, postfix=synapse_id
+                    file_size=1, postfix=synapse_id, synapse_client=syn
                 )
 
                 def download_fn(
@@ -496,7 +498,7 @@ def download_fn(
             else:
                 loop = asyncio.get_running_loop()
                 progress_bar = get_or_create_download_progress_bar(
-                    file_size=1, postfix=synapse_id
+                    file_size=1, postfix=synapse_id, synapse_client=syn
                 )
                 downloaded_path = await loop.run_in_executor(
                     syn._get_thread_pool_executor(asyncio_event_loop=loop),

synapseclient/core/transfer_bar.py

@@ -85,7 +85,7 @@ def shared_download_progress_bar(
 
     syn = Synapse.get_client(synapse_client=synapse_client)
     with logging_redirect_tqdm(loggers=[syn.logger]):
-        get_or_create_download_progress_bar(file_size=file_size)
+        get_or_create_download_progress_bar(file_size=file_size, synapse_client=syn)
         try:
             yield
         finally:

tests/integration/synapseclient/integration_test.py

@@ -78,7 +78,7 @@ async def testCustomConfigFile(schedule_for_cleanup):
         shutil.copyfile(client.CONFIG_FILE, configPath)
         schedule_for_cleanup(configPath)
 
-        syn2 = Synapse(configPath=configPath)
+        syn2 = Synapse(configPath=configPath, cache_client=False)
         syn2.login()
     else:
         raise ValueError(

tests/integration/synapseclient/test_command_line_client.py

@@ -900,7 +900,7 @@ async def test_table_query(test_state):
 
 
 async def test_login(test_state):
-    alt_syn = Synapse()
+    alt_syn = Synapse(cache_client=False)
     username = "username"
     auth_token = "my_auth_token"
     with patch.object(alt_syn, "login") as mock_login, patch.object(

tests/integration/synapseclient/test_evaluations.py

@@ -178,7 +178,7 @@ async def test_teams(syn: Synapse, schedule_for_cleanup):
     schedule_for_cleanup(team)
 
     # not logged in, teams are public
-    anonymous_syn = Synapse()
+    anonymous_syn = Synapse(cache_client=False)
 
     found_team = anonymous_syn.getTeam(team.id)
     assert team == found_team

tests/unit/synapseclient/core/unit_test_download.py

@@ -162,6 +162,7 @@ async def test_mock_download(syn: Synapse) -> None:
             destination=temp_dir,
             file_handle_id=12345,
             expected_md5=contents_md5,
+            synapse_client=syn,
         )
 
     # 2. Multiple redirects
@@ -181,6 +182,7 @@ async def test_mock_download(syn: Synapse) -> None:
             destination=temp_dir,
             file_handle_id=12345,
             expected_md5=contents_md5,
+            synapse_client=syn,
         )
 
     # 3. recover from partial download
@@ -286,6 +288,7 @@ async def test_mock_download(syn: Synapse) -> None:
             synapse_id=objectId,
             entity_type=objectType,
             destination=temp_dir,
+            synapse_client=syn,
         )
 
     # 5. don't recover, a partial download that never completes
@@ -337,6 +340,7 @@ async def test_mock_download(syn: Synapse) -> None:
                 synapse_id=objectId,
                 entity_type=objectType,
                 destination=temp_dir,
+                synapse_client=syn,
             )
 
     # 6. 206 Range header not supported, respond with 200 and full file
@@ -377,6 +381,7 @@ async def test_mock_download(syn: Synapse) -> None:
             synapse_id=objectId,
             entity_type=objectType,
             destination=temp_dir,
+            synapse_client=syn,
         )
 
     # 7. Too many redirects
@@ -408,6 +413,7 @@ async def test_mock_download(syn: Synapse) -> None:
                 synapse_id=objectId,
                 entity_type=objectType,
                 destination=temp_dir,
+                synapse_client=syn,
             )
 
 
@@ -445,6 +451,7 @@ async def test_multithread_true_s3_file_handle(self) -> None:
                 synapse_id=456,
                 entity_type="FileEntity",
                 destination="/myfakepath",
+                synapse_client=self.syn,
             )
 
             mock_multi_thread_download.assert_called_once_with(
@@ -478,6 +485,7 @@ async def _multithread_not_applicable(self, file_handle: Dict[str, str]) -> None
                 synapse_id=456,
                 entity_type="FileEntity",
                 destination="/myfakepath",
+                synapse_client=self.syn,
             )
 
             mock_download_from_URL.assert_called_once_with(
@@ -534,6 +542,7 @@ async def test_multithread_false_s3_file_handle(self) -> None:
                 synapse_id=456,
                 entity_type="FileEntity",
                 destination="/myfakepath",
+                synapse_client=self.syn,
             )
 
             mock_download_from_URL.assert_called_once_with(
@@ -674,7 +683,7 @@ async def test_download_end_early_retry(syn: Synapse) -> None:
         shutil, "move"
     ) as mocked_move:
         # function under test
-        download_from_url(url=url, destination=destination)
+        download_from_url(url=url, destination=destination, synapse_client=syn)
 
         # assert temp_download_filename() called 2 times with same parameters
         assert [
@@ -742,7 +751,10 @@ async def test_download_md5_mismatch__not_local_file(syn: Synapse) -> None:
         # function under test
         with pytest.raises(SynapseMd5MismatchError):
             await download_from_url(
-                url=url, destination=destination, expected_md5="fake md5 is fake"
+                url=url,
+                destination=destination,
+                expected_md5="fake md5 is fake",
+                synapse_client=syn,
             )
 
         # assert temp_download_filename() called once

tests/unit/synapseclient/core/unit_test_sts_transfer.py

@@ -366,8 +366,8 @@ def synGET(uri):
     @mock.patch("synapseclient.core.sts_transfer.StsTokenStore._fetch_token")
     def test_synapse_client__discrete_sts_token_stores(self, mock_fetch_token):
         """Verify that two Synapse objects will not share the same cached tokens"""
-        syn1 = Synapse(skip_checks=True)
-        syn2 = Synapse(skip_checks=True)
+        syn1 = Synapse(skip_checks=True, cache_client=False)
+        syn2 = Synapse(skip_checks=True, cache_client=False)
 
         expected_token = {
             "awsAccessKeyId": "ABC",

tests/unit/synapseclient/core/upload/unit_test_multipart_upload.py

@@ -727,15 +727,19 @@ def test_multipart_upload_file(self):
             )
 
             # Test when call the multipart_upload_file, md5_for_file pass in the correct callback function
-            syn_with_silent_mode = Synapse(silent=True, skip_checks=True)
+            syn_with_silent_mode = Synapse(
+                silent=True, skip_checks=True, cache_client=False
+            )
             multipart_upload_file(
                 syn_with_silent_mode,
                 file_path,
                 storage_location_id=storage_location_id,
             )
             md5_for_file.assert_called_with(file_path, callback=None)
 
-            syn_with_no_silent_mode = Synapse(debug=False, skip_checks=True)
+            syn_with_no_silent_mode = Synapse(
+                debug=False, skip_checks=True, cache_client=False
+            )
             multipart_upload_file(
                 syn_with_no_silent_mode,
                 file_path,

tests/unit/synapseclient/unit_test_Entity.py

@@ -390,7 +390,7 @@ def test_is_versionable_dict_representation_of_entity():
     )
 
 
-def test_create_Link_to_entity_with_the_same_parent():
+def test_create_Link_to_entity_with_the_same_parent(syn: Synapse):
     parent = "syn123"
     file = File("new file", parent=parent, id="syn456")
     file_bundle = {
@@ -414,6 +414,5 @@ def test_create_Link_to_entity_with_the_same_parent():
         "versionUrl": "/repo/v1/entity/syn456/version/1",
     }
     link = Link(targetId=file, parent=parent)
-    syn = Synapse(skip_checks=True)
     with patch.object(syn, "_getEntity", return_value=file_bundle):
         pytest.raises(ValueError, syn.store, link)

tests/unit/synapseclient/unit_test_Wiki.py

@@ -72,8 +72,7 @@ def test_Wiki__markdownFile_path_not_exist():
         )
 
 
-def test_wiki_with_none_attachments():
-    syn = Synapse(skip_checks=True)
+def test_wiki_with_none_attachments(syn: Synapse):
     with patch.object(syn, "restPOST"):
         w = Wiki(owner="syn1", markdown="markdown", attachments=None)
         syn.store(w)