SQ-925: Refactor DivBase API's exception handling #107
Conversation
1. Use class attributes on DivBaseAPIException subclasses to define how each exception should be handled and then have a generic exception handler that uses those attributes to determine the response. Rather than 1 handler per exception. 2. Use exception __name__ as the exception "type" in the API response, rather than a camel case form of the name 3. Set more appropriate logging levels for diff exception types. 4. Delete unused exceptions and reorder exception classes to group them by category.
There was a problem hiding this comment.
Pull request overview
Refactors DivBase API exception handling by consolidating most custom exception handlers into a single generic handler driven by per-exception class attributes, while updating CLI/E2E tests and frontend error rendering to match the new error “type” format (__name__).
Changes:
- Introduces a generic
divbase_api_exception_handlerforDivBaseAPIExceptionsubclasses and streamlines handler registration. - Updates E2E CLI tests to expect PascalCase exception types (class
__name__) in error outputs/responses. - Improves frontend 404 rendering by allowing a specific error message to be displayed on the 404 template.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_integration/cli_commands/test_version_cli.py | Updates expected API error type strings to match exception class names. |
| tests/e2e_integration/cli_commands/test_task_history_cli.py | Updates expected authorization/project-not-found error identifiers to PascalCase. |
| tests/e2e_integration/cli_commands/test_queue_status.py | Updates expected queue-closed error identifiers to PascalCase. |
| tests/e2e_integration/cli_commands/test_personal_access_tokens.py | Updates expected auth/authz error identifiers to PascalCase. |
| tests/e2e_integration/cli_commands/test_middleware.py | Updates expected CLI-version error type to CLIVersionOutdatedError. |
| tests/e2e_integration/cli_commands/test_dimensions_cli.py | Updates expected dimensions-related error identifiers to PascalCase. |
| tests/e2e_integration/cli_commands/test_auth_cli.py | Updates expected CLI-version error identifiers to PascalCase. |
| packages/divbase-api/src/divbase_api/templates/404.html | Allows optional error_message to be shown on the 404 page (fallbacks to default text). |
| packages/divbase-api/src/divbase_api/middleware.py | Aligns CLI-version rejection response type and uses status.HTTP_400_BAD_REQUEST. |
| packages/divbase-api/src/divbase_api/exceptions.py | Adds per-exception handling metadata (log level, traceback inclusion, frontend behavior) and standardizes error_type. |
| packages/divbase-api/src/divbase_api/exception_handlers.py | Replaces many specific handlers with a single generic handler + a few special cases; adjusts logging and response type values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if is_api_request(request): | ||
| if _is_api_request(request): | ||
| return JSONResponse( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, |
There was a problem hiding this comment.
https://fastapi.tiangolo.com/reference/status/#fastapi.status.HTTP_422_UNPROCESSABLE_CONTENT
HTTP_422_UNPROCESSABLE_CONTENT is correct, comment is outdated advice.
| return await _render_error_page( | ||
| request=request, | ||
| message=f"Badly formatted request. Please check your input and try again. Details: {errors}", | ||
| status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, |
There was a problem hiding this comment.
https://fastapi.tiangolo.com/reference/status/#fastapi.status.HTTP_422_UNPROCESSABLE_CONTENT
HTTP_422_UNPROCESSABLE_CONTENT is correct, comment is outdated advice.
| class ProjectCreationError(DivBaseAPIException): | ||
| def __init__(self, message: str = "Project creation failed"): | ||
| super().__init__(message=message, status_code=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| log_level = logging.WARNING | ||
| include_exc_info = True |
There was a problem hiding this comment.
There is no frontend route that can raise this (as Project Creation is only done by admin panel or API), so was just dead code.
| log_level = logging.INFO | ||
| include_exc_info = False | ||
|
|
There was a problem hiding this comment.
This error does not get raised by the frontend so it was dead code, I see the worker (which user could potentially see because we give user logs for vcf queries), catches and re-raises with a custom error, so safe to ignore this comment.
This PR refactors the current
exceptions.pyandexception_handlers.pyapproach used for DivBase API. The prior strategy was ready for some cleanup as each exception_handler fn was repeating essentially the exact same steps. Now that we have quite a lot of exceptions, it makes sense to reduce some of this duplication.New approach:
DivBaseAPIExceptionsubclass uses class attributes to define how it should be handled by a generic exception handler function (divbase_api_exception_handler).exception_handlers.py. For example UserRegistrationError (error can be raised by admin or signing up user) or RequestValidationError - pydantic error which contains a list of 1 or more exceptions. This works fine because fastapi uses the exception handler that is most specific to the exception.Other changes
__name__as the exception "type" in the API response, rather than a camel case form of the name