fix: extract build_episode_output_dir helper and test production code
Tests were reimplementing UUID logic locally instead of testing the actual production code path. Extract the path-building logic into a testable helper function and import it directly in tests.
This commit is contained in:
parent
0619fa4d3b
commit
e9040ef97b
2 changed files with 46 additions and 39 deletions
|
|
@ -23,6 +23,20 @@ except ImportError as e:
|
||||||
raise ValueError("podcast_creator library not available")
|
raise ValueError("podcast_creator library not available")
|
||||||
|
|
||||||
|
|
||||||
|
def build_episode_output_dir(data_folder: str) -> tuple[str, Path]:
|
||||||
|
"""Build a filesystem-safe output directory path for a podcast episode.
|
||||||
|
|
||||||
|
Uses a UUID as the directory name so the path is safe regardless of
|
||||||
|
what the user typed as episode name (spaces, special chars, etc.).
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
A tuple of (episode_dir_name, output_dir_path).
|
||||||
|
"""
|
||||||
|
episode_dir_name = str(uuid.uuid4())
|
||||||
|
output_dir = Path(f"{data_folder}/podcasts/episodes/{episode_dir_name}")
|
||||||
|
return episode_dir_name, output_dir
|
||||||
|
|
||||||
|
|
||||||
def full_model_dump(model):
|
def full_model_dump(model):
|
||||||
if isinstance(model, BaseModel):
|
if isinstance(model, BaseModel):
|
||||||
return model.model_dump()
|
return model.model_dump()
|
||||||
|
|
@ -222,8 +236,7 @@ async def generate_podcast_command(
|
||||||
logger.info(f"Generated briefing (length: {len(briefing)} chars)")
|
logger.info(f"Generated briefing (length: {len(briefing)} chars)")
|
||||||
|
|
||||||
# 7. Create output directory using UUID for filesystem-safe paths
|
# 7. Create output directory using UUID for filesystem-safe paths
|
||||||
episode_dir_name = str(uuid.uuid4())
|
episode_dir_name, output_dir = build_episode_output_dir(DATA_FOLDER)
|
||||||
output_dir = Path(f"{DATA_FOLDER}/podcasts/episodes/{episode_dir_name}")
|
|
||||||
output_dir.mkdir(parents=True, exist_ok=True)
|
output_dir.mkdir(parents=True, exist_ok=True)
|
||||||
|
|
||||||
logger.info(f"Created output directory: {output_dir}")
|
logger.info(f"Created output directory: {output_dir}")
|
||||||
|
|
|
||||||
|
|
@ -7,68 +7,62 @@ spaces and special characters (GitHub issue #663).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import uuid
|
import uuid
|
||||||
from pathlib import Path, PurePosixPath
|
from pathlib import PurePosixPath
|
||||||
|
|
||||||
|
from commands.podcast_commands import build_episode_output_dir
|
||||||
|
|
||||||
|
|
||||||
def _build_episode_output_dir(data_folder: str) -> tuple[str, Path]:
|
class TestBuildEpisodeOutputDir:
|
||||||
"""Replicate the directory naming logic from generate_podcast_command."""
|
"""Test the actual production helper that builds episode output paths."""
|
||||||
episode_dir_name = str(uuid.uuid4())
|
|
||||||
output_dir = Path(f"{data_folder}/podcasts/episodes/{episode_dir_name}")
|
|
||||||
return episode_dir_name, output_dir
|
|
||||||
|
|
||||||
|
def test_directory_name_is_valid_uuid(self):
|
||||||
class TestPodcastEpisodeDirectory:
|
dir_name, _ = build_episode_output_dir("/data")
|
||||||
"""Verify that episode directories are always filesystem-safe."""
|
|
||||||
|
|
||||||
def test_directory_uses_uuid_format(self):
|
|
||||||
dir_name, _ = _build_episode_output_dir("/data")
|
|
||||||
# Should be a valid UUID
|
|
||||||
parsed = uuid.UUID(dir_name)
|
parsed = uuid.UUID(dir_name)
|
||||||
assert str(parsed) == dir_name
|
assert str(parsed) == dir_name
|
||||||
|
|
||||||
def test_directory_path_is_valid(self):
|
def test_path_structure(self):
|
||||||
_, output_dir = _build_episode_output_dir("/data")
|
dir_name, output_dir = build_episode_output_dir("/data")
|
||||||
# Path should have exactly the expected structure
|
assert str(output_dir) == f"/data/podcasts/episodes/{dir_name}"
|
||||||
assert str(output_dir).startswith("/data/podcasts/episodes/")
|
|
||||||
# Directory name should be the last component
|
|
||||||
assert len(output_dir.name) == 36 # UUID string length
|
|
||||||
|
|
||||||
def test_no_collision_between_calls(self):
|
def test_no_collision_between_calls(self):
|
||||||
dir1, _ = _build_episode_output_dir("/data")
|
dir1, _ = build_episode_output_dir("/data")
|
||||||
dir2, _ = _build_episode_output_dir("/data")
|
dir2, _ = build_episode_output_dir("/data")
|
||||||
assert dir1 != dir2
|
assert dir1 != dir2
|
||||||
|
|
||||||
def test_path_has_no_spaces_or_special_chars(self):
|
def test_path_is_independent_of_episode_name(self):
|
||||||
"""Regardless of episode name, path should be clean."""
|
"""The returned path must never contain user-supplied episode names.
|
||||||
|
|
||||||
|
Since build_episode_output_dir does not accept an episode name at all,
|
||||||
|
any name the user types is structurally excluded from the path.
|
||||||
|
"""
|
||||||
problematic_names = [
|
problematic_names = [
|
||||||
"My Episode Name",
|
"My Episode Name",
|
||||||
"Episode: Part 1",
|
"Episode: Part 1",
|
||||||
'test "quotes"',
|
'test "quotes"',
|
||||||
"path/traversal",
|
"path/traversal",
|
||||||
"dots..and...more",
|
|
||||||
"café résumé",
|
"café résumé",
|
||||||
" spaces ",
|
" spaces ",
|
||||||
"",
|
|
||||||
"?*<>|",
|
"?*<>|",
|
||||||
]
|
]
|
||||||
for name in problematic_names:
|
for name in problematic_names:
|
||||||
dir_name, output_dir = _build_episode_output_dir("/data")
|
_, output_dir = build_episode_output_dir("/data")
|
||||||
# UUID path is independent of the episode name
|
|
||||||
path_str = str(output_dir)
|
path_str = str(output_dir)
|
||||||
assert " " not in path_str, f"Space found in path for name: {name}"
|
# The episode name must not appear anywhere in the path
|
||||||
for char in ['<', '>', ':', '"', '|', '?', '*']:
|
assert name not in path_str
|
||||||
assert char not in path_str, (
|
# UUID paths contain only hex digits and hyphens after the base
|
||||||
f"Unsafe char '{char}' in path for name: {name}"
|
dir_component = output_dir.name
|
||||||
)
|
assert all(c in "0123456789abcdef-" for c in dir_component), (
|
||||||
|
f"Unexpected chars in directory name: {dir_component}"
|
||||||
|
)
|
||||||
|
|
||||||
def test_path_works_on_posix(self):
|
def test_path_works_on_posix(self):
|
||||||
_, output_dir = _build_episode_output_dir("/data")
|
dir_name, output_dir = build_episode_output_dir("/data")
|
||||||
posix = PurePosixPath(str(output_dir))
|
posix = PurePosixPath(str(output_dir))
|
||||||
assert posix.parts == ("/", "data", "podcasts", "episodes", output_dir.name)
|
assert posix.parts == ("/", "data", "podcasts", "episodes", dir_name)
|
||||||
|
|
||||||
def test_uuid_directory_can_be_created(self, tmp_path):
|
def test_directory_can_be_created(self, tmp_path):
|
||||||
"""Actually create the directory to verify it works on the real filesystem."""
|
"""Create the directory on the real filesystem."""
|
||||||
dir_name, output_dir = _build_episode_output_dir(str(tmp_path))
|
_, output_dir = build_episode_output_dir(str(tmp_path))
|
||||||
output_dir.mkdir(parents=True, exist_ok=True)
|
output_dir.mkdir(parents=True, exist_ok=True)
|
||||||
assert output_dir.exists()
|
assert output_dir.exists()
|
||||||
assert output_dir.is_dir()
|
assert output_dir.is_dir()
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue