You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jason810496 (via GitHub)" <gi...@apache.org> on 2026/05/23 04:45:10 UTC
[PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
jason810496 opened a new pull request, #67367:
URL: https://github.com/apache/airflow/pull/67367
- related: https://github.com/apache/airflow/pull/67363
## Why
`fastapi.HTTPException`, `starlette.exceptions.HTTPException`, and `http.client.HTTPException` are three different classes — mixing them silently breaks `isinstance` / `pytest.raises` checks, and `http.client.HTTPException` has a different constructor so routes return 500 instead of the intended status (the exact bug #67363 fixed in `core_api/routes/ui/dags.py`).
## What
- Add `scripts/ci/prek/check_http_exception_import_from_fastapi.py` — an AST-based prek hook that flags any `from <module> import HTTPException` where `<module>` is not `fastapi` (or a `fastapi.*` submodule).
- Register it in `.pre-commit-config.yaml`, scoped to the FastAPI-using trees: `airflow-core/src/airflow/api_fastapi/`, `airflow-core/tests/unit/api_fastapi/`, and the `src/` + `tests/` of the providers that wire FastAPI apps (`amazon`, `common/ai`, `edge3`, `fab`, `keycloak`).
---
##### Was generative AI tooling used to co-author this PR?
- [x] Yes, with help of Claude Code Opus 4.7 following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #67367:
URL: https://github.com/apache/airflow/pull/67367#discussion_r3292435883
##########
.pre-commit-config.yaml:
##########
Review Comment:
I'd propose to keep the check general where it is but making the prek entries into the specific subfolders. e.g. the core things into /airflow-core/.pre-commit-config.yaml
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #67367:
URL: https://github.com/apache/airflow/pull/67367#discussion_r3298640344
##########
scripts/ci/prek/check_http_exception_import_from_fastapi.py:
##########
@@ -0,0 +1,131 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Check that ``HTTPException`` is imported from ``fastapi`` in fastapi-using trees.
+
+The hook is wired into per-distribution ``.pre-commit-config.yaml`` files
+(``airflow-core``, ``providers/amazon``, ``providers/common/ai``,
+``providers/edge3``, ``providers/fab``, ``providers/keycloak``), each
+scoped to the subtree that actually wires a FastAPI app. Provider trees
+that mix client and server code (e.g. edge3's ``cli/`` is a client) are
+scoped to the server-side subfolders only to avoid false positives on
+stdlib HTTP usage in the client. Within those scopes, every
+``HTTPException`` must come from ``fastapi`` (which re-exports the
+Starlette class). Two common mistakes this hook catches:
+
+* ``from starlette.exceptions import HTTPException`` — a different class at
+ runtime; ``isinstance(exc, fastapi.HTTPException)`` and
+ ``pytest.raises(fastapi.HTTPException)`` will not match it.
+* ``from http.client import HTTPException`` — an unrelated stdlib exception
+ whose constructor signature differs, so the route returns 500 instead of
+ the intended HTTP status.
+"""
+
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+from __future__ import annotations
+
+import argparse
+import ast
+import sys
+from pathlib import Path
+
+from common_prek_utils import console
+
+
+def _is_fastapi_module(module: str) -> bool:
+ """Return True if *module* is ``fastapi`` or a submodule of it."""
+ return module == "fastapi" or module.startswith("fastapi.")
+
+
+def check_file(file_path: Path) -> list[tuple[int, str]]:
+ """Return list of ``(line_number, import_statement)`` violations."""
Review Comment:
Addressed both in https://github.com/apache/airflow/pull/67367/commits/2afe8c889d6cc9f342cc30b945cfa7e758bb76a5
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #67367:
URL: https://github.com/apache/airflow/pull/67367#discussion_r3292437802
##########
.pre-commit-config.yaml:
##########
@@ -490,6 +490,20 @@ repos:
^providers/.*/src/.*\.py$|
^task-sdk/src/.*\.py$|
^shared/.*/src/.*\.py$
+ - id: check-http-exception-import-from-fastapi
+ name: Check HTTPException is imported from fastapi in fastapi-using trees
+ entry: ./scripts/ci/prek/check_http_exception_import_from_fastapi.py
+ language: python
+ pass_filenames: true
+ files: >
+ (?x)
+ ^airflow-core/src/airflow/api_fastapi/.*\.py$|
+ ^airflow-core/tests/unit/api_fastapi/.*\.py$|
+ ^providers/amazon/(src|tests)/.*\.py$|
+ ^providers/common/ai/(src|tests)/.*\.py$|
+ ^providers/edge3/(src|tests)/.*\.py$|
Review Comment:
That check willr aise false-positives. Edge containes both client and server sides.
All code in providers/edge3/src/airflow/providers/edge3/worker_api and providers/edge3/src/airflow/providers/edge3/plugins is the API/Server side but in providers/edge3/src/airflow/providers/edge3/cli is is the client (where the FastAPI import will be wrong!)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #67367:
URL: https://github.com/apache/airflow/pull/67367#discussion_r3294389426
##########
airflow-core/.pre-commit-config.yaml:
##########
@@ -148,6 +148,15 @@ repos:
language: python
pass_filenames: true
files: ^src/airflow/.*\.py$
+ - id: check-http-exception-import-from-fastapi
+ name: Check HTTPException is imported from fastapi
+ entry: ../scripts/ci/prek/check_http_exception_import_from_fastapi.py
+ language: python
+ pass_filenames: true
+ files: >
+ (?x)
+ ^src/airflow/api_fastapi/.*\.py$|
Review Comment:
Coverage gap worth widening: `airflow-core/src/airflow/utils/serve_logs/log_server.py` wires a FastAPI app and raises `HTTPException` in ~10 places, but it lives outside `src/airflow/api_fastapi/`, so this regex skips it. If someone there swaps `from fastapi import HTTPException` for `from starlette.exceptions import HTTPException` (or `http.client`), the hook will not catch it -- the exact bug class this PR is meant to prevent.
Options: add `^src/airflow/utils/serve_logs/.*\.py$` to the union, or rescope the regex to FastAPI usage rather than path.
##########
scripts/ci/prek/check_http_exception_import_from_fastapi.py:
##########
@@ -0,0 +1,131 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Check that ``HTTPException`` is imported from ``fastapi`` in fastapi-using trees.
+
+The hook is wired into per-distribution ``.pre-commit-config.yaml`` files
+(``airflow-core``, ``providers/amazon``, ``providers/common/ai``,
+``providers/edge3``, ``providers/fab``, ``providers/keycloak``), each
+scoped to the subtree that actually wires a FastAPI app. Provider trees
+that mix client and server code (e.g. edge3's ``cli/`` is a client) are
+scoped to the server-side subfolders only to avoid false positives on
+stdlib HTTP usage in the client. Within those scopes, every
+``HTTPException`` must come from ``fastapi`` (which re-exports the
+Starlette class). Two common mistakes this hook catches:
+
+* ``from starlette.exceptions import HTTPException`` — a different class at
+ runtime; ``isinstance(exc, fastapi.HTTPException)`` and
+ ``pytest.raises(fastapi.HTTPException)`` will not match it.
+* ``from http.client import HTTPException`` — an unrelated stdlib exception
+ whose constructor signature differs, so the route returns 500 instead of
+ the intended HTTP status.
+"""
+
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+from __future__ import annotations
+
+import argparse
+import ast
+import sys
+from pathlib import Path
+
+from common_prek_utils import console
+
+
+def _is_fastapi_module(module: str) -> bool:
+ """Return True if *module* is ``fastapi`` or a submodule of it."""
+ return module == "fastapi" or module.startswith("fastapi.")
+
+
+def check_file(file_path: Path) -> list[tuple[int, str]]:
+ """Return list of ``(line_number, import_statement)`` violations."""
Review Comment:
Could you add a test for this under `scripts/tests/ci/prek/test_check_http_exception_import_from_fastapi.py`? The sibling hook `check_conf_import_in_providers` has one, and CLAUDE.md asks for tests with every new feature. A small parametrized test over a handful of good/bad import strings (plain, aliased, dotted-from `fastapi.exceptions`, `starlette.exceptions`, `http.client`) would lock in the AST logic and guard against regressions when the alias-rendering / module-prefix logic is touched.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #67367:
URL: https://github.com/apache/airflow/pull/67367#discussion_r3294268358
##########
.pre-commit-config.yaml:
##########
@@ -490,6 +490,20 @@ repos:
^providers/.*/src/.*\.py$|
^task-sdk/src/.*\.py$|
^shared/.*/src/.*\.py$
+ - id: check-http-exception-import-from-fastapi
+ name: Check HTTPException is imported from fastapi in fastapi-using trees
+ entry: ./scripts/ci/prek/check_http_exception_import_from_fastapi.py
+ language: python
+ pass_filenames: true
+ files: >
+ (?x)
+ ^airflow-core/src/airflow/api_fastapi/.*\.py$|
+ ^airflow-core/tests/unit/api_fastapi/.*\.py$|
+ ^providers/amazon/(src|tests)/.*\.py$|
+ ^providers/common/ai/(src|tests)/.*\.py$|
+ ^providers/edge3/(src|tests)/.*\.py$|
Review Comment:
Nice catch for both comments, fixed them in https://github.com/apache/airflow/pull/67367/changes/49e36b07b8554a13f1d9ae9ec1ae4d89c5b06d2b .
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 merged PR #67367:
URL: https://github.com/apache/airflow/pull/67367
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add prek hook to enforce HTTPException is imported from fastapi [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on PR #67367:
URL: https://github.com/apache/airflow/pull/67367#issuecomment-4524188314
CI should fail before https://github.com/apache/airflow/pull/67363 get merged.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org