Route SQLAlchemyError through global exception handler#69267
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
I agree with the idea of centralizing the handling of SQLAlchemyError under a single handler i.e. SQLAlchemyErrorHandler, but there are 2 things to keep in mind here:
-
Please check for other places in the API that are catching
SQLAlchemyErrorto make sure they are using the new handler. -
It appears that you are not just moving the exception handling, but also changing the API response. Previously these endpoints returned
{"detail": "Database error occurred"}, whereas they now return the structured payload fromSQLAlchemyErrorHandler. I think that's reasonable, but it is an externally observable behaviour change rather than a pure refactor.
Looks good to me otherwise.
henry3260
left a comment
There was a problem hiding this comment.
Thanks for contribution :)
|
cc @jason810496, what do you think about this? |
jason810496
left a comment
There was a problem hiding this comment.
LGTM overall, should be ready to merged after addressing the comments from henry and me. Thanks.
Co-authored-by: Jason(Zhe-You) Liu <68415893+jason810496@users.noreply.github.com>
Hello
This change removes the route-local SQLAlchemyError-to-HTTPException handling from the task instance execution route and let the FastAPI app-level exception handlers deal with database errors consistently.
DataError still maps to HTTP 422, while generic SQLAlchemyError now returns HTTP 500 through the shared handler in airflow-core/src/airflow/api_fastapi/common/exceptions.py.
I also added regression coverage to verify both the global handler path and the task_instances route behavior.
Was generative AI tooling used to co-author this PR?
Generated-by: [Codex] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.