From 29b11f66c824ab1939bffe3abb7f0173ed7893ea Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 16 Dec 2020 17:06:15 -0500 Subject: [PATCH] Unify exception handling across FastAPI and wsgi. Also use JSON requests for roles tests instead of form-data - not sure how we're going to tackle that. --- lib/galaxy/web/framework/decorators.py | 32 ++++++++++++---------- lib/galaxy/webapps/galaxy/api/roles.py | 6 ++-- lib/galaxy/webapps/galaxy/fast_app.py | 38 ++++++++++++++++++++++++-- lib/galaxy_test/api/test_roles.py | 19 +++++++------ 4 files changed, 66 insertions(+), 29 deletions(-) diff --git a/lib/galaxy/web/framework/decorators.py b/lib/galaxy/web/framework/decorators.py index 679df878524..764c973b74f 100644 --- a/lib/galaxy/web/framework/decorators.py +++ b/lib/galaxy/web/framework/decorators.py @@ -299,17 +299,7 @@ def expose_api(func, to_json=True, user_required=True, user_or_session_required= try: rval = func(self, trans, *args, **kwargs) except ValidationError as e: - invalid_found = False - missing_found = False - for error in e.errors(): - if error["type"] == "value_error.missing": - missing_found = True - elif error["type"].startswith("type_error"): - invalid_found = True - if missing_found and not invalid_found: - raise RequestParameterMissingException(str(e)) - else: - raise RequestParameterInvalidException(str(e)) + raise validation_error_to_message_exception(e) if to_json: rval = format_return_as_json(rval, jsonp_callback, pretty=trans.debug) return rval @@ -353,7 +343,21 @@ def format_return_as_json(rval, jsonp_callback=None, pretty=False): return json -def __api_error_message(trans, **kwds): +def validation_error_to_message_exception(e): + invalid_found = False + missing_found = False + for error in e.errors(): + if error["type"] == "value_error.missing" or error["type"] == "type_error.none.not_allowed": + missing_found = True + elif error["type"].startswith("type_error"): + invalid_found = True + if missing_found and not invalid_found: + return RequestParameterMissingException(str(e), validation_errors=loads(e.json())) + else: + return RequestParameterInvalidException(str(e), validation_errors=loads(e.json())) + + +def api_error_message(trans, **kwds): exception = kwds.get("exception", None) if exception: # If we are passed a MessageException use err_msg. @@ -379,13 +383,13 @@ def __api_error_message(trans, **kwds): # err_msg used a good number of places already. Might as well not change # it? error_response = dict(err_msg=err_msg, err_code=error_code, **extra_error_info) - if trans.debug: # TODO: Should admins get to see traceback as well? + if trans and trans.debug: # TODO: Should admins get to see traceback as well? error_response["traceback"] = traceback_string return error_response def __api_error_response(trans, **kwds): - error_dict = __api_error_message(trans, **kwds) + error_dict = api_error_message(trans, **kwds) exception = kwds.get("exception", None) # If we are given an status code directly - use it - otherwise check # the exception for a status_code attribute. diff --git a/lib/galaxy/webapps/galaxy/api/roles.py b/lib/galaxy/webapps/galaxy/api/roles.py index a780b51fa4e..9bd52de8acc 100644 --- a/lib/galaxy/webapps/galaxy/api/roles.py +++ b/lib/galaxy/webapps/galaxy/api/roles.py @@ -63,18 +63,18 @@ def get_role_manager(app: UniverseApplication = Depends(get_app)) -> RoleManager class FastAPIRoles: role_manager: RoleManager = Depends(get_role_manager) - @router.get('/') + @router.get('/api/roles') def index(self, trans: SessionRequestContext = Depends(get_trans)) -> RoleListModel: roles = self.role_manager.list_displayable_roles(trans) return RoleListModel(__root__=[role_to_model(trans, r) for r in roles]) - @router.get('/{id}') + @router.get('/api/roles/{id}') def show(self, id: EncodedDatabaseIdField, trans: SessionRequestContext = Depends(get_trans)) -> RoleModel: role_id = trans.app.security.decode_id(id) role = self.role_manager.get(trans, role_id) return role_to_model(trans, role) - @router.put("/") + @router.post("/api/roles") def create(self, trans: SessionRequestContext = Depends(get_trans), admin_user=Depends(get_admin_user), role_definition_model: RoleDefeinitionModel = Body(...)) -> RoleModel: role = self.role_manager.create(trans, role_definition_model) return role_to_model(trans, role) diff --git a/lib/galaxy/webapps/galaxy/fast_app.py b/lib/galaxy/webapps/galaxy/fast_app.py index 90ef4830997..a2fb911b48f 100644 --- a/lib/galaxy/webapps/galaxy/fast_app.py +++ b/lib/galaxy/webapps/galaxy/fast_app.py @@ -1,14 +1,46 @@ -from fastapi import FastAPI +from fastapi import FastAPI, Request from fastapi.middleware.wsgi import WSGIMiddleware +from fastapi.responses import JSONResponse +from pydantic.error_wrappers import ValidationError +from fastapi.exceptions import RequestValidationError +from starlette.middleware.base import RequestResponseEndpoint +from starlette.responses import Response + +from galaxy.exceptions import MessageException +from galaxy.web.framework.decorators import api_error_message, validation_error_to_message_exception + + +def add_exception_handler( + app: FastAPI +) -> None: + + @app.exception_handler(RequestValidationError) + async def exception_middleware(request: Request, exc: MessageException) -> Response: + exc = validation_error_to_message_exception(exc) + error_dict = api_error_message(None, exception=exc) + return JSONResponse( + status_code=exc.status_code, + content=error_dict + ) + + @app.exception_handler(MessageException) + async def exception_middleware(request: Request, exc: MessageException) -> Response: + error_dict = api_error_message(None, exception=exc) + return JSONResponse( + status_code=exc.status_code, + content=error_dict + ) def initialize_fast_app(gx_app, app=None): if app is None: app = FastAPI() + + add_exception_handler(app) wsgi_handler = WSGIMiddleware(gx_app) from galaxy.webapps.galaxy.api import jobs from galaxy.webapps.galaxy.api import roles - app.include_router(jobs.router, prefix='/api/jobs') - app.include_router(roles.router, prefix='/api/roles') + app.include_router(jobs.router, prefix="/api/jobs") + app.include_router(roles.router) app.mount('/', wsgi_handler) return app diff --git a/lib/galaxy_test/api/test_roles.py b/lib/galaxy_test/api/test_roles.py index e043c34e58c..7a46eae8f11 100644 --- a/lib/galaxy_test/api/test_roles.py +++ b/lib/galaxy_test/api/test_roles.py @@ -65,9 +65,9 @@ class RolesApiTestCase(ApiTestCase): description = "A test role." payload = { "name": name, - "user_ids": json.dumps([self.dataset_populator.user_id()]), + "user_ids": [self.dataset_populator.user_id()], } - response = self._post("roles", payload, admin=True) + response = self._post("roles", payload, admin=True, json=True) assert_status_code_is(response, 400) assert_error_code_is(response, error_codes.USER_REQUEST_MISSING_PARAMETER) assert "description" in response.json()["err_msg"] @@ -76,9 +76,9 @@ class RolesApiTestCase(ApiTestCase): payload = { "name": None, "description": description, - "user_ids": json.dumps([self.dataset_populator.user_id()]), + "user_ids": [self.dataset_populator.user_id()], } - response = self._post("roles", payload, admin=True) + response = self._post("roles", payload, admin=True, json=True) assert_status_code_is(response, 400) assert_error_code_is(response, error_codes.USER_REQUEST_MISSING_PARAMETER) assert "name" in response.json()["err_msg"] @@ -87,12 +87,13 @@ class RolesApiTestCase(ApiTestCase): payload = { "name": ["a test", "name"], "description": description, - "user_ids": json.dumps([self.dataset_populator.user_id()]), + "user_ids": [self.dataset_populator.user_id()], } - response = self._post("roles", payload, admin=True) + response = self._post("roles", payload, admin=True, json=True) assert_status_code_is(response, 400) assert_error_code_is(response, error_codes.USER_REQUEST_INVALID_PARAMETER) assert "name" in response.json()["err_msg"] + assert "validation_errors" in response.json() def test_create_valid(self): name = self.dataset_populator.get_random_name() @@ -100,9 +101,9 @@ class RolesApiTestCase(ApiTestCase): payload = { "name": name, "description": description, - "user_ids": json.dumps([self.dataset_populator.user_id()]), + "user_ids": [self.dataset_populator.user_id()], } - response = self._post("roles", payload, admin=True) + response = self._post("roles", payload, admin=True, json=True) assert_status_code_is(response, 200) role = response.json() RolesApiTestCase.check_role_dict(role) @@ -133,7 +134,7 @@ class RolesApiTestCase(ApiTestCase): assert_status_code_is(response, 400) def test_create_only_admin(self): - response = self._post("roles") + response = self._post("roles", json=True) assert_status_code_is(response, 403) @staticmethod