Merge pull request #102 from TheJacksonLaboratory/task/fix-qa-feedback

Fixes for QA feedback on API changes
TheJacksonLaboratory · Nov 6, 2024 · 6e464b2 · 6e464b2
2 parents 452db5a + f90acea
commit 6e464b2
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 84 deletions.
diff --git a/pyproject.toml b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "geneweaver-api"
-version = "0.11.0a0"
+version = "0.11.0a1"
 description = "The Geneweaver API"
 authors = [
     "Alexander Berger <alexander.berger@jax.org>",

diff --git a/src/geneweaver/api/controller/genesets.py b/src/geneweaver/api/controller/genesets.py
@@ -20,6 +20,7 @@
 from typing_extensions import Annotated
 
 from . import message as api_message
+from .utilities import raise_http_error
 
 router = APIRouter(prefix="/genesets", tags=["genesets"])
 
@@ -141,11 +142,7 @@ def get_visible_genesets(
         offset=offset,
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+    raise_http_error(response)
 
     return response
 
@@ -189,11 +186,7 @@ def get_geneset(
             cursor=cursor, geneset_id=geneset_id, user=user, in_threshold=in_threshold
         )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+    raise_http_error(response)
 
     return response
 
@@ -217,11 +210,7 @@ def get_geneset_values(
         in_threshold=in_threshold,
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+    raise_http_error(response)
 
     if response.get("data") is None:
         raise HTTPException(
@@ -254,10 +243,7 @@ def get_export_geneset_by_id_type(
         response = genset_service.get_geneset(cursor, geneset_id, user)
 
     if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+        raise_http_error(response)
 
     id_type = response.get("gene_identifier_type")
     if id_type:
@@ -296,11 +282,7 @@ def get_geneset_metadata(
         cursor, geneset_id, user, include_pub_info
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+    raise_http_error(response)
 
     return response
 
@@ -317,10 +299,7 @@ def get_publication_for_geneset(
     geneset_resp = genset_service.get_geneset_metadata(cursor, geneset_id, user, True)
 
     if "error" in geneset_resp:
-        if geneset_resp.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-        else:
-            raise HTTPException(status_code=500, detail=api_message.UNEXPECTED_ERROR)
+        raise_http_error(geneset_resp)
 
     geneset = geneset_resp.get("geneset")
     if geneset is None:
@@ -352,9 +331,7 @@ def put_geneset_threshold(
         cursor, geneset_id, gene_score_type, user
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
+    raise_http_error(response)
 
 
 @router.get("/{geneset_id}/ontologies")
@@ -388,14 +365,7 @@ def get_geneset_ontology_terms(
         cursor, geneset_id, user, limit, offset
     )
 
-    if "error" in terms_resp:
-        if terms_resp.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-
-        if terms_resp.get("message") == api_message.INACCESSIBLE_OR_FORBIDDEN:
-            raise HTTPException(
-                status_code=404, detail=api_message.INACCESSIBLE_OR_FORBIDDEN
-            )
+    raise_http_error(terms_resp)
 
     return terms_resp
 
@@ -423,22 +393,7 @@ def put_geneset_ontology_term(
         user=user,
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-
-        if response.get("message") == api_message.RECORD_NOT_FOUND_ERROR:
-            raise HTTPException(
-                status_code=404, detail=api_message.RECORD_NOT_FOUND_ERROR
-            )
-
-        if response.get("message") == api_message.RECORD_EXISTS:
-            raise HTTPException(status_code=412, detail=api_message.RECORD_EXISTS)
-
-        if response.get("message") == api_message.INACCESSIBLE_OR_FORBIDDEN:
-            raise HTTPException(
-                status_code=404, detail=api_message.INACCESSIBLE_OR_FORBIDDEN
-            )
+    raise_http_error(response)
 
 
 @router.delete("/{geneset_id}/ontologies/{ontology_id}", status_code=204)
@@ -464,16 +419,4 @@ def delete_geneset_ontology_term(
         user=user,
     )
 
-    if "error" in response:
-        if response.get("message") == api_message.ACCESS_FORBIDDEN:
-            raise HTTPException(status_code=403, detail=api_message.ACCESS_FORBIDDEN)
-
-        if response.get("message") == api_message.RECORD_NOT_FOUND_ERROR:
-            raise HTTPException(
-                status_code=404, detail=api_message.RECORD_NOT_FOUND_ERROR
-            )
-
-        if response.get("message") == api_message.INACCESSIBLE_OR_FORBIDDEN:
-            raise HTTPException(
-                status_code=404, detail=api_message.INACCESSIBLE_OR_FORBIDDEN
-            )
+    raise_http_error(response)
diff --git a/src/geneweaver/api/controller/utilities.py b/src/geneweaver/api/controller/utilities.py
@@ -0,0 +1,36 @@
+"""Utilities for FastAPI Controller."""
+
+from fastapi import HTTPException
+
+from . import message as api_message
+
+EXCEPTION_MAP = {
+    api_message.ACCESS_FORBIDDEN: HTTPException(
+        status_code=403, detail=api_message.ACCESS_FORBIDDEN
+    ),
+    api_message.INACCESSIBLE_OR_FORBIDDEN: HTTPException(
+        status_code=404, detail=api_message.INACCESSIBLE_OR_FORBIDDEN
+    ),
+    api_message.RECORD_NOT_FOUND_ERROR: HTTPException(
+        status_code=404, detail=api_message.RECORD_NOT_FOUND_ERROR
+    ),
+    api_message.RECORD_EXISTS: HTTPException(
+        status_code=412, detail=api_message.RECORD_EXISTS
+    ),
+}
+
+
+def raise_http_error(response: dict) -> None:
+    """Raise HTTPException based on response message.
+
+    :param response: dict
+
+    raises: HTTPException if "error" key is in response dict.
+    """
+    if "error" in response:
+        try:
+            raise EXCEPTION_MAP[response.get("message")]
+        except KeyError:
+            raise HTTPException(
+                status_code=500, detail=api_message.UNEXPECTED_ERROR
+            ) from None
diff --git a/src/geneweaver/api/services/geneset.py b/src/geneweaver/api/services/geneset.py
@@ -196,6 +196,10 @@ def get_geneset_metadata(
             gs_id=geneset_id,
             with_publication_info=include_pub_info,
         )
+
+        if len(results) <= 0:
+            return {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
+
         return {"geneset": results[0]}
 
     except Exception as err:
@@ -221,8 +225,9 @@ def get_geneset(
             gs_id=geneset_id,
             with_publication_info=False,
         )
+
         if len(results) <= 0:
-            return {"data": None}
+            return {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
         geneset = results[0]
         geneset_values = db_geneset_value.by_geneset_id(
@@ -260,8 +265,9 @@ def get_geneset_gene_values(
             is_readable_by=determine_user_id(user),
             with_publication_info=False,
         )
+
         if len(results) <= 0:
-            return {"data": None}
+            return {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
         # If gene id type is given, check gene species homology to
         # construct proper gene species mapping
@@ -316,6 +322,10 @@ def get_geneset_w_gene_id_type(
             gs_id=geneset_id,
             with_publication_info=False,
         )
+
+        if len(results) <= 0:
+            return {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
+
         geneset = results[0]
         geneset_values = get_gsv_w_gene_homology_update(
             cursor=cursor,
@@ -444,6 +454,7 @@ def get_geneset_ontology_terms(
         is_gs_readable = db_geneset.is_readable(
             cursor=cursor, user_id=determine_user_id(user), geneset_id=geneset_id
         )
+
         if is_gs_readable is False:
             return {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 

diff --git a/tests/services/test_genset.py b/tests/services/test_genset.py
@@ -38,13 +38,15 @@ def test_get_geneset_no_user_access(mock_db_geneset):
     """Test get geneset by ID with no user access."""
     mock_db_geneset.get.return_value = []
     response = geneset.get_geneset(None, 1234, None)
-    assert "data" in response
-    assert response["data"] is None
+    assert "error" in response
+    assert "data" not in response
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
     mock_db_geneset.get.return_value = []
     response = geneset.get_geneset(None, 1234, mock_user)
-    assert "data" in response
-    assert response["data"] is None
+    assert "error" in response
+    assert "data" not in response
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
 
 @patch("geneweaver.api.services.geneset.db_geneset")
@@ -138,18 +140,20 @@ def test_get_geneset_w_gene_id_type_no_user(mock_db_geneset, mock_gsv):
     response = geneset.get_geneset_w_gene_id_type(None, 1234, None, GeneIdentifier(2))
     assert response is not None
     assert isinstance(response, dict)
-    assert "gene_identifier_type" in response
-    assert "geneset" in response
-    assert "geneset_values" in response
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
+    assert "gene_identifier_type" not in response
+    assert "geneset" not in response
+    assert "geneset_values" not in response
 
     response = geneset.get_geneset_w_gene_id_type(
         None, 1234, User(id=None), GeneIdentifier(2)
     )
     assert response is not None
     assert isinstance(response, dict)
-    assert "gene_identifier_type" in response
-    assert "geneset" in response
-    assert "geneset_values" in response
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
+    assert "gene_identifier_type" not in response
+    assert "geneset" not in response
+    assert "geneset_values" not in response
 
 
 @patch("geneweaver.api.services.geneset.db_geneset")
@@ -402,8 +406,9 @@ def test_get_geneset_gene_values_invalid_user(
     response = geneset.get_geneset_gene_values(
         None, user=None, geneset_id=1234, gene_id_type=None
     )
-    assert "data" in response
-    assert response["data"] is None
+    assert "error" in response
+    assert "data" not in response
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
 
 @patch("geneweaver.api.services.geneset.db_geneset")
@@ -499,7 +504,7 @@ def test_geneset_gene_value_w_gene_id_type_none_resp2(mock_db_geneset):
     response = geneset.get_geneset_gene_values(
         None, user=mock_user, geneset_id=1234, gene_id_type=GeneIdentifier(2)
     )
-    assert response == {"data": None}
+    assert response == {"error": True, "message": message.INACCESSIBLE_OR_FORBIDDEN}
 
 
 @patch("geneweaver.api.services.geneset.db_geneset")