diff --git a/llama_stack/providers/inline/files/localfs/files.py b/llama_stack/providers/inline/files/localfs/files.py index 1e9dca3b5..4f6d571a4 100644 --- a/llama_stack/providers/inline/files/localfs/files.py +++ b/llama_stack/providers/inline/files/localfs/files.py @@ -11,6 +11,7 @@ from typing import Annotated from fastapi import File, Form, Response, UploadFile +from llama_stack.apis.common.errors import ResourceNotFoundError from llama_stack.apis.common.responses import Order from llama_stack.apis.files import ( Files, @@ -20,12 +21,15 @@ from llama_stack.apis.files import ( OpenAIFilePurpose, ) from llama_stack.core.datatypes import AccessRule +from llama_stack.log import get_logger from llama_stack.providers.utils.sqlstore.api import ColumnDefinition, ColumnType from llama_stack.providers.utils.sqlstore.authorized_sqlstore import AuthorizedSqlStore from llama_stack.providers.utils.sqlstore.sqlstore import sqlstore_impl from .config import LocalfsFilesImplConfig +logger = get_logger(name=__name__, category="files") + class LocalfsFilesImpl(Files): def __init__(self, config: LocalfsFilesImplConfig, policy: list[AccessRule]) -> None: @@ -65,6 +69,18 @@ class LocalfsFilesImpl(Files): """Get the filesystem path for a file ID.""" return Path(self.config.storage_dir) / file_id + async def _lookup_file_id(self, file_id: str) -> tuple[OpenAIFileObject, Path]: + """Look up a OpenAIFileObject and filesystem path from its ID.""" + if not self.sql_store: + raise RuntimeError("Files provider not initialized") + + row = await self.sql_store.fetch_one("openai_files", policy=self.policy, where={"id": file_id}) + if not row: + raise ResourceNotFoundError(file_id, "File", "client.files.list()") + + file_path = Path(row.pop("file_path")) + return OpenAIFileObject(**row), file_path + # OpenAI Files API Implementation async def openai_upload_file( self, @@ -157,37 +173,19 @@ class LocalfsFilesImpl(Files): async def openai_retrieve_file(self, file_id: str) -> OpenAIFileObject: """Returns information about a specific file.""" - if not self.sql_store: - raise RuntimeError("Files provider not initialized") + file_obj, _ = await self._lookup_file_id(file_id) - row = await self.sql_store.fetch_one("openai_files", policy=self.policy, where={"id": file_id}) - if not row: - raise ValueError(f"File with id {file_id} not found") - - return OpenAIFileObject( - id=row["id"], - filename=row["filename"], - purpose=OpenAIFilePurpose(row["purpose"]), - bytes=row["bytes"], - created_at=row["created_at"], - expires_at=row["expires_at"], - ) + return file_obj async def openai_delete_file(self, file_id: str) -> OpenAIFileDeleteResponse: """Delete a file.""" - if not self.sql_store: - raise RuntimeError("Files provider not initialized") - - row = await self.sql_store.fetch_one("openai_files", policy=self.policy, where={"id": file_id}) - if not row: - raise ValueError(f"File with id {file_id} not found") - # Delete physical file - file_path = Path(row["file_path"]) + _, file_path = await self._lookup_file_id(file_id) if file_path.exists(): file_path.unlink() # Delete metadata from database + assert self.sql_store is not None, "Files provider not initialized" await self.sql_store.delete("openai_files", where={"id": file_id}) return OpenAIFileDeleteResponse( @@ -197,25 +195,17 @@ class LocalfsFilesImpl(Files): async def openai_retrieve_file_content(self, file_id: str) -> Response: """Returns the contents of the specified file.""" - if not self.sql_store: - raise RuntimeError("Files provider not initialized") - - # Get file metadata - row = await self.sql_store.fetch_one("openai_files", policy=self.policy, where={"id": file_id}) - if not row: - raise ValueError(f"File with id {file_id} not found") - # Read file content - file_path = Path(row["file_path"]) - if not file_path.exists(): - raise ValueError(f"File content not found on disk: {file_path}") + file_obj, file_path = await self._lookup_file_id(file_id) - with open(file_path, "rb") as f: - content = f.read() + if not file_path.exists(): + logger.warning(f"File '{file_id}'s underlying '{file_path}' is missing, deleting metadata.") + await self.openai_delete_file(file_id) + raise ResourceNotFoundError(file_id, "File", "client.files.list()") # Return as binary response with appropriate content type return Response( - content=content, + content=file_path.read_bytes(), media_type="application/octet-stream", - headers={"Content-Disposition": f'attachment; filename="{row["filename"]}"'}, + headers={"Content-Disposition": f'attachment; filename="{file_obj.filename}"'}, ) diff --git a/tests/integration/files/test_files.py b/tests/integration/files/test_files.py index b17c7db83..67351d4f7 100644 --- a/tests/integration/files/test_files.py +++ b/tests/integration/files/test_files.py @@ -8,20 +8,27 @@ from io import BytesIO from unittest.mock import patch import pytest -from openai import OpenAI from llama_stack.core.datatypes import User -from llama_stack.core.library_client import LlamaStackAsLibraryClient -def test_openai_client_basic_operations(compat_client, client_with_models): +# a fixture to skip all these tests if a files provider is not available +@pytest.fixture(autouse=True) +def skip_if_no_files_provider(llama_stack_client): + if not [provider for provider in llama_stack_client.providers.list() if provider.api == "files"]: + pytest.skip("No files providers found") + + +def test_openai_client_basic_operations(openai_client): """Test basic file operations through OpenAI client.""" - if isinstance(client_with_models, LlamaStackAsLibraryClient) and isinstance(compat_client, OpenAI): - pytest.skip("OpenAI files are not supported when testing with LlamaStackAsLibraryClient") - client = compat_client + from openai import NotFoundError + + client = openai_client test_content = b"files test content" + uploaded_file = None + try: # Upload file using OpenAI client with BytesIO(test_content) as file_buffer: @@ -31,6 +38,7 @@ def test_openai_client_basic_operations(compat_client, client_with_models): # Verify basic response structure assert uploaded_file.id.startswith("file-") assert hasattr(uploaded_file, "filename") + assert uploaded_file.filename == "openai_test.txt" # List files files_list = client.files.list() @@ -43,37 +51,41 @@ def test_openai_client_basic_operations(compat_client, client_with_models): # Retrieve file content - OpenAI client returns httpx Response object content_response = client.files.content(uploaded_file.id) - # The response is an httpx Response object with .content attribute containing bytes - if isinstance(content_response, str): - # Llama Stack Client returns a str - # TODO: fix Llama Stack Client - content = bytes(content_response, "utf-8") - else: - content = content_response.content - assert content == test_content + assert content_response.content == test_content # Delete file delete_response = client.files.delete(uploaded_file.id) assert delete_response.deleted is True - except Exception as e: - # Cleanup in case of failure - try: + # Retrieve file should fail + with pytest.raises(NotFoundError, match="not found"): + client.files.retrieve(uploaded_file.id) + + # File should not be found in listing + files_list = client.files.list() + file_ids = [f.id for f in files_list.data] + assert uploaded_file.id not in file_ids + + # Double delete should fail + with pytest.raises(NotFoundError, match="not found"): client.files.delete(uploaded_file.id) - except Exception: - pass - raise e + + finally: + # Cleanup in case of failure + if uploaded_file is not None: + try: + client.files.delete(uploaded_file.id) + except NotFoundError: + pass # ignore 404 +@pytest.mark.xfail(message="User isolation broken for current providers, must be fixed.") @patch("llama_stack.providers.utils.sqlstore.authorized_sqlstore.get_authenticated_user") -def test_files_authentication_isolation(mock_get_authenticated_user, compat_client, client_with_models): +def test_files_authentication_isolation(mock_get_authenticated_user, llama_stack_client): """Test that users can only access their own files.""" - if isinstance(client_with_models, LlamaStackAsLibraryClient) and isinstance(compat_client, OpenAI): - pytest.skip("OpenAI files are not supported when testing with LlamaStackAsLibraryClient") - if not isinstance(client_with_models, LlamaStackAsLibraryClient): - pytest.skip("Authentication tests require LlamaStackAsLibraryClient (library mode)") + from llama_stack_client import NotFoundError - client = compat_client + client = llama_stack_client # Create two test users user1 = User("user1", {"roles": ["user"], "teams": ["team-a"]}) @@ -117,7 +129,7 @@ def test_files_authentication_isolation(mock_get_authenticated_user, compat_clie # User 1 cannot retrieve user2's file mock_get_authenticated_user.return_value = user1 - with pytest.raises(ValueError, match="not found"): + with pytest.raises(NotFoundError, match="not found"): client.files.retrieve(user2_file.id) # User 1 can access their file content @@ -131,7 +143,7 @@ def test_files_authentication_isolation(mock_get_authenticated_user, compat_clie # User 1 cannot access user2's file content mock_get_authenticated_user.return_value = user1 - with pytest.raises(ValueError, match="not found"): + with pytest.raises(NotFoundError, match="not found"): client.files.content(user2_file.id) # User 1 can delete their own file @@ -141,7 +153,7 @@ def test_files_authentication_isolation(mock_get_authenticated_user, compat_clie # User 1 cannot delete user2's file mock_get_authenticated_user.return_value = user1 - with pytest.raises(ValueError, match="not found"): + with pytest.raises(NotFoundError, match="not found"): client.files.delete(user2_file.id) # User 2 can still access their file after user1's file is deleted @@ -169,14 +181,9 @@ def test_files_authentication_isolation(mock_get_authenticated_user, compat_clie @patch("llama_stack.providers.utils.sqlstore.authorized_sqlstore.get_authenticated_user") -def test_files_authentication_shared_attributes(mock_get_authenticated_user, compat_client, client_with_models): +def test_files_authentication_shared_attributes(mock_get_authenticated_user, llama_stack_client): """Test access control with users having identical attributes.""" - if isinstance(client_with_models, LlamaStackAsLibraryClient) and isinstance(compat_client, OpenAI): - pytest.skip("OpenAI files are not supported when testing with LlamaStackAsLibraryClient") - if not isinstance(client_with_models, LlamaStackAsLibraryClient): - pytest.skip("Authentication tests require LlamaStackAsLibraryClient (library mode)") - - client = compat_client + client = llama_stack_client # Create users with identical attributes (required for default policy) user_a = User("user-a", {"roles": ["user"], "teams": ["shared-team"]}) @@ -231,14 +238,8 @@ def test_files_authentication_shared_attributes(mock_get_authenticated_user, com @patch("llama_stack.providers.utils.sqlstore.authorized_sqlstore.get_authenticated_user") -def test_files_authentication_anonymous_access(mock_get_authenticated_user, compat_client, client_with_models): - """Test anonymous user behavior when no authentication is present.""" - if isinstance(client_with_models, LlamaStackAsLibraryClient) and isinstance(compat_client, OpenAI): - pytest.skip("OpenAI files are not supported when testing with LlamaStackAsLibraryClient") - if not isinstance(client_with_models, LlamaStackAsLibraryClient): - pytest.skip("Authentication tests require LlamaStackAsLibraryClient (library mode)") - - client = compat_client +def test_files_authentication_anonymous_access(mock_get_authenticated_user, llama_stack_client): + client = llama_stack_client # Simulate anonymous user (no authentication) mock_get_authenticated_user.return_value = None diff --git a/tests/integration/fixtures/common.py b/tests/integration/fixtures/common.py index 0b7132d71..9cf56f6f5 100644 --- a/tests/integration/fixtures/common.py +++ b/tests/integration/fixtures/common.py @@ -263,8 +263,21 @@ def instantiate_llama_stack_client(session): @pytest.fixture(scope="session") -def openai_client(client_with_models): - base_url = f"{client_with_models.base_url}/v1/openai/v1" +def require_server(llama_stack_client): + """ + Skip test if no server is running. + + We use the llama_stack_client to tell if a server was started or not. + + We use this with openai_client because it relies on a running server. + """ + if isinstance(llama_stack_client, LlamaStackAsLibraryClient): + pytest.skip("No server running") + + +@pytest.fixture(scope="session") +def openai_client(llama_stack_client, require_server): + base_url = f"{llama_stack_client.base_url}/v1/openai/v1" return OpenAI(base_url=base_url, api_key="fake") diff --git a/tests/unit/files/test_files.py b/tests/unit/files/test_files.py index 04f33e97d..e14e033b9 100644 --- a/tests/unit/files/test_files.py +++ b/tests/unit/files/test_files.py @@ -7,6 +7,7 @@ import pytest +from llama_stack.apis.common.errors import ResourceNotFoundError from llama_stack.apis.common.responses import Order from llama_stack.apis.files import OpenAIFilePurpose from llama_stack.core.access_control.access_control import default_policy @@ -190,7 +191,7 @@ class TestOpenAIFilesAPI: async def test_retrieve_file_not_found(self, files_provider): """Test retrieving a non-existent file.""" - with pytest.raises(ValueError, match="File with id file-nonexistent not found"): + with pytest.raises(ResourceNotFoundError, match="not found"): await files_provider.openai_retrieve_file("file-nonexistent") async def test_retrieve_file_content_success(self, files_provider, sample_text_file): @@ -208,7 +209,7 @@ class TestOpenAIFilesAPI: async def test_retrieve_file_content_not_found(self, files_provider): """Test retrieving content of a non-existent file.""" - with pytest.raises(ValueError, match="File with id file-nonexistent not found"): + with pytest.raises(ResourceNotFoundError, match="not found"): await files_provider.openai_retrieve_file_content("file-nonexistent") async def test_delete_file_success(self, files_provider, sample_text_file): @@ -229,12 +230,12 @@ class TestOpenAIFilesAPI: assert delete_response.deleted is True # Verify file no longer exists - with pytest.raises(ValueError, match=f"File with id {uploaded_file.id} not found"): + with pytest.raises(ResourceNotFoundError, match="not found"): await files_provider.openai_retrieve_file(uploaded_file.id) async def test_delete_file_not_found(self, files_provider): """Test deleting a non-existent file.""" - with pytest.raises(ValueError, match="File with id file-nonexistent not found"): + with pytest.raises(ResourceNotFoundError, match="not found"): await files_provider.openai_delete_file("file-nonexistent") async def test_file_persistence_across_operations(self, files_provider, sample_text_file):