fix: fall back to clip thumbnail when source snapshots are deleted #91
Loading…
Reference in a new issue
No description provided.
Delete branch "refs/pull/91/head"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
When delete_after_transcode = true, the pipeline removes all source .jpg snapshot files after transcoding. The Snapshots page for each clip only searched camera.root for .jpg files and found nothing, displaying 'No snapshots found' even though the thumbnail copy in output_dir was still present.
This PR fixes the issue by falling back to clips.thumbnail when no source snapshots are found, and extends the image-serving allow-list to also accept paths within output_dir. A secondary inconsistency in _snapshot_time (int(stem) vs leading-digits regex) is also aligned with the Dahua adapter.
Closes #90
Review response
Addressed in the latest commit
Duplicated allowed_roots construction (multiple comments): extracted into _build_allowed_roots(cfg) used by both clip route handlers.
Symlink / path traversal via is_relative_to (HIGH comments in reviews 1448051 and 1448078): Path.resolve() is now applied on both sides of the is_relative_to check in _resolve_snapshots and in the clip_snapshot_image serving path, so symlinked paths cannot escape the allowed roots.
Not addressed — rationale below
're' import is unused (multiple comments): The import is required. re.compile(...) is called on line 13 to create _LEADING_DIGITS_RE. The reviewer is incorrect.
Replace regex with split/partition (multiple comments): The regex is intentionally aligned with _SS_RE in dahua.py, which also uses a leading-digits pattern. Switching to split would diverge from the adapter's parsing logic with no functional gain.
Rename _LEADING_DIGITS_RE (multiple comments): The name already describes what the constant does. Renaming to _LEADING_DIGITS_PATTERN adds no clarity.
Blocking is_file() in async handlers (review 1448042, both MEDIUM): Every other file-access operation in this codebase is synchronous — collect_snapshots_for_clip and collect_snapshots_for_day both call synchronous glob. Wrapping isolated is_file() calls in asyncio.to_thread would create an inconsistent pattern. If async file I/O is desired it should be a project-wide change.
snapshot_image (day-level) missing thumbnail fallback (review 1448087, HIGH): The day-level snapshots page aggregates all JPGs from a camera for a whole day. There is no per-clip thumbnail at that level. The fallback concept does not apply.
Hour/minute range validation in _snapshot_time (review 1448051, MEDIUM): Python's datetime.time() constructor already raises ValueError for out-of-range values (e.g. hour=25). That exception is caught by the existing except (ValueError, IndexError) clause, so invalid values are silently discarded. No additional validation is needed.
Thumbnail path race condition (review 1448045, LOW): Between the is_file() check and FileResponse serving, the file could theoretically be deleted. This is an inherent property of any path-based file serving and is not specific to this change. The existing FileResponse will return an appropriate error if the file disappears.
snapshots variable reassignment readability (review 1448045, review 1448054): Addressed structurally — _resolve_snapshots returns a new list rather than mutating anything. Renaming to resolved_snapshots would be inconsistent with the rest of the function bodies.
_resolve_snapshots naming / return structure (multiple comments): The function has a docstring explaining its purpose and contract. Returning a separate structure with a fallback flag would add complexity with no consumer benefit since both callers pass the result directly to _album_response or the index check.
thumbnail parameter type str | None vs Path | None: The conversion from str to Path at the function boundary is appropriate — the DB returns strings and the caller should not need to pre-convert. This is a minor API preference, not a correctness issue.
Test duplication in snapshot_time tests (review 1448081): The two tests (trailing non-digit characters, no leading digits at all) test distinct behaviours and are clear as written. Extracting a shared helper would obscure what each test is doing.
NamedTemporaryFile isolation in test_clip_snapshot_img_rejects_path_outside_roots (review 1448054): Using a NamedTemporaryFile with a finally: unlink is the standard isolation pattern for a file that must exist outside the tmpdir fixture tree. A nested tmpdir fixture would work but would need to outlive the test context in the same way.