fix: unify status code and move user error to 400 (pinterest#1161)

* fix: unify status code and move user error to 400

* convert more numbers to const

querybook/server/app/auth/permission.py

@@ -208,7 +208,7 @@ def verify_query_execution_access(execution_id, user_envs, session=None):
             session=session,
         ),
         "CANNOT_ACCESS_QUERY_EXECUTION",
-        403,
+        ACCESS_RESTRICTED_STATUS_CODE,
     )
 
 

querybook/server/app/auth/utils.py

@@ -59,7 +59,7 @@ def load_user(uid, session=None):
     if user is None:
         # Invalid user, clear session
         flask_session.clear()
-        flask.abort(401, description="Invalid cookie")
+        flask.abort(UNAUTHORIZED_STATUS_CODE, description="Invalid cookie")
 
     return AuthUser(user)
 
@@ -74,9 +74,11 @@ def load_user_with_api_access_token(request):
                     user = get_user_by_id(token_validation.creator_uid, session=session)
                     return AuthUser(user)
                 else:
-                    flask.abort(401, description="Token is disabled.")
+                    flask.abort(
+                        UNAUTHORIZED_STATUS_CODE, description="Token is disabled."
+                    )
             else:
-                flask.abort(401, description="Token is invalid.")
+                flask.abort(UNAUTHORIZED_STATUS_CODE, description="Token is invalid.")
     return None
 
 

querybook/server/app/datasource.py

@@ -10,7 +10,15 @@
 
 from app.flask_app import flask_app, limiter
 from app.db import get_session
-from const.datasources import DS_PATH
+from const.datasources import (
+    DS_PATH,
+    OK_STATUS_CODE,
+    UNAUTHORIZED_STATUS_CODE,
+    INVALID_SEMANTIC_STATUS_CODE,
+    ACCESS_RESTRICTED_STATUS_CODE,
+    UNKNOWN_CLIENT_ERROR_STATUS_CODE,
+    UNKNOWN_SERVER_ERROR_STATUS_CODE,
+)
 from lib.logger import get_logger
 from logic.impression import create_impression
 from lib.event_logger import event_logger
@@ -49,15 +57,15 @@ def wrapper(fn):
         @functools.wraps(fn)
         def handler(**kwargs):
             if require_auth and not current_user.is_authenticated:
-                flask.abort(401, description="Login required.")
+                flask.abort(UNAUTHORIZED_STATUS_CODE, description="Login required.")
 
             params = {}
             if flask.request.method == "GET":
                 params = json.loads(flask.request.args.get("params", "{}"))
             elif flask.request.is_json:
                 params = flask.request.json
 
-            status = 200
+            status = OK_STATUS_CODE
             try:
                 kwargs.update(params)
 
@@ -80,18 +88,18 @@ def handler(**kwargs):
                 status = e.code
                 results = {"host": _host, "error": e.description}
             except RequestException as e:
-                status = e.status_code or 500
+                status = e.status_code or UNKNOWN_CLIENT_ERROR_STATUS_CODE
                 results = {"host": _host, "error": str(e), "request_exception": True}
             except Exception as e:
                 LOG.error(e, exc_info=True)
-                status = 500
+                status = UNKNOWN_SERVER_ERROR_STATUS_CODE
                 results = {
                     "host": _host,
                     "error": str(e),
                     "traceback": traceback.format_exc(),
                 }
             finally:
-                if status != 200 and "database_session" in flask.g:
+                if status != OK_STATUS_CODE and "database_session" in flask.g:
                     flask.g.database_session.rollback()
             if custom_response:
                 return results
@@ -140,19 +148,21 @@ def handler(*args, **kwargs):
         if current_user.is_authenticated and current_user.is_admin:
             return fn(*args, **kwargs)
         else:
-            flask.abort(403)
+            flask.abort(ACCESS_RESTRICTED_STATUS_CODE)
 
     handler.__raw__ = fn
     return handler
 
 
-def api_assert(value, message="Assertion has failed", status_code=500):
+def api_assert(
+    value, message="Assertion has failed", status_code=INVALID_SEMANTIC_STATUS_CODE
+):
     if not value:
         abort_request(status_code, message)
 
 
 def abort_request(
-    status_code=500,
+    status_code=UNKNOWN_CLIENT_ERROR_STATUS_CODE,
     message=None,
 ):
     raise RequestException(message, status_code)

