From 1a71f58299cc2f209edb78bbbb189e1e9d6ecc68 Mon Sep 17 00:00:00 2001 From: Bedram Tamang Date: Sat, 12 Sep 2026 13:24:32 -0700 Subject: [PATCH 1/2] fix: make get_files(directory) work and agree across storage drivers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_files(directory) was broken on both real drivers, in different ways, and the two disagreed about what a listing even means — so it could not be used from disk-agnostic code. S3 filtered nested keys by testing the whole key with `"/" not in key` instead of the part below the prefix, so any non-empty Prefix always returned [] while root-level keys leaked through. The remainder below the prefix is now what gets tested, a trailing slash on the directory is tolerated, and the directory placeholder object is skipped. Local listed bare names from os.listdir but read them back with self.get(name), which resolves against the disk root rather than root/. get() swallows FileNotFoundError, so callers silently received File objects with None content. The read is now joined to the directory, a missing directory returns [] instead of raising, and entries are sorted to match the order S3 returns. All three drivers now share one documented contract: a non-recursive listing of the files directly under the given directory, no directory entries. FakeDriver delegates to LocalDriver so it cannot drift. Adds get_files to the StorageManager/Storage passthroughs, and covers the S3 path with real integration tests against a local MinIO (skipped when no endpoint is reachable). Fixes #218 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FyozUDQzC8en24yx1Je1ct --- fastapi_startkit/pyproject.toml | 1 + .../fastapi_startkit/storage/drivers/fake.py | 10 +- .../fastapi_startkit/storage/drivers/local.py | 19 +++- .../fastapi_startkit/storage/drivers/s3.py | 26 +++-- .../src/fastapi_startkit/storage/storage.py | 7 ++ .../tests/storage/test_local_driver.py | 82 ++++++++++++++ .../tests/storage/test_s3_driver.py | 80 ++++++++++++++ .../storage/test_s3_minio_integration.py | 101 ++++++++++++++++++ fastapi_startkit/uv.lock | 53 ++++++++- 9 files changed, 365 insertions(+), 14 deletions(-) create mode 100644 fastapi_startkit/tests/storage/test_s3_minio_integration.py diff --git a/fastapi_startkit/pyproject.toml b/fastapi_startkit/pyproject.toml index 6358cf7b..68ff1c11 100644 --- a/fastapi_startkit/pyproject.toml +++ b/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", ] diff --git a/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py b/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py index b8f48d28..a9577e7d 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py +++ b/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py @@ -24,6 +24,14 @@ def __exit__(self, *_): def cleanup(self): shutil.rmtree(self._root, ignore_errors=True) + def get_files(self, directory=""): + """List files directly under ``directory`` in the fake's temp root. + + Delegates to LocalDriver rather than reimplementing the listing so the + fake can never drift from the semantics it stands in for. + """ + return super().get_files(directory) + def assert_exists(self, paths, content=None): if isinstance(paths, str): paths = [paths] @@ -57,7 +65,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 diff --git a/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py b/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py index b932a0f0..468639b7 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py +++ b/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py @@ -115,13 +115,24 @@ 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. + """ + directory = (directory or "").strip("/") + directory_path = self.get_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(self.get(join(directory, name)), name)) return files diff --git a/fastapi_startkit/src/fastapi_startkit/storage/drivers/s3.py b/fastapi_startkit/src/fastapi_startkit/storage/drivers/s3.py index a5ca66fe..e84e846c 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/drivers/s3.py +++ b/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", diff --git a/fastapi_startkit/src/fastapi_startkit/storage/storage.py b/fastapi_startkit/src/fastapi_startkit/storage/storage.py index 9c6ce0a8..90e868a6 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/storage.py +++ b/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) diff --git a/fastapi_startkit/tests/storage/test_local_driver.py b/fastapi_startkit/tests/storage/test_local_driver.py index de2b9bcb..0619866c 100644 --- a/fastapi_startkit/tests/storage/test_local_driver.py +++ b/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,84 @@ 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_file_content_is_populated_not_none(self, driver): + contents = {f.name(): f.stream() 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"): + assert [f.name() for f in fake.get_files(directory)] == [f.name() for f in local.get_files(directory)] + assert [f.stream() for f in fake.get_files(directory)] == [ + f.stream() for f in local.get_files(directory) + ] + + 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") diff --git a/fastapi_startkit/tests/storage/test_s3_driver.py b/fastapi_startkit/tests/storage/test_s3_driver.py index 90c10ed4..c36c50fa 100644 --- a/fastapi_startkit/tests/storage/test_s3_driver.py +++ b/fastapi_startkit/tests/storage/test_s3_driver.py @@ -278,3 +278,83 @@ def test_get_connection_returns_same_session(self): c2 = d.get_connection() assert c1 is c2 mock_boto3.Session.assert_called_once() + + +# --------------------------------------------------------------------------- +# get_files — non-recursive listing under a prefix (issue #218) +# --------------------------------------------------------------------------- + + +def _summaries(*keys): + objects = [] + for key in keys: + summary = MagicMock() + summary.key = key + objects.append(summary) + return objects + + +@pytest.fixture +def bucket_with_keys(driver, mock_resource): + """Make the mocked bucket serve a fixed key list, filtered by Prefix.""" + keys = [ + "root.txt", + "backups/a.sql", + "backups/b.sql", + "backups/2024/deep.sql", + "audio/x/y.mp3", + ] + + def filter_(Prefix=""): + return _summaries(*[k for k in keys if k.startswith(Prefix)]) + + mock_resource.Bucket().objects.filter.side_effect = filter_ + return mock_resource + + +class TestS3DriverGetFiles: + def test_lists_files_directly_under_the_prefix(self, driver, bucket_with_keys): + assert [f.name() for f in driver.get_files("backups")] == ["a.sql", "b.sql"] + + def test_excludes_nested_keys(self, driver, bucket_with_keys): + assert driver.get_files("audio") == [] + + def test_root_level_keys_do_not_leak_into_a_prefixed_listing(self, driver, bucket_with_keys): + assert "root.txt" not in [f.name() for f in driver.get_files("backups")] + + def test_trailing_slash_is_tolerated(self, driver, bucket_with_keys): + assert [f.name() for f in driver.get_files("backups/")] == [f.name() for f in driver.get_files("backups")] + + def test_no_directory_lists_the_root_only(self, driver, bucket_with_keys): + assert [f.name() for f in driver.get_files()] == ["root.txt"] + + def test_empty_directory_is_treated_as_the_root(self, driver, bucket_with_keys): + assert [f.name() for f in driver.get_files("")] == ["root.txt"] + + def test_unknown_directory_returns_empty_list(self, driver, bucket_with_keys): + assert driver.get_files("nope") == [] + + def test_directory_placeholder_object_is_skipped(self, driver, mock_resource): + mock_resource.Bucket().objects.filter.return_value = _summaries("backups/", "backups/a.sql") + assert [f.name() for f in driver.get_files("backups")] == ["a.sql"] + + def test_file_content_is_the_object_summary(self, driver, bucket_with_keys): + file = driver.get_files("backups")[0] + assert file.stream().key == "backups/a.sql" + + +class TestS3DriverNormalizeDirectory: + @pytest.mark.parametrize( + "directory,expected", + [ + (None, ""), + ("", ""), + ("/", ""), + ("backups", "backups/"), + ("backups/", "backups/"), + ("/backups/", "backups/"), + ("a/b", "a/b/"), + ], + ) + def test_normalizes_to_a_key_prefix(self, driver, directory, expected): + assert driver.normalize_directory(directory) == expected diff --git a/fastapi_startkit/tests/storage/test_s3_minio_integration.py b/fastapi_startkit/tests/storage/test_s3_minio_integration.py new file mode 100644 index 00000000..77d34b70 --- /dev/null +++ b/fastapi_startkit/tests/storage/test_s3_minio_integration.py @@ -0,0 +1,101 @@ +"""Real S3 integration tests for get_files, run against a local MinIO (issue #218). + +Skipped unless a MinIO endpoint is reachable. Point the suite at a different +instance with S3_TEST_ENDPOINT / S3_TEST_KEY / S3_TEST_SECRET. +""" + +import os +import socket +from unittest.mock import MagicMock +from urllib.parse import urlparse + +import pytest + +from fastapi_startkit.storage.drivers.s3 import S3Driver + +ENDPOINT = os.getenv("S3_TEST_ENDPOINT", "http://127.0.0.1:9002") +KEY = os.getenv("S3_TEST_KEY", "minio") +SECRET = os.getenv("S3_TEST_SECRET", "minio123") +BUCKET = os.getenv("S3_TEST_BUCKET", "fsk-get-files-test") + +pytest.importorskip("boto3", reason="boto3 is required for the S3 integration tests") + + +def _endpoint_is_reachable(): + url = urlparse(ENDPOINT) + try: + with socket.create_connection((url.hostname, url.port or 80), timeout=1): + return True + except OSError: + return False + + +pytestmark = pytest.mark.skipif( + not _endpoint_is_reachable(), + reason=f"no S3-compatible endpoint at {ENDPOINT} — start MinIO to run these", +) + +LAYOUT = { + "root.txt": b"root content", + "backups/a.sql": b"a content", + "backups/b.sql": b"b content", + "backups/2024/deep.sql": b"deep content", + "audio/x/y.mp3": b"nested only", +} + + +@pytest.fixture(scope="module") +def driver(): + d = S3Driver(MagicMock()) + d.set_options( + { + "bucket": BUCKET, + "key": KEY, + "secret": SECRET, + "region": "us-east-1", + "endpoint": ENDPOINT, + "use_path_style_endpoint": True, + } + ) + + bucket = d.get_resource().Bucket(BUCKET) + if bucket.creation_date is None: + bucket.create() + bucket.objects.all().delete() + + for key, body in LAYOUT.items(): + bucket.put_object(Key=key, Body=body) + + yield d + + bucket.objects.all().delete() + + +class TestS3GetFilesAgainstMinio: + def test_lists_files_directly_under_the_prefix(self, driver): + assert [f.name() for f in driver.get_files("backups")] == ["a.sql", "b.sql"] + + def test_nested_keys_are_excluded(self, driver): + assert driver.get_files("audio") == [] + + def test_deeper_keys_do_not_leak_into_the_listing(self, driver): + assert "deep.sql" not in [f.name() for f in driver.get_files("backups")] + + def test_root_level_keys_do_not_leak_into_a_prefixed_listing(self, driver): + assert "root.txt" not in [f.name() for f in driver.get_files("backups")] + + def test_trailing_slash_is_tolerated(self, driver): + assert [f.name() for f in driver.get_files("backups/")] == [f.name() for f in driver.get_files("backups")] + + def test_no_directory_lists_the_root_only(self, driver): + assert [f.name() for f in driver.get_files()] == ["root.txt"] + + def test_nested_prefix_is_listable(self, driver): + assert [f.name() for f in driver.get_files("backups/2024")] == ["deep.sql"] + + def test_unknown_directory_returns_empty_list(self, driver): + assert driver.get_files("nope") == [] + + def test_listed_objects_resolve_back_to_their_content(self, driver): + for file in driver.get_files("backups"): + assert driver.get(f"backups/{file.name()}") == LAYOUT[f"backups/{file.name()}"].decode() diff --git a/fastapi_startkit/uv.lock b/fastapi_startkit/uv.lock index c47982b9..e7af7307 100644 --- a/fastapi_startkit/uv.lock +++ b/fastapi_startkit/uv.lock @@ -99,6 +99,34 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/3c/d7/8fb3044eaef08a310acfe23dae9a8e2e07d305edc29a53497e52bc76eca7/asyncpg-0.31.0-cp314-cp314t-win_amd64.whl", hash = "sha256:bd4107bb7cdd0e9e65fae66a62afd3a249663b844fa34d479f6d5b3bef9c04c3", size = 706062, upload-time = "2025-11-24T23:26:44.086Z" }, ] +[[package]] +name = "boto3" +version = "1.43.93" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "botocore" }, + { name = "jmespath" }, + { name = "s3transfer" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/c2/37/7a09d8320685b3b8c8a014e392e7595025d00965c46c5f410797905fab7a/boto3-1.43.93.tar.gz", hash = "sha256:196bfc8b4c9cd5505f9f7b963e30956db3a00fd47e20dd0ee3574a243c1fb212", size = 112752, upload-time = "2026-09-11T19:23:03.484Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/ea/68/f8f661b9e68daba4f775bfa1e750732ec52fc89b32d55f300aef530e1d62/boto3-1.43.93-py3-none-any.whl", hash = "sha256:3c948fe231490d446bf90bf3322d1452632107329d3683b37d88b7399bf481a0", size = 140022, upload-time = "2026-09-11T19:23:01.995Z" }, +] + +[[package]] +name = "botocore" +version = "1.43.93" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "jmespath" }, + { name = "python-dateutil" }, + { name = "urllib3" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/c8/a0/2ce10897323d67dd85de6190fdee159013a75741d1dc48b74d4815ec0592/botocore-1.43.93.tar.gz", hash = "sha256:82da355d18a7f784347b00444be33942834651f31b6c5ffef49999cd47364c5e", size = 16103276, upload-time = "2026-09-11T19:22:58.933Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/2c/7e/f858c401f32d980f924c8f8328b62fab463313834a9ad2326f44613b97ec/botocore-1.43.93-py3-none-any.whl", hash = "sha256:3ca57bb5d26d88b554a74de708a5c991f45306436c91aacca931252d1d4d54ff", size = 15794156, upload-time = "2026-09-11T19:22:55.964Z" }, +] + [[package]] name = "certifi" version = "2026.4.22" @@ -527,7 +555,7 @@ wheels = [ [[package]] name = "fastapi-startkit" -version = "0.51.0" +version = "0.56.0" source = { editable = "." } dependencies = [ { name = "cleo" }, @@ -575,6 +603,7 @@ dev = [ { name = "aiomysql" }, { name = "aiosqlite" }, { name = "asyncpg" }, + { name = "boto3" }, { name = "dumpdie" }, { name = "faker" }, { name = "fastapi", extra = ["standard"] }, @@ -620,6 +649,7 @@ dev = [ { name = "aiomysql", specifier = ">=0.2.0" }, { name = "aiosqlite", specifier = ">=0.22.1" }, { name = "asyncpg", specifier = ">=0.29.0" }, + { name = "boto3", specifier = ">=1.35.0" }, { name = "dumpdie", specifier = ">=1.5.0" }, { name = "faker", specifier = ">=40.13.0" }, { name = "fastapi", extras = ["standard"], specifier = ">=0.124.4" }, @@ -922,6 +952,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/62/a1/3d680cbfd5f4b8f15abc1d571870c5fc3e594bb582bc3b64ea099db13e56/jinja2-3.1.6-py3-none-any.whl", hash = "sha256:85ece4451f492d0c13c5dd7c13a64681a86afae63a5f347908daf103ce6d2f67", size = 134899, upload-time = "2025-03-05T20:05:00.369Z" }, ] +[[package]] +name = "jmespath" +version = "1.1.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/d3/59/322338183ecda247fb5d1763a6cbe46eff7222eaeebafd9fa65d4bf5cb11/jmespath-1.1.0.tar.gz", hash = "sha256:472c87d80f36026ae83c6ddd0f1d05d4e510134ed462851fd5f754c8c3cbb88d", size = 27377, upload-time = "2026-01-22T16:35:26.279Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/14/2f/967ba146e6d58cf6a652da73885f52fc68001525b4197effc174321d70b4/jmespath-1.1.0-py3-none-any.whl", hash = "sha256:a5663118de4908c91729bea0acadca56526eb2698e83de10cd116ae0f4e97c64", size = 20419, upload-time = "2026-01-22T16:35:24.919Z" }, +] + [[package]] name = "jsonpatch" version = "1.33" @@ -1871,6 +1910,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c0/98/6beb4b351e472e5f4c4613f7c35a5290b8be2497e183825310c4c3a3984b/ruff-0.15.12-py3-none-win_arm64.whl", hash = "sha256:a538f7a82d061cee7be55542aca1d86d1393d55d81d4fcc314370f4340930d4f", size = 11120821, upload-time = "2026-04-24T18:16:57.979Z" }, ] +[[package]] +name = "s3transfer" +version = "0.19.2" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "botocore" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/76/43/35e4d8aa320bffe8287fe8f65f578fa2d2db0a64212f0e710dce58267854/s3transfer-0.19.2.tar.gz", hash = "sha256:ba0309fd86be3c27dbf78cdd813c13c5e1df16e5874b99d2535ebbdfb9892993", size = 165592, upload-time = "2026-07-22T19:30:44.432Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/bc/e7/5c595c75e9f41a44f30e526eda465ea0b4eec93470e074e4a111b253f13a/s3transfer-0.19.2-py3-none-any.whl", hash = "sha256:d8168eccca828cbb2cd573675333f3bddd254313a9c42494b84c76b539e8ba25", size = 90216, upload-time = "2026-07-22T19:30:43.251Z" }, +] + [[package]] name = "secretstorage" version = "3.5.0" From ddb17cb9c5497f004b0b062b3c2432bd91bef23b Mon Sep 17 00:00:00 2001 From: Bedram Tamang Date: Sat, 12 Sep 2026 13:31:23 -0700 Subject: [PATCH 2/2] fix: make local get_files a read-only, body-free listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two blockers on PR #220. Listing read every file through get(), which opens in text mode, so one non-UTF-8 file raised UnicodeDecodeError and killed the whole listing — and issue #218's motivating example is an audio/ directory. It also broke the cross-driver contract the rest of this branch establishes: the same layout listed fine on S3, which never reads bodies, and exploded on local. Listings no longer carry a body at all; content is fetched on demand via get(join(directory, file.name())). get_path() calls make_file_path_if_not_exists(), so listing a missing nested directory silently os.makedirs'd its parent — a read-only operation mutating the disk. Path resolution is now split out into resolve_path(), which resolves against the disk root and touches nothing; get_path() keeps its create-on-demand behaviour for the write paths that want it, and get_files() uses resolve_path(). The earlier test missed this because a single-segment name's parent already exists; the new test uses a multi-segment path and asserts nothing was created. Documents on File what content means per driver, including that file.stream().key is how the full S3 key is recovered, and drops FakeDriver.get_files, which was a pure super() passthrough. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FyozUDQzC8en24yx1Je1ct --- .../fastapi_startkit/storage/drivers/fake.py | 8 -- .../fastapi_startkit/storage/drivers/local.py | 18 +++- .../src/fastapi_startkit/storage/file.py | 16 ++++ .../tests/storage/test_local_driver.py | 89 +++++++++++++++++-- 4 files changed, 114 insertions(+), 17 deletions(-) diff --git a/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py b/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py index a9577e7d..47a35db2 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py +++ b/fastapi_startkit/src/fastapi_startkit/storage/drivers/fake.py @@ -24,14 +24,6 @@ def __exit__(self, *_): def cleanup(self): shutil.rmtree(self._root, ignore_errors=True) - def get_files(self, directory=""): - """List files directly under ``directory`` in the fake's temp root. - - Delegates to LocalDriver rather than reimplementing the listing so the - fake can never drift from the semantics it stands in for. - """ - return super().get_files(directory) - def assert_exists(self, paths, content=None): if isinstance(paths, str): paths = [paths] diff --git a/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py b/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py index 468639b7..1ee553da 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/drivers/local.py +++ b/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 @@ -119,9 +123,15 @@ def get_files(self, directory=""): 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("/") - directory_path = self.get_path(directory) + # 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 [] @@ -132,7 +142,7 @@ def get_files(self, directory=""): if not isfile(join(directory_path, name)): continue - files.append(File(self.get(join(directory, name)), name)) + files.append(File(None, name)) return files diff --git a/fastapi_startkit/src/fastapi_startkit/storage/file.py b/fastapi_startkit/src/fastapi_startkit/storage/file.py index c19ddc7a..c9cb43b9 100644 --- a/fastapi_startkit/src/fastapi_startkit/storage/file.py +++ b/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 diff --git a/fastapi_startkit/tests/storage/test_local_driver.py b/fastapi_startkit/tests/storage/test_local_driver.py index 0619866c..b40df866 100644 --- a/fastapi_startkit/tests/storage/test_local_driver.py +++ b/fastapi_startkit/tests/storage/test_local_driver.py @@ -336,8 +336,8 @@ def driver(self, tmp_path): 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_file_content_is_populated_not_none(self, driver): - contents = {f.name(): f.stream() for f in driver.get_files("audio")} + 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): @@ -373,9 +373,14 @@ def test_matches_the_local_driver_for_the_same_layout(self, tmp_path): _layout(Path(fake._root)) for directory in ("audio", "audio/", "", "nope"): - assert [f.name() for f in fake.get_files(directory)] == [f.name() for f in local.get_files(directory)] - assert [f.stream() for f in fake.get_files(directory)] == [ - f.stream() for f in local.get_files(directory) + 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): @@ -387,3 +392,77 @@ def test_assert_count_scopes_to_a_directory(self, tmp_path): 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()