Dunitz/tombstone redirect (#52)

* redirect to db when dataset has been tombstoned * update tests, update cache error catching * add logging * lower case * add more logs * remove logs * Add logs. * Modify tombstone check to be bool. * Add TombstoneException handler; add catch in root route; nitpicks. * Remove logs. * Linter changes. * Refine. * Edit per comments. Co-authored-by: Leslie Park <[email protected]> Co-authored-by: Leslie Park <[email protected]>
chanzuckerberg · Sep 17, 2021 · 7238853 · 7238853
1 parent 2c354db
commit 7238853
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 26 deletions.
diff --git a/server/app/app.py b/server/app/app.py
@@ -22,7 +22,7 @@
 
 import server.common.rest as common_rest
 from server.common.utils.data_locator import DataLocator
-from server.common.errors import DatasetAccessError, RequestException, DatasetNotFoundError
+from server.common.errors import DatasetAccessError, RequestException, DatasetNotFoundError, TombstoneError
 from server.common.health import health_check
 from server.common.utils.utils import path_join, Float32JSONEncoder
 from server.data_common.dataset_metadata import get_dataset_metadata_for_explorer_location
@@ -67,6 +67,11 @@ def cache_control_always(**cache_kwargs):
     return _cache_control(True, **cache_kwargs)
 
 
+@webbp.errorhandler(RequestException)
+def handle_request_exception(error):
+    return common_rest.abort_and_log(error.status_code, error.message, loglevel=logging.INFO, include_exc_info=True)
+
+
 # tell the client not to cache the index.html page so that changes to the app work on redeployment
 # note that the bulk of the data needed by the client (datasets) will still be cached
 @webbp.route("/", methods=["GET"])
@@ -94,11 +99,9 @@ def dataset_index(url_dataroot=None, dataset=None):
         return common_rest.abort_and_log(
             e.status_code, f"Invalid dataset {dataset}: {e.message}", loglevel=logging.INFO, include_exc_info=True
         )
-
-
-@webbp.errorhandler(RequestException)
-def handle_request_exception(error):
-    return common_rest.abort_and_log(error.status_code, error.message, loglevel=logging.INFO, include_exc_info=True)
+    except TombstoneError as e:
+        parent_collection_url = f"{current_app.app_config.server_config.get_web_base_url()}/collections/{e.collection_id}"  # noqa E501
+        return redirect(f"{parent_collection_url}?tombstoned_dataset_id={e.dataset_id}")
 
 
 def get_dataset_metadata(url_dataroot: str = None, dataset: str = None):
@@ -135,6 +138,9 @@ def wrapped_function(self, dataset=None):
             return common_rest.abort_and_log(
                 e.status_code, f"Invalid dataset {dataset}: {e.message}", loglevel=logging.INFO, include_exc_info=True
             )
+        except TombstoneError as e:
+            parent_collection_url = f"{current_app.app_config.server_config.get_web_base_url()}/collections/{e.collection_id}"  # noqa E501
+            return redirect(f"{parent_collection_url}?tombstoned_dataset_id={e.dataset_id}")
 
     return wrapped_function
 

diff --git a/server/common/errors.py b/server/common/errors.py
@@ -2,15 +2,15 @@
 
 
 class CellxgeneException(Exception):
-    """Base class for cellxgene exceptions"""
+    """Base class for cellxgene exceptions."""
 
     def __init__(self, message):
         self.message = message
         super().__init__(message)
 
 
 class RequestException(CellxgeneException):
-    """Baseclass for exceptions that can be raised from a request."""
+    """Base class for exceptions that can be raised from a request."""
 
     # The default status code is 400 (Bad Request)
     default_status_code = HTTPStatus.BAD_REQUEST
@@ -20,6 +20,18 @@ def __init__(self, message, status_code=None):
         self.status_code = status_code or self.default_status_code
 
 
+class TombstoneException(RequestException):
+    """Base class for tombstoned dataset exception."""
+
+    # The default status code is 302 (Found) for redirect
+    default_status_code = HTTPStatus.FOUND
+
+    def __init__(self, message, collection_id, dataset_id, status_code=None):
+        super().__init__(message, status_code)
+        self.collection_id = collection_id
+        self.dataset_id = dataset_id
+
+
 def define_exception(name, doc):
     globals()[name] = type(name, (CellxgeneException,), dict(__doc__=doc))
 
@@ -28,6 +40,18 @@ def define_request_exception(name, doc, default_status_code=HTTPStatus.BAD_REQUE
     globals()[name] = type(name, (RequestException,), dict(__doc__=doc, default_status_code=default_status_code))
 
 
+def define_tombstone_exception(name, doc, default_status_code=HTTPStatus.FOUND):
+    globals()[name] = type(name, (TombstoneException,), dict(__doc__=doc, default_status_code=default_status_code))
+
+
+# Define CellxgeneException Errors
+define_exception("ConfigurationError", "Raised when checking configuration errors")
+define_exception("PrepareError", "Raised when data is misprepared")
+define_exception("SecretKeyRetrievalError", "Raised when get_secret_key from AWS fails")
+define_exception("ObsoleteRequest", "Raised when the request is no longer valid.")
+define_exception("UnsupportedSummaryMethod", "Raised when a gene set summary method is unknown or unsupported.")
+
+# Define RequestException Errors
 define_request_exception("FilterError", "Raised when filter is malformed")
 define_request_exception("JSONEncodingValueError", "Raised when data cannot be encoded into json")
 define_request_exception("MimeTypeError", "Raised when incompatible MIME type selected")
@@ -51,8 +75,8 @@ def define_request_exception(name, doc, default_status_code=HTTPStatus.BAD_REQUE
     default_status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
 )
 
-define_exception("ConfigurationError", "Raised when checking configuration errors")
-define_exception("PrepareError", "Raised when data is misprepared")
-define_exception("SecretKeyRetrievalError", "Raised when get_secret_key from AWS fails")
-define_exception("ObsoleteRequest", "Raised when the request is no longer valid.")
-define_exception("UnsupportedSummaryMethod", "Raised when a gene set summary method is unknown or unsupported.")
+# Define TombstoneException Errors
+define_tombstone_exception(
+    "TombstoneError",
+    "Raised when a user attempts to view a tombstoned dataset",
+)
diff --git a/server/data_common/cache.py b/server/data_common/cache.py
@@ -5,7 +5,7 @@
 
 import typing
 
-from server.common.errors import DatasetAccessError, DatasetNotFoundError
+from server.common.errors import DatasetAccessError, DatasetNotFoundError, TombstoneError
 from contextlib import contextmanager
 
 from server.data_common.rwlock import RWLock
@@ -184,7 +184,7 @@ def get(self, cache_key: str, create_data_function: Optional[Callable] = None, c
                         create_data_function=create_data_function,
                         create_data_args=create_data_args,
                     )
-                except (DatasetNotFoundError, DatasetAccessError) as e:
+                except (DatasetNotFoundError, DatasetAccessError, TombstoneError) as e:
                     cache_item_info.error = e
                     raise
                 finally:

diff --git a/server/data_common/dataset_metadata.py b/server/data_common/dataset_metadata.py
@@ -1,11 +1,10 @@
 import json
 import logging
-
 import requests
 from flask import current_app
 
 from server.common.utils.utils import path_join
-from server.common.errors import DatasetNotFoundError, DatasetAccessError
+from server.common.errors import DatasetNotFoundError, DatasetAccessError, TombstoneError
 from server.common.config.app_config import AppConfig
 from server.common.config.server_config import ServerConfig
 
@@ -18,6 +17,7 @@ def request_dataset_metadata_from_data_portal(data_portal_api_base: str, explore
     headers = {"Content-Type": "application/json", "Accept": "application/json"}
     try:
         response = requests.get(url=f"{data_portal_api_base}/datasets/meta?url={explorer_url}/", headers=headers)
+
         if response.status_code == 200:
             dataset_identifiers = json.loads(response.content)
             return dataset_identifiers
@@ -71,7 +71,16 @@ def get_dataset_metadata_for_explorer_location(dataset_explorer_location: str, a
         dataset_metadata = request_dataset_metadata_from_data_portal(
             data_portal_api_base=app_config.server_config.data_locator__api_base, explorer_url=explorer_url_path
         )
+
         if dataset_metadata:
+            if dataset_metadata["tombstoned"]:
+                dataset_id = dataset_metadata["dataset_id"]
+                collection_id = dataset_metadata["collection_id"]
+                msg = f"Dataset {dataset_id} from collection {collection_id} has been tombstoned and is no " \
+                      "longer available"
+
+                current_app.logger.log(logging.INFO, msg)
+                raise TombstoneError(message=msg, collection_id=collection_id, dataset_id=dataset_id)
             return dataset_metadata
 
     server_config = app_config.server_config

diff --git a/server/tests/unit/common/test_api.py b/server/tests/unit/common/test_api.py
@@ -510,6 +510,7 @@ def setUpClass(cls, mock_get):
         cls.config = AppConfig()
         cls.config.update_server_config(
             data_locator__api_base=cls.data_locator_api_base,
+            app__web_base_url="https://cellxgene.staging.single-cell.czi.technology.com",
             multi_dataset__dataroot={"e": {"base_url": "e", "dataroot": FIXTURES_ROOT}},
             app__flask_secret_key="testing",
             app__debug=True,
@@ -526,7 +527,7 @@ def setUpClass(cls, mock_get):
                 "collection_visibility": "PUBLIC",
                 "dataset_id": "2fa37b10-ab4d-49c9-97a8-b4b3d80bf939",
                 "s3_uri": f"{FIXTURES_ROOT}/pbmc3k.cxg",
-                "tombstoned": "False",
+                "tombstoned": False,
             }
         )
         mock_get.return_value = MockResponse(body=cls.response_body, status_code=200)
@@ -538,10 +539,7 @@ def setUpClass(cls, mock_get):
         cls.schema = json.loads(result.data)
 
         assert mock_get.call_count == 1
-        assert (
-            f"http://{mock_get._mock_call_args[1]['url']}"
-            == f"http://{cls.data_locator_api_base}/datasets/meta?url={cls.config.server_config.get_web_base_url()}{cls.TEST_DATASET_URL_BASE}/"
-        )  # noqa
+        assert f"http://{mock_get._mock_call_args[1]['url']}" == f"http://{cls.data_locator_api_base}/datasets/meta?url={cls.config.server_config.get_web_base_url()}{cls.TEST_DATASET_URL_BASE}/"  # noqa E501
 
     @patch("server.data_common.dataset_metadata.requests.get")
     def test_data_adaptor_uses_corpora_api(self, mock_get):
@@ -605,7 +603,7 @@ def test_metadata_api_called_for_new_dataset(self, mock_get):
                 "collection_visibility": "PUBLIC",
                 "dataset_id": "2fa37b10-ab4d-49c9-97a8-b4b3d80bf939",
                 "s3_uri": f"{FIXTURES_ROOT}/pbmc3k.cxg",
-                "tombstoned": "False",
+                "tombstoned": False,
             }
         )
         mock_get.return_value = MockResponse(body=response_body, status_code=200)
@@ -621,7 +619,7 @@ 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=None/e/pbmc3k_v0.cxg/",
+            "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")
@@ -638,7 +636,7 @@ 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=None/e/pbmc3k.cxg/",
+            '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
@@ -728,7 +726,7 @@ def test_config_with_portal_metadata(self, mock_get):
                 "collection_visibility": "PUBLIC",
                 "dataset_id": "2fa37b10-ab4d-49c9-97a8-b4b3d80bf939",
                 "s3_uri": f"{FIXTURES_ROOT}/pbmc3k.cxg",
-                "tombstoned": "False",
+                "tombstoned": False,
             }
         )
         mock_get.return_value = MockResponse(body=response_body, status_code=200)
@@ -748,6 +746,23 @@ def test_config_with_portal_metadata(self, mock_get):
             },
         )
 
+    @patch("server.data_common.dataset_metadata.requests.get")
+    def test_tombstoned_datasets_redirect_to_data_portal(self, mock_get):
+        response_body = json.dumps({
+            "collection_id": "4f098ff4-4a12-446b-a841-91ba3d8e3fa6",
+            "collection_visibility": "PUBLIC",
+            "dataset_id": "2fa37b10-ab4d-49c9-97a8-b4b3d80bf939",
+            "s3_uri": None,
+            "tombstoned": True,
+        })
+        mock_get.return_value = MockResponse(body=response_body, status_code=200)
+        endpoint = "config"
+        self.TEST_DATASET_URL_BASE = "/e/pbmc3k_v2.cxg"
+        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
+
 
 class MockResponse:
     def __init__(self, body, status_code):