From 8e4dee1bb6a4eb978e261970ccf6f612696c7b14 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 13 Aug 2026 00:34:42 +0200 Subject: [PATCH 1/5] =?UTF-8?q?refactor(downloads):=20introduce=20SourceAd?= =?UTF-8?q?apter=20=E2=80=94=20a=20thin=20per-source=20Strategy=20over=20y?= =?UTF-8?q?t-dlp?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates source-specific policy (youtube / facebook / reddit / generic) that was scattered across worker.py, backfill.py, service.py, formats.py and storage.py with no shared source identity. app/downloads/adapters.py holds the base (= generic yt-dlp behaviour) + per-source overrides; resolve_adapter(source_kind, source_ref, info) picks one by source_kind, the yt-dlp extractor_key, or the URL host, falling back to generic. yt-dlp stays the engine — the adapter is policy only. Moved into adapters (behaviour-equivalent): - uploader_url derivation (YouTube channel_url / Facebook uploader_id / reddit None) — removes the worker/backfill duplication (build_uploader_url + _should_refetch gone). - the player-client fallback sets + retriable markers + YouTube extractor_args/POT (build_ydl_opts now takes source-agnostic opts plus an `extra` dict from the adapter). - source_url reconstruction (service.source_url delegates to the adapter). Two intentional (approved) behaviour changes: - the player-client fallback is now YouTube-only; non-YouTube sources make a single extraction attempt instead of a pointless double attempt with youtube-only client args. - the .nfo follows the source (adapter.nfo_uniqueid_type) instead of the hardcoded "youtube" that mislabelled every url-source download. Deliberately left generic (documented non-moves): title cleaning (normalize_title is harmless across sources), the classify marker table, thumbnail_to_jpg, the format selectors, and og.py's YouTube-hqdefault og:image preference (a self-contained thumbnail heuristic on the share path). Verified end-to-end on localdev across all four adapters: downloads succeed and uploader_url + the .nfo uniqueid type are correct per source. --- backend/app/downloads/adapters.py | 136 ++++++++++++++++++++++++++++++ backend/app/downloads/backfill.py | 33 +++----- backend/app/downloads/formats.py | 21 ++--- backend/app/downloads/service.py | 8 +- backend/app/downloads/storage.py | 5 +- backend/app/worker.py | 51 +++-------- backend/tests/test_adapters.py | 83 ++++++++++++++++++ backend/tests/test_formats.py | 9 ++ backend/tests/test_storage.py | 10 +++ 9 files changed, 276 insertions(+), 80 deletions(-) create mode 100644 backend/app/downloads/adapters.py create mode 100644 backend/tests/test_adapters.py diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py new file mode 100644 index 00000000..a5392751 --- /dev/null +++ b/backend/app/downloads/adapters.py @@ -0,0 +1,136 @@ +"""Per-source adapters — a thin Strategy over yt-dlp, keyed by the media source (youtube / facebook +/ reddit / generic). + +yt-dlp stays the download engine; an adapter only contributes source-specific POLICY around it. The +base class IS the generic behaviour (yt-dlp does everything, the ref is already a URL), and a +subclass overrides ONLY what its source genuinely needs. This replaces knowledge that used to be +scattered across worker.py / backfill.py / service.py with no shared source identity. + +Adapters are stateless singletons; get one with `resolve_adapter(source_kind, source_ref, info)`. +Resolution works both before extraction (from source_kind / URL host) and after (from the yt-dlp +`extractor_key` in the info dict), so a non-YouTube "url" job still lands on the right adapter. +""" +from urllib.parse import urlparse + +from app.config import settings + + +class SourceAdapter: + """Generic source: yt-dlp handles everything and the ref is already a fetchable URL.""" + + key = "generic" + + # --- extraction (worker._extract_with_fallback / backfill) --------------------------------- + def client_sets(self) -> list: + """Player-client sets to attempt, in order. A generic source needs no client override, so a + single default attempt (`[None]`) — the per-client fallback is a YouTube-only concept.""" + return [None] + + def ydl_opts_extra(self, clients) -> dict: + """yt-dlp options this source contributes (merged into `build_ydl_opts`). `clients` is the + current entry from `client_sets()`. Generic: nothing.""" + return {} + + def retriable(self, error_text: str) -> bool: + """Retry the extraction with the NEXT client set on this error? Generic: no (single attempt).""" + return False + + # --- metadata ----------------------------------------------------------------------------- + def uploader_url(self, info: dict) -> str | None: + """The channel/uploader page URL. Generic: whatever yt-dlp exposes directly (YouTube gives + `channel_url`), else None (reddit / a raw media URL have no derivable channel page).""" + return info.get("channel_url") or info.get("uploader_url") + + def wants_metadata_backfill(self) -> bool: + """Whether the one-off metadata backfill should re-fetch this source. Generic: no.""" + return False + + # --- source ref ↔ URL --------------------------------------------------------------------- + def source_url(self, source_ref: str) -> str: + """A fetchable URL for `source_ref`. Generic: the ref is already a URL.""" + return source_ref + + def nfo_uniqueid_type(self) -> str: + """The `` written into the Kodi/Plex `.nfo` sidecar.""" + return self.key + + +class YouTubeAdapter(SourceAdapter): + key = "youtube" + # Errors worth retrying with a different player-client set (YouTube per-client flakiness). + _RETRIABLE = ( + "not a bot", + "DRM protected", + "Requested format is not available", + "Only images are available", + "Sign in to confirm", + ) + + def client_sets(self) -> list: + return [ + c for c in (settings.player_client_list, settings.player_client_fallback_list) if c + ] or [None] + + def ydl_opts_extra(self, clients) -> dict: + # == --extractor-args "youtube:player_client=…;youtubepot-bgutilhttp:base_url=…" (list values). + ea: dict = {} + if clients: + ea["youtube"] = {"player_client": clients} + if settings.download_pot_base_url: + ea["youtubepot-bgutilhttp"] = {"base_url": [settings.download_pot_base_url]} + return {"extractor_args": ea} if ea else {} + + def retriable(self, error_text: str) -> bool: + return any(m in error_text for m in self._RETRIABLE) + + def wants_metadata_backfill(self) -> bool: + return True + + def source_url(self, source_ref: str) -> str: + return f"https://www.youtube.com/watch?v={source_ref}" + + +class FacebookAdapter(SourceAdapter): + key = "facebook" + + def uploader_url(self, info: dict) -> str | None: + # Facebook gives neither channel_url nor uploader_url, but exposes a numeric `uploader_id` + # (the page id) that resolves at facebook.com/. + base = super().uploader_url(info) + if base: + return base + uid = info.get("uploader_id") + return f"https://www.facebook.com/{uid}" if uid else None + + def wants_metadata_backfill(self) -> bool: + return True + + +class RedditAdapter(SourceAdapter): + key = "reddit" + + +_YOUTUBE = YouTubeAdapter() +_FACEBOOK = FacebookAdapter() +_REDDIT = RedditAdapter() +_GENERIC = SourceAdapter() + + +def resolve_adapter( + source_kind: str | None, source_ref: str | None = None, info: dict | None = None +) -> SourceAdapter: + """Pick the adapter for a job. Matches on `source_kind` first, then the yt-dlp `extractor_key` + from `info` (post-extraction), then the URL host in `source_ref` (pre-extraction). Falls back to + the generic adapter — so behaviour is unchanged for any source without a dedicated one.""" + tag = "" + if info: + extractor = info.get("extractor_key") or info.get("extractor") or "" + tag = f"{extractor} {info.get('webpage_url') or ''}".lower() + host = urlparse(source_ref).netloc.lower() if source_ref else "" + if source_kind == "youtube" or "youtube" in tag or "youtu.be" in host or "youtube.com" in host: + return _YOUTUBE + if "facebook" in tag or "facebook.com" in host or "fb.watch" in host: + return _FACEBOOK + if "reddit" in tag or "reddit.com" in host: + return _REDDIT + return _GENERIC diff --git a/backend/app/downloads/backfill.py b/backend/app/downloads/backfill.py index 3db13bf6..49fd5462 100644 --- a/backend/app/downloads/backfill.py +++ b/backend/app/downloads/backfill.py @@ -11,40 +11,26 @@ Run once per environment: docker exec siftlode-worker-1 python -c "from app.downloads import backfill; print(backfill.run())" """ import logging -from urllib.parse import urlparse import yt_dlp from sqlalchemy import select from app.config import settings from app.db import SessionLocal +from app.downloads import adapters from app.downloads import edit as editmod from app.downloads import formats, service from app.models import MediaAsset -from app.worker import build_uploader_url log = logging.getLogger("siftlode.backfill") -def _should_refetch(asset: MediaAsset) -> bool: - """Only YouTube + Facebook sources have a derivable channel URL worth a network round-trip.""" - if asset.source_kind == "youtube": - return True - if asset.source_kind == "url": - host = urlparse(asset.source_ref).netloc.lower() - return "facebook.com" in host or "fb.watch" in host - return False - - -def _extract_meta(url: str) -> dict | None: - """Metadata-only yt-dlp extraction, trying the same client sets as the worker (YouTube needs the - POT/player-client strategy even just to extract).""" - client_sets = [ - c for c in (settings.player_client_list, settings.player_client_fallback_list) if c - ] or [None] - for clients in client_sets: +def _extract_meta(url: str, adapter: adapters.SourceAdapter) -> dict | None: + """Metadata-only yt-dlp extraction, using the source adapter's client sets + options (YouTube + needs the POT/player-client strategy even just to extract).""" + for clients in adapter.client_sets(): opts = formats.build_ydl_opts( - {}, "%(id)s.%(ext)s", lambda d: None, settings.download_pot_base_url, clients + {}, "%(id)s.%(ext)s", lambda d: None, extra=adapter.ydl_opts_extra(clients) ) opts["skip_download"] = True try: @@ -70,10 +56,11 @@ def run() -> dict: .all() ) for a in assets: - if a.uploader_url is None and _should_refetch(a): + adapter = adapters.resolve_adapter(a.source_kind, a.source_ref) + if a.uploader_url is None and adapter.wants_metadata_backfill(): url = a.source_webpage_url or service.source_url(a.source_kind, a.source_ref) - info = _extract_meta(url) - uu = build_uploader_url(info) if info else None + info = _extract_meta(url, adapter) + uu = adapter.uploader_url(info) if info else None if uu: a.uploader_url = uu filled_url += 1 diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index 662c85d9..6ffbd46b 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -96,13 +96,13 @@ def build_ydl_opts( spec: dict, outtmpl: str, progress_hook, - pot_base_url: str | None = None, - player_clients: list[str] | None = None, + extra: dict | None = None, ) -> dict: """Assemble yt-dlp options for a single download to `outtmpl` (a full path template). - `pot_base_url` points the bgutil PO-token provider plugin at the sidecar server; - `player_clients` picks which YouTube clients to use (POT-capable ones like web use the token).""" + `extra` is source-specific options contributed by the SourceAdapter (e.g. YouTube's + `extractor_args` with the player-client set + bgutil PO-token base_url); it is merged onto the + generic options below. Everything here is source-agnostic.""" s = normalize(spec) h = s["max_height"] opts: dict = { @@ -127,16 +127,9 @@ def build_ydl_opts( "socket_timeout": 30, } - # Extractor args: which YouTube clients to use + where the bgutil PO-token sidecar lives - # (== --extractor-args "youtube:player_client=…;youtubepot-bgutilhttp:base_url=…"). Values - # are lists in the Python API. - ea: dict = {} - if player_clients: - ea["youtube"] = {"player_client": player_clients} - if pot_base_url: - ea["youtubepot-bgutilhttp"] = {"base_url": [pot_base_url]} - if ea: - opts["extractor_args"] = ea + # Source-specific options from the adapter (e.g. YouTube's extractor_args: player_client + POT). + if extra: + opts.update(extra) if s["mode"] == "a": opts["format"] = "bestaudio/best" diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index d6fe7a17..5d79a4a6 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -13,6 +13,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from app.config import settings +from app.downloads import adapters from app.downloads import edit as editmod from app.downloads import formats, quota, storage from app.models import Channel, DownloadJob, MediaAsset, Video @@ -27,10 +28,9 @@ class EditError(Exception): def source_url(source_kind: str, source_ref: str) -> str: - """The URL yt-dlp should fetch. YouTube stores the 11-char id; ad-hoc stores the URL.""" - if source_kind == "youtube": - return f"https://www.youtube.com/watch?v={source_ref}" - return source_ref + """The URL yt-dlp should fetch. YouTube stores the 11-char id (the adapter reconstructs the watch + URL); ad-hoc sources store the URL verbatim.""" + return adapters.resolve_adapter(source_kind, source_ref).source_url(source_ref) def reference_url(job, asset) -> str | None: diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index 1b3b7099..cb405f3b 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -59,6 +59,9 @@ class MediaMeta: title: str uploader: str upload_date: date | None + # The .nfo — the source's scraper type (youtube / facebook / …), set from + # the SourceAdapter. Defaults to "youtube" only as a safety net for un-updated callers. + uniqueid_type: str = "youtube" duration_s: int | None = None description: str | None = None thumbnail_url: str | None = None @@ -170,7 +173,7 @@ def _nfo_xml(meta: MediaMeta) -> str: f" {aired}", f" {year}", f" {escape(meta.description or '')}", - f" {escape(meta.video_id)}", + f" {escape(meta.video_id)}", ] if meta.duration_s: parts.append(f" {round(meta.duration_s / 60)}") diff --git a/backend/app/worker.py b/backend/app/worker.py index 7bd3f764..f2f4175a 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -35,7 +35,7 @@ from app.config import settings from app.db import SessionLocal from app.downloads import edit as editmod from app.downloads import errors as dlerrors -from app.downloads import formats, quota, service, storage +from app.downloads import adapters, formats, quota, service, storage from app.models import DownloadJob, MediaAsset from app.notifications import create_notification from app.titles import normalize_title @@ -475,21 +475,6 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: db.commit() -def build_uploader_url(info: dict) -> str | None: - """The channel/uploader page URL for the auto-clickable channel line. YouTube gives a - `channel_url`/`uploader_url` directly; Facebook gives neither but exposes a numeric - `uploader_id` (the page id) that resolves at facebook.com/. Returns None for sources with - no derivable channel page (e.g. reddit / a raw media URL).""" - url = info.get("channel_url") or info.get("uploader_url") - if url: - return url - uid = info.get("uploader_id") - tag = f"{info.get('extractor_key') or info.get('extractor') or ''} {info.get('webpage_url') or ''}".lower() - if uid and "facebook" in tag: - return f"https://www.facebook.com/{uid}" - return None - - def _fill_asset_meta_early(asset_id: int, info: dict) -> None: """Populate an asset's display metadata from yt-dlp's info_dict the first time it's seen (only if not already filled at enqueue from our catalog) — so an ad-hoc URL's row shows a @@ -503,7 +488,7 @@ def _fill_asset_meta_early(asset_id: int, info: dict) -> None: # the final title with its own source_ref fallback. asset.title = normalize_title(info.get("title")) or info.get("id") asset.uploader = info.get("uploader") or info.get("channel") - asset.uploader_url = build_uploader_url(info) + asset.uploader_url = adapters.resolve_adapter(None, None, info).uploader_url(info) asset.thumbnail_url = info.get("thumbnail") asset.duration_s = int(info["duration"]) if info.get("duration") else None asset.upload_date = _parse_upload_date(info.get("upload_date")) @@ -713,28 +698,17 @@ def _media_info(info: dict) -> dict: return {**info, **chosen} -# Errors worth retrying with a different player-client set (YouTube per-client flakiness). -_RETRIABLE_MARKERS = ( - "not a bot", - "DRM protected", - "Requested format is not available", - "Only images are available", - "Sign in to confirm", -) - - -def _extract_with_fallback(job_id: int, asset_id: int, url: str, spec: dict, staging: Path): - """Download via yt-dlp, trying the primary player clients then the POT-backed fallback set - on a bot/DRM/format failure. Returns the info dict of the successful attempt.""" - client_sets = [ - c for c in (settings.player_client_list, settings.player_client_fallback_list) if c - ] or [None] +def _extract_with_fallback(job_id, asset_id, url, spec, staging, adapter): + """Download via yt-dlp using the source `adapter`'s client sets + options. For YouTube the adapter + supplies the primary player-client set then a POT-backed fallback (retried on a bot/DRM/format + error); other sources make a single attempt. Returns the info dict of the successful attempt.""" + client_sets = adapter.client_sets() outtmpl = str(staging / "%(id)s.%(ext)s") last_exc: Exception | None = None for i, clients in enumerate(client_sets): opts = formats.build_ydl_opts( spec, outtmpl, _make_progress_hook(job_id, asset_id), - settings.download_pot_base_url, clients, + extra=adapter.ydl_opts_extra(clients), ) opts["postprocessor_hooks"] = [_make_pp_hook(job_id)] _touch_job(job_id) # stall-watchdog heartbeat: extraction writes no progress, so a slow / @@ -746,8 +720,7 @@ def _extract_with_fallback(job_id: int, asset_id: int, url: str, spec: dict, sta return ydl.extract_info(url, download=True) except yt_dlp.utils.DownloadError as exc: last_exc = exc - retriable = any(m in str(exc) for m in _RETRIABLE_MARKERS) - if i < len(client_sets) - 1 and retriable: + if i < len(client_sets) - 1 and adapter.retriable(str(exc)): log.info("job %s: clients %s failed (%s); retrying with fallback", job_id, clients, str(exc)[:70]) for p in list(staging.iterdir()): # drop partials before the retry try: @@ -815,8 +788,9 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe staging.mkdir(parents=True, exist_ok=True) _set_job(job_id, phase="downloading", progress=0) + adapter = adapters.resolve_adapter(source_kind, source_ref) url = service.source_url(source_kind, source_ref) - info = _media_info(_extract_with_fallback(job_id, asset_id, url, spec, staging)) + info = _media_info(_extract_with_fallback(job_id, asset_id, url, spec, staging, adapter)) produced = _produced_file(info, staging) produced = _ensure_browser_playable(job_id, produced, spec, info) @@ -826,6 +800,7 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe title=normalize_title(info.get("title")) or source_ref, uploader=info.get("uploader") or info.get("channel") or "Unknown Channel", upload_date=_parse_upload_date(info.get("upload_date")), + uniqueid_type=adapter.nfo_uniqueid_type(), duration_s=int(info["duration"]) if info.get("duration") else None, description=info.get("description"), thumbnail_url=info.get("thumbnail"), @@ -859,7 +834,7 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe asset.acodec = info.get("acodec") if info.get("acodec") not in (None, "none") else None asset.title = meta.title asset.uploader = meta.uploader - asset.uploader_url = build_uploader_url(info) + asset.uploader_url = adapters.resolve_adapter(source_kind, source_ref, info).uploader_url(info) asset.upload_date = meta.upload_date asset.thumbnail_url = meta.thumbnail_url # Record a self-hosted poster for every download: it's the reliable link-preview diff --git a/backend/tests/test_adapters.py b/backend/tests/test_adapters.py new file mode 100644 index 00000000..ead15401 --- /dev/null +++ b/backend/tests/test_adapters.py @@ -0,0 +1,83 @@ +"""Per-source adapter selection + the source-specific policy each one carries. These lock the +behaviour that used to live inline in worker.build_uploader_url / backfill._should_refetch.""" +from app.downloads.adapters import ( + FacebookAdapter, + RedditAdapter, + SourceAdapter, + YouTubeAdapter, + resolve_adapter, +) + + +def test_resolve_by_source_kind_and_host_and_info(): + assert resolve_adapter("youtube", "abc123").key == "youtube" + assert resolve_adapter("url", "https://www.youtube.com/watch?v=x").key == "youtube" + assert resolve_adapter("url", "https://youtu.be/x").key == "youtube" + assert resolve_adapter("url", "https://www.facebook.com/reel/123/").key == "facebook" + assert resolve_adapter("url", "https://fb.watch/abc/").key == "facebook" + assert resolve_adapter("url", "https://www.reddit.com/r/x/comments/y/z").key == "reddit" + assert resolve_adapter("url", "https://example.com/video.mp4").key == "generic" + # After extraction the info dict's extractor decides even for a bare/redirected url. + assert resolve_adapter("url", None, {"extractor_key": "facebook"}).key == "facebook" + assert resolve_adapter("url", None, {"extractor_key": "Reddit"}).key == "reddit" + + +def test_host_match_is_netloc_not_a_loose_substring(): + # A path that merely CONTAINS "facebook.com" must not select the Facebook adapter. + assert resolve_adapter("url", "https://example.com/facebook.com-mirror").key == "generic" + + +def test_uploader_url_youtube_uses_channel_url(): + a = YouTubeAdapter() + assert a.uploader_url({"channel_url": "https://youtube.com/@chan"}) == "https://youtube.com/@chan" + assert a.uploader_url({"uploader_url": "https://youtube.com/user/u"}) == "https://youtube.com/user/u" + assert a.uploader_url({}) is None + + +def test_uploader_url_facebook_falls_back_to_uploader_id(): + a = FacebookAdapter() + assert a.uploader_url({"uploader_id": "12345"}) == "https://www.facebook.com/12345" + # A direct channel_url still wins over the id fallback. + assert a.uploader_url({"channel_url": "https://facebook.com/pageX", "uploader_id": "9"}) == ( + "https://facebook.com/pageX" + ) + assert a.uploader_url({}) is None + + +def test_uploader_url_reddit_and_generic_have_no_channel(): + assert RedditAdapter().uploader_url({"uploader_id": "99"}) is None # not facebook → no fallback + assert SourceAdapter().uploader_url({}) is None + + +def test_generic_source_url_is_passthrough_youtube_reconstructs(): + assert SourceAdapter().source_url("https://x/y") == "https://x/y" + assert YouTubeAdapter().source_url("VIDID12345_") == "https://www.youtube.com/watch?v=VIDID12345_" + + +def test_client_sets_and_retriable_are_youtube_only(): + # Generic sources make a single attempt and never retry on the YouTube per-client markers. + assert SourceAdapter().client_sets() == [None] + assert SourceAdapter().retriable("Sign in to confirm you're not a bot") is False + assert YouTubeAdapter().retriable("Sign in to confirm you're not a bot") is True + assert YouTubeAdapter().retriable("some unrelated network blip") is False + + +def test_youtube_ydl_opts_extra_carries_player_client_and_pot(): + ea = YouTubeAdapter().ydl_opts_extra(["android_vr"])["extractor_args"] + assert ea["youtube"]["player_client"] == ["android_vr"] + assert "youtubepot-bgutilhttp" in ea # the bgutil PO-token base_url is wired + # A client-less attempt still wires POT but no player_client key. + assert "youtube" not in YouTubeAdapter().ydl_opts_extra(None)["extractor_args"] + assert SourceAdapter().ydl_opts_extra(["android_vr"]) == {} # generic contributes nothing + + +def test_wants_metadata_backfill_matches_old_should_refetch(): + assert YouTubeAdapter().wants_metadata_backfill() is True + assert FacebookAdapter().wants_metadata_backfill() is True + assert RedditAdapter().wants_metadata_backfill() is False + assert SourceAdapter().wants_metadata_backfill() is False + + +def test_nfo_uniqueid_type_is_the_adapter_key(): + assert YouTubeAdapter().nfo_uniqueid_type() == "youtube" + assert SourceAdapter().nfo_uniqueid_type() == "generic" diff --git a/backend/tests/test_formats.py b/backend/tests/test_formats.py index 44db9d2b..f6c214a3 100644 --- a/backend/tests/test_formats.py +++ b/backend/tests/test_formats.py @@ -9,6 +9,15 @@ def _opts(spec): return formats.build_ydl_opts(spec, "/tmp/%(id)s.%(ext)s", lambda d: None) +def test_adapter_extra_opts_are_merged_in(): + # The SourceAdapter contributes source-specific opts (e.g. YouTube's extractor_args) via `extra`. + extra = {"extractor_args": {"youtube": {"player_client": ["web"]}}} + opts = formats.build_ydl_opts({}, "/tmp/%(id)s.%(ext)s", lambda d: None, extra=extra) + assert opts["extractor_args"]["youtube"]["player_client"] == ["web"] + # No extra → no extractor_args (a generic source contributes nothing). + assert "extractor_args" not in formats.build_ydl_opts({}, "/tmp/x", lambda d: None) + + def test_mp4_default_selects_avc1_with_progressive_fallback(): fmt = _opts({"mode": "av", "container": "mp4"})["format"] # avc1 video-only first, then the avc1 progressive, then an unrestricted fallback. diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index 71acb60c..29b7fccb 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -137,3 +137,13 @@ class TestSafeAbsPath: def test_returns_none_for_a_nonexistent_file_inside_the_root(self, tmp_path: Path): assert safe_abs_path(str(tmp_path), "does/not/exist.mp4") is None + + +def test_nfo_uniqueid_type_reflects_the_source(): + # The .nfo now follows the source (via SourceAdapter.nfo_uniqueid_type), + # instead of the old hardcoded "youtube" that mislabelled every url-source download. + from app.downloads.storage import _nfo_xml + + base = dict(video_id="v1", title="t", uploader="c", upload_date=None) + assert ' Date: Thu, 13 Aug 2026 00:47:51 +0200 Subject: [PATCH 2/5] fix(downloads): keep sidecar basenames intact when the title contains a dot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_sidecars / delete_asset_files computed `base = media.with_suffix("")` and then `base.with_suffix(".nfo")`. Stripping the real `.mp4` first leaves a stem whose own last dot may be inside the title (e.g. "Sonka Házilag."), which `with_suffix` then treats as the extension and clobbers — writing the .nfo/.jpg under a TRUNCATED basename that no longer matches the media, so Plex/Kodi never associate them (and delete_asset_files targets the wrong name, leaking orphans). Swap the extension on the FULL media path via a shared `_sidecar` helper instead. Pre-existing bug (predates the source-adapters work); found during its E2E and folded in here. Verified end-to-end: a dotted-title Facebook reel now writes a single shared basename. --- backend/app/downloads/storage.py | 16 +++++++++++----- backend/tests/test_storage.py | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index cb405f3b..ec21cf3e 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -181,16 +181,23 @@ def _nfo_xml(meta: MediaMeta) -> str: return "\n".join(parts) +def _sidecar(media: Path, ext: str) -> Path: + """The sibling sidecar path (`.nfo` / `.jpg`) for a media file. Swaps the extension on the FULL + media path — NOT via `media.with_suffix("")` first: stripping the real extension leaves a stem + whose OWN last dot may be inside the title (e.g. "Sonka Házilag."), which `with_suffix` would + then read as the extension and clobber, orphaning the sidecar under a truncated name.""" + return media.with_suffix(ext) + + def write_sidecars( download_root: str, rel: str, meta: MediaMeta, thumb_src: Path | None ) -> bool: """Write `.nfo` + `.jpg` next to the media file. Returns True if written.""" media = abs_path(download_root, rel) - base = media.with_suffix("") try: - base.with_suffix(".nfo").write_text(_nfo_xml(meta), encoding="utf-8") + _sidecar(media, ".nfo").write_text(_nfo_xml(meta), encoding="utf-8") if thumb_src and thumb_src.exists(): - shutil.copyfile(str(thumb_src), str(base.with_suffix(".jpg"))) + shutil.copyfile(str(thumb_src), str(_sidecar(media, ".jpg"))) return True except OSError: return False @@ -199,8 +206,7 @@ def write_sidecars( def delete_asset_files(download_root: str, rel: str) -> None: """Remove the media file and its sidecars; prune now-empty parent dirs (best effort).""" media = abs_path(download_root, rel) - base = media.with_suffix("") - for p in (media, base.with_suffix(".nfo"), base.with_suffix(".jpg")): + for p in (media, _sidecar(media, ".nfo"), _sidecar(media, ".jpg")): try: p.unlink() except OSError: diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index 29b7fccb..d467e9a4 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -147,3 +147,26 @@ def test_nfo_uniqueid_type_reflects_the_source(): base = dict(video_id="v1", title="t", uploader="c", upload_date=None) assert ' Date: Thu, 13 Aug 2026 01:09:38 +0200 Subject: [PATCH 3/5] fix(downloads): tighten SourceAdapter resolution + neutral nfo default (review) - resolve_adapter: identify from the yt-dlp extractor_key only, not the full webpage_url (a sibling source's name in a URL path no longer misresolves); add youtube-nocookie.com to the YouTube host match so nocookie embeds keep the POT/player-client extraction strategy. - _download: re-resolve the adapter with the post-extraction info dict so the .nfo uniqueid type and uploader_url agree on one source (dedups the inline resolve too). - MediaMeta.uniqueid_type defaults to source-neutral "generic" instead of "youtube", so an un-updated caller can never falsely label a non-YouTube download as youtube. - Tests: lock extractor-key-only resolution + nocookie host; default nfo type. --- backend/app/downloads/adapters.py | 15 ++++++++++----- backend/app/downloads/storage.py | 5 +++-- backend/app/worker.py | 5 ++++- backend/tests/test_adapters.py | 13 +++++++++++++ backend/tests/test_storage.py | 4 +++- 5 files changed, 33 insertions(+), 9 deletions(-) diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index a5392751..dff1ef6a 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -122,12 +122,17 @@ def resolve_adapter( """Pick the adapter for a job. Matches on `source_kind` first, then the yt-dlp `extractor_key` from `info` (post-extraction), then the URL host in `source_ref` (pre-extraction). Falls back to the generic adapter — so behaviour is unchanged for any source without a dedicated one.""" - tag = "" - if info: - extractor = info.get("extractor_key") or info.get("extractor") or "" - tag = f"{extractor} {info.get('webpage_url') or ''}".lower() + # Identify from the yt-dlp extractor key alone — NOT the webpage_url, whose path could contain a + # sibling source's name (e.g. a reddit post slug '.../youtube_is_dead') and cause a misresolve. + tag = (info.get("extractor_key") or info.get("extractor") or "").lower() if info else "" host = urlparse(source_ref).netloc.lower() if source_ref else "" - if source_kind == "youtube" or "youtube" in tag or "youtu.be" in host or "youtube.com" in host: + if ( + source_kind == "youtube" + or "youtube" in tag + or "youtu.be" in host + or "youtube.com" in host + or "youtube-nocookie.com" in host + ): return _YOUTUBE if "facebook" in tag or "facebook.com" in host or "fb.watch" in host: return _FACEBOOK diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index ec21cf3e..a8dc02fb 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -60,8 +60,9 @@ class MediaMeta: uploader: str upload_date: date | None # The .nfo — the source's scraper type (youtube / facebook / …), set from - # the SourceAdapter. Defaults to "youtube" only as a safety net for un-updated callers. - uniqueid_type: str = "youtube" + # the SourceAdapter. Defaults to the source-neutral "generic" so an un-updated caller is at worst + # uninformative, never actively mislabelling a non-YouTube download as "youtube". + uniqueid_type: str = "generic" duration_s: int | None = None description: str | None = None thumbnail_url: str | None = None diff --git a/backend/app/worker.py b/backend/app/worker.py index f2f4175a..96c56956 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -791,6 +791,9 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe adapter = adapters.resolve_adapter(source_kind, source_ref) url = service.source_url(source_kind, source_ref) info = _media_info(_extract_with_fallback(job_id, asset_id, url, spec, staging, adapter)) + # Re-resolve now that yt-dlp's info reveals the true source (a bare/redirected url only + # identifies post-extraction) so the .nfo uniqueid type + uploader_url agree on one source. + adapter = adapters.resolve_adapter(source_kind, source_ref, info) produced = _produced_file(info, staging) produced = _ensure_browser_playable(job_id, produced, spec, info) @@ -834,7 +837,7 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe asset.acodec = info.get("acodec") if info.get("acodec") not in (None, "none") else None asset.title = meta.title asset.uploader = meta.uploader - asset.uploader_url = adapters.resolve_adapter(source_kind, source_ref, info).uploader_url(info) + asset.uploader_url = adapter.uploader_url(info) asset.upload_date = meta.upload_date asset.thumbnail_url = meta.thumbnail_url # Record a self-hosted poster for every download: it's the reliable link-preview diff --git a/backend/tests/test_adapters.py b/backend/tests/test_adapters.py index ead15401..d52233c3 100644 --- a/backend/tests/test_adapters.py +++ b/backend/tests/test_adapters.py @@ -27,6 +27,19 @@ def test_host_match_is_netloc_not_a_loose_substring(): assert resolve_adapter("url", "https://example.com/facebook.com-mirror").key == "generic" +def test_info_match_is_extractor_key_not_the_webpage_url(): + # The webpage_url path may carry a SIBLING source's name (a reddit slug about youtube); only the + # yt-dlp extractor_key identifies the source, so this must resolve reddit, not youtube. + info = {"extractor_key": "Reddit", "webpage_url": "https://reddit.com/r/x/comments/y/youtube_is_dead"} + assert resolve_adapter("url", None, info).key == "reddit" + + +def test_youtube_nocookie_host_still_gets_the_youtube_adapter(): + # An embed on youtube-nocookie.com is YouTube content and needs the POT/player-client strategy, + # even though its host does not contain the substring "youtube.com". + assert resolve_adapter("url", "https://www.youtube-nocookie.com/embed/abc123").key == "youtube" + + def test_uploader_url_youtube_uses_channel_url(): a = YouTubeAdapter() assert a.uploader_url({"channel_url": "https://youtube.com/@chan"}) == "https://youtube.com/@chan" diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index d467e9a4..addcb473 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -145,8 +145,10 @@ def test_nfo_uniqueid_type_reflects_the_source(): from app.downloads.storage import _nfo_xml base = dict(video_id="v1", title="t", uploader="c", upload_date=None) - assert ' Date: Thu, 13 Aug 2026 01:18:25 +0200 Subject: [PATCH 4/5] fix(downloads): netloc-scoped adapter host match (review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_adapter matched sources by loose substring over the netloc, so a look-alike host ('myyoutube.com', 'youtube.com.evil') misresolved. Match the host exactly or as a subdomain via a _host_is() helper, and identify from the netloc of BOTH source_ref and yt-dlp's webpage_url — the latter by host only, never a substring over its path. This also restores a host signal to the early-fill path (resolve_adapter(None, None, info)), which has no source_ref, so a Facebook asset's uploader_url no longer depends on extractor_key being present in the progress-hook info_dict. Tests: look-alike hosts stay generic while real subdomains resolve; a webpage_url netloc identifies the source when extractor_key is absent. --- backend/app/downloads/adapters.py | 40 +++++++++++++++++++------------ backend/tests/test_adapters.py | 19 +++++++++++++++ 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index dff1ef6a..24282bf3 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -116,26 +116,36 @@ _REDDIT = RedditAdapter() _GENERIC = SourceAdapter() +def _host_is(host: str, *domains: str) -> bool: + """True if `host` is exactly one of `domains` or a subdomain of it — a netloc-scoped match, NOT a + loose substring, so a look-alike host ('myyoutube.com', 'youtube.com.evil') never matches.""" + return any(host == d or host.endswith("." + d) for d in domains) + + def resolve_adapter( source_kind: str | None, source_ref: str | None = None, info: dict | None = None ) -> SourceAdapter: - """Pick the adapter for a job. Matches on `source_kind` first, then the yt-dlp `extractor_key` - from `info` (post-extraction), then the URL host in `source_ref` (pre-extraction). Falls back to - the generic adapter — so behaviour is unchanged for any source without a dedicated one.""" - # Identify from the yt-dlp extractor key alone — NOT the webpage_url, whose path could contain a - # sibling source's name (e.g. a reddit post slug '.../youtube_is_dead') and cause a misresolve. - tag = (info.get("extractor_key") or info.get("extractor") or "").lower() if info else "" - host = urlparse(source_ref).netloc.lower() if source_ref else "" - if ( - source_kind == "youtube" - or "youtube" in tag - or "youtu.be" in host - or "youtube.com" in host - or "youtube-nocookie.com" in host + """Pick the adapter for a job. Matches on `source_kind`, the yt-dlp `extractor_key` from `info` + (post-extraction, authoritative), and the netloc of any URL we hold — the `source_ref` + (pre-extraction) and yt-dlp's `webpage_url` (post-extraction). Host matching is netloc-scoped, and + the webpage_url is used by HOST only (never a substring over its path, which could carry a sibling + source's name). Falls back to the generic adapter — behaviour unchanged for any un-dedicated source.""" + extractor = (info.get("extractor_key") or info.get("extractor") or "").lower() if info else "" + hosts = [] + if source_ref: + hosts.append(urlparse(source_ref).netloc.lower()) + if info and info.get("webpage_url"): + hosts.append(urlparse(info["webpage_url"]).netloc.lower()) + + def host_is(*domains: str) -> bool: + return any(_host_is(h, *domains) for h in hosts) + + if source_kind == "youtube" or "youtube" in extractor or host_is( + "youtube.com", "youtu.be", "youtube-nocookie.com" ): return _YOUTUBE - if "facebook" in tag or "facebook.com" in host or "fb.watch" in host: + if "facebook" in extractor or host_is("facebook.com", "fb.watch"): return _FACEBOOK - if "reddit" in tag or "reddit.com" in host: + if "reddit" in extractor or host_is("reddit.com"): return _REDDIT return _GENERIC diff --git a/backend/tests/test_adapters.py b/backend/tests/test_adapters.py index d52233c3..5b9ce684 100644 --- a/backend/tests/test_adapters.py +++ b/backend/tests/test_adapters.py @@ -40,6 +40,25 @@ def test_youtube_nocookie_host_still_gets_the_youtube_adapter(): assert resolve_adapter("url", "https://www.youtube-nocookie.com/embed/abc123").key == "youtube" +def test_lookalike_hosts_do_not_misresolve_but_subdomains_do(): + # Netloc-scoped match: a look-alike host that merely CONTAINS the domain string is NOT the source. + assert resolve_adapter("url", "https://myyoutube.com/watch?v=x").key == "generic" + assert resolve_adapter("url", "https://youtube.com.evil.example/x").key == "generic" + assert resolve_adapter("url", "https://notreddit.com/r/x").key == "generic" + # …but real subdomains still resolve. + assert resolve_adapter("url", "https://m.youtube.com/watch?v=x").key == "youtube" + assert resolve_adapter("url", "https://music.youtube.com/watch?v=x").key == "youtube" + + +def test_webpage_url_netloc_identifies_source_when_extractor_key_absent(): + # The early-fill path passes info with no source_ref; the webpage_url netloc still resolves the + # source even if extractor_key is missing from the progress-hook info_dict. + assert resolve_adapter(None, None, {"webpage_url": "https://www.facebook.com/reel/123"}).key == "facebook" + # A sibling source's name in the webpage_url PATH must not misresolve (host, not substring). + info = {"extractor_key": "Reddit", "webpage_url": "https://www.reddit.com/r/x/comments/y/youtube"} + assert resolve_adapter(None, None, info).key == "reddit" + + def test_uploader_url_youtube_uses_channel_url(): a = YouTubeAdapter() assert a.uploader_url({"channel_url": "https://youtube.com/@chan"}) == "https://youtube.com/@chan" From ee8aa72abe13bd04efefcd8b9a8bd4e59a179d6d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 13 Aug 2026 01:21:22 +0200 Subject: [PATCH 5/5] =?UTF-8?q?release:=200.63.1=20=E2=80=94=20SourceAdapt?= =?UTF-8?q?er=20refactor=20+=20sidecar/nfo=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- VERSION | 2 +- frontend/src/lib/releaseNotes.ts | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/VERSION b/VERSION index c149bbff..1f6db1c8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.63.0 \ No newline at end of file +0.63.1 \ No newline at end of file diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index 3a6673fc..c7bc720c 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -14,6 +14,17 @@ export interface ReleaseEntry { } export const RELEASE_NOTES: ReleaseEntry[] = [ + { + version: "0.63.1", + date: "2026-08-13", + fixes: [ + "Downloaded videos whose title ends in a period (some Facebook reels, for example) now save their Plex/Kodi info and poster files right next to the video so Plex picks them up — previously those files landed under a slightly different name and were missed.", + "The info file for a non-YouTube download (Facebook, Reddit, a direct link) now records the correct source instead of always labelling it as YouTube.", + ], + chores: [ + "Internal refactor: consolidated the per-source download behaviour (YouTube / Facebook / Reddit / generic) behind one small adapter layer. No change to how downloading looks or works.", + ], + }, { version: "0.63.0", date: "2026-08-12",