From b4d1c905bfad31e07330b1b0bbf608a525702f24 Mon Sep 17 00:00:00 2001 From: Muzammil Siddiq Date: Tue, 22 Sep 2026 13:36:52 +0100 Subject: [PATCH 1/3] fix: handle database connection errors gracefully --- .../auth/fga/fga_provider_db.py | 2 +- .../exception_handlers.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/register_your_data_api/auth/fga/fga_provider_db.py b/src/register_your_data_api/auth/fga/fga_provider_db.py index 56854a6..dd3235d 100644 --- a/src/register_your_data_api/auth/fga/fga_provider_db.py +++ b/src/register_your_data_api/auth/fga/fga_provider_db.py @@ -49,7 +49,7 @@ def __init__(self, connection_str: str): self._connection_str = connection_str def setup(self) -> None: - self._engine = create_engine(self._connection_str, echo=True) + self._engine = create_engine(self._connection_str, echo=True, pool_pre_ping=True) def get_user_fine_grained_permissions(self, user: UUID) -> list[FineGrainedAuthorisationRoleAssociation]: with Session(self._engine) as session: diff --git a/src/register_your_data_api/exception_handlers.py b/src/register_your_data_api/exception_handlers.py index 432a8f1..36d142a 100644 --- a/src/register_your_data_api/exception_handlers.py +++ b/src/register_your_data_api/exception_handlers.py @@ -1,5 +1,6 @@ import fastapi import fastapi.responses +import sqlalchemy.exc import starlette.exceptions from fastapi.exceptions import RequestValidationError from starlette.requests import Request @@ -94,6 +95,22 @@ async def validation_exception_handler( ) +async def db_error_handler(request: Request, exc: sqlalchemy.exc.DBAPIError) -> fastapi.responses.JSONResponse: + """Exception handler for database connection errors (e.g. the connection being dropped + or the database being unreachable), so these are clearly distinguishable in logs from other + unhandled application errors. + """ + + context = request.app.state.context # type: Context + + context.app_logger.error(f"A database error occurred: {exc}", exc_info=True) + + return fastapi.responses.JSONResponse( + {"status": "failed", "data": None, "error": {"status_code": 503, "error_msg": "Service Unavailable"}}, + status_code=503, + ) + + async def unhandled_exception_handler(request: Request, exc: Exception) -> fastapi.responses.JSONResponse: """Catches all unhandled exceptions and returns a generic 500 server error with a simple error message""" @@ -118,4 +135,5 @@ def add_exception_handlers(app: fastapi.FastAPI) -> None: app.add_exception_handler(RYDUserException, ryd_user_exception_handler) # type: ignore[arg-type] app.add_exception_handler(RequestValidationError, validation_exception_handler) # type: ignore[arg-type] app.add_exception_handler(starlette.exceptions.HTTPException, http_exception_handler) # type: ignore[arg-type] + app.add_exception_handler(sqlalchemy.exc.DBAPIError, db_error_handler) # type: ignore[arg-type] app.add_exception_handler(Exception, unhandled_exception_handler) From ebecccbec0b0ea40866946bbdc0a4ec6d16b78e0 Mon Sep 17 00:00:00 2001 From: Muzammil Siddiq Date: Tue, 22 Sep 2026 13:37:42 +0100 Subject: [PATCH 2/3] test: add unit tests for the database error exception handler --- tests/unit/test_exception_handlers.py | 47 +++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 tests/unit/test_exception_handlers.py diff --git a/tests/unit/test_exception_handlers.py b/tests/unit/test_exception_handlers.py new file mode 100644 index 0000000..bb48c67 --- /dev/null +++ b/tests/unit/test_exception_handlers.py @@ -0,0 +1,47 @@ +"""Tests for the application's exception handlers.""" + +import asyncio +import json +from unittest import mock + +import sqlalchemy.exc + +from register_your_data_api import exception_handlers + + +def test_db_error_handler_logs_and_returns_service_unavailable() -> None: + """A dropped/unavailable database connection (e.g. psycopg.errors.AdminShutdown surfacing as a + sqlalchemy.exc.OperationalError) must be logged clearly, distinct from other unhandled errors, + and reported to the client as a 503.""" + + exc = sqlalchemy.exc.OperationalError( + statement="SELECT 1", + params=None, + orig=Exception("terminating connection due to administrator command"), + ) + + request = mock.MagicMock() + context = request.app.state.context + + response = asyncio.run(exception_handlers.db_error_handler(request, exc)) + + assert response.status_code == 503 + assert json.loads(bytes(response.body)) == { + "status": "failed", + "data": None, + "error": {"status_code": 503, "error_msg": "Service Unavailable"}, + } + + context.app_logger.error.assert_called_once_with(f"A database error occurred: {exc}", exc_info=True) + + +def test_db_error_handler_registered_for_dbapierror() -> None: + """The handler must be registered against sqlalchemy.exc.DBAPIError (the common base for + OperationalError and other driver-level errors), not just the specific OperationalError + subclass, so connection failures of any flavour are handled consistently.""" + + app = mock.MagicMock() + + exception_handlers.add_exception_handlers(app) + + app.add_exception_handler.assert_any_call(sqlalchemy.exc.DBAPIError, exception_handlers.db_error_handler) From 68033bd67c19142bb592a7bf05fad32682bb2faf Mon Sep 17 00:00:00 2001 From: Muzammil Siddiq Date: Thu, 1 Oct 2026 19:54:18 +0100 Subject: [PATCH 3/3] fix: return 503 only for retryable database errors --- .../exception_handlers.py | 30 ++++++-- tests/unit/test_exception_handlers.py | 72 ++++++++++++++++--- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/src/register_your_data_api/exception_handlers.py b/src/register_your_data_api/exception_handlers.py index 36d142a..53ece9f 100644 --- a/src/register_your_data_api/exception_handlers.py +++ b/src/register_your_data_api/exception_handlers.py @@ -95,15 +95,17 @@ async def validation_exception_handler( ) -async def db_error_handler(request: Request, exc: sqlalchemy.exc.DBAPIError) -> fastapi.responses.JSONResponse: - """Exception handler for database connection errors (e.g. the connection being dropped - or the database being unreachable), so these are clearly distinguishable in logs from other - unhandled application errors. +async def db_operational_error_handler( + request: Request, exc: sqlalchemy.exc.OperationalError +) -> fastapi.responses.JSONResponse: + """Exception handler for database connectivity errors (e.g. the connection being dropped + or the database being unreachable). These are runtime conditions outside the application's + control that a retry is likely to resolve, so they are reported as a 503 rather than a 500. """ context = request.app.state.context # type: Context - context.app_logger.error(f"A database error occurred: {exc}", exc_info=True) + context.app_logger.error(f"A database connectivity error occurred: {exc}", exc_info=True) return fastapi.responses.JSONResponse( {"status": "failed", "data": None, "error": {"status_code": 503, "error_msg": "Service Unavailable"}}, @@ -111,6 +113,23 @@ async def db_error_handler(request: Request, exc: sqlalchemy.exc.DBAPIError) -> ) +async def db_error_handler(request: Request, exc: sqlalchemy.exc.DBAPIError) -> fastapi.responses.JSONResponse: + """Exception handler for database errors that are not connectivity issues (e.g. IntegrityError, + DataError). Unlike OperationalError, retrying these will not help, so they are reported as a 500, + while still being logged clearly as database errors rather than falling through to the generic + unhandled exception handler. + """ + + context = request.app.state.context # type: Context + + context.app_logger.error(f"A database error occurred: {exc}", exc_info=True) + + return fastapi.responses.JSONResponse( + {"status": "failed", "data": None, "error": {"status_code": 500, "error_msg": "Internal Server Error"}}, + status_code=500, + ) + + async def unhandled_exception_handler(request: Request, exc: Exception) -> fastapi.responses.JSONResponse: """Catches all unhandled exceptions and returns a generic 500 server error with a simple error message""" @@ -135,5 +154,6 @@ def add_exception_handlers(app: fastapi.FastAPI) -> None: app.add_exception_handler(RYDUserException, ryd_user_exception_handler) # type: ignore[arg-type] app.add_exception_handler(RequestValidationError, validation_exception_handler) # type: ignore[arg-type] app.add_exception_handler(starlette.exceptions.HTTPException, http_exception_handler) # type: ignore[arg-type] + app.add_exception_handler(sqlalchemy.exc.OperationalError, db_operational_error_handler) # type: ignore[arg-type] app.add_exception_handler(sqlalchemy.exc.DBAPIError, db_error_handler) # type: ignore[arg-type] app.add_exception_handler(Exception, unhandled_exception_handler) diff --git a/tests/unit/test_exception_handlers.py b/tests/unit/test_exception_handlers.py index bb48c67..6fdb198 100644 --- a/tests/unit/test_exception_handlers.py +++ b/tests/unit/test_exception_handlers.py @@ -4,15 +4,17 @@ import json from unittest import mock +import fastapi import sqlalchemy.exc +from fastapi.testclient import TestClient from register_your_data_api import exception_handlers -def test_db_error_handler_logs_and_returns_service_unavailable() -> None: +def test_db_operational_error_handler_logs_and_returns_service_unavailable() -> None: """A dropped/unavailable database connection (e.g. psycopg.errors.AdminShutdown surfacing as a - sqlalchemy.exc.OperationalError) must be logged clearly, distinct from other unhandled errors, - and reported to the client as a 503.""" + sqlalchemy.exc.OperationalError) is a runtime condition a retry is likely to resolve, so it must + be logged clearly, distinct from other unhandled errors, and reported to the client as a 503.""" exc = sqlalchemy.exc.OperationalError( statement="SELECT 1", @@ -23,7 +25,7 @@ def test_db_error_handler_logs_and_returns_service_unavailable() -> None: request = mock.MagicMock() context = request.app.state.context - response = asyncio.run(exception_handlers.db_error_handler(request, exc)) + response = asyncio.run(exception_handlers.db_operational_error_handler(request, exc)) assert response.status_code == 503 assert json.loads(bytes(response.body)) == { @@ -32,16 +34,70 @@ def test_db_error_handler_logs_and_returns_service_unavailable() -> None: "error": {"status_code": 503, "error_msg": "Service Unavailable"}, } + context.app_logger.error.assert_called_once_with(f"A database connectivity error occurred: {exc}", exc_info=True) + + +def test_db_error_handler_logs_and_returns_internal_server_error() -> None: + """A database error that isn't a connectivity problem (e.g. IntegrityError from bad data) will + not be fixed by the client retrying, so it must be reported as a 500, while still being logged + clearly as a database error rather than falling through to the generic unhandled handler.""" + + exc = sqlalchemy.exc.IntegrityError( + statement="INSERT INTO foo VALUES (1)", + params=None, + orig=Exception("duplicate key value violates unique constraint"), + ) + + request = mock.MagicMock() + context = request.app.state.context + + response = asyncio.run(exception_handlers.db_error_handler(request, exc)) + + assert response.status_code == 500 + assert json.loads(bytes(response.body)) == { + "status": "failed", + "data": None, + "error": {"status_code": 500, "error_msg": "Internal Server Error"}, + } + context.app_logger.error.assert_called_once_with(f"A database error occurred: {exc}", exc_info=True) -def test_db_error_handler_registered_for_dbapierror() -> None: - """The handler must be registered against sqlalchemy.exc.DBAPIError (the common base for - OperationalError and other driver-level errors), not just the specific OperationalError - subclass, so connection failures of any flavour are handled consistently.""" +def test_handlers_registered_for_operational_error_and_dbapierror() -> None: + """Both handlers must be registered: the specific OperationalError handler for connectivity + errors, and the DBAPIError handler for other database errors (e.g. IntegrityError, which is a + DBAPIError but not an OperationalError).""" app = mock.MagicMock() exception_handlers.add_exception_handlers(app) + app.add_exception_handler.assert_any_call( + sqlalchemy.exc.OperationalError, exception_handlers.db_operational_error_handler + ) app.add_exception_handler.assert_any_call(sqlalchemy.exc.DBAPIError, exception_handlers.db_error_handler) + + +def test_operational_error_and_dbapierror_use_different_handlers() -> None: + """End-to-end check of FastAPI's exception dispatch (not just the handler bodies in isolation): + since OperationalError is itself a DBAPIError subclass, registering a handler for each only + behaves correctly if FastAPI picks the most specific match. An OperationalError must be routed + to the 503 handler, while a sibling DBAPIError subclass (IntegrityError, not fixed by retrying) + must fall through to the 500 handler rather than both being treated the same.""" + + app = fastapi.FastAPI() + app.state.context = mock.MagicMock() + exception_handlers.add_exception_handlers(app) + + @app.get("/operational-error") + def raise_operational_error() -> None: + raise sqlalchemy.exc.OperationalError(statement="SELECT 1", params=None, orig=Exception("connection lost")) + + @app.get("/integrity-error") + def raise_integrity_error() -> None: + raise sqlalchemy.exc.IntegrityError(statement="INSERT INTO foo VALUES (1)", params=None, orig=Exception("x")) + + client = TestClient(app, raise_server_exceptions=False) + + assert client.get("/operational-error").status_code == 503 + assert client.get("/integrity-error").status_code == 500