mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-10 03:28:53 +00:00
fix(files): return 400 for a limit outside the documented range
The unscoped GET /v1/files limit check accepted 0, which OpenAI's minimum of 1 does not allow, and the route's except block rebuilt every error with getattr(e, "status_code", 500). ProxyException has no status_code, so the 400 it raises went out as a 500 and the OpenAI SDK retried it three times. Errors now go through handle_exception_on_proxy, the helper the sibling batches route already uses, and the unknown-cursor error is a ProxyException so it carries type invalid_request_error and param after instead of the literal "None". The cursor still 400s whether the file belongs to someone else or does not exist at all
This commit is contained in:
parent
a15b81d3d7
commit
ba64a1c451
5 changed files with 165 additions and 36 deletions
|
|
@ -1391,8 +1391,6 @@ class _PROXY_LiteLLMManagedFiles(CustomLogger, BaseFileEndpoints):
|
|||
usable even when every file on the page was filtered out.
|
||||
"""
|
||||
validate_file_list_limit(limit)
|
||||
if limit == 0:
|
||||
return build_list_page([])
|
||||
|
||||
owner_filter: Final = build_owner_filter(user_api_key_dict)
|
||||
if owner_filter is None:
|
||||
|
|
@ -1403,9 +1401,12 @@ class _PROXY_LiteLLMManagedFiles(CustomLogger, BaseFileEndpoints):
|
|||
where={**owner_filter, "unified_file_id": after}
|
||||
)
|
||||
if cursor_row is None:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=f"Invalid 'after' cursor: no file found with id '{after}'.",
|
||||
raise ProxyException(
|
||||
message=f"Invalid 'after' cursor: no file found with id '{after}'.",
|
||||
type="invalid_request_error",
|
||||
param="after",
|
||||
code=400,
|
||||
openai_code="invalid_value",
|
||||
)
|
||||
|
||||
page_size: Final = min(limit or MAX_FILE_LIST_LIMIT, MAX_FILE_LIST_LIMIT)
|
||||
|
|
|
|||
|
|
@ -28,11 +28,11 @@ MAX_FILE_LIST_LIMIT: Final = 10000
|
|||
|
||||
def validate_file_list_limit(limit: int | None) -> None:
|
||||
"""Reject a ``limit`` outside the range OpenAI documents for GET /v1/files."""
|
||||
if limit is None or 0 <= limit <= MAX_FILE_LIST_LIMIT:
|
||||
if limit is None or 1 <= limit <= MAX_FILE_LIST_LIMIT:
|
||||
return
|
||||
bound, expected, openai_code = (
|
||||
("below minimum", ">= 0", "integer_below_min_value")
|
||||
if limit < 0
|
||||
("below minimum", ">= 1", "integer_below_min_value")
|
||||
if limit < 1
|
||||
else ("above maximum", f"<= {MAX_FILE_LIST_LIMIT}", "integer_above_max_value")
|
||||
)
|
||||
raise ProxyException(
|
||||
|
|
|
|||
|
|
@ -68,7 +68,7 @@ from litellm.proxy.openai_files_endpoints.common_utils import (
|
|||
validate_managed_files_requirement,
|
||||
validate_managed_id_requirement,
|
||||
)
|
||||
from litellm.proxy.utils import ProxyLogging, is_known_model
|
||||
from litellm.proxy.utils import ProxyLogging, handle_exception_on_proxy, is_known_model
|
||||
from litellm.repositories.table_repositories import ManagedFileRepository
|
||||
from litellm.router import Router
|
||||
from litellm.types.llms.openai import (
|
||||
|
|
@ -1569,18 +1569,4 @@ async def list_files(
|
|||
)
|
||||
verbose_proxy_logger.error("litellm.proxy.proxy_server.list_files(): Exception occured - %s", e)
|
||||
verbose_proxy_logger.debug(traceback.format_exc())
|
||||
if isinstance(e, HTTPException):
|
||||
raise ProxyException(
|
||||
message=getattr(e, "message", str(e.detail)),
|
||||
type=getattr(e, "type", "None"),
|
||||
param=getattr(e, "param", "None"),
|
||||
code=getattr(e, "status_code", status.HTTP_400_BAD_REQUEST),
|
||||
)
|
||||
else:
|
||||
error_msg: Final = f"{e}"
|
||||
raise ProxyException(
|
||||
message=getattr(e, "message", error_msg),
|
||||
type=getattr(e, "type", "None"),
|
||||
param=getattr(e, "param", "None"),
|
||||
code=getattr(e, "status_code", 500),
|
||||
)
|
||||
raise handle_exception_on_proxy(e)
|
||||
|
|
|
|||
|
|
@ -415,9 +415,14 @@ async def test_afile_list_pages_through_every_file_without_overlap():
|
|||
assert table.find_many_calls[1]["skip"] == 1
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"unknown_cursor",
|
||||
["unified-theirs", "unified-nowhere"],
|
||||
ids=["another-users-file", "no-such-file"],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_afile_list_rejects_an_after_cursor_outside_the_callers_files():
|
||||
from fastapi import HTTPException
|
||||
async def test_afile_list_rejects_an_after_cursor_outside_the_callers_files(unknown_cursor):
|
||||
from litellm.proxy._types import ProxyException
|
||||
|
||||
managed_files, table = _make_managed_files_over_rows(
|
||||
[
|
||||
|
|
@ -426,24 +431,35 @@ async def test_afile_list_rejects_an_after_cursor_outside_the_callers_files():
|
|||
]
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
with pytest.raises(ProxyException) as exc_info:
|
||||
await managed_files.afile_list(
|
||||
purpose=None,
|
||||
litellm_parent_otel_span=None,
|
||||
user_api_key_dict=_make_user_api_key_dict(),
|
||||
after="unified-theirs",
|
||||
after=unknown_cursor,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 400
|
||||
assert exc_info.value.code == "400"
|
||||
assert exc_info.value.type == "invalid_request_error"
|
||||
assert exc_info.value.param == "after"
|
||||
assert exc_info.value.message == f"Invalid 'after' cursor: no file found with id '{unknown_cursor}'."
|
||||
assert table.find_first_calls[0] == {
|
||||
"created_by": "test-user",
|
||||
"unified_file_id": "unified-theirs",
|
||||
"unified_file_id": unknown_cursor,
|
||||
}
|
||||
assert table.find_many_calls == []
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"limit, bound, expected_range",
|
||||
[
|
||||
(0, "below minimum", ">= 1"),
|
||||
(-1, "below minimum", ">= 1"),
|
||||
(10001, "above maximum", "<= 10000"),
|
||||
],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_afile_list_rejects_a_limit_above_the_openai_maximum():
|
||||
async def test_afile_list_rejects_a_limit_outside_the_openai_range(limit, bound, expected_range):
|
||||
from litellm.proxy._types import ProxyException
|
||||
|
||||
managed_files, table = _make_managed_files_over_rows([_make_managed_file_row("unified-mine")])
|
||||
|
|
@ -453,28 +469,33 @@ async def test_afile_list_rejects_a_limit_above_the_openai_maximum():
|
|||
purpose=None,
|
||||
litellm_parent_otel_span=None,
|
||||
user_api_key_dict=_make_user_api_key_dict(),
|
||||
limit=10001,
|
||||
limit=limit,
|
||||
)
|
||||
|
||||
assert exc_info.value.code == "400"
|
||||
assert exc_info.value.type == "invalid_request_error"
|
||||
assert exc_info.value.param == "limit"
|
||||
assert exc_info.value.message == (
|
||||
f"Invalid 'limit': integer {bound} value. Expected a value {expected_range}, but got {limit} instead."
|
||||
)
|
||||
assert table.find_many_calls == []
|
||||
|
||||
|
||||
@pytest.mark.parametrize("limit", [1, 10000])
|
||||
@pytest.mark.asyncio
|
||||
async def test_afile_list_returns_an_empty_page_for_a_zero_limit():
|
||||
async def test_afile_list_accepts_the_ends_of_the_openai_limit_range(limit):
|
||||
managed_files, table = _make_managed_files_over_rows([_make_managed_file_row("unified-mine")])
|
||||
|
||||
response = await managed_files.afile_list(
|
||||
purpose=None,
|
||||
litellm_parent_otel_span=None,
|
||||
user_api_key_dict=_make_user_api_key_dict(),
|
||||
limit=0,
|
||||
limit=limit,
|
||||
)
|
||||
|
||||
assert response["data"] == []
|
||||
assert [file.id for file in response["data"]] == ["unified-mine"]
|
||||
assert response["has_more"] is False
|
||||
assert table.find_many_calls == []
|
||||
assert table.find_many_calls[0]["take"] == limit + 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -2591,6 +2591,127 @@ def test_unscoped_list_files_forwards_limit_and_after_to_the_managed_file_store(
|
|||
proxy_logging_obj.post_call_failure_hook.assert_not_called()
|
||||
|
||||
|
||||
def _setup_unscoped_list_files_route(mocker, monkeypatch, llm_router: Router, afile_list):
|
||||
"""Wire GET /v1/files to the managed file store, with afile_list as the store."""
|
||||
import litellm.proxy.proxy_server as ps
|
||||
from litellm.llms.base_llm.files.transformation import BaseFileEndpoints
|
||||
from litellm.proxy._types import LitellmUserRoles
|
||||
|
||||
proxy_logging_obj = setup_proxy_logging_object(monkeypatch, llm_router)
|
||||
managed_files = mocker.MagicMock(spec=BaseFileEndpoints)
|
||||
managed_files.afile_list = mocker.AsyncMock(side_effect=afile_list)
|
||||
proxy_logging_obj.proxy_hook_mapping["managed_files"] = managed_files
|
||||
proxy_logging_obj.update_request_status = mocker.AsyncMock()
|
||||
proxy_logging_obj.post_call_success_hook = mocker.AsyncMock(return_value=None)
|
||||
proxy_logging_obj.post_call_failure_hook = mocker.AsyncMock()
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.master_key", None)
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", None)
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", llm_router)
|
||||
mocker.patch.object(litellm, "afile_list", new=mocker.AsyncMock())
|
||||
|
||||
app.dependency_overrides[ps.user_api_key_auth] = lambda: UserAPIKeyAuth(
|
||||
api_key="test-key",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
user_id="test-user",
|
||||
)
|
||||
return managed_files
|
||||
|
||||
|
||||
def _get_unscoped_list_files(query: str):
|
||||
try:
|
||||
return client.get(f"/v1/files{query}", headers={"Authorization": "Bearer test-key"})
|
||||
finally:
|
||||
import litellm.proxy.proxy_server as ps
|
||||
|
||||
app.dependency_overrides.pop(ps.user_api_key_auth, None)
|
||||
|
||||
|
||||
async def _validating_afile_list(**kwargs):
|
||||
"""Stand in for the managed file store, applying the real limit validation."""
|
||||
from litellm.proxy.openai_files_endpoints.common_utils import validate_file_list_limit
|
||||
|
||||
validate_file_list_limit(kwargs.get("limit"))
|
||||
return {
|
||||
"object": "list",
|
||||
"data": [],
|
||||
"first_id": None,
|
||||
"last_id": None,
|
||||
"has_more": False,
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"limit, bound, expected_range",
|
||||
[
|
||||
(0, "below minimum", ">= 1"),
|
||||
(-1, "below minimum", ">= 1"),
|
||||
(10001, "above maximum", "<= 10000"),
|
||||
],
|
||||
)
|
||||
def test_unscoped_list_files_returns_400_for_a_limit_outside_the_openai_range(
|
||||
mocker: MockerFixture, monkeypatch, llm_router: Router, limit, bound, expected_range
|
||||
):
|
||||
"""An out-of-range limit is the caller's mistake, so it must not read as a 500 the SDK retries."""
|
||||
_setup_unscoped_list_files_route(mocker, monkeypatch, llm_router, _validating_afile_list)
|
||||
|
||||
response = _get_unscoped_list_files(f"?limit={limit}")
|
||||
|
||||
assert response.status_code == 400, response.text
|
||||
assert response.json() == {
|
||||
"error": {
|
||||
"message": (
|
||||
f"Invalid 'limit': integer {bound} value. "
|
||||
f"Expected a value {expected_range}, but got {limit} instead."
|
||||
),
|
||||
"type": "invalid_request_error",
|
||||
"param": "limit",
|
||||
"code": "400",
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.parametrize("limit", [1, 10000])
|
||||
def test_unscoped_list_files_accepts_the_ends_of_the_openai_limit_range(
|
||||
mocker: MockerFixture, monkeypatch, llm_router: Router, limit
|
||||
):
|
||||
managed_files = _setup_unscoped_list_files_route(mocker, monkeypatch, llm_router, _validating_afile_list)
|
||||
|
||||
response = _get_unscoped_list_files(f"?limit={limit}")
|
||||
|
||||
assert response.status_code == 200, response.text
|
||||
assert response.json()["data"] == []
|
||||
assert managed_files.afile_list.await_args.kwargs["limit"] == limit
|
||||
|
||||
|
||||
def test_unscoped_list_files_returns_400_for_an_unknown_after_cursor(
|
||||
mocker: MockerFixture, monkeypatch, llm_router: Router
|
||||
):
|
||||
from litellm.proxy._types import ProxyException
|
||||
|
||||
async def _unknown_cursor(**kwargs):
|
||||
raise ProxyException(
|
||||
message=f"Invalid 'after' cursor: no file found with id '{kwargs['after']}'.",
|
||||
type="invalid_request_error",
|
||||
param="after",
|
||||
code=400,
|
||||
openai_code="invalid_value",
|
||||
)
|
||||
|
||||
_setup_unscoped_list_files_route(mocker, monkeypatch, llm_router, _unknown_cursor)
|
||||
|
||||
response = _get_unscoped_list_files("?after=file-does-not-exist-xyz")
|
||||
|
||||
assert response.status_code == 400, response.text
|
||||
assert response.json() == {
|
||||
"error": {
|
||||
"message": "Invalid 'after' cursor: no file found with id 'file-does-not-exist-xyz'.",
|
||||
"type": "invalid_request_error",
|
||||
"param": "after",
|
||||
"code": "400",
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
def test_list_files_restricted_team_does_not_leak_global_openai_credentials(
|
||||
mocker: MockerFixture, monkeypatch
|
||||
):
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue