chore(downloads): C3-C6 — DRY the source-URL, filename, and serialize helpers
- C3: `_reference_url` (downloads.py) and public.py's inline source-URL block were the same rule → extract `service.reference_url(job, asset)`; both surfaces now share it so the "downloaded from" link can't drift between them. - C4: Content-Disposition filename derivation (ext pick + doubled-ext strip + join) was duplicated in download_file and watch_file → extract `storage.download_filename(display_name, container, path)`. - C5: inline the one-line `_clean_basename` passthrough (folded into C4). - C6: the `db.get(MediaAsset, job.asset_id) if job.asset_id else None; _serialize(...)` resolve-then-serialize dance was repeated across 6 single-job handlers → fold into `_serialize_job(db, job)`. (File-serving handlers that use the asset for their own checks keep their explicit resolve.) Behavior-neutral; ruff/parse clean, localdev boots, downloads routes load.
This commit is contained in:
parent
2a44db04d8
commit
cac5526399
5 changed files with 44 additions and 49 deletions
|
|
@ -75,13 +75,7 @@ def public_meta(link: DownloadLink, job, asset, file_url: str, poster_url: str |
|
||||||
auto-extracted values, and surfaces the source + any extra reference links as clickable URLs."""
|
auto-extracted values, and surfaces the source + any extra reference links as clickable URLs."""
|
||||||
from app.downloads import service
|
from app.downloads import service
|
||||||
|
|
||||||
source_url = None
|
source_url = service.reference_url(job, asset)
|
||||||
if job is not None and job.job_kind != "edit":
|
|
||||||
source_url = asset.source_webpage_url or (
|
|
||||||
service.source_url(job.source_kind, job.source_ref)
|
|
||||||
if job.source_kind in ("youtube", "url")
|
|
||||||
else None
|
|
||||||
)
|
|
||||||
return {
|
return {
|
||||||
"title": (job and job.display_name) or asset.title,
|
"title": (job and job.display_name) or asset.title,
|
||||||
"uploader": (job and job.display_uploader) or asset.uploader,
|
"uploader": (job and job.display_uploader) or asset.uploader,
|
||||||
|
|
|
||||||
|
|
@ -32,6 +32,20 @@ def source_url(source_kind: str, source_ref: str) -> str:
|
||||||
return source_ref
|
return source_ref
|
||||||
|
|
||||||
|
|
||||||
|
def reference_url(job, asset) -> str | None:
|
||||||
|
"""The clean "downloaded from" URL for a job — shared by the private Downloads page and the public
|
||||||
|
watch page so both surfaces always agree. None for edit derivatives (a clip's source is the user's
|
||||||
|
own earlier download, not a web page); else the canonical page URL yt-dlp recorded; else one derived
|
||||||
|
from source_kind+source_ref (covers queued/older rows before the worker fills it)."""
|
||||||
|
if job is None or job.job_kind == "edit":
|
||||||
|
return None
|
||||||
|
if asset is not None and asset.source_webpage_url:
|
||||||
|
return asset.source_webpage_url
|
||||||
|
if job.source_kind in ("youtube", "url"):
|
||||||
|
return source_url(job.source_kind, job.source_ref)
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def get_or_create_asset(
|
def get_or_create_asset(
|
||||||
db: Session, source_kind: str, source_ref: str, format_sig: str
|
db: Session, source_kind: str, source_ref: str, format_sig: str
|
||||||
) -> MediaAsset:
|
) -> MediaAsset:
|
||||||
|
|
|
||||||
|
|
@ -96,6 +96,17 @@ def edit_rel_path(user_id: int, asset_id: int, ext: str) -> str:
|
||||||
return f".edits/{user_id}/clip_{asset_id}.{ext}"
|
return f".edits/{user_id}/clip_{asset_id}.{ext}"
|
||||||
|
|
||||||
|
|
||||||
|
def download_filename(display_name: str, container: str | None, path: Path) -> str:
|
||||||
|
"""The Content-Disposition filename for a served download: the cleaned display name carrying a single
|
||||||
|
correct extension — the asset container, else the file's own suffix — without doubling it when the
|
||||||
|
name already ends in that extension. Shared by the private download and the public watch endpoints."""
|
||||||
|
ext = container or path.suffix.lstrip(".")
|
||||||
|
base = display_filename(display_name)
|
||||||
|
if base.lower().endswith(f".{ext.lower()}"):
|
||||||
|
base = base[: -(len(ext) + 1)]
|
||||||
|
return f"{base}.{ext}"
|
||||||
|
|
||||||
|
|
||||||
def abs_path(download_root: str, rel: str) -> Path:
|
def abs_path(download_root: str, rel: str) -> Path:
|
||||||
return Path(download_root) / rel
|
return Path(download_root) / rel
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -67,20 +67,6 @@ def resolve_source(raw: str) -> tuple[str, str]:
|
||||||
raise HTTPException(status_code=400, detail="That doesn't look like a valid video link.")
|
raise HTTPException(status_code=400, detail="That doesn't look like a valid video link.")
|
||||||
|
|
||||||
|
|
||||||
def _reference_url(job: DownloadJob, asset: MediaAsset | None) -> str | None:
|
|
||||||
"""The clean "downloaded from" URL for the Downloads page: the canonical page URL yt-dlp
|
|
||||||
recorded, else one derived from source_kind+source_ref (covers queued/older rows before the
|
|
||||||
worker fills it). Returns None for edit derivatives — a clip's source is the user's own earlier
|
|
||||||
download, not a web page, so there's no meaningful external URL to show."""
|
|
||||||
if job.job_kind == "edit":
|
|
||||||
return None
|
|
||||||
if asset is not None and asset.source_webpage_url:
|
|
||||||
return asset.source_webpage_url
|
|
||||||
if job.source_kind in ("youtube", "url"):
|
|
||||||
return service.source_url(job.source_kind, job.source_ref)
|
|
||||||
return None
|
|
||||||
|
|
||||||
|
|
||||||
def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict:
|
def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict:
|
||||||
ready = asset is not None and asset.status == "ready" and bool(asset.rel_path)
|
ready = asset is not None and asset.status == "ready" and bool(asset.rel_path)
|
||||||
return {
|
return {
|
||||||
|
|
@ -95,7 +81,7 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict:
|
||||||
"created_at": job.created_at.isoformat() if job.created_at else None,
|
"created_at": job.created_at.isoformat() if job.created_at else None,
|
||||||
"source_kind": job.source_kind,
|
"source_kind": job.source_kind,
|
||||||
"source_ref": job.source_ref,
|
"source_ref": job.source_ref,
|
||||||
"source_url": _reference_url(job, asset),
|
"source_url": service.reference_url(job, asset),
|
||||||
# A locally-generated poster to fall back to when the source had no thumbnail.
|
# A locally-generated poster to fall back to when the source had no thumbnail.
|
||||||
"poster_url": f"/api/downloads/{job.id}/poster.jpg"
|
"poster_url": f"/api/downloads/{job.id}/poster.jpg"
|
||||||
if (asset is not None and asset.poster_path)
|
if (asset is not None and asset.poster_path)
|
||||||
|
|
@ -139,6 +125,13 @@ def _assets_for(db: Session, jobs: list[DownloadJob]) -> dict[int, MediaAsset]:
|
||||||
return {a.id: a for a in rows}
|
return {a.id: a for a in rows}
|
||||||
|
|
||||||
|
|
||||||
|
def _serialize_job(db: Session, job: DownloadJob) -> dict:
|
||||||
|
"""Resolve a job's cache asset (if any) and serialize the pair — the resolve-then-serialize step
|
||||||
|
every single-job handler shares."""
|
||||||
|
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
||||||
|
return _serialize(job, asset)
|
||||||
|
|
||||||
|
|
||||||
def _own_job(db: Session, user: User, job_id: int) -> DownloadJob:
|
def _own_job(db: Session, user: User, job_id: int) -> DownloadJob:
|
||||||
job = db.get(DownloadJob, job_id)
|
job = db.get(DownloadJob, job_id)
|
||||||
if job is None or job.user_id != user.id:
|
if job is None or job.user_id != user.id:
|
||||||
|
|
@ -272,8 +265,7 @@ def enqueue_download(
|
||||||
)
|
)
|
||||||
except quota.QuotaExceeded as e:
|
except quota.QuotaExceeded as e:
|
||||||
raise HTTPException(status_code=422, detail=_quota_message(e))
|
raise HTTPException(status_code=422, detail=_quota_message(e))
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
@router.post("/edit")
|
@router.post("/edit")
|
||||||
|
|
@ -298,8 +290,7 @@ def enqueue_edit(
|
||||||
raise HTTPException(status_code=422, detail=_quota_message(e))
|
raise HTTPException(status_code=422, detail=_quota_message(e))
|
||||||
except service.EditError as e:
|
except service.EditError as e:
|
||||||
raise HTTPException(status_code=400, detail=_edit_message(e))
|
raise HTTPException(status_code=400, detail=_edit_message(e))
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
def _edit_message(e: service.EditError) -> str:
|
def _edit_message(e: service.EditError) -> str:
|
||||||
|
|
@ -429,8 +420,7 @@ def update_download_meta(
|
||||||
cleaned = [u for u in (_clean_url(x) for x in raw) if u][:_MAX_EXTRA_LINKS]
|
cleaned = [u for u in (_clean_url(x) for x in raw) if u][:_MAX_EXTRA_LINKS]
|
||||||
job.extra_links = cleaned or None
|
job.extra_links = cleaned or None
|
||||||
db.commit()
|
db.commit()
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
def _set_status(db: Session, job: DownloadJob, status: str) -> None:
|
def _set_status(db: Session, job: DownloadJob, status: str) -> None:
|
||||||
|
|
@ -445,8 +435,7 @@ def pause_download(
|
||||||
job = _own_job(db, user, job_id)
|
job = _own_job(db, user, job_id)
|
||||||
if job.status in ("queued", "running"):
|
if job.status in ("queued", "running"):
|
||||||
_set_status(db, job, "paused") # a running worker aborts cooperatively via the hook
|
_set_status(db, job, "paused") # a running worker aborts cooperatively via the hook
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
@router.post("/{job_id}/resume")
|
@router.post("/{job_id}/resume")
|
||||||
|
|
@ -466,8 +455,7 @@ def resume_download(
|
||||||
asset.status = "pending"
|
asset.status = "pending"
|
||||||
asset.error = None
|
asset.error = None
|
||||||
_set_status(db, job, "queued")
|
_set_status(db, job, "queued")
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
@router.post("/{job_id}/cancel")
|
@router.post("/{job_id}/cancel")
|
||||||
|
|
@ -479,8 +467,7 @@ def cancel_download(
|
||||||
_release_asset(db, job) # release while the job still counts as holding
|
_release_asset(db, job) # release while the job still counts as holding
|
||||||
job.status = "canceled"
|
job.status = "canceled"
|
||||||
db.commit()
|
db.commit()
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None
|
return _serialize_job(db, job)
|
||||||
return _serialize(job, asset)
|
|
||||||
|
|
||||||
|
|
||||||
@router.delete("/{job_id}")
|
@router.delete("/{job_id}")
|
||||||
|
|
@ -514,11 +501,6 @@ def _release_asset(db: Session, job: DownloadJob) -> None:
|
||||||
|
|
||||||
# --- file download (range-aware, custom display name) --------------------------------------
|
# --- file download (range-aware, custom display name) --------------------------------------
|
||||||
|
|
||||||
def _clean_basename(name: str) -> str:
|
|
||||||
# Emoji/symbol-free but keeps spaces + accents (a readable device filename).
|
|
||||||
return storage.display_filename(name)
|
|
||||||
|
|
||||||
|
|
||||||
def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob:
|
def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob:
|
||||||
job = db.get(DownloadJob, job_id)
|
job = db.get(DownloadJob, job_id)
|
||||||
if job is None:
|
if job is None:
|
||||||
|
|
@ -550,11 +532,9 @@ def download_file(
|
||||||
if path is None:
|
if path is None:
|
||||||
raise HTTPException(status_code=404, detail="File is no longer available.")
|
raise HTTPException(status_code=404, detail="File is no longer available.")
|
||||||
|
|
||||||
ext = asset.container or path.suffix.lstrip(".")
|
filename = storage.download_filename(
|
||||||
base = _clean_basename(job.display_name or asset.title or asset.source_ref)
|
job.display_name or asset.title or asset.source_ref, asset.container, path
|
||||||
if base.lower().endswith(f".{ext.lower()}"):
|
)
|
||||||
base = base[: -(len(ext) + 1)]
|
|
||||||
filename = f"{base}.{ext}"
|
|
||||||
|
|
||||||
asset.last_access_at = datetime.now(timezone.utc)
|
asset.last_access_at = datetime.now(timezone.utc)
|
||||||
db.commit()
|
db.commit()
|
||||||
|
|
|
||||||
|
|
@ -118,11 +118,7 @@ def watch_file(token: str, g: str | None = None, db: Session = Depends(get_db)):
|
||||||
if path is None:
|
if path is None:
|
||||||
raise HTTPException(status_code=404, detail="This video is no longer available.")
|
raise HTTPException(status_code=404, detail="This video is no longer available.")
|
||||||
|
|
||||||
ext = asset.container or path.suffix.lstrip(".")
|
filename = storage.download_filename(asset.title or "video", asset.container, path)
|
||||||
base = storage.display_filename(asset.title or "video")
|
|
||||||
if base.lower().endswith(f".{ext.lower()}"):
|
|
||||||
base = base[: -(len(ext) + 1)]
|
|
||||||
filename = f"{base}.{ext}"
|
|
||||||
disposition = "attachment" if link.allow_download else "inline"
|
disposition = "attachment" if link.allow_download else "inline"
|
||||||
# Starlette's FileResponse honours the Range header (206) for seeking/scrubbing.
|
# Starlette's FileResponse honours the Range header (206) for seeking/scrubbing.
|
||||||
return FileResponse(path, filename=filename, content_disposition_type=disposition)
|
return FileResponse(path, filename=filename, content_disposition_type=disposition)
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue