Copilot commented on code in PR #62320:
URL: https://github.com/apache/airflow/pull/62320#discussion_r2921833827
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -258,6 +259,16 @@ def custom_generate_unique_id(route: APIRoute):
app.generate_and_include_versioned_routers(execution_api_router)
+ @app.exception_handler(SQLAlchemyError)
+ def handle_sqlalchemy_error(request: Request, exc: SQLAlchemyError):
+ logger.exception(
+ "Database error",
+ exc_info=(type(exc), exc, exc.__traceback__),
+ path=request.url.path,
+ method=request.method,
+ )
+ return JSONResponse(status_code=500, content={"detail": "Database
error occurred"})
Review Comment:
`handle_sqlalchemy_error` returns a different error envelope than the
existing catch-all handler (uses `{"detail": ...}` and omits the
`correlation-id` field). Since DB errors are often operationally important to
trace, consider including the correlation-id in this response the same way as
`handle_exceptions` does, and keep the response schema consistent across 500s.
```suggestion
content = {"message": "Database error occurred"}
if correlation_id := request.headers.get("correlation-id"):
content["correlation-id"] = correlation_id
return JSONResponse(status_code=500, content=content)
```
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -258,6 +259,16 @@ def custom_generate_unique_id(route: APIRoute):
app.generate_and_include_versioned_routers(execution_api_router)
+ @app.exception_handler(SQLAlchemyError)
+ def handle_sqlalchemy_error(request: Request, exc: SQLAlchemyError):
+ logger.exception(
+ "Database error",
+ exc_info=(type(exc), exc, exc.__traceback__),
+ path=request.url.path,
+ method=request.method,
+ )
+ return JSONResponse(status_code=500, content={"detail": "Database
error occurred"})
Review Comment:
The new `SQLAlchemyError` exception handler isn’t covered by tests. Current
tests for "Database error occurred" appear to exercise route-level `try/except`
(e.g. `ti_update_state`) rather than the app-level handler. Add/adjust a unit
test that triggers a `SQLAlchemyError` from an endpoint that no longer catches
it (e.g. `ti_run`) and assert the response body/status (and optionally
correlation-id behavior) to prevent regressions.
```suggestion
content: dict[str, Any] = {"message": "Database error occurred"}
if correlation_id := request.headers.get("correlation-id"):
content["correlation-id"] = correlation_id
return JSONResponse(status_code=500, content=content)
```
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -32,7 +32,7 @@
from pydantic import JsonValue
from sqlalchemy import and_, func, or_, tuple_, update
from sqlalchemy.engine import CursorResult
-from sqlalchemy.exc import NoResultFound, SQLAlchemyError
+from sqlalchemy.exc import NoResultFound
Review Comment:
`SQLAlchemyError` was removed from imports, but the module still references
it later (e.g. in `except SQLAlchemyError as e:` in `ti_update_state`). This
will raise a `NameError` when a DB error occurs and mask the original
exception. Either re-add the `SQLAlchemyError` import here or remove the
remaining `except SQLAlchemyError` blocks and rely on the new app-level
exception handler consistently.
```suggestion
from sqlalchemy.exc import NoResultFound, SQLAlchemyError
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