querybook/server/app/server.py

@@ -5,6 +5,10 @@
 from app.datasource import register, abort_request
 from app.flask_app import flask_app, limiter
 from const.path import WEBAPP_INDEX_PATH
+from const.datasources import (
+    RESOURCE_NOT_FOUND_STATUS_CODE,
+    LOCKED_FOR_DEPLOYMENT_STATUS_CODE,
+)
 
 
 import datasources
@@ -18,15 +22,15 @@
 @register("/<path:ignore>")
 @limiter.exempt
 def datasource_four_oh_four(*args, **kwargs):
-    abort_request(404)
+    abort_request(RESOURCE_NOT_FOUND_STATUS_CODE)
 
 
 @flask_app.route("/ping/")
 @limiter.exempt
 def get_health_check():
     """This is a health check endpoint"""
     if os.path.exists("/tmp/querybook/deploying"):
-        abort(503)
+        abort(LOCKED_FOR_DEPLOYMENT_STATUS_CODE)
     return "pong"
 
 

querybook/server/const/datasources.py

@@ -1,5 +1,10 @@
+OK_STATUS_CODE = 200
+UNKNOWN_CLIENT_ERROR_STATUS_CODE = 400
+UNAUTHORIZED_STATUS_CODE = 401
 ACCESS_RESTRICTED_STATUS_CODE = 403
 RESOURCE_NOT_FOUND_STATUS_CODE = 404
-UNAUTHORIZED_STATUS_CODE = 401
+INVALID_SEMANTIC_STATUS_CODE = 422
+LOCKED_FOR_DEPLOYMENT_STATUS_CODE = 423
+UNKNOWN_SERVER_ERROR_STATUS_CODE = 500
 
 DS_PATH = "/ds"

querybook/server/datasources/board.py

@@ -8,6 +8,7 @@
     get_data_doc_environment_ids,
     get_board_environment_ids,
 )
+from const.datasources import RESOURCE_NOT_FOUND_STATUS_CODE
 from logic import board as logic
 from logic import user as user_logic
 from logic.board_permission import assert_can_read, assert_can_edit, assert_is_owner
@@ -51,7 +52,9 @@ def get_board_by_id(board_id, environment_id):
 
         assert_can_read(board_id, session=session)
         board = Board.get(id=board_id, session=session)
