diff --git a/airflow-core/.pre-commit-config.yaml b/airflow-core/.pre-commit-config.yaml index c59444e234d5d..d9406af4eedb1 100644 --- a/airflow-core/.pre-commit-config.yaml +++ b/airflow-core/.pre-commit-config.yaml @@ -159,6 +159,12 @@ repos: ^src/airflow/utils/serve_logs/.*\.py$| ^tests/unit/api_fastapi/.*\.py$| ^tests/unit/utils/test_serve_logs\.py$ + - id: check-openapi-exception-doc-in-sync + name: Check API routes declare the HTTP statuses they raise + entry: ../scripts/ci/prek/check_openapi_exception_doc_in_sync.py + language: python + pass_filenames: true + files: ^src/airflow/api_fastapi/.*/routes/.*\.py$ - id: create-missing-init-py-files-tests name: Create missing init.py files in tests entry: ../scripts/ci/prek/check_init_in_tests.py diff --git a/airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml b/airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml index 5abfa412e58a7..593fdd2732417 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml +++ b/airflow-core/src/airflow/api_fastapi/core_api/openapi/_private_ui.yaml @@ -304,6 +304,12 @@ paths: application/json: schema: $ref: '#/components/schemas/NextRunAssetsResponse' + '404': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Not Found '422': description: Validation Error content: @@ -403,6 +409,12 @@ paths: application/json: schema: $ref: '#/components/schemas/PartitionedDagRunCollectionResponse' + '404': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Not Found '422': description: Validation Error content: @@ -439,6 +451,12 @@ paths: application/json: schema: $ref: '#/components/schemas/PartitionedDagRunDetailResponse' + '404': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Not Found '422': description: Validation Error content: @@ -2130,6 +2148,12 @@ paths: application/json: schema: $ref: '#/components/schemas/TeamCollectionResponse' + '403': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Forbidden '422': description: Validation Error content: diff --git a/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml b/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml index 69ac00b6e37fd..18a07a4c8874d 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml +++ b/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml @@ -1991,6 +1991,12 @@ paths: application/json: schema: $ref: '#/components/schemas/HTTPExceptionResponse' + '400': + description: Bad Request + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' '422': description: Validation Error content: @@ -9878,6 +9884,12 @@ paths: schema: $ref: '#/components/schemas/HTTPExceptionResponse' description: Forbidden + '404': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Not Found '409': content: application/json: @@ -10026,6 +10038,12 @@ paths: schema: $ref: '#/components/schemas/HTTPExceptionResponse' description: Forbidden + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/HTTPExceptionResponse' + description: Bad Request '404': content: application/json: diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py index 19378dd8aeaaf..2a76f435f6968 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py @@ -293,7 +293,11 @@ def patch_connection( return connection -@connections_router.post("/test", dependencies=[Depends(requires_access_connection(method="POST"))]) +@connections_router.post( + "/test", + responses=create_openapi_http_exception_doc([status.HTTP_400_BAD_REQUEST]), + dependencies=[Depends(requires_access_connection(method="POST"))], +) def test_connection( test_body: ConnectionBody, user: GetUserDep, diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/log.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/log.py index 6b41aa3f5ac67..da8a47dbe5038 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/log.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/log.py @@ -79,7 +79,7 @@ def _buffered_ndjson_stream( @task_instances_log_router.get( "/{task_id}/logs/{try_number}", responses={ - **create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + **create_openapi_http_exception_doc([status.HTTP_400_BAD_REQUEST, status.HTTP_404_NOT_FOUND]), status.HTTP_200_OK: { "description": "Successful Response", "content": ndjson_example_response_for_get_log, diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py index 705880c4898a6..9b6ba4e43e922 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py @@ -156,7 +156,7 @@ def patch_variable( @variables_router.post( "", status_code=status.HTTP_201_CREATED, - responses=create_openapi_http_exception_doc([status.HTTP_409_CONFLICT]), + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND, status.HTTP_409_CONFLICT]), dependencies=[Depends(action_logging()), Depends(requires_access_variable("POST"))], ) def post_variable( diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py index 61384896625e1..1eea3cb6a4bf5 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py @@ -46,6 +46,7 @@ NextRunAssetEventResponse, NextRunAssetsResponse, ) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.core_api.routes.public.assets import OnlyActiveFilter from airflow.api_fastapi.core_api.security import ( requires_access_asset, @@ -142,6 +143,7 @@ def get_assets( @assets_router.get( "/next_run_assets/{dag_id}", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), dependencies=[Depends(requires_access_asset(method="GET")), Depends(requires_access_dag(method="GET"))], ) def next_run_assets( diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py index 60c98f39cd314..d1cec0d8b2700 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/partitioned_dag_runs.py @@ -40,6 +40,7 @@ PartitionedDagRunDetailResponse, PartitionedDagRunResponse, ) +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.core_api.security import ( ReadableDagsFilterDep, requires_access_asset, @@ -242,6 +243,7 @@ def _build_response(row, required_count: int, received_count: int) -> Partitione @partitioned_dag_runs_router.get( "/partitioned_dag_runs", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), dependencies=[Depends(requires_access_asset(method="GET"))], ) def get_partitioned_dag_runs( @@ -355,6 +357,7 @@ def get_partitioned_dag_runs( @partitioned_dag_runs_router.get( "/pending_partitioned_dag_run/{dag_id}", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), dependencies=[Depends(requires_access_asset(method="GET")), Depends(requires_access_dag(method="GET"))], ) def get_pending_partitioned_dag_run( diff --git a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/teams.py b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/teams.py index 32cc975e96ac9..2840954c1ee11 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/teams.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/ui/teams.py @@ -29,6 +29,7 @@ ) from airflow.api_fastapi.common.router import AirflowRouter from airflow.api_fastapi.core_api.datamodels.ui.teams import TeamCollectionResponse, TeamResponse +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.core_api.security import ( ReadableTeamsFilterDep, requires_authenticated, @@ -41,6 +42,7 @@ @teams_router.get( path="", + responses=create_openapi_http_exception_doc([status.HTTP_403_FORBIDDEN]), dependencies=[Depends(requires_authenticated())], ) def list_teams( diff --git a/airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py b/airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py index 4fd73b3067208..476344a00ee59 100644 --- a/airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py +++ b/airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_events.py @@ -29,6 +29,7 @@ QueryAssetEventPartitionKeyRegex, ) from airflow.api_fastapi.common.types import UtcDateTime +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.execution_api.datamodels.asset import AssetResponse from airflow.api_fastapi.execution_api.datamodels.asset_event import ( AssetEventResponse, @@ -106,7 +107,12 @@ def _parse_extra_params(extra: list[str] | None) -> dict[str, str]: return result -@router.get("/by-asset") +@router.get( + "/by-asset", + responses=create_openapi_http_exception_doc( + [(status.HTTP_400_BAD_REQUEST, "Neither name nor uri was supplied")] + ), +) def get_asset_event_by_asset_name_uri( name: Annotated[str | None, Query(description="The name of the Asset")], uri: Annotated[str | None, Query(description="The URI of the Asset")], diff --git a/airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py b/airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py index 74cda44efe74c..8d9b567feeae6 100644 --- a/airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py +++ b/airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py @@ -25,6 +25,7 @@ from airflow._shared.timezones import timezone from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.execution_api.datamodels.hitl import ( HITLDetailRequest, HITLDetailResponse, @@ -109,7 +110,12 @@ def _check_hitl_detail_exists(hitl_detail_model: HITLDetail | None) -> HITLDetai return hitl_detail_model -@router.patch("/{task_instance_id}") +@router.patch( + "/{task_instance_id}", + responses=create_openapi_http_exception_doc( + [(status.HTTP_409_CONFLICT, "A response has already been received for this HITLDetail")] + ), +) def update_hitl_detail( task_instance_id: UUID, payload: UpdateHITLDetailPayload, diff --git a/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py b/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py index 02d723a82185e..f5867669220c1 100644 --- a/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py +++ b/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py @@ -126,6 +126,10 @@ (status.HTTP_404_NOT_FOUND, "Task Instance not found"), (status.HTTP_409_CONFLICT, "The TI is already in the requested state"), (HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for the state transition"), + ( + status.HTTP_500_INTERNAL_SERVER_ERROR, + "The serialized TaskFlow arg spec for this stub task is not valid", + ), ] ), response_model_exclude_unset=True, diff --git a/airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py b/airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py index 0cb6ccb23ce54..75110aecce6b4 100644 --- a/airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py +++ b/airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py @@ -27,6 +27,7 @@ from airflow.api_fastapi.common.db.common import SessionDep from airflow.api_fastapi.core_api.base import BaseModel +from airflow.api_fastapi.core_api.openapi.exceptions import create_openapi_http_exception_doc from airflow.api_fastapi.execution_api.datamodels.xcom import ( XComResponse, XComSequenceIndexResponse, @@ -263,6 +264,9 @@ def get_mapped_xcom_by_slice( @router.head( "/{dag_id}/{run_id}/{task_id}/{key:path}", responses={ + **create_openapi_http_exception_doc( + [(status.HTTP_400_BAD_REQUEST, "map_index cannot be specified in a HEAD request")] + ), status.HTTP_200_OK: { "description": "Metadata about the number of matching XCom values", "headers": { @@ -361,6 +365,14 @@ def get_xcom( @router.post( "/{dag_id}/{run_id}/{task_id}/{key:path}", status_code=status.HTTP_201_CREATED, + responses=create_openapi_http_exception_doc( + [ + ( + status.HTTP_400_BAD_REQUEST, + "The key is empty, the value is too large to map, or is unserializable", + ) + ] + ), ) def set_xcom( dag_id: str, diff --git a/airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts b/airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts index 18f475eb78d81..45fcb8c5a6b14 100644 --- a/airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts +++ b/airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts @@ -473,6 +473,7 @@ export class AssetService { dag_id: data.dagId }, errors: { + 404: 'Not Found', 422: 'Validation Error' } }); @@ -941,6 +942,7 @@ export class ConnectionService { body: data.requestBody, mediaType: 'application/json', errors: { + 400: 'Bad Request', 401: 'Unauthorized', 403: 'Forbidden', 422: 'Validation Error' @@ -3172,6 +3174,7 @@ export class TaskInstanceService { token: data.token }, errors: { + 400: 'Bad Request', 401: 'Unauthorized', 403: 'Forbidden', 404: 'Not Found', @@ -4495,6 +4498,7 @@ export class VariableService { errors: { 401: 'Unauthorized', 403: 'Forbidden', + 404: 'Not Found', 409: 'Conflict', 422: 'Validation Error' } @@ -4765,6 +4769,7 @@ export class PartitionedDagRunService { has_created_dag_run_id: data.hasCreatedDagRunId }, errors: { + 404: 'Not Found', 422: 'Validation Error' } }); @@ -4790,6 +4795,7 @@ export class PartitionedDagRunService { partition_key: data.partitionKey }, errors: { + 404: 'Not Found', 422: 'Validation Error' } }); @@ -5261,6 +5267,7 @@ export class TeamsService { order_by: data.orderBy }, errors: { + 403: 'Forbidden', 422: 'Validation Error' } }); diff --git a/airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts b/airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts index 5afe50bcd14f1..141dfd08d2fe9 100644 --- a/airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts +++ b/airflow-core/src/airflow/ui/openapi-gen/requests/types.gen.ts @@ -5204,6 +5204,10 @@ export type $OpenApiTs = { * Successful Response */ 200: NextRunAssetsResponse; + /** + * Not Found + */ + 404: HTTPExceptionResponse; /** * Validation Error */ @@ -5690,6 +5694,10 @@ export type $OpenApiTs = { * Successful Response */ 200: ConnectionTestResponse; + /** + * Bad Request + */ + 400: HTTPExceptionResponse; /** * Unauthorized */ @@ -7247,6 +7255,10 @@ export type $OpenApiTs = { * Successful Response */ 200: TaskInstancesLogResponse; + /** + * Bad Request + */ + 400: HTTPExceptionResponse; /** * Unauthorized */ @@ -8314,6 +8326,10 @@ export type $OpenApiTs = { * Forbidden */ 403: HTTPExceptionResponse; + /** + * Not Found + */ + 404: HTTPExceptionResponse; /** * Conflict */ @@ -8527,6 +8543,10 @@ export type $OpenApiTs = { * Successful Response */ 200: PartitionedDagRunCollectionResponse; + /** + * Not Found + */ + 404: HTTPExceptionResponse; /** * Validation Error */ @@ -8542,6 +8562,10 @@ export type $OpenApiTs = { * Successful Response */ 200: PartitionedDagRunDetailResponse; + /** + * Not Found + */ + 404: HTTPExceptionResponse; /** * Validation Error */ @@ -8788,6 +8812,10 @@ export type $OpenApiTs = { * Successful Response */ 200: TeamCollectionResponse; + /** + * Forbidden + */ + 403: HTTPExceptionResponse; /** * Validation Error */ diff --git a/scripts/ci/prek/check_openapi_exception_doc_in_sync.py b/scripts/ci/prek/check_openapi_exception_doc_in_sync.py new file mode 100755 index 0000000000000..fe9aad01e0128 --- /dev/null +++ b/scripts/ci/prek/check_openapi_exception_doc_in_sync.py @@ -0,0 +1,239 @@ +#!/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 API route handlers declare every HTTP status they raise. + +``responses=`` is what the generated OpenAPI spec — and every client built from +it — uses to model error responses, but nothing ties it to the statuses a +handler actually raises. The two drift apart silently, and that drift has been +patched by hand repeatedly (#67570, #67571, #70992, #71011). + +A handler violates the rule when it raises ``HTTPException()`` in its +own body with a status neither its own ``responses=`` block nor its router's +declares. The check is deliberately conservative so it can gate CI: ``401``, +``403`` and ``422`` are never required (FastAPI and the routers' auth +dependencies supply them), only the handler's own body is inspected, and +anything it cannot resolve statically is skipped rather than guessed at. It +therefore under-reports rather than over-reports. +""" + +# /// script +# requires-python = ">=3.10,<3.11" +# dependencies = [ +# "rich>=13.6.0", +# ] +# /// +from __future__ import annotations + +import argparse +import ast +import re +import sys +from pathlib import Path + +from common_prek_utils import console + +ROUTE_METHODS = {"get", "post", "put", "patch", "delete", "head", "options"} +ROUTER_CLASSES = {"APIRouter", "AirflowRouter", "VersionedAPIRouter"} +DOC_HELPER = "create_openapi_http_exception_doc" +# 422 is added to every route by FastAPI itself; 401/403 come from the router's auth +# dependencies, declared once on a router this file-scoped check often cannot reach. +ALWAYS_DOCUMENTED = {401, 403, 422} + +_STATUS_CONSTANT = re.compile(r"^HTTP_(\d{3})_") + + +def _resolve_status(node: ast.expr) -> int | None: + """Resolve a status code expression to its numeric value, or None if unknown.""" + if isinstance(node, ast.Attribute): + name = node.attr + elif isinstance(node, ast.Name): + name = node.id + elif isinstance(node, ast.Constant) and isinstance(node.value, int): + return node.value + else: + return None + match = _STATUS_CONSTANT.match(name) + return int(match.group(1)) if match else None + + +def _statuses_from_responses(responses: ast.expr) -> set[int] | None: + """Resolve a ``responses=`` value to its statuses, or None when unanalyzable.""" + # Routes that document a success body spell it as a mapping that unpacks the helper + # alongside literal entries; routers use a plain ``{status: {"description": ...}}``. + if isinstance(responses, ast.Dict): + collected: set[int] = set() + for key, value in zip(responses.keys, responses.values): + if key is None: + # A ``None`` key is a ``**`` unpacking; its value carries the real statuses. + unpacked = _statuses_from_responses(value) + if unpacked is None: + return None + collected |= unpacked + else: + status = _resolve_status(key) + if status is None: + return None + collected.add(status) + return collected + + if not ( + isinstance(responses, ast.Call) + and isinstance(responses.func, ast.Name) + and responses.func.id == DOC_HELPER + and responses.args + and isinstance(entries := responses.args[0], (ast.List, ast.Tuple)) + ): + return None + + declared: set[int] = set() + for entry in entries.elts: + # Entries are either a bare status or a ``(status, description)`` pair. + target = entry.elts[0] if isinstance(entry, ast.Tuple) and entry.elts else entry + status = _resolve_status(target) + if status is None: + return None + declared.add(status) + return declared + + +def _declared_statuses(decorator: ast.Call) -> set[int] | None: + """Return statuses declared by the route's own ``responses=``.""" + responses = next((kw.value for kw in decorator.keywords if kw.arg == "responses"), None) + return set() if responses is None else _statuses_from_responses(responses) + + +def _router_statuses(tree: ast.Module) -> dict[str, set[int] | None]: + """Map each router built in this module to the statuses it declares for every route on it.""" + routers: dict[str, set[int] | None] = {} + for node in ast.walk(tree): + if not isinstance(node, ast.Assign): + continue + call = node.value + if not ( + isinstance(call, ast.Call) and isinstance(call.func, ast.Name) and call.func.id in ROUTER_CLASSES + ): + continue + responses = next((kw.value for kw in call.keywords if kw.arg == "responses"), None) + statuses = set() if responses is None else _statuses_from_responses(responses) + for target in node.targets: + if isinstance(target, ast.Name): + routers[target.id] = statuses + return routers + + +def _raised_statuses(handler: ast.FunctionDef | ast.AsyncFunctionDef) -> dict[int, int]: + """Map each status raised as ``HTTPException`` in the body to its first line.""" + raised: dict[int, int] = {} + for node in ast.walk(handler): + if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Name)): + continue + if node.func.id != "HTTPException": + continue + argument = next( + (kw.value for kw in node.keywords if kw.arg == "status_code"), + node.args[0] if node.args else None, + ) + if argument is None: + continue + if (status := _resolve_status(argument)) is not None: + raised.setdefault(status, node.lineno) + return raised + + +def _route_decorators(handler: ast.FunctionDef | ast.AsyncFunctionDef) -> list[ast.Call]: + return [ + decorator + for decorator in handler.decorator_list + if isinstance(decorator, ast.Call) + and isinstance(decorator.func, ast.Attribute) + and decorator.func.attr in ROUTE_METHODS + ] + + +def check_file(file_path: Path) -> list[tuple[str, int, int]]: + """Return ``(handler_name, status, line_number)`` for each undeclared status.""" + try: + tree = ast.parse(file_path.read_text(encoding="utf-8"), filename=str(file_path)) + except (OSError, UnicodeDecodeError, SyntaxError): + return [] + + routers = _router_statuses(tree) + violations: list[tuple[str, int, int]] = [] + for handler in ast.walk(tree): + if not isinstance(handler, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + for decorator in _route_decorators(handler): + declared = _declared_statuses(decorator) + # A route inherits whatever its router declares for every route on it. + router = decorator.func.value if isinstance(decorator.func, ast.Attribute) else None + inherited = routers.get(router.id, set()) if isinstance(router, ast.Name) else set() + if declared is None or inherited is None: + continue + undeclared = { + status: lineno + for status, lineno in _raised_statuses(handler).items() + if status not in declared | inherited | ALWAYS_DOCUMENTED + } + violations.extend((handler.name, status, lineno) for status, lineno in sorted(undeclared.items())) + return violations + + +def main() -> int: + parser = argparse.ArgumentParser(description="Check API routes declare the statuses they raise") + parser.add_argument("files", nargs="*", help="Files to check") + args = parser.parse_args() + + total = 0 + for file_path in (Path(f) for f in args.files): + violations = check_file(file_path) + if not violations: + continue + total += len(violations) + lines = [ + f" Line {lineno}: {handler}() raises {status} but never declares it" + for handler, status, lineno in violations + ] + if console: + console.print(f"[red]{file_path}[/red]:") + for line in lines: + console.print(f"[yellow]{line}[/yellow]") + else: + print(f"{file_path}:") + print("\n".join(lines)) + + if total: + message = ( + f"Found {total} HTTP status(es) raised by a route handler but missing from its " + f"`responses=` block.\n" + f"Add each one to `{DOC_HELPER}([...])` on the route decorator so the generated " + "OpenAPI spec — and the clients generated from it — model the response the API " + "really returns." + ) + if console: + console.print() + console.print(f"[red]{message}[/red]") + else: + print() + print(message) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/ci/prek/test_check_openapi_exception_doc_in_sync.py b/scripts/tests/ci/prek/test_check_openapi_exception_doc_in_sync.py new file mode 100644 index 0000000000000..ef9adc6bb2088 --- /dev/null +++ b/scripts/tests/ci/prek/test_check_openapi_exception_doc_in_sync.py @@ -0,0 +1,335 @@ +# 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. +from __future__ import annotations + +from pathlib import Path + +import pytest +from check_openapi_exception_doc_in_sync import check_file + + +class TestCheckFile: + @pytest.mark.parametrize( + "code, expected", + [ + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 4)], + id="no-responses-block-at-all", + ), + pytest.param( + """ + @router.get( + "/x", + responses=create_openapi_http_exception_doc([status.HTTP_409_CONFLICT]), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 7)], + id="status-missing-from-responses", + ), + pytest.param( + """ + @router.post("/x") + def handler(): + if a: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="bad") + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail="taken") + """, + [("handler", 400, 5), ("handler", 409, 6)], + id="several-undeclared-statuses-are-all-reported", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 4)], + id="bare-status-constant", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(404, "nope") + """, + [("handler", 404, 4)], + id="literal-status-code", + ), + pytest.param( + """ + @router.get("/x") + async def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 4)], + id="async-handler", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + def fail(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + fail() + """, + [("handler", 404, 5)], + id="raise-nested-inside-handler", + ), + pytest.param( + """ + @router.delete( + "/x", + responses=create_openapi_http_exception_doc( + [(status.HTTP_409_CONFLICT, "conflict")] + ), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 9)], + id="tuple-form-declares-a-different-status", + ), + pytest.param( + """ + router = APIRouter( + responses={status.HTTP_409_CONFLICT: {"description": "conflict"}}, + ) + + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 8)], + id="router-declares-a-different-status", + ), + pytest.param( + """ + @router.get( + "/x", + responses={ + status.HTTP_200_OK: {"description": "ok"}, + }, + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + [("handler", 404, 9)], + id="mapping-responses-without-the-status", + ), + ], + ) + def test_violations_detected(self, write_python_file, code: str, expected): + assert check_file(write_python_file(code)) == expected + + @pytest.mark.parametrize( + "code", + [ + pytest.param( + """ + @router.get( + "/x", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="status-declared-plainly", + ), + pytest.param( + """ + @router.get( + "/x", + responses=create_openapi_http_exception_doc( + [(status.HTTP_404_NOT_FOUND, "not found")] + ), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="status-declared-with-description", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_422_UNPROCESSABLE_CONTENT, "invalid") + """, + id="422-is-documented-by-fastapi-itself", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "who?") + """, + id="401-is-declared-on-the-router", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_403_FORBIDDEN, "no") + """, + id="403-is-declared-on-the-router", + ), + pytest.param( + """ + router = APIRouter( + responses={status.HTTP_404_NOT_FOUND: {"description": "not found"}}, + ) + + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="router-declares-the-status-for-every-route", + ), + pytest.param( + """ + router = VersionedAPIRouter( + responses={status.HTTP_404_NOT_FOUND: {"description": "not found"}}, + ) + + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="cadwyn-versioned-router-declares-the-status", + ), + pytest.param( + """ + teams_router = AirflowRouter() + + @teams_router.get( + "/x", + responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="airflow-router-without-shared-responses", + ), + pytest.param( + """ + @router.get( + "/x", + responses={ + **create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), + status.HTTP_200_OK: {"description": "ok"}, + }, + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="helper-unpacked-into-a-mapping-alongside-a-success-body", + ), + pytest.param( + """ + router = APIRouter(responses=SHARED_ERRORS) + + @router.get("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="router-responses-cannot-be-resolved", + ), + pytest.param( + """ + @router.get( + "/x", + responses={SOME_ALIAS: {"description": "?"}}, + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="mapping-key-cannot-be-resolved", + ), + pytest.param( + """ + @router.get("/x", responses={404: {"model": Foo}}) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="unrecognised-responses-shape-is-skipped", + ), + pytest.param( + """ + @router.get("/x", responses=create_openapi_http_exception_doc(SHARED_ERRORS)) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="declared-list-is-not-a-literal", + ), + pytest.param( + """ + @router.get( + "/x", + responses=create_openapi_http_exception_doc([SOME_ALIAS]), + ) + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="declared-entry-cannot-be-resolved", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + raise HTTPException(chosen_status, "nope") + """, + id="raised-status-cannot-be-resolved", + ), + pytest.param( + """ + def helper(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="not-a-route-handler", + ), + pytest.param( + """ + @router.websocket("/x") + def handler(): + raise HTTPException(status.HTTP_404_NOT_FOUND, "nope") + """, + id="not-an-http-route-method", + ), + pytest.param( + """ + @router.get("/x") + def handler(): + return 1 + """, + id="handler-raises-nothing", + ), + ], + ) + def test_no_violation(self, write_python_file, code: str): + assert check_file(write_python_file(code)) == [] + + def test_syntax_error_is_silently_skipped(self, write_python_file): + assert check_file(write_python_file("def broken(:\n")) == [] + + def test_missing_file_is_silently_skipped(self, tmp_path: Path): + assert check_file(tmp_path / "does_not_exist.py") == []