Add support for delete and GET via file_id for gemini - #20329
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile OverviewGreptile SummaryThis PR adds support for deleting files using just the file ID (e.g.,
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| litellm/llms/gemini/files/transformation.py | Added support for bare file_id in delete, but introduced bug where files/ prefix gets duplicated if already present |
Sequence Diagram
sequenceDiagram
participant User
participant LiteLLM
participant GoogleAIStudio as Google AI Studio API
Note over User,GoogleAIStudio: Delete File Flow
User->>LiteLLM: delete_file(file_id)
Note over LiteLLM: file_id can be:<br/>"abc123" OR<br/>"files/abc123" OR<br/>"https://.../files/abc123"
LiteLLM->>LiteLLM: transform_delete_file_request()
alt file_id starts with "http"
LiteLLM->>LiteLLM: Extract path from full URI<br/>file_name = file_id.split("/v1beta/")[-1]
else file_id is plain or prefixed
LiteLLM->>LiteLLM: Add "files/" prefix<br/>file_name = f"files/{file_id}"
Note over LiteLLM: ⚠️ BUG: If file_id already has "files/"<br/>prefix, this creates "files/files/..."
end
LiteLLM->>GoogleAIStudio: DELETE {api_base}/v1beta/{file_name}
GoogleAIStudio-->>LiteLLM: 200 OK {}
LiteLLM->>LiteLLM: transform_delete_file_response()
LiteLLM-->>User: FileDeleted(id, deleted=True)
| file_name = file_id.split("/v1beta/")[-1] | ||
| else: | ||
| file_name = file_id | ||
| file_name = f"files/{file_id}" |
There was a problem hiding this comment.
this creates duplicate files/ prefix when file_id is already files/abc123, resulting in files/files/abc123
| file_name = f"files/{file_id}" | |
| file_name = file_id if file_id.startswith("files/") else f"files/{file_id}" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: litellm/llms/gemini/files/transformation.py
Line: 302:302
Comment:
this creates duplicate `files/` prefix when `file_id` is already `files/abc123`, resulting in `files/files/abc123`
```suggestion
file_name = file_id if file_id.startswith("files/") else f"files/{file_id}"
```
How can I resolve this? If you propose a fix, please make it concise.| ) | ||
|
|
||
| # Verify URL is constructed correctly | ||
| assert "generativelanguage.googleapis.com" in url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, the fix is to avoid treating the URL as an opaque string when validating its host or structure. Instead, the URL should be parsed using a standard URL parser (for example, urllib.parse.urlparse in Python), and assertions or checks should be made against structured components like scheme, netloc, and hostname. This prevents accidentally accepting URLs that merely contain an allowed host as a substring in an unsafe location.
For this specific test, we want to keep the existing intent—verifying that the constructed URL targets the Gemini API endpoint—but replace the substring assertion with a structured check on the parsed hostname. We will import urlparse from urllib.parse at the top of the file, and in test_transform_retrieve_file_request_with_file_name_only, we will replace assert "generativelanguage.googleapis.com" in url with code that parses url and asserts that parsed.hostname == "generativelanguage.googleapis.com". This keeps behavior equivalent (or stricter) while eliminating the substring-host pattern that CodeQL flags. No other test logic or functional behavior needs to change.
Concretely:
- Add
from urllib.parse import urlparsealongside the existing imports intests/test_litellm/llms/gemini/files/test_gemini_files_transformation.py. - In
test_transform_retrieve_file_request_with_file_name_only, replace the line at/around 63 with parsingurl = ...and asserting onhostname. For example:parsed_url = urlparse(url)assert parsed_url.hostname == "generativelanguage.googleapis.com"
This keeps the rest of the assertions (forfile_idand API key presence, and empty params) unchanged.
| @@ -7,6 +7,7 @@ | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| import httpx | ||
| from urllib.parse import urlparse | ||
|
|
||
| from litellm.llms.gemini.files.transformation import GoogleAIStudioFilesHandler | ||
| from litellm.types.llms.openai import OpenAIFileObject | ||
| @@ -60,7 +61,8 @@ | ||
| ) | ||
|
|
||
| # Verify URL is constructed correctly | ||
| assert "generativelanguage.googleapis.com" in url | ||
| parsed_url = urlparse(url) | ||
| assert parsed_url.hostname == "generativelanguage.googleapis.com" | ||
| assert file_id in url | ||
| assert "key=test-api-key" in url | ||
|
|
|
|
||
| # Verify URL extraction | ||
| assert "files/test123" in url | ||
| assert "generativelanguage.googleapis.com" in url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, instead of checking that a trusted host appears as a substring of a URL, you should parse the URL and verify the hostname (and optionally scheme and path) explicitly. In Python, urllib.parse.urlparse provides a robust way to extract the hostname from a URL string and compare it against an expected value.
For this concrete test file, we only need to adjust the assertions that currently perform substring checks on the full URL. We can keep the behavior under test exactly the same (i.e., we don’t change GoogleAIStudioFilesHandler), but assert on parsed components instead of substring membership:
- In
test_get_complete_url, replaceassert api_base in urlwith a parse-based check that the URL’s scheme and hostname match the expected base, and that the path contains the expected upload endpoint. - In
test_transform_delete_file_request_with_full_uri, replaceassert "generativelanguage.googleapis.com" in urlwith an assertion thaturlparse(url).hostname == "generativelanguage.googleapis.com". - In
test_transform_delete_file_request_with_file_name_only, replaceassert "generativelanguage.googleapis.com" in urlwith a similar hostname assertion, and replaceassert file_id in urlwith a more specific path-based assertion.
To implement this, we need to:
- Import
urlparse(orurllib.parse) at the top oftests/test_litellm/llms/gemini/files/test_gemini_files_transformation.py. - Update the relevant assertions to use
urlparse(url)and comparehostnameandpath.
No changes to production code are required, and there is no change in the intended behavior being tested; we are only tightening how the tests verify the domain and path.
| @@ -5,6 +5,7 @@ | ||
| import os | ||
| import pytest | ||
| from unittest.mock import Mock, patch | ||
| from urllib.parse import urlparse | ||
|
|
||
| import httpx | ||
|
|
||
| @@ -253,8 +254,11 @@ | ||
| ) | ||
|
|
||
| # Verify URL structure | ||
| assert api_base in url | ||
| assert "upload/v1beta/files" in url | ||
| parsed = urlparse(url) | ||
| parsed_api_base = urlparse(api_base) | ||
| assert parsed.scheme == parsed_api_base.scheme | ||
| assert parsed.hostname == parsed_api_base.hostname | ||
| assert "upload/v1beta/files" in parsed.path | ||
| assert f"key={api_key}" in url | ||
|
|
||
| def test_transform_delete_file_request_with_full_uri(self): | ||
| @@ -272,8 +276,9 @@ | ||
| ) | ||
|
|
||
| # Verify URL extraction | ||
| assert "files/test123" in url | ||
| assert "generativelanguage.googleapis.com" in url | ||
| parsed = urlparse(url) | ||
| assert parsed.hostname == "generativelanguage.googleapis.com" | ||
| assert parsed.path.endswith("/files/test123") | ||
|
|
||
| # Params should be empty (API key goes in header via validate_environment) | ||
| assert params == {} | ||
| @@ -293,6 +298,7 @@ | ||
| ) | ||
|
|
||
| # Verify URL construction | ||
| assert file_id in url | ||
| assert "generativelanguage.googleapis.com" in url | ||
| parsed = urlparse(url) | ||
| assert parsed.hostname == "generativelanguage.googleapis.com" | ||
| assert parsed.path.endswith("/files/test123") | ||
| assert params == {} |
|
|
||
| # Verify URL construction | ||
| assert file_id in url | ||
| assert "generativelanguage.googleapis.com" in url |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, the problem with substring-based checks is that they do not ensure the host component of a URL is what you expect; the string can appear anywhere. The robust way to validate or assert URL structure is to parse the URL using a URL parser (such as urllib.parse.urlparse) and then inspect the specific components—scheme, netloc (host and port), path, and query parameters.
For this test, instead of asserting that "generativelanguage.googleapis.com" in url, we should parse url and assert that its netloc (or hostname) is exactly generativelanguage.googleapis.com. Similarly, for other structure-related checks (like assert file_id in url), we can tighten the test to assert on parsed.path.endswith(file_id) or similar, which still verifies the same behavior but with structured checks. Concretely:
- Add an import of
urllib.parse.urlparsenear the top oftests/test_litellm/llms/gemini/files/test_gemini_files_transformation.py. - In
test_transform_delete_file_request_with_full_uri, parseurland replace the existing host substring assertion with an assertion onparsed_url.netloc. - In
test_transform_delete_file_request_with_file_name_only, parseurland:- Replace
assert file_id in urlwith an assertion based onparsed_url.path(e.g.,endswith(file_id)). - Replace
assert "generativelanguage.googleapis.com" in urlwith an assertion onparsed_url.netloc.
- Replace
This preserves the intended checks (correct host and inclusion of file id in the URL) but removes reliance on unsafe substring matches.
| @@ -7,6 +7,7 @@ | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| import httpx | ||
| from urllib.parse import urlparse | ||
|
|
||
| from litellm.llms.gemini.files.transformation import GoogleAIStudioFilesHandler | ||
| from litellm.types.llms.openai import OpenAIFileObject | ||
| @@ -272,8 +273,9 @@ | ||
| ) | ||
|
|
||
| # Verify URL extraction | ||
| assert "files/test123" in url | ||
| assert "generativelanguage.googleapis.com" in url | ||
| parsed_url = urlparse(url) | ||
| assert parsed_url.netloc == "generativelanguage.googleapis.com" | ||
| assert parsed_url.path.endswith("/v1beta/files/test123") | ||
|
|
||
| # Params should be empty (API key goes in header via validate_environment) | ||
| assert params == {} | ||
| @@ -293,6 +295,7 @@ | ||
| ) | ||
|
|
||
| # Verify URL construction | ||
| assert file_id in url | ||
| assert "generativelanguage.googleapis.com" in url | ||
| parsed_url = urlparse(url) | ||
| assert parsed_url.netloc == "generativelanguage.googleapis.com" | ||
| assert parsed_url.path.endswith(f"/{file_id}") | ||
| assert params == {} |
Add support for delete and GET via file_id for gemini
Relevant issues
Fixes #20054
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes