pierrejeambrun commented on code in PR #71647: URL: https://github.com/apache/airflow/pull/71647#discussion_r3820433461
########## scripts/ci/prek/check_openapi_exception_doc_in_sync.py: ########## @@ -0,0 +1,254 @@ +#!/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. + +``create_openapi_http_exception_doc(...)`` feeds the ``responses=`` block of a +route, which is what the generated OpenAPI spec — and every client built from +it — uses to model error responses. Nothing ties that list to the statuses the +handler actually raises, so the two drift apart silently and a client ends up +with no model for a response the API really returns. That drift has been fixed +by hand repeatedly (#67570, #67571, #70992, #71011); this hook catches it +instead. + +A handler violates the rule when it raises ``HTTPException(<status>)`` in its +own body with a status neither its own ``responses=`` block nor its router's +declares. To stay free of false positives the check is deliberately +conservative and stays silent when it cannot see the whole picture: + +* ``422`` is never required — FastAPI documents validation errors natively. +* ``401`` and ``403`` are never required. Routers contribute them wholesale via + their auth dependencies, and the router that does so is often built in + another module (``routes/public/__init__.py`` declares both for every public + route), which a per-file check cannot see. +* A status that is not a resolvable constant (``status.HTTP_404_NOT_FOUND``, + a bare ``HTTP_404_NOT_FOUND``, or a literal ``404``) is skipped. +* A handler whose ``responses=`` is not a ``create_openapi_http_exception_doc`` + call over a literal list is skipped entirely, as are handlers where any + declared entry cannot be resolved. + +Because only the handler's own body is inspected, statuses raised by a shared +dependency or a service helper are not required to be declared. The hook +therefore under-reports rather than over-reports. +""" + +# /// script +# requires-python = ">=3.10,<3.11" +# dependencies = [ +# "rich>=13.6.0", +# ] +# /// Review Comment: To remove? ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_state_store.py: ########## @@ -106,7 +107,12 @@ def _resolve_asset_id_by_uri(uri: str, session: SessionDep) -> int: return asset_id [email protected]("/by-name/value") [email protected]( + "/by-name/value", + responses=create_openapi_http_exception_doc( + [(status.HTTP_404_NOT_FOUND, "Asset not found, or it has no value for the key")] + ), +) Review Comment: execution_api routes are built on cadwyn's VersionedAPIRouter, which isn't in ROUTER_CLASSES, so the check can't see router-level responses= there. That's why it reported the task_state_store / asset_state_store 404s as undeclared even though both routers already declare HTTP_404_NOT_FOUND (task_state_store.py:43, asset_state_store.py:85) — the local additions here are actually redundant with the router. Could we add it? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