-        api_assert(board is not None, "Invalid board id", 404)
+        api_assert(
+            board is not None, "Invalid board id", RESOURCE_NOT_FOUND_STATUS_CODE
+        )
         verify_environment_permission([board.environment_id])
         return board.to_dict(
             extra_fields=["docs", "tables", "boards", "queries", "items"]

querybook/server/datasources/metastore.py

@@ -15,6 +15,7 @@
 from const.impression import ImpressionItemType
 from const.metastore import DataTableWarningSeverity, MetadataType
 from const.time import seconds_in_a_day
+from const.datasources import RESOURCE_NOT_FOUND_STATUS_CODE
 from lib.lineage.utils import lineage
 from lib.metastore.utils import DataTableFinder
 from lib.metastore import get_metastore_loader
@@ -75,7 +76,7 @@ def get_table(table_id, with_schema=True, with_column=True, with_warnings=True):
         api_assert(
             table,
             "Table doesn't exist or has been deleted from Metastore",
-            status_code=404,
+            status_code=RESOURCE_NOT_FOUND_STATUS_CODE,
         )
         verify_data_schema_permission(table.schema_id, session=session)
         result = table.to_dict(with_schema, with_column, with_warnings)

querybook/server/datasources/query_execution.py

@@ -21,7 +21,10 @@
 from lib.data_doc.meta import var_config_to_var_dict
 from lib.data_doc.doc_types import DataDocMetaVarConfig
 from const.query_execution import QueryExecutionExportStatus, QueryExecutionStatus
-from const.datasources import RESOURCE_NOT_FOUND_STATUS_CODE
+from const.datasources import (
+    RESOURCE_NOT_FOUND_STATUS_CODE,
+    INVALID_SEMANTIC_STATUS_CODE,
+)
 from logic import (
     query_execution as logic,
     datadoc as datadoc_logic,
@@ -458,7 +461,7 @@ def get_templated_query(
             query, var_config_to_var_dict(var_config), engine_id
         )
     except QueryTemplatingError as e:
-        raise RequestException(e)
+        raise RequestException(e, status_code=INVALID_SEMANTIC_STATUS_CODE)
 
 
 @register("/query_execution/templated_query_params/", methods=["POST"])

querybook/server/logic/board_permission.py

@@ -3,6 +3,10 @@
 from app.datasource import api_assert
 from app.db import with_session
 from models.board import Board, BoardEditor
+from const.datasources import (
+    ACCESS_RESTRICTED_STATUS_CODE,
+    RESOURCE_NOT_FOUND_STATUS_CODE,
+)
 
 
 class BoardDoesNotExist(Exception):
@@ -58,10 +62,10 @@ def assert_can_edit(board_id, session=None):
         api_assert(
             user_can_edit(board_id, uid=current_user.id, session=session),
             "CANNOT_EDIT_BOARD",
-            403,
+            ACCESS_RESTRICTED_STATUS_CODE,
         )
     except BoardDoesNotExist:
-        api_assert(False, "BOARD_DNE", 404)
+        api_assert(False, "BOARD_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)
 
 
 @with_session
@@ -70,10 +74,10 @@ def assert_can_read(board_id, session=None):
         api_assert(
             user_can_read(board_id, uid=current_user.id, session=session),
             "CANNOT_READ_BOARD",
-            403,
+            ACCESS_RESTRICTED_STATUS_CODE,
         )
     except BoardDoesNotExist:
-        api_assert(False, "BOARD_DNE", 404)
+        api_assert(False, "BOARD_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)
 
 
 @with_session
@@ -85,7 +89,7 @@ def assert_is_owner(board_id, session=None):
         api_assert(
             board.owner_uid == current_user.id,
             "NOT_BOARD_OWNER",
-            403,
+            ACCESS_RESTRICTED_STATUS_CODE,
         )
     except BoardDoesNotExist:
-        api_assert(False, "BOARD_DNE", 404)
+        api_assert(False, "BOARD_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)

querybook/server/logic/datadoc_permission.py

@@ -4,6 +4,7 @@
 from app.datasource import api_assert
 from app.db import with_session
 from models.datadoc import DataDoc, DataDocEditor
+from const.datasources import UNAUTHORIZED_STATUS_CODE, RESOURCE_NOT_FOUND_STATUS_CODE
 
 
 class DocDoesNotExist(Exception):
@@ -48,10 +49,10 @@ def assert_can_read(doc_id, session=None):
         api_assert(
             user_can_read(doc_id, uid=current_user.id, session=session),
             "CANNOT_READ_DATADOC",
-            403,
+            UNAUTHORIZED_STATUS_CODE,
         )
     except DocDoesNotExist:
-        api_assert(False, "DOC_DNE", 404)
+        api_assert(False, "DOC_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)
 
 
 @with_session
@@ -60,10 +61,10 @@ def assert_can_write(doc_id, session=None):
         api_assert(
             user_can_write(doc_id, uid=current_user.id, session=session),
             "CANNOT_WRITE_DATADOC",
-            403,
+            UNAUTHORIZED_STATUS_CODE,
         )
     except DocDoesNotExist:
-        api_assert(False, "DOC_DNE", 404)
+        api_assert(False, "DOC_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)
 
 
 @with_session
@@ -75,7 +76,7 @@ def assert_is_owner(doc_id, session=None):
         api_assert(
             doc.owner_uid == current_user.id,
             "NOT_DATADOC_OWNER",
-            403,
+            UNAUTHORIZED_STATUS_CODE,
         )
     except DocDoesNotExist:
-        api_assert(False, "DOC_DNE", 404)
+        api_assert(False, "DOC_DNE", RESOURCE_NOT_FOUND_STATUS_CODE)

querybook/tests/test_app/test_server.py

@@ -15,7 +15,7 @@ def test_ping_deploy_api(flask_client, monkeypatch):
             "exists",
             lambda p: True if p == "/tmp/querybook/deploying" else False,
         )
-        assert 503 == flask_client.get("/ping/").status_code
+        assert 423 == flask_client.get("/ping/").status_code
 
 
 def test_four_oh_four(flask_client, fake_user):