updates for pr comments

chanzuckerberg · Sep 30, 2021 · 5d8fb58 · 5d8fb58
1 parent aff02ed
commit 5d8fb58
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 11 deletions.
diff --git a/server/app/app.py b/server/app/app.py
@@ -135,8 +135,11 @@ def get_data_adaptor(url_dataroot: str = None, dataset: str = None):
         )
 
 
-def expire_metadata_cache(url_dataroot: str = None, dataset: str = None):
-    current_app.dataset_metadata_cache_manager.evict_by_key(f"{url_dataroot}/{dataset}")
+def evict_dataset_from_metadata_cache(url_dataroot: str = None, dataset: str = None):
+    try:
+        current_app.dataset_metadata_cache_manager.evict_by_key(f"{url_dataroot}/{dataset}")
+    except Exception as e:
+        logging.error(e)
 
 
 def rest_get_data_adaptor(func):
@@ -147,7 +150,9 @@ def wrapped_function(self, dataset=None):
                 data_adaptor.set_uri_path(f"{self.url_dataroot}/{dataset}")
                 return func(self, data_adaptor)
         except (DatasetAccessError, DatasetNotFoundError, DatasetMetadataError) as e:
-            expire_metadata_cache(self.url_dataroot, dataset)
+            # if the dataset can not be found or accessed assume there is an issue with the stored metadata and remove
+            # it from the cache
+            evict_dataset_from_metadata_cache(self.url_dataroot, dataset)
             return common_rest.abort_and_log(
                 e.status_code, f"Invalid dataset {dataset}: {e.message}", loglevel=logging.INFO, include_exc_info=True
             )

diff --git a/server/tests/unit/common/test_api.py b/server/tests/unit/common/test_api.py
@@ -8,7 +8,7 @@
 
 import requests
 
-from server.app.app import expire_metadata_cache
+from server.app.app import evict_dataset_from_metadata_cache
 from server.common.config.app_config import AppConfig
 from server.tests import decode_fbs, FIXTURES_ROOT
 from server.tests.fixtures.fixtures import pbmc3k_colors
@@ -616,10 +616,10 @@ def test_metadata_api_called_for_new_dataset(self, mock_get):
         self.assertEqual(mock_get.call_count, 1)
         self.assertEqual(
             f"http://{mock_get._mock_call_args[1]['url']}",
-            "http://api.cellxgene.staging.single-cell.czi.technology/dp/v1/datasets/meta?url=https://cellxgene.staging.single-cell.czi.technology.com/e/pbmc3k_v0.cxg/",  # noqa E501
+            "http://api.cellxgene.staging.single-cell.czi.technology/dp/v1/datasets/meta?url=https://cellxgene.staging.single-cell.czi.technology.com/e/pbmc3k_v0.cxg/",
+            # noqa E501
         )
 
-
     @patch("server.data_common.dataset_metadata.requests.get")
     def test_data_locator_defaults_to_name_based_lookup_if_metadata_api_throws_error(self, mock_get):
         self.TEST_DATASET_URL_BASE = "/e/pbmc3k.cxg"
@@ -634,7 +634,8 @@ def test_data_locator_defaults_to_name_based_lookup_if_metadata_api_throws_error
         self.assertEqual(mock_get.call_count, 1)
         self.assertEqual(
             f"http://{mock_get._mock_call_args[1]['url']}",
-            'http://api.cellxgene.staging.single-cell.czi.technology/dp/v1/datasets/meta?url=https://cellxgene.staging.single-cell.czi.technology.com/e/pbmc3k.cxg/',  # noqa E501
+            'http://api.cellxgene.staging.single-cell.czi.technology/dp/v1/datasets/meta?url=https://cellxgene.staging.single-cell.czi.technology.com/e/pbmc3k.cxg/',
+            # noqa E501
         )
 
         # check schema loads correctly even with metadata api exception
@@ -716,7 +717,6 @@ def test_dataset_does_not_exist(self):
         response = self.client.get(url)
         self.assertEqual(response.status_code, 404)
 
-
     @patch("server.data_common.dataset_metadata.requests.get")
     def test_tombstoned_datasets_redirect_to_data_portal(self, mock_get):
         response_body = json.dumps({
@@ -732,12 +732,13 @@ def test_tombstoned_datasets_redirect_to_data_portal(self, mock_get):
         url = f"{self.TEST_DATASET_URL_BASE}/api/v0.2/{endpoint}"
         result = self.client.get(url)
         self.assertEqual(result.status_code, 302)
-        self.assertEqual(result.headers['Location'], "https://cellxgene.staging.single-cell.czi.technology.com/collections/4f098ff4-4a12-446b-a841-91ba3d8e3fa6?tombstoned_dataset_id=2fa37b10-ab4d-49c9-97a8-b4b3d80bf939") # noqa E501
+        self.assertEqual(result.headers['Location'],
+                         "https://cellxgene.staging.single-cell.czi.technology.com/collections/4f098ff4-4a12-446b-a841-91ba3d8e3fa6?tombstoned_dataset_id=2fa37b10-ab4d-49c9-97a8-b4b3d80bf939")  # noqa E501
 
     @patch("server.app.app.expire_metadata_cache")
     @patch("server.data_common.dataset_metadata.request_dataset_metadata_from_data_portal")
     def test_metadata_cache_item_invalidated_on_errors(self, mock_dp, mock_expire):
-        mock_expire.side_effect = expire_metadata_cache
+        mock_expire.side_effect = evict_dataset_from_metadata_cache
         response_body_bad = {
             "collection_id": "4f098ff4-4a12-446b-a841-91ba3d8e3fa6",
             "collection_visibility": "PUBLIC",
@@ -791,7 +792,6 @@ def setUpClass(cls):
         cls.app.testing = True
         cls.client = cls.app.test_client()
 
-
     @patch("server.data_common.dataset_metadata.request_dataset_metadata_from_data_portal")
     @patch("server.data_common.dataset_metadata.requests.get")
     def test_dataset_metadata_api_called(self, mock_get, mock_dp):