diff --git a/litellm/proxy/list_api/common.py b/litellm/proxy/list_api/common.py index 7ef2827f30e..77d942e622a 100644 --- a/litellm/proxy/list_api/common.py +++ b/litellm/proxy/list_api/common.py @@ -1,11 +1,11 @@ """Contract machinery shared by every LiteLLM-defined list route, on any surface.""" +from collections.abc import Iterator from typing import Final from urllib.parse import urlencode from fastapi import Request -from fastapi.dependencies.utils import get_flat_params -from fastapi.params import ParamTypes +from fastapi.dependencies.models import Dependant from fastapi.responses import JSONResponse from litellm.types.proxy.management_endpoints.management_v1 import ( @@ -37,18 +37,19 @@ def problem_response(problem: ProblemDetail) -> JSONResponse: ) +def _query_param_aliases(dependant: Dependant) -> Iterator[str]: + for field in dependant.query_params: + yield field.alias + for sub_dependant in dependant.dependencies: + yield from _query_param_aliases(sub_dependant) + + def _declared_query_params(request: Request) -> frozenset[str]: route: Final = request.scope.get("route") dependant: Final = getattr(route, "dependant", None) - if dependant is None: + if not isinstance(dependant, Dependant): return frozenset() - # fastapi>=0.140.7 removed get_flat_dependant(); get_flat_params() returns the - # flattened (deduped) param list. Filter to query params to match the old behavior. - return frozenset( - field.alias - for field in get_flat_params(dependant) - if getattr(field.field_info, "in_", None) == ParamTypes.query - ) + return frozenset(_query_param_aliases(dependant)) def escape_like(value: str) -> str: diff --git a/tests/code_coverage_tests/recursive_detector.py b/tests/code_coverage_tests/recursive_detector.py index e9f87ba6cae..2886aee3693 100644 --- a/tests/code_coverage_tests/recursive_detector.py +++ b/tests/code_coverage_tests/recursive_detector.py @@ -67,6 +67,7 @@ IGNORE_FUNCTIONS = [ "_redact_agent_params_tree", # max depth set (default 10), same shape as _redact_sensitive_litellm_params. "_restore_redacted_nested_value", # max depth set (default 10), mirrors _redact_agent_params_tree on the write side. "_unqualified", # bounded by the qualifier depth of a static TypedDict annotation (Annotated, Required/NotRequired, ReadOnly around one type, no cycles possible). + "_query_param_aliases", # bounded by the route's Depends() tree, fixed at registration and already resolved acyclically by fastapi itself. ] diff --git a/tests/test_litellm/proxy/list_api/test_common.py b/tests/test_litellm/proxy/list_api/test_common.py index 7275b3544fa..59f7a89ccc0 100644 --- a/tests/test_litellm/proxy/list_api/test_common.py +++ b/tests/test_litellm/proxy/list_api/test_common.py @@ -52,11 +52,8 @@ def test_an_unknown_query_param_is_rejected_as_a_problem(): def test_a_path_param_name_is_not_a_declared_query_param(): - """The flatten step returns path+query+header together; only query names count as declared. - - If the ParamTypes.query filter were dropped, `thing_id` (a path param) would leak - into the declared set and this request would be wrongly accepted. - """ + """`_query_param_aliases` walks `Dependant.query_params`; if it were ever widened + to also read `.path_params`, `thing_id` would leak into the declared set.""" response = _client().get("/things/abc", params={"thing_id": "x"}) assert response.status_code == 400 assert "thing_id" in response.json()["detail"] @@ -69,7 +66,6 @@ def test_a_header_param_name_is_not_a_declared_query_param(): def test_declared_query_params_isolates_query_aliases_from_other_param_types(): - captured: dict[str, frozenset[str]] = {} app = FastAPI() @app.get("/things/{thing_id}") @@ -79,12 +75,11 @@ def test_declared_query_params_isolates_query_aliases_from_other_param_types(): status: Annotated[str | None, Query(alias="filter[status]")] = None, page: Annotated[int, Query(ge=1)] = 1, x_trace: Annotated[str | None, Header()] = None, - ) -> dict[str, bool]: - captured["declared"] = _declared_query_params(request) - return {"ok": True} + ) -> dict[str, list[str]]: + return {"declared": sorted(_declared_query_params(request))} - TestClient(app).get("/things/abc") - assert captured["declared"] == frozenset({"filter[status]", "page"}) + response = TestClient(app).get("/things/abc") + assert frozenset(response.json()["declared"]) == frozenset({"filter[status]", "page"}) def test_declared_query_params_is_empty_when_the_route_has_no_dependant(): @@ -102,9 +97,23 @@ def test_declared_query_params_is_empty_when_the_route_has_no_dependant(): assert _declared_query_params(request) == frozenset() -# fastapi removed these in 0.140.7, which `pyproject.toml` still allows via -# `fastapi>=0.136.3,<1.0`. Add a name here whenever a supported release drops one. -FASTAPI_NAMES_REMOVED_IN_0_140_7 = frozenset({"get_flat_dependant"}) +def _shared_pagination(page: Annotated[int, Query()] = 1) -> int: + return page + + +def test_declared_query_params_includes_a_param_declared_only_on_a_shared_sub_dependency(): + """`page` sits on `_shared_pagination`'s own Dependant, nested under the route's + `dependencies`, not on the route function's own `query_params` — the case a + non-recursive walk over just the top-level Dependant would miss.""" + app = FastAPI() + + @app.get("/probe") + def _handler(request: Request, page: Annotated[int, Depends(_shared_pagination)]) -> dict[str, list[str]]: + return {"declared": sorted(_declared_query_params(request))} + + response = TestClient(app).get("/probe") + assert frozenset(response.json()["declared"]) == frozenset({"page"}) + LIST_API_PACKAGE = Path(str(common_module.__file__)).parent PROXY_PACKAGE = LIST_API_PACKAGE.parent @@ -123,37 +132,24 @@ def _public_names(module: ModuleType) -> frozenset[str]: return frozenset(name for name in vars(module) if not name.startswith("_")) -def _fastapi_names_imported_by(source_file: Path) -> frozenset[str]: +def _modules_imported_from_by(source_file: Path) -> frozenset[str]: tree = ast.parse(source_file.read_text()) - return frozenset( - alias.name - for node in ast.walk(tree) - if isinstance(node, ast.ImportFrom) and (node.module or "").startswith("fastapi") - for alias in node.names - ) + return frozenset(node.module for node in ast.walk(tree) if isinstance(node, ast.ImportFrom) and node.module) @pytest.mark.parametrize("source_file", FRAMEWORK_SOURCE_FILES, ids=lambda path: f"{path.parent.name}/{path.name}") -def test_no_module_imports_a_fastapi_name_removed_in_a_supported_release(source_file: Path): - """`pyproject.toml` allows fastapi up to <1.0, but CI only ever resolves 0.136.3. - - Every other test here passes just as well against a module importing a name - fastapi has since deleted, because the pinned fastapi still has it. On a user's - fastapi>=0.140.7 that import is an ImportError, and `proxy_server` imports every one - of these packages unguarded at module level, so it takes the whole proxy down rather - than just these routes. Globbing them means a new module is covered on sight. +def test_no_module_imports_from_the_private_fastapi_dependencies_utils_module(source_file: Path): + """fastapi.dependencies.utils is a private module fastapi has already removed a name + from once (get_flat_dependant, in 0.140.7) without notice; the names it still exposes + (e.g. get_flat_params) carry the same risk. `proxy_server` imports every one of these + packages unguarded at module level, so an ImportError here takes down the whole proxy + rather than just these routes. Globbing them means a new module is covered on sight. """ - assert not _fastapi_names_imported_by(source_file) & FASTAPI_NAMES_REMOVED_IN_0_140_7 + assert "fastapi.dependencies.utils" not in _modules_imported_from_by(source_file) -def test_common_still_imports_when_fastapi_has_dropped_those_names(monkeypatch: pytest.MonkeyPatch): - """The static check above cannot prove the module actually loads; this does. - - Behaviour cannot be asserted under the same simulation: on 0.136.3 - `get_flat_params` calls `get_flat_dependant` internally, so it raises NameError - once the name is gone. Loading is the part this pins. - """ - for name in FASTAPI_NAMES_REMOVED_IN_0_140_7: +def test_common_still_imports_with_fastapi_dependencies_utils_emptied_out(monkeypatch: pytest.MonkeyPatch): + for name in ("get_flat_dependant", "get_flat_params"): monkeypatch.delattr(fastapi_dependency_utils, name, raising=False) spec = importlib.util.spec_from_file_location( "list_api_common__simulated_fastapi", Path(str(common_module.__file__))