Skip to content

Commit 585d7aa

Browse files
authored
Add prek hook to enforce HTTPException is imported from fastapi (apache#67367)
* Add prek hook to enforce HTTPException is imported from fastapi * Move HTTPException-import hook into per-distribution prek configs Splits the single root-level hook entry into per-distribution `.pre-commit-config.yaml` files (airflow-core, providers/amazon, providers/common/ai, providers/edge3, providers/fab, providers/keycloak). Each entry is scoped to the subtree that actually wires a FastAPI app, so edge3's `cli/` (client) is excluded and only `worker_api/` and `plugins/` are checked. Also fixes a latent bug the hook caught in `airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py`, where `HTTPException` was imported from `http.client` and called with FastAPI's `(status_code, detail)` signature -- the route would return 500 instead of the intended 400 for `dag_id == "~"`. * Cover serve_logs FastAPI app and add unit tests for the import-guard hook Widens the ``airflow-core`` hook scope to ``src/airflow/utils/serve_logs/`` (and its test file) so the worker log-serving FastAPI app -- which raises ``HTTPException`` in ~10 places but lives outside ``api_fastapi/`` -- is also guarded against ``starlette.exceptions`` / ``http.client`` imports of ``HTTPException``. Adds parametrized unit tests at ``scripts/tests/ci/prek/test_check_http_exception_import_from_fastapi.py`` covering plain, aliased, and dotted (``fastapi.exceptions``) good imports, ``starlette.exceptions`` / ``http.client`` violations, aliased violations, and graceful handling of syntax errors and missing files.
1 parent 0120ba7 commit 585d7aa

8 files changed

Lines changed: 306 additions & 0 deletions

File tree

airflow-core/.pre-commit-config.yaml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,17 @@ repos:
148148
language: python
149149
pass_filenames: true
150150
files: ^src/airflow/.*\.py$
151+
- id: check-http-exception-import-from-fastapi
152+
name: Check HTTPException is imported from fastapi
153+
entry: ../scripts/ci/prek/check_http_exception_import_from_fastapi.py
154+
language: python
155+
pass_filenames: true
156+
files: >
157+
(?x)
158+
^src/airflow/api_fastapi/.*\.py$|
159+
^src/airflow/utils/serve_logs/.*\.py$|
160+
^tests/unit/api_fastapi/.*\.py$|
161+
^tests/unit/utils/test_serve_logs\.py$
151162
- id: create-missing-init-py-files-tests
152163
name: Create missing init.py files in tests
153164
entry: ../scripts/ci/prek/check_init_in_tests.py
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
---
18+
default_stages: [pre-commit, pre-push]
19+
minimum_prek_version: '0.3.4'
20+
default_language_version:
21+
python: python3
22+
node: 22.19.0
23+
golang: 1.24.0
24+
repos:
25+
- repo: local
26+
hooks:
27+
- id: check-http-exception-import-from-fastapi
28+
name: Check HTTPException is imported from fastapi
29+
entry: ../../scripts/ci/prek/check_http_exception_import_from_fastapi.py
30+
language: python
31+
pass_filenames: true
32+
files: >
33+
(?x)
34+
^src/airflow/providers/amazon/aws/auth_manager/.*\.py$|
35+
^tests/unit/amazon/aws/auth_manager/.*\.py$

providers/common/ai/.pre-commit-config.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,12 @@ repos:
4747
entry: ../../../scripts/ci/prek/compile_provider_assets.py ai
4848
pass_filenames: false
4949
additional_dependencies: ['pnpm@10.25.0']
50+
- id: check-http-exception-import-from-fastapi
51+
name: Check HTTPException is imported from fastapi
52+
entry: ../../../scripts/ci/prek/check_http_exception_import_from_fastapi.py
53+
language: python
54+
pass_filenames: true
55+
files: >
56+
(?x)
57+
^src/airflow/providers/common/ai/plugins/.*\.py$|
58+
^tests/unit/common/ai/plugins/.*\.py$

providers/edge3/.pre-commit-config.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ repos:
3030
entry: ../../scripts/ci/prek/generate_openapi_spec_providers.py edge
3131
pass_filenames: false
3232
files: ^src/airflow/providers/edge3/worker_api/.*\.py$
33+
- id: check-http-exception-import-from-fastapi
34+
name: Check HTTPException is imported from fastapi
35+
entry: ../../scripts/ci/prek/check_http_exception_import_from_fastapi.py
36+
language: python
37+
pass_filenames: true
38+
files: >
39+
(?x)
40+
^src/airflow/providers/edge3/(worker_api|plugins)/.*\.py$|
41+
^tests/unit/edge3/(worker_api|plugins)/.*\.py$
3342
- id: ts-compile-lint-edge-ui
3443
name: Compile / format / lint edge UI
3544
description: TS types generation / ESLint / Prettier new UI files in Edge Provider

providers/fab/.pre-commit-config.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,15 @@ repos:
4242
entry: ../../scripts/ci/prek/generate_openapi_spec_providers.py fab
4343
pass_filenames: false
4444
files: ^src/airflow/providers/fab/auth_manager/api_fastapi/.*\.py$
45+
- id: check-http-exception-import-from-fastapi
46+
name: Check HTTPException is imported from fastapi
47+
entry: ../../scripts/ci/prek/check_http_exception_import_from_fastapi.py
48+
language: python
49+
pass_filenames: true
50+
files: >
51+
(?x)
52+
^src/airflow/providers/fab/auth_manager/api_fastapi/.*\.py$|
53+
^tests/unit/fab/auth_manager/api_fastapi/.*\.py$
4554
- id: update-migration-references-fab
4655
name: Update migration ref doc for FAB
4756
language: python

providers/keycloak/.pre-commit-config.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,12 @@ repos:
3030
entry: ../../scripts/ci/prek/generate_openapi_spec_providers.py keycloak
3131
pass_filenames: false
3232
files: ^src/airflow/providers/keycloak/auth_manager/.*\.py$
33+
- id: check-http-exception-import-from-fastapi
34+
name: Check HTTPException is imported from fastapi
35+
entry: ../../scripts/ci/prek/check_http_exception_import_from_fastapi.py
36+
language: python
37+
pass_filenames: true
38+
files: >
39+
(?x)
40+
^src/airflow/providers/keycloak/auth_manager/.*\.py$|
41+
^tests/unit/keycloak/auth_manager/.*\.py$
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
#!/usr/bin/env python
2+
#
3+
# Licensed to the Apache Software Foundation (ASF) under one
4+
# or more contributor license agreements. See the NOTICE file
5+
# distributed with this work for additional information
6+
# regarding copyright ownership. The ASF licenses this file
7+
# to you under the Apache License, Version 2.0 (the
8+
# "License"); you may not use this file except in compliance
9+
# with the License. You may obtain a copy of the License at
10+
#
11+
# http://www.apache.org/licenses/LICENSE-2.0
12+
#
13+
# Unless required by applicable law or agreed to in writing,
14+
# software distributed under the License is distributed on an
15+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
16+
# KIND, either express or implied. See the License for the
17+
# specific language governing permissions and limitations
18+
# under the License.
19+
"""Check that ``HTTPException`` is imported from ``fastapi`` in fastapi-using trees.
20+
21+
The hook is wired into per-distribution ``.pre-commit-config.yaml`` files
22+
(``airflow-core``, ``providers/amazon``, ``providers/common/ai``,
23+
``providers/edge3``, ``providers/fab``, ``providers/keycloak``), each
24+
scoped to the subtree that actually wires a FastAPI app. In
25+
``airflow-core`` that includes ``api_fastapi/`` and
26+
``utils/serve_logs/`` (the worker log-serving FastAPI app). Provider
27+
trees that mix client and server code (e.g. edge3's ``cli/`` is a
28+
client) are scoped to the server-side subfolders only to avoid false
29+
positives on stdlib HTTP usage in the client. Within those scopes,
30+
every ``HTTPException`` must come from ``fastapi`` (which re-exports
31+
the Starlette class). Two common mistakes this hook catches:
32+
33+
* ``from starlette.exceptions import HTTPException`` — a different class at
34+
runtime; ``isinstance(exc, fastapi.HTTPException)`` and
35+
``pytest.raises(fastapi.HTTPException)`` will not match it.
36+
* ``from http.client import HTTPException`` — an unrelated stdlib exception
37+
whose constructor signature differs, so the route returns 500 instead of
38+
the intended HTTP status.
39+
"""
40+
41+
# /// script
42+
# requires-python = ">=3.10,<3.11"
43+
# dependencies = [
44+
# "rich>=13.6.0",
45+
# ]
46+
# ///
47+
from __future__ import annotations
48+
49+
import argparse
50+
import ast
51+
import sys
52+
from pathlib import Path
53+
54+
from common_prek_utils import console
55+
56+
57+
def _is_fastapi_module(module: str) -> bool:
58+
"""Return True if *module* is ``fastapi`` or a submodule of it."""
59+
return module == "fastapi" or module.startswith("fastapi.")
60+
61+
62+
def check_file(file_path: Path) -> list[tuple[int, str]]:
63+
"""Return list of ``(line_number, import_statement)`` violations."""
64+
try:
65+
source = file_path.read_text(encoding="utf-8")
66+
tree = ast.parse(source, filename=str(file_path))
67+
except (OSError, UnicodeDecodeError, SyntaxError):
68+
return []
69+
70+
violations: list[tuple[int, str]] = []
71+
72+
for node in ast.walk(tree):
73+
if not isinstance(node, ast.ImportFrom) or not node.module:
74+
continue
75+
if _is_fastapi_module(node.module):
76+
continue
77+
bad_aliases = [alias for alias in node.names if alias.name == "HTTPException"]
78+
if not bad_aliases:
79+
continue
80+
rendered = ", ".join(
81+
alias.name if not alias.asname else f"{alias.name} as {alias.asname}" for alias in bad_aliases
82+
)
83+
violations.append((node.lineno, f"from {node.module} import {rendered}"))
84+
85+
return violations
86+
87+
88+
def main() -> None:
89+
parser = argparse.ArgumentParser(description="Check that HTTPException is imported from fastapi")
90+
parser.add_argument("files", nargs="*", help="Files to check")
91+
args = parser.parse_args()
92+
93+
if not args.files:
94+
return
95+
96+
total_violations = 0
97+
98+
for file_path in [Path(f) for f in args.files]:
99+
violations = check_file(file_path)
100+
if not violations:
101+
continue
102+
if console:
103+
console.print(f"[red]{file_path}[/red]:")
104+
for line_num, statement in violations:
105+
console.print(f" [yellow]Line {line_num}[/yellow]: {statement}")
106+
else:
107+
print(f"{file_path}:")
108+
for line_num, statement in violations:
109+
print(f" Line {line_num}: {statement}")
110+
total_violations += len(violations)
111+
112+
if total_violations:
113+
message = (
114+
f"Found {total_violations} HTTPException import(s) not coming from `fastapi`.\n"
115+
"Use `from fastapi import HTTPException` instead. Importing it from "
116+
"`starlette.exceptions`, `http.client`, or any other module yields a "
117+
"different class at runtime and breaks `isinstance` / `pytest.raises` "
118+
"checks against `fastapi.HTTPException` (and, for `http.client`, calls "
119+
"the wrong constructor so the route returns 500 instead of the intended "
120+
"status)."
121+
)
122+
if console:
123+
console.print()
124+
console.print(f"[red]{message}[/red]")
125+
else:
126+
print()
127+
print(message)
128+
sys.exit(1)
129+
130+
131+
if __name__ == "__main__":
132+
main()
133+
sys.exit(0)
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
from __future__ import annotations
18+
19+
from pathlib import Path
20+
21+
import pytest
22+
from check_http_exception_import_from_fastapi import check_file
23+
24+
25+
class TestCheckFile:
26+
@pytest.mark.parametrize(
27+
"code, expected",
28+
[
29+
pytest.param(
30+
"from starlette.exceptions import HTTPException\n",
31+
[(1, "from starlette.exceptions import HTTPException")],
32+
id="starlette-exceptions",
33+
),
34+
pytest.param(
35+
"from http.client import HTTPException\n",
36+
[(1, "from http.client import HTTPException")],
37+
id="http-client",
38+
),
39+
pytest.param(
40+
"from starlette.exceptions import HTTPException as StarletteHTTPException\n",
41+
[(1, "from starlette.exceptions import HTTPException as StarletteHTTPException")],
42+
id="aliased-starlette",
43+
),
44+
pytest.param(
45+
"from http.client import HTTPException, HTTPSConnection\n",
46+
[(1, "from http.client import HTTPException")],
47+
id="mixed-import-with-extra-names",
48+
),
49+
pytest.param(
50+
"from fastapi import HTTPException\n"
51+
"from starlette.exceptions import HTTPException as StarletteHTTPException\n",
52+
[(2, "from starlette.exceptions import HTTPException as StarletteHTTPException")],
53+
id="good-and-bad-mixed",
54+
),
55+
],
56+
)
57+
def test_violations_detected(self, write_python_file, code: str, expected: list[tuple[int, str]]):
58+
f = write_python_file(code)
59+
assert check_file(f) == expected
60+
61+
@pytest.mark.parametrize(
62+
"code",
63+
[
64+
pytest.param("from fastapi import HTTPException\n", id="from-fastapi"),
65+
pytest.param(
66+
"from fastapi.exceptions import HTTPException\n",
67+
id="from-fastapi-exceptions",
68+
),
69+
pytest.param(
70+
"from fastapi import Depends, HTTPException, status\n",
71+
id="multi-name-from-fastapi",
72+
),
73+
pytest.param(
74+
"from fastapi import HTTPException as HTTPExc\n",
75+
id="aliased-from-fastapi",
76+
),
77+
pytest.param("import fastapi\n", id="import-fastapi-module"),
78+
pytest.param("from http.client import HTTPSConnection\n", id="unrelated-import"),
79+
pytest.param("x = 1\n", id="no-imports"),
80+
],
81+
)
82+
def test_no_violation(self, write_python_file, code: str):
83+
f = write_python_file(code)
84+
assert check_file(f) == []
85+
86+
def test_syntax_error_is_silently_skipped(self, write_python_file):
87+
f = write_python_file("def broken(:\n")
88+
assert check_file(f) == []
89+
90+
def test_missing_file_is_silently_skipped(self, tmp_path: Path):
91+
assert check_file(tmp_path / "does_not_exist.py") == []

0 commit comments

Comments
 (0)