From 623278cabaeda5767ad715b67df5894ee30db7ce Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 31 Jan 2024 16:38:29 -0800 Subject: [PATCH] Use Annotated for dependency arguments FastAPI now recommends using Annotated to indicate arguments that are set by FastAPI via dependency injection. Among other benefits, this aligns Python's opinion about whether the argument has a default with reality when the function is called outside of a FastAPI context, which can avoid bugs in some edge case situations. It also allows removing special Ruff configuration whitelisting the special FastAPI dependency functions, since Annotated makes the intent obvious. Also make the same change to route definitions in the test suite. Adjust the docstring for auth_delegated_token_dependency to use a proper link with anchor text. --- pyproject.toml | 9 --------- src/safir/dependencies/gafaelfawr.py | 18 +++++++++++------- tests/dependencies/arq_test.py | 18 +++++++++++------- tests/dependencies/db_session_test.py | 9 +++++++-- tests/dependencies/gafaelfawr_test.py | 9 ++++++--- tests/dependencies/http_client_test.py | 3 ++- tests/dependencies/logger_test.py | 5 +++-- 7 files changed, 40 insertions(+), 31 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index df069ec7..1ea618b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -248,15 +248,6 @@ target-version = "py311" known-first-party = ["safir", "tests"] split-on-trailing-comma = false -[tool.ruff.flake8-bugbear] -extend-immutable-calls = [ - "fastapi.Form", - "fastapi.Header", - "fastapi.Depends", - "fastapi.Path", - "fastapi.Query", -] - # These are too useful as attributes or methods to allow the conflict with the # built-in to rule out their use. [tool.ruff.flake8-builtins] diff --git a/src/safir/dependencies/gafaelfawr.py b/src/safir/dependencies/gafaelfawr.py index 6a31b5b1..ef4b26de 100644 --- a/src/safir/dependencies/gafaelfawr.py +++ b/src/safir/dependencies/gafaelfawr.py @@ -1,5 +1,7 @@ """Gafaelfawr authentication dependencies.""" +from typing import Annotated + from fastapi import Depends, Header from structlog.stdlib import BoundLogger @@ -13,7 +15,7 @@ async def auth_dependency( - x_auth_request_user: str = Header(..., include_in_schema=False) + x_auth_request_user: Annotated[str, Header(include_in_schema=False)] ) -> str: """Retrieve authentication information from HTTP headers. @@ -25,22 +27,24 @@ async def auth_dependency( async def auth_delegated_token_dependency( - x_auth_request_token: str = Header(..., include_in_schema=False) + x_auth_request_token: Annotated[str, Header(include_in_schema=False)] ) -> str: """Retrieve Gafaelfawr delegated token from HTTP headers. Intended for use with applications protected by Gafaelfawr, this retrieves a delegated token from headers added to the incoming request by the - Gafaelfawr ``auth_request`` NGINX subhandler. The delegated token can - be used to make requests to other services on the user's behalf, see - https://gafaelfawr.lsst.io/user-guide/gafaelfawringress.html#requesting-delegated-tokens + Gafaelfawr ``auth_request`` NGINX subhandler. The delegated token can be + used to make requests to other services on the user's behalf. See `the + Gafaelfawr documentation + `__ + for more details. """ return x_auth_request_token async def auth_logger_dependency( - user: str = Depends(auth_dependency), - logger: BoundLogger = Depends(logger_dependency), + user: Annotated[str, Depends(auth_dependency)], + logger: Annotated[BoundLogger, Depends(logger_dependency)], ) -> BoundLogger: """Logger bound to the authenticated user.""" return logger.bind(user=user) diff --git a/tests/dependencies/arq_test.py b/tests/dependencies/arq_test.py index e0ad2b64..f9022d04 100644 --- a/tests/dependencies/arq_test.py +++ b/tests/dependencies/arq_test.py @@ -4,7 +4,7 @@ from collections.abc import AsyncIterator from contextlib import asynccontextmanager -from typing import Any +from typing import Annotated, Any import pytest from arq.constants import default_queue_name @@ -29,7 +29,8 @@ async def test_arq_dependency_mock() -> None: @app.post("/") async def post_job( - arq_queue: MockArqQueue = Depends(arq_dependency), + *, + arq_queue: Annotated[MockArqQueue, Depends(arq_dependency)], ) -> dict[str, Any]: """Create a job.""" job = await arq_queue.enqueue("test_task", "hello", a_number=42) @@ -44,9 +45,10 @@ async def post_job( @app.get("/jobs/{job_id}") async def get_metadata( + *, job_id: str, queue_name: str | None = None, - arq_queue: MockArqQueue = Depends(arq_dependency), + arq_queue: Annotated[MockArqQueue, Depends(arq_dependency)], ) -> dict[str, Any]: """Get metadata about a job.""" try: @@ -66,9 +68,10 @@ async def get_metadata( @app.get("/results/{job_id}") async def get_result( + *, job_id: str, queue_name: str | None = None, - arq_queue: MockArqQueue = Depends(arq_dependency), + arq_queue: Annotated[MockArqQueue, Depends(arq_dependency)], ) -> dict[str, Any]: """Get the results for a job.""" try: @@ -90,9 +93,10 @@ async def get_result( @app.post("/jobs/{job_id}/inprogress") async def post_job_inprogress( + *, job_id: str, queue_name: str | None = None, - arq_queue: MockArqQueue = Depends(arq_dependency), + arq_queue: Annotated[MockArqQueue, Depends(arq_dependency)], ) -> None: """Toggle a job to in-progress, for testing.""" try: @@ -102,12 +106,12 @@ async def post_job_inprogress( @app.post("/jobs/{job_id}/complete") async def post_job_complete( - job_id: str, *, + job_id: str, queue_name: str | None = None, result: str | None = None, success: bool = True, - arq_queue: MockArqQueue = Depends(arq_dependency), + arq_queue: Annotated[MockArqQueue, Depends(arq_dependency)], ) -> None: """Toggle a job to complete, for testing.""" try: diff --git a/tests/dependencies/db_session_test.py b/tests/dependencies/db_session_test.py index 35c02b17..1b88b4f0 100644 --- a/tests/dependencies/db_session_test.py +++ b/tests/dependencies/db_session_test.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +from typing import Annotated import pytest import structlog @@ -48,14 +49,18 @@ async def test_session() -> None: @app.post("/add") async def add( - session: async_scoped_session = Depends(db_session_dependency), + session: Annotated[ + async_scoped_session, Depends(db_session_dependency) + ], ) -> None: async with session.begin(): session.add(User(username="foo")) @app.get("/list") async def get_list( - session: async_scoped_session = Depends(db_session_dependency), + session: Annotated[ + async_scoped_session, Depends(db_session_dependency) + ], ) -> list[str]: async with session.begin(): result = await session.scalars(select(User.username)) diff --git a/tests/dependencies/gafaelfawr_test.py b/tests/dependencies/gafaelfawr_test.py index 94565bff..7992f1f6 100644 --- a/tests/dependencies/gafaelfawr_test.py +++ b/tests/dependencies/gafaelfawr_test.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from typing import Annotated from unittest.mock import ANY import pytest @@ -24,7 +25,9 @@ async def test_auth_dependency() -> None: app = FastAPI() @app.get("/") - async def handler(user: str = Depends(auth_dependency)) -> dict[str, str]: + async def handler( + user: Annotated[str, Depends(auth_dependency)] + ) -> dict[str, str]: return {"user": user} async with AsyncClient(app=app, base_url="https://example.com") as client: @@ -42,7 +45,7 @@ async def test_auth_delegated_token_dependency() -> None: @app.get("/") async def handler( - token: str = Depends(auth_delegated_token_dependency), + token: Annotated[str, Depends(auth_delegated_token_dependency)], ) -> dict[str, str]: return {"token": token} @@ -65,7 +68,7 @@ async def test_auth_logger_dependency(caplog: LogCaptureFixture) -> None: @app.get("/") async def handler( - logger: BoundLogger = Depends(auth_logger_dependency), + logger: Annotated[BoundLogger, Depends(auth_logger_dependency)], ) -> dict[str, str]: logger.info("something") return {} diff --git a/tests/dependencies/http_client_test.py b/tests/dependencies/http_client_test.py index 5c4e86a4..734077e9 100644 --- a/tests/dependencies/http_client_test.py +++ b/tests/dependencies/http_client_test.py @@ -4,6 +4,7 @@ from collections.abc import AsyncIterator from contextlib import asynccontextmanager +from typing import Annotated import pytest import respx @@ -32,7 +33,7 @@ async def test_http_client(respx_mock: respx.Router) -> None: @app.get("/") async def handler( - http_client: AsyncClient = Depends(http_client_dependency), + http_client: Annotated[AsyncClient, Depends(http_client_dependency)], ) -> dict[str, str]: assert isinstance(http_client, AsyncClient) await http_client.get("https://www.google.com") diff --git a/tests/dependencies/logger_test.py b/tests/dependencies/logger_test.py index e7dec1d2..a939b8a0 100644 --- a/tests/dependencies/logger_test.py +++ b/tests/dependencies/logger_test.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from typing import Annotated from unittest.mock import ANY import pytest @@ -24,7 +25,7 @@ async def test_logger(caplog: LogCaptureFixture) -> None: @app.get("/") async def handler( - logger: BoundLogger = Depends(logger_dependency), + logger: Annotated[BoundLogger, Depends(logger_dependency)], ) -> dict[str, str]: logger.info("something", param="value") return {} @@ -61,7 +62,7 @@ async def test_logger_xforwarded(caplog: LogCaptureFixture) -> None: @app.get("/") async def handler( - logger: BoundLogger = Depends(logger_dependency), + logger: Annotated[BoundLogger, Depends(logger_dependency)], ) -> dict[str, str]: logger.info("something", param="value") return {}