Fix get_files(directory) on the S3 and local storage drivers

fastapi_startkit/pyproject.toml

@@ -108,6 +108,7 @@ dev = [
     "langchain>=1.0.0",
     "langchain-core>=1.0.0",
     "pyright>=1.1.411",
+    "boto3>=1.35.0",
 ]
 
 

fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py

@@ -57,7 +57,7 @@ def assert_count(self, count: int, directory: str = ""):
         assert len(files) == count, (
             f"Storage::fake({self._disk_name!r}): "
             f"expected {count} file(s) in [{directory or '/'}], "
-            f"found {len(files)}: {[f.name for f in files]}"
+            f"found {len(files)}: {[f.name() for f in files]}"
         )
         return self
 

fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py

@@ -17,11 +17,15 @@ def set_options(self, options):
         self.options = options
         return self
 
-    def get_path(self, path):
+    def resolve_path(self, path):
+        """Resolve ``path`` against the disk root without touching the filesystem."""
         root = self.options.get("root") or self.options.get("path")
         if not os.path.isabs(root):
             root = os.path.join(str(self.application.base_path), root)
-        file_path = os.path.join(root, path)
+        return os.path.join(root, path)
+
+    def get_path(self, path):
+        file_path = self.resolve_path(path)
         self.make_file_path_if_not_exists(file_path)
         return file_path
 
@@ -115,13 +119,30 @@ def make_file_path_if_not_exists(self, file_path):
         return False
 
     def get_files(self, directory=""):
-        file_path = self.get_path(directory)
+        """List the files directly under ``directory``, non-recursively.
+
+        Sub-directories are skipped and a missing directory yields an empty
+        list. An empty or omitted ``directory`` lists the root of the disk.
+
+        Listing never reads a file body — content would have to be decoded as
+        text, so a single binary file would blow up the whole listing. Fetch
+        content on demand with ``get(join(directory, file.name()))``.
+        """
+        directory = (directory or "").strip("/")
+        # resolve_path, not get_path: listing is read-only and must not create
+        # the directory it was asked about.
+        directory_path = self.resolve_path(directory)
+
+        if not os.path.isdir(directory_path):
+            return []
+
         files = []
-        for f in os.listdir(file_path):
-            if not isfile(join(file_path, f)):
+        # Sorted so listings match the lexicographic order S3 returns.
+        for name in sorted(os.listdir(directory_path)):
+            if not isfile(join(directory_path, name)):
                 continue
 
-            files.append(File(self.get(f), f))
+            files.append(File(None, name))
 
         return files
 

fastapi_startkit/src/fastapi_startkit/storage/drivers/s3.py

@@ -148,20 +148,30 @@ def make_file_path_if_not_exists(self, file_path):
         return False
 
     def get_files(self, directory=None):
-        bucket = self.get_resource().Bucket(self.get_bucket())
+        """List the files directly under ``directory``, non-recursively.
 
-        if directory:
-            objects = bucket.objects.all().filter(Prefix=directory)
-        else:
-            objects = bucket.objects.all()
+        Keys nested in a deeper prefix and the placeholder object for the
+        directory itself are excluded. An empty or omitted ``directory``
+        lists the root of the bucket.
+        """
+        prefix = self.normalize_directory(directory)
+        objects = self.get_resource().Bucket(self.get_bucket()).objects.filter(Prefix=prefix)
 
         files = []
-        for my_bucket_object in objects.all():
-            if "/" not in my_bucket_object.key:
-                files.append(File(my_bucket_object, my_bucket_object.key))
+        for summary in objects:
+            name = summary.key[len(prefix) :]
+            if not name or "/" in name:
+                continue
+
+            files.append(File(summary, name))
 
         return files
 
+    def normalize_directory(self, directory):
+        """Turn a directory name into a key prefix ("backups/", or "" for the root)."""
+        directory = (directory or "").strip("/")
+        return f"{directory}/" if directory else ""
+
     def download(self, file_path, name=None, force=False):
         url = self.get_client().generate_presigned_url(
             "get_object",

fastapi_startkit/src/fastapi_startkit/storage/file.py

@@ -4,6 +4,22 @@
 
 
 class File:
+    """A stored file.
+
+    ``content`` means different things depending on where the File came from.
+    Construct one yourself (to ``put``/``store`` it) and it holds the body. Get
+    one back from a driver's ``get_files`` listing and it holds no body at all,
+    because listing every body is expensive on S3 and undecodable for binary
+    files on disk — what it holds there is driver-specific:
+
+    - local/fake: ``None``. Read the body with ``driver.get(join(directory, file.name()))``.
+    - s3: the boto3 ``ObjectSummary``, so ``file.stream().key`` recovers the
+      full key that ``name()`` strips the prefix off of.
+
+    ``name()`` is the one thing that means the same on every driver: the bare
+    filename, with no directory prefix.
+    """
+
     def __init__(self, content, filename=None):
         self.content = content
         self.filename = filename

fastapi_startkit/src/fastapi_startkit/storage/storage.py

@@ -102,6 +102,9 @@ def delete(self, *args, **kwargs):
     def store(self, *args, **kwargs):
         return self.disk().store(*args, **kwargs)
 
+    def get_files(self, *args, **kwargs):
+        return self.disk().get_files(*args, **kwargs)
+
     def download(self, *args, **kwargs):
         return self.disk().download(*args, **kwargs)
 
@@ -177,6 +180,10 @@ def delete(cls, *args, **kwargs):
     def store(cls, *args, **kwargs):
         return cls.init().store(*args, **kwargs)
 
+    @classmethod
+    def get_files(cls, *args, **kwargs):
+        return cls.init().get_files(*args, **kwargs)
+
     @classmethod
     def download(cls, *args, **kwargs):
         return cls.init().download(*args, **kwargs)

fastapi_startkit/tests/storage/test_local_driver.py

@@ -1,6 +1,7 @@
 """Tests for the LocalDriver, FakeDriver, and FileStream (task #14)."""
 
 import os
+from pathlib import Path
 from unittest.mock import MagicMock
 
 import pytest
@@ -305,3 +306,163 @@ def test_filestream_extension_from_explicit_name(self, tmp_path):
         with open(f) as fh:
             stream = FileStream(fh, name="renamed.csv")
             assert stream.extension() == ".csv"
+
+
+# ---------------------------------------------------------------------------
+# get_files in a sub-directory (issue #218)
+# ---------------------------------------------------------------------------
+
+
+def _layout(root):
+    """The shared layout every driver's get_files is asserted against."""
+    (root / "audio").mkdir(parents=True, exist_ok=True)
+    (root / "audio" / "one.txt").write_text("one content")
+    (root / "audio" / "two.txt").write_text("two content")
+    (root / "audio" / "nested").mkdir(exist_ok=True)
+    (root / "audio" / "nested" / "deep.txt").write_text("deep content")
+    (root / "root.txt").write_text("root content")
+
+
+class TestLocalDriverGetFilesInDirectory:
+    @pytest.fixture
+    def driver(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+        d = LocalDriver(app)
+        d.set_options({"root": str(tmp_path / "storage")})
+        _layout(tmp_path / "storage")
+        return d
+
+    def test_lists_files_directly_under_the_directory(self, driver):
+        assert [f.name() for f in driver.get_files("audio")] == ["one.txt", "two.txt"]
+
+    def test_listed_names_resolve_back_to_their_content(self, driver):
+        contents = {f.name(): driver.get(os.path.join("audio", f.name())) for f in driver.get_files("audio")}
+        assert contents == {"one.txt": "one content", "two.txt": "two content"}
+
+    def test_subdirectories_are_skipped(self, driver):
+        assert "nested" not in [f.name() for f in driver.get_files("audio")]
+
+    def test_listing_is_non_recursive(self, driver):
+        assert "deep.txt" not in [f.name() for f in driver.get_files("audio")]
+
+    def test_root_listing_excludes_directory_contents(self, driver):
+        assert [f.name() for f in driver.get_files()] == ["root.txt"]
+
+    def test_trailing_slash_is_tolerated(self, driver):
+        assert [f.name() for f in driver.get_files("audio/")] == [f.name() for f in driver.get_files("audio")]
+
+    def test_missing_directory_returns_empty_list(self, driver):
+        assert driver.get_files("nope") == []
+
+    def test_empty_directory_returns_empty_list(self, driver, tmp_path):
+        (tmp_path / "storage" / "empty").mkdir()
+        assert driver.get_files("empty") == []
+
+
+class TestFakeDriverGetFilesParity:
+    def test_matches_the_local_driver_for_the_same_layout(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+
+        local = LocalDriver(app)
+        local.set_options({"root": str(tmp_path / "storage")})
+        _layout(tmp_path / "storage")
+
+        with FakeDriver(app) as fake:
+            _layout(Path(fake._root))
+
+            for directory in ("audio", "audio/", "", "nope"):
+                names = [f.name() for f in fake.get_files(directory)]
+                assert names == [f.name() for f in local.get_files(directory)]
+
+                # Listings carry no body on either driver; the names must still
+                # resolve to identical content through each driver's own get().
+                assert [f.stream() for f in fake.get_files(directory)] == [None] * len(names)
+                assert [fake.get(os.path.join(directory, n)) for n in names] == [
+                    local.get(os.path.join(directory, n)) for n in names
+                ]
+
+    def test_assert_count_scopes_to_a_directory(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+
+        with FakeDriver(app) as fake:
+            _layout(Path(fake._root))
+            fake.assert_count(2, "audio")
+            fake.assert_count(1)
+            fake.assert_directory_empty("nope")
+
+
+class TestLocalDriverGetFilesDoesNotReadContent:
+    """A listing is a listing: it must not open file bodies (PR #220 review)."""
+
+    @pytest.fixture
+    def driver(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+        d = LocalDriver(app)
+        d.set_options({"root": str(tmp_path / "storage")})
+        return d
+
+    def test_binary_file_does_not_break_the_listing(self, driver, tmp_path):
+        audio = tmp_path / "storage" / "audio"
+        audio.mkdir(parents=True)
+        (audio / "clip.mp3").write_bytes(b"\xff\xfb\x90\x00\x00\x00\x00\x00")
+        (audio / "notes.txt").write_text("fine")
+
+        assert [f.name() for f in driver.get_files("audio")] == ["clip.mp3", "notes.txt"]
+
+    def test_binary_file_at_the_root_does_not_break_the_listing(self, driver, tmp_path):
+        storage = tmp_path / "storage"
+        storage.mkdir(parents=True)
+        (storage / "track.mp3").write_bytes(b"\xff\xfb\x90\x00")
+
+        assert [f.name() for f in driver.get_files()] == ["track.mp3"]
+
+    def test_content_is_fetched_on_demand_not_during_the_listing(self, driver, tmp_path):
+        audio = tmp_path / "storage" / "audio"
+        audio.mkdir(parents=True)
+        (audio / "one.txt").write_text("one content")
+
+        listed = driver.get_files("audio")[0]
+        assert listed.stream() is None
+        assert driver.get(os.path.join("audio", listed.name())) == "one content"
+
+
+class TestLocalDriverGetFilesDoesNotMutateDisk:
+    """Listing is read-only: it must never create directories (PR #220 review)."""
+
+    @pytest.fixture
+    def driver(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+        d = LocalDriver(app)
+        d.set_options({"root": str(tmp_path / "storage")})
+        (tmp_path / "storage").mkdir(parents=True)
+        return d
+
+    def test_missing_nested_directory_returns_empty_and_creates_nothing(self, driver, tmp_path):
+        assert driver.get_files("reports/2024") == []
+
+        assert not (tmp_path / "storage" / "reports").exists()
+        assert not (tmp_path / "storage" / "reports" / "2024").exists()
+
+    def test_missing_single_segment_directory_creates_nothing(self, driver, tmp_path):
+        assert driver.get_files("nope") == []
+        assert not (tmp_path / "storage" / "nope").exists()
+
+    def test_listing_a_disk_whose_root_is_absent_creates_nothing(self, tmp_path):
+        app = MagicMock()
+        app.base_path = str(tmp_path)
+        d = LocalDriver(app)
+        d.set_options({"root": str(tmp_path / "absent_root")})
+
+        assert d.get_files("anything") == []
+        assert not (tmp_path / "absent_root").exists()
+
+    def test_resolve_path_does_not_touch_the_filesystem(self, driver, tmp_path):
+        resolved = driver.resolve_path(os.path.join("reports", "2024", "q1.csv"))
+
+        assert resolved == str(tmp_path / "storage" / "reports" / "2024" / "q1.csv")
+        assert not (tmp_path / "storage" / "reports").exists()