Skip to content

Route SQLAlchemyError through global exception handler#69267

Open
fat-catTW wants to merge 6 commits into
apache:mainfrom
fat-catTW:fix/sqlalchemy-error-handler
Open

Route SQLAlchemyError through global exception handler#69267
fat-catTW wants to merge 6 commits into
apache:mainfrom
fat-catTW:fix/sqlalchemy-error-handler

Conversation

@fat-catTW

@fat-catTW fat-catTW commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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?
  • Yes (please specify the tool below)

Generated-by: [Codex] following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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.

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Please check for other places in the API that are catching SQLAlchemyError to make sure they are using the new handler.

  2. 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 from SQLAlchemyErrorHandler. I think that's reasonable, but it is an externally observable behaviour change rather than a pure refactor.

Looks good to me otherwise.

Comment thread airflow-core/src/airflow/api_fastapi/common/exceptions.py Outdated

@henry3260 henry3260 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution :)

Comment thread airflow-core/src/airflow/api_fastapi/common/exceptions.py Outdated
@henry3260

Copy link
Copy Markdown
Contributor

cc @jason810496, what do you think about this?

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, should be ready to merged after addressing the comments from henry and me. Thanks.

Comment thread airflow-core/src/airflow/api_fastapi/common/exceptions.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:task-sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants