diff --git a/py/api/gallery_listing.py b/py/api/gallery_listing.py index 5c21b602..af5b86b2 100644 --- a/py/api/gallery_listing.py +++ b/py/api/gallery_listing.py @@ -91,9 +91,19 @@ def folder_sets(root, files) -> tuple[frozenset, frozenset]: ``str(relative.parent)`` (native separators) for the folders holding files, plus every ancestor joined by ``/``. """ + # From the path strings: relative_to per file was half of a large walk. + # The separator is the path flavour's own (tests use PureWindowsPath). + sep = str(type(root)("a", "b"))[1] + prefix = str(root) + if not prefix.endswith(sep): + prefix += sep folders = set() for path, _mtime in files: - rel_dir = str(path.relative_to(root).parent) + text = str(path) + if text.startswith(prefix): + rel_dir = text[len(prefix) :].rpartition(sep)[0] + else: + rel_dir = str(path.relative_to(root).parent) if rel_dir and rel_dir != ".": folders.add(rel_dir) with_ancestors = set(folders) diff --git a/py/api/images.py b/py/api/images.py index 519f6850..87017bc7 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -772,32 +772,54 @@ def _favorite_root_key(root) -> str: return _favorite_relative_key(os.path.realpath(str(root))) -def _iter_media_files(output_path, max_files): - """Yield media files under *output_path* with bounded effort. +def _walk_media(output_path, max_files): + """Yield the ``os.DirEntry`` of each media file under *output_path*. Symlinks are never followed, ``thumbnails/`` is skipped, descent stops - at MAX_SCAN_DEPTH and at most *max_files* paths are produced. + at MAX_SCAN_DEPTH and at most *max_files* entries are produced. The order + is ``os.walk``'s (top-down, subfolders in ``os.scandir`` order, files by + name), which the gallery's ties in mtime keep. Each entry's type comes + from the directory listing itself, so a walk builds no ``Path`` and makes + no extra ``lstat`` per file (#348: a 20,000-file walk spent half its time + there). """ - output_path = Path(output_path) - root_depth = len(output_path.parts) + stack = [(os.fspath(output_path), ())] produced = 0 - for root, dirs, files in os.walk(output_path): - root_path = Path(root) - rel_parts = root_path.parts[root_depth:] + while stack: + top, rel_parts = stack.pop() if "thumbnails" in rel_parts: - dirs[:] = [] continue - if len(rel_parts) >= MAX_SCAN_DEPTH: - dirs[:] = [] - for name in sorted(files): - if Path(name).suffix.lower() not in MEDIA_EXTENSIONS: + dirs, files = [], [] + try: + with os.scandir(top) as entries: + for entry in entries: + try: + is_dir = entry.is_dir() + except OSError: + is_dir = False + (dirs if is_dir else files).append(entry) + except OSError: + continue # as os.walk: an unreadable folder is skipped + for entry in sorted(files, key=lambda item: item.name): + if os.path.splitext(entry.name)[1].lower() not in MEDIA_EXTENSIONS: continue - if (root_path / name).is_symlink(): + if entry.is_symlink(): continue # never read through a link out of the output tree if produced >= max_files: return produced += 1 - yield root_path / name + yield entry + if len(rel_parts) >= MAX_SCAN_DEPTH: + continue + for entry in reversed(dirs): + if not entry.is_symlink(): + stack.append((entry.path, rel_parts + (entry.name,))) + + +def _iter_media_files(output_path, max_files): + """Yield the ``Path`` of each media file under *output_path* (see _walk_media).""" + for entry in _walk_media(output_path, max_files): + yield Path(entry.path) def media_thumbnail_rel(rel_path, settings=None): @@ -1294,11 +1316,14 @@ class ImageRoutesMixin: walk is bounded by MAX_SCAN_DEPTH and MAX_GALLERY_FILES. """ found = [] - for media_path in _iter_media_files(output_path, MAX_GALLERY_FILES): + for entry in _walk_media(output_path, MAX_GALLERY_FILES): try: - found.append((media_path, media_path.stat().st_mtime)) + # Never a symlink (_walk_media skips them), so this is the + # file's own mtime, read from the listing where it can be. + mtime = entry.stat(follow_symlinks=False).st_mtime except OSError: continue + found.append((Path(entry.path), mtime)) if len(found) >= MAX_GALLERY_FILES: self.logger.warning(f"Gallery scan capped at {MAX_GALLERY_FILES} files") found.sort(key=lambda x: x[1], reverse=True) diff --git a/tests/test_api_images.py b/tests/test_api_images.py index 7c561f1f..f4a22743 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -1455,12 +1455,15 @@ class TestGalleryListing(ImageRouteCoverageCase): self.assertEqual(data["images"], []) def test_gallery_scan_skips_unreadable_entries(self): - ghost = self.output_dir / "ghost.png" + make_png(self.output_dir / "real.png") + listed = list(images_module._walk_media(self.output_dir, 10)) + ghost = MagicMock(path=str(self.output_dir / "ghost.png")) + ghost.stat.side_effect = FileNotFoundError("gone") with patch.object( - images_module, "_iter_media_files", return_value=iter([ghost]) + images_module, "_walk_media", return_value=iter([ghost, *listed]) ): found = self.api._scan_gallery_files_sync(self.output_dir) - self.assertEqual(found, []) + self.assertEqual([path.name for path, _mtime in found], ["real.png"]) class TestThumbnailUnits(ImageRouteCoverageCase): @@ -2033,6 +2036,54 @@ class TestOutputScanHelpers(ImageAPITestCase): expected = hashlib.sha1(str(media).encode("utf-8")).hexdigest()[:16] self.assertEqual(entry["id"], expected) + def test_media_walk_matches_the_os_walk_reference(self): + # The scandir walk (#348) must list exactly what the os.walk version + # listed, in the same order: ties in mtime keep this order. + output = Path(self.output_dir) + for rel in ( + "z.png", + "a.PNG", + "notes.txt", + "b/2.png", + "b/1.webp", + "b/c/deep.mp4", + "b/c/d/too_deep.png", + "thumbnails/t.png", + "b/thumbnails/u.png", + "e/.hidden.png", + "e/f.jpeg", + ): + make_png(output / rel) + try: + os.symlink(output / "z.png", output / "b" / "link.png") + os.symlink(output / "b", output / "dirlink", target_is_directory=True) + except (OSError, NotImplementedError): + pass # the rest of the tree still checks order and filters + with patch.object(images_module, "MAX_SCAN_DEPTH", 2): + listed = list(images_module._iter_media_files(output, 100)) + expected = list(_reference_iter_media_files(output, 100)) + capped = list(images_module._iter_media_files(output, 3)) + self.assertEqual(listed, expected) + self.assertEqual(capped, expected[:3]) + self.assertTrue(all(isinstance(path, Path) for path in listed)) + + def test_gallery_scan_reads_each_file_type_and_mtime_from_scandir(self): + # Path.stat and Path.is_symlink per file were the other half of the + # live walk's cost (#348); the scandir entries already carry both. + output = Path(self.output_dir) + make_png(output / "a.png") + make_png(output / "sub" / "b.png") + with ( + patch.object(Path, "stat", side_effect=AssertionError), + patch.object(Path, "is_symlink", side_effect=AssertionError), + ): + found = self.api._scan_gallery_files_sync(output) + self.assertEqual( + sorted(path.name for path, _mtime in found), ["a.png", "b.png"] + ) + for path, mtime in found: + self.assertEqual(mtime, os.stat(path).st_mtime) + def test_iter_media_files_skips_symlinked_files(self): output = Path(self.output_dir) make_png(output / "real.png") @@ -2355,6 +2406,30 @@ class TestRootScopeServing(ImageAPITestCase): _HANG_LIMIT = 30 +def _reference_iter_media_files(output_path, max_files): + """``_iter_media_files`` as it was before the scandir walk (#348).""" + output_path = Path(output_path) + root_depth = len(output_path.parts) + produced = 0 + for root, dirs, files in os.walk(output_path): + root_path = Path(root) + rel_parts = root_path.parts[root_depth:] + if "thumbnails" in rel_parts: + dirs[:] = [] + continue + if len(rel_parts) >= images_module.MAX_SCAN_DEPTH: + dirs[:] = [] + for name in sorted(files): + if Path(name).suffix.lower() not in images_module.MEDIA_EXTENSIONS: + continue + if (root_path / name).is_symlink(): + continue + if produced >= max_files: + return + produced += 1 + yield root_path / name + + class _GatedScan: """Wraps the real scan, counts calls and can hold one call after it listed. diff --git a/tests/test_gallery_listing.py b/tests/test_gallery_listing.py index 261b9ce1..9d39b652 100644 --- a/tests/test_gallery_listing.py +++ b/tests/test_gallery_listing.py @@ -4,7 +4,8 @@ import os import sys import tempfile import unittest -from pathlib import Path, PureWindowsPath +from pathlib import Path, PurePath, PureWindowsPath +from unittest.mock import patch sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -70,6 +71,23 @@ class FolderSetsTests(unittest.TestCase): def test_an_empty_listing_has_no_folders(self): self.assertEqual(folder_sets(Path("/out"), []), (frozenset(), frozenset())) + def test_folders_come_from_the_path_strings_not_relative_to(self): + # relative_to per file was half of a 20,000-file walk (#348 live test). + root = Path("/out") + files = [(root / "a" / "b" / "one.png", 1.0), (root / "two.png", 2.0)] + with patch.object(PurePath, "relative_to", side_effect=AssertionError): + folders, with_ancestors = folder_sets(root, files) + self.assertEqual(folders, {os.path.join("a", "b")}) + self.assertEqual(with_ancestors, {os.path.join("a", "b"), "a"}) + + def test_a_file_outside_the_root_string_still_gets_its_folder(self): + root = Path("/out") + odd = Path("/out/./x/y.png") # normalizes to /out/x/y.png + files = [(odd, 1.0)] + self.assertEqual( + folder_sets(root, files)[0], _legacy_subfolders(root, files, False) + ) + class SameListingTests(unittest.TestCase): def test_equal_path_and_mtime_lists_are_the_same(self):