From 246ff528f5f963df030fb2b241c6a9ecfde4df24 Mon Sep 17 00:00:00 2001 From: imSp4rky Date: Tue, 9 Jun 2026 18:17:29 -0600 Subject: [PATCH] fix(dl): apply per-service dl config overrides for all options services..dl values only applied when the key was also set in the global dl: section (equality check against config.dl missed Click's declared defaults). Gate on Click's ParameterSource instead: CLI/env > service dl > global dl > defaults, converted via each option's own type. - record parameter sources on serve's hand-built context so client values are never clobbered - accept range/list as natural keys for range_/list_ - harden QualityList (YAML int) and SlowDelayRange (YAML bool) converts --- docs/DOWNLOAD_CONFIG.md | 20 +-- docs/SERVICE_CONFIG.md | 13 +- tests/core/test_click_types.py | 28 ++++- tests/core/test_service_dl_overrides.py | 154 ++++++++++++++++++++++++ unshackle/commands/dl.py | 83 ++++++++----- unshackle/core/api/download_manager.py | 8 ++ unshackle/core/utils/click_types.py | 15 ++- unshackle/unshackle-example.yaml | 28 ++++- 8 files changed, 302 insertions(+), 47 deletions(-) create mode 100644 tests/core/test_service_dl_overrides.py diff --git a/docs/DOWNLOAD_CONFIG.md b/docs/DOWNLOAD_CONFIG.md index 0d7a8fa..72144b2 100644 --- a/docs/DOWNLOAD_CONFIG.md +++ b/docs/DOWNLOAD_CONFIG.md @@ -32,8 +32,9 @@ There is no `downloader:` config key to set anymore. Setting one to a legacy val Pre-define default options and switches of the `dl` command. The values will be ignored if explicitly set in the CLI call. -The Key must be the same value Python click would resolve it to as an argument. -E.g., `@click.option("-r", "--range", "range_", type=...` actually resolves as `range_` variable. +The Key is the option name with dashes as underscores (`--v-lang` -> `v_lang`). `range` and `list` +work as-is; their internal Python names (`range_`, `list_`, suffixed to avoid the builtins) are +also accepted. For example to set the default primary language to download to German, @@ -88,7 +89,7 @@ to a CLI option on the `dl` command. CLI arguments always take priority over con | `abitrate_range` | str | none | Audio bitrate window in kbps, format `MIN-MAX` | | `real_video_bitrate` | bool | `false` | Probe actual media size to compute true video bitrates, overriding the manifest's declared value (`-rvb`). See [Real bitrate probing](#real-bitrate-probing) | | `real_audio_bitrate` | bool | `false` | Same as above for audio tracks (`-rab`). Slower than video (more renditions) | -| `range_` | str or list | `SDR` | Color range(s): `SDR`, `HDR10`, `HDR10+`, `HLG`, `DV`, `HYBRID` | +| `range` (or `range_`) | str or list | `SDR` | Color range(s): `SDR`, `HDR10`, `HDR10+`, `HLG`, `DV`, `HYBRID` | | `channels` | float | any | Audio channels (e.g., `5.1`, `7.1`) | | `worst` | bool | `false` | Select the lowest bitrate track within the specified quality. Requires `quality` | | `best_available` | bool | `false` | Continue if requested quality is unavailable | @@ -186,18 +187,21 @@ How it works: Per-track before→after values are logged at debug level (run with `-d`); the corrected values always appear in the Available Tracks panel. -You can also set per-service `dl` overrides (see [Service Integration & Authentication Configuration](SERVICE_CONFIG.md)): +You can also set per-service `dl` overrides under the `services` section (see +[Service Integration & Authentication Configuration](SERVICE_CONFIG.md)). Per-service values beat the +global `dl:` section; explicit CLI arguments beat both: ```yaml dl: lang: en downloads: 4 workers: 16 + +services: EXAMPLE: - bitrate: CVBR - EXAMPLE2: - worst: true - quality: 1080 + dl: + v_lang: [en] + quality: 1080 ``` --- diff --git a/docs/SERVICE_CONFIG.md b/docs/SERVICE_CONFIG.md index 37d80bf..7ec5439 100644 --- a/docs/SERVICE_CONFIG.md +++ b/docs/SERVICE_CONFIG.md @@ -30,7 +30,18 @@ service tag in the `services` section. Supported override keys include: `dl`, `s `headers`, `proxy_map`, `title_map`, and more. Overrides are merged with global config (not replaced) -- only specified keys are overridden, others -use global defaults. CLI arguments always take priority over service-specific config. +use global defaults. + +Any `dl` command option can be overridden per service. Use the option name with dashes as underscores +(`--v-lang` -> `v_lang`). `range` and `list` work as-is; their internal Python names (`range_`, +`list_`, suffixed to avoid the builtins) are also accepted. + +Precedence (highest first): + +1. Explicit CLI arguments / environment variables (e.g. `--v-lang en`) +2. Per-service config (`services..dl`) +3. Global `dl:` config +4. Built-in option defaults For example, diff --git a/tests/core/test_click_types.py b/tests/core/test_click_types.py index b50dac1..6b875ec 100644 --- a/tests/core/test_click_types.py +++ b/tests/core/test_click_types.py @@ -6,7 +6,7 @@ from __future__ import annotations import pytest from unshackle.core.tracks.subtitle import Subtitle -from unshackle.core.utils.click_types import SubtitleCodecChoice +from unshackle.core.utils.click_types import QUALITY_LIST, SLOW_DELAY_RANGE, SubtitleCodecChoice choice = SubtitleCodecChoice(Subtitle.Codec) @@ -31,3 +31,29 @@ def test_codecs_still_map(value, expected): def test_empty_is_none(): assert choice.convert(None) is None + + +@pytest.mark.parametrize( + "value,expected", + [ + (1080, [1080]), + ([720, 1080], [1080, 720]), + ("1080p", [1080]), + ("720,1080", [1080, 720]), + ], +) +def test_quality_list_accepts_yaml_native_values(value, expected): + assert QUALITY_LIST.convert(value) == expected + + +@pytest.mark.parametrize( + "value,expected", + [ + (True, (60, 120)), + (False, None), + ("20-40", (20, 40)), + ((25, 30), (25, 30)), + ], +) +def test_slow_delay_range_accepts_bool(value, expected): + assert SLOW_DELAY_RANGE.convert(value, None, None) == expected diff --git a/tests/core/test_service_dl_overrides.py b/tests/core/test_service_dl_overrides.py new file mode 100644 index 0000000..704f1f6 --- /dev/null +++ b/tests/core/test_service_dl_overrides.py @@ -0,0 +1,154 @@ +"""Tests for ``apply_service_dl_overrides`` — precedence: CLI/env > service dl > global dl > defaults.""" + +from __future__ import annotations + +import logging +from typing import Any, Optional + +import click +import pytest +from click.core import ParameterSource + +from unshackle.commands.dl import apply_service_dl_overrides, normalize_dl_config +from unshackle.core.tracks import Video +from unshackle.core.utils.click_types import LANGUAGE_RANGE, MultipleChoice + +log = logging.getLogger("test_service_dl_overrides") + + +def make_ctx(args: Optional[list[str]] = None, default_map: Optional[dict[str, Any]] = None) -> click.Context: + """Build a parsed context for a mirror of the dl group's option shapes.""" + + @click.group("dl", invoke_without_command=True) + @click.option("-vl", "--v-lang", type=LANGUAGE_RANGE, default=[]) + @click.option( + "-r", "--range", "range_", type=MultipleChoice(Video.Range, case_sensitive=False), default=[Video.Range.SDR] + ) + @click.option("--repack", is_flag=True, default=False) + @click.option("--workers", type=int, default=None, envvar="TEST_DL_WORKERS") + def cmd(**__: Any) -> None: + pass + + return cmd.make_context("dl", args or [], default_map=default_map) + + +def test_service_only_key_applies(): + """The reported bug: key set only under services..dl, not under global dl:.""" + ctx = make_ctx() + assert ctx.params["v_lang"] == [] + apply_service_dl_overrides(ctx, {"v_lang": ["en"]}, log) + assert ctx.params["v_lang"] == ["en"] + + +def test_cli_value_beats_service_config(): + ctx = make_ctx(["-vl", "fr"]) + apply_service_dl_overrides(ctx, {"v_lang": ["en"]}, log) + assert ctx.params["v_lang"] == ["fr"] + + +def test_env_value_beats_service_config(monkeypatch): + monkeypatch.setenv("TEST_DL_WORKERS", "5") + ctx = make_ctx() + apply_service_dl_overrides(ctx, {"workers": 9}, log) + assert ctx.params["workers"] == 5 + + +def test_service_config_beats_global_default_map(): + ctx = make_ctx(default_map={"v_lang": "de"}) + assert ctx.params["v_lang"] == ["de"] + apply_service_dl_overrides(ctx, {"v_lang": ["en"]}, log) + assert ctx.params["v_lang"] == ["en"] + + +def test_flag_global_true_service_false(): + ctx = make_ctx(default_map={"repack": True}) + assert ctx.params["repack"] is True + apply_service_dl_overrides(ctx, {"repack": False}, log) + assert ctx.params["repack"] is False + + +def test_range_scalar_converts_to_enum_list(): + ctx = make_ctx() + apply_service_dl_overrides(ctx, {"range_": "HDR10"}, log) + assert ctx.params["range_"] == [Video.Range.HDR10] + + +def test_range_alias_applies(): + """`range` in config is an accepted alias for the `range_` parameter.""" + ctx = make_ctx() + apply_service_dl_overrides(ctx, {"range": "HDR10"}, log) + assert ctx.params["range_"] == [Video.Range.HDR10] + + +def test_normalize_dl_config_maps_aliases(): + assert normalize_dl_config({"range": "SDR", "list": True, "v_lang": ["en"]}) == { + "range_": "SDR", + "list_": True, + "v_lang": ["en"], + } + + +def test_unknown_key_warns(caplog): + ctx = make_ctx() + with caplog.at_level(logging.WARNING, logger=log.name): + apply_service_dl_overrides(ctx, {"bogus": 1}, log) + assert any("unknown dl option 'bogus'" in r.message for r in caplog.records) + + +def test_none_value_is_skipped(): + ctx = make_ctx() + apply_service_dl_overrides(ctx, {"v_lang": None}, log) + assert ctx.params["v_lang"] == [] + + +def test_invalid_value_warns_and_keeps_current(caplog): + ctx = make_ctx() + with caplog.at_level(logging.WARNING, logger=log.name): + apply_service_dl_overrides(ctx, {"workers": "abc"}, log) + assert ctx.params["workers"] is None + assert any("Failed to apply service dl override 'workers'" in r.message for r in caplog.records) + + +def hand_built_ctx(body: dict[str, Any]) -> click.Context: + """Mirror the serve path: ctx.params set directly, sources recorded manually.""" + parsed = make_ctx() + ctx = click.Context(parsed.command) + ctx.params = {"repack": body.get("repack", False)} + for name in ctx.params: + ctx.set_parameter_source( + name, ParameterSource.COMMANDLINE if name in body else ParameterSource.DEFAULT + ) + return ctx + + +def test_hand_built_ctx_default_gets_override(): + ctx = hand_built_ctx({}) + apply_service_dl_overrides(ctx, {"repack": True}, log) + assert ctx.params["repack"] is True + + +def test_hand_built_ctx_client_value_wins(): + ctx = hand_built_ctx({"repack": False}) + apply_service_dl_overrides(ctx, {"repack": True}, log) + assert ctx.params["repack"] is False + + +def test_hand_built_ctx_missing_param_is_skipped(): + """Known dl option absent from a hand-built context must not raise or be injected.""" + ctx = hand_built_ctx({}) + apply_service_dl_overrides(ctx, {"v_lang": ["en"]}, log) + assert "v_lang" not in ctx.params + + +@pytest.mark.parametrize( + "value,expected", + [ + (["en", "de"], ["en", "de"]), + ("en,de", ["en", "de"]), + ("en", ["en"]), + ], +) +def test_language_range_yaml_shapes(value, expected): + ctx = make_ctx() + apply_service_dl_overrides(ctx, {"v_lang": value}, log) + assert ctx.params["v_lang"] == expected diff --git a/unshackle/commands/dl.py b/unshackle/commands/dl.py index cc51f79..581d8b0 100644 --- a/unshackle/commands/dl.py +++ b/unshackle/commands/dl.py @@ -25,6 +25,7 @@ from uuid import UUID import click import yaml +from click.core import ParameterSource from langcodes import Language from pymediainfo import MediaInfo from rich.console import Group @@ -80,6 +81,42 @@ class SkippedSubtitle(TypedDict): title: str +# Config keys accepted as natural names for params whose Python name dodges a builtin. +DL_OPTION_ALIASES = {"range": "range_", "list": "list_"} + + +def normalize_dl_config(dl_config: dict[str, Any]) -> dict[str, Any]: + """Map aliased config keys (e.g. ``range``) to their Click parameter names (``range_``).""" + return {DL_OPTION_ALIASES.get(key, key): value for key, value in dl_config.items()} + + +def apply_service_dl_overrides(ctx: click.Context, service_dl_config: dict[str, Any], log: logging.Logger) -> None: + """Apply ``services..dl`` config onto ``ctx.params``. Explicit CLI/env values win; + defaults and global ``dl:`` default_map values are replaced.""" + params_by_name = {param.name: param for param in ctx.command.params if param.name} + for name, value in normalize_dl_config(service_dl_config).items(): + param = params_by_name.get(name) + if param is None: + log.warning(f"Ignoring unknown dl option '{name}' in service config") + continue + if name not in ctx.params: + log.debug(f"Skipping service dl override '{name}': not present in this context") + continue + if value is None: + continue + source = ctx.get_parameter_source(name) + if source not in (ParameterSource.DEFAULT, ParameterSource.DEFAULT_MAP): + continue + try: + ctx.params[name] = param.type_cast_value(ctx, value) + log.debug(f"Applied service dl override '{name}': {ctx.params[name]}") + except Exception as e: + log.warning( + f"Failed to apply service dl override '{name}'={value!r}: {e}. " + f"Check that the value is valid for this parameter." + ) + + def download_tracks_in_passes( tracks: Iterable[AnyTrack], max_concurrent: int, @@ -326,7 +363,9 @@ class dl: @click.command( short_help="Download, Decrypt, and Mux tracks for titles from a Service.", cls=Services, - context_settings=dict(**context_settings, default_map=config.dl, token_normalize_func=Services.get_tag), + context_settings=dict( + **context_settings, default_map=normalize_dl_config(config.dl), token_normalize_func=Services.get_tag + ), ) @click.option( "-p", "--profile", type=str, default=None, help="Profile to use for Credentials and Cookies (if available)." @@ -687,37 +726,19 @@ class dl: self.service = Services.get_tag(ctx.invoked_subcommand) self.vault_service = Services.get_vault_tag(self.service) - service_dl_config = config.services.get(self.service, {}).get("dl", {}) - if service_dl_config: - param_types = {param.name: param.type for param in ctx.command.params if param.name} + apply_service_dl_overrides(ctx, config.services.get(self.service, {}).get("dl", {}), self.log) - for param_name, service_value in service_dl_config.items(): - if param_name not in ctx.params: - continue - - current_value = ctx.params[param_name] - global_default = config.dl.get(param_name) - param_type = param_types.get(param_name) - - try: - if param_type and global_default is not None: - global_default = param_type.convert(global_default, None, ctx) - except Exception as e: - self.log.debug(f"Failed to convert global default for '{param_name}': {e}") - - if current_value == global_default or (current_value is None and global_default is None): - try: - converted_value = service_value - if param_type and service_value is not None: - converted_value = param_type.convert(service_value, None, ctx) - - ctx.params[param_name] = converted_value - self.log.debug(f"Applied service-specific '{param_name}' override: {converted_value}") - except Exception as e: - self.log.warning( - f"Failed to apply service-specific '{param_name}' override: {e}. " - f"Check that the value '{service_value}' is valid for this parameter." - ) + # Refresh locals Click bound before the overrides ran. + no_proxy = ctx.params.get("no_proxy", no_proxy) + profile = ctx.params.get("profile", profile) + proxy = ctx.params.get("proxy", proxy) + repack = ctx.params.get("repack", repack) + tag = ctx.params.get("tag", tag) + tmdb_id = ctx.params.get("tmdb_id", tmdb_id) + imdb_id = ctx.params.get("imdb_id", imdb_id) + animeapi_id = ctx.params.get("animeapi_id", animeapi_id) + enrich = ctx.params.get("enrich", enrich) + output_dir = ctx.params.get("output_dir", output_dir) self.profile = profile self.proxy_requested = bool(proxy) diff --git a/unshackle/core/api/download_manager.py b/unshackle/core/api/download_manager.py index 4b3a40e..54ee507 100644 --- a/unshackle/core/api/download_manager.py +++ b/unshackle/core/api/download_manager.py @@ -292,6 +292,14 @@ def _perform_download( "no_cache": params.get("no_cache", False), "reset_cache": params.get("reset_cache", False), } + # Hand-built context: record parameter sources so service dl overrides + # apply to defaults but never clobber client-sent values. + from click.core import ParameterSource + + for param_name in ctx.params: + ctx.set_parameter_source( + param_name, ParameterSource.COMMANDLINE if param_name in params else ParameterSource.DEFAULT + ) dl_instance = dl( ctx=ctx, diff --git a/unshackle/core/utils/click_types.py b/unshackle/core/utils/click_types.py index 542c380..ca4e13a 100644 --- a/unshackle/core/utils/click_types.py +++ b/unshackle/core/utils/click_types.py @@ -254,16 +254,19 @@ class QualityList(click.ParamType): name = "quality_list" def convert( - self, value: Union[str, list[str]], param: Optional[click.Parameter] = None, ctx: Optional[click.Context] = None + self, + value: Union[str, int, list[Union[str, int]]], + param: Optional[click.Parameter] = None, + ctx: Optional[click.Context] = None, ) -> list[int]: if not value: return [] if not isinstance(value, list): - value = value.split(",") + value = str(value).split(",") resolutions = [] for resolution in value: try: - resolutions.append(int(resolution.lower().rstrip("p"))) + resolutions.append(int(str(resolution).lower().rstrip("p"))) except TypeError: self.fail( f"Expected string for int() conversion, got {resolution!r} of type {type(resolution).__name__}", @@ -370,9 +373,13 @@ class SlowDelayRange(click.ParamType): name = "delay_range" - def convert(self, value: Any, param: Optional[click.Parameter], ctx: Optional[click.Context]) -> tuple[int, int]: + def convert( + self, value: Any, param: Optional[click.Parameter], ctx: Optional[click.Context] + ) -> Optional[tuple[int, int]]: if isinstance(value, tuple): return value + if isinstance(value, bool): + return (60, 120) if value else None match = re.match(r"^(\d+)-(\d+)$", str(value)) if not match: diff --git a/unshackle/unshackle-example.yaml b/unshackle/unshackle-example.yaml index ac30d24..407eeb5 100644 --- a/unshackle/unshackle-example.yaml +++ b/unshackle/unshackle-example.yaml @@ -414,6 +414,13 @@ audio: # Atmos still trumps codec priority. Valid names: AAC, AC3, EC3, AC4, OPUS, OGG, DTS, ALAC, FLAC. # codec_priority: [FLAC, ALAC, AC4, EC3, DTS, AC3, OPUS, AAC, OGG] +# Global defaults for the dl command. Any dl option can be set here. +# Use the option name with dashes as underscores (--v-lang -> v_lang). +# range and list work as-is (range_/list_ also accepted). +# +# Any of these can also be overridden per service under services..dl +# (see the services section below). Precedence, highest first: +# CLI arguments > services..dl > this dl section > built-in defaults dl: sub_format: srt downloads: 4 @@ -421,6 +428,14 @@ dl: lang: - en - fr + v_lang: [en] # --v-lang: video language(s), if different from audio + s_lang: [en, de] # --s-lang: subtitle language(s) + range: SDR # --range: SDR, HDR10, HDR10+, DV + quality: [1080] # --quality: resolution(s), int or list + slow: false # --slow: true = random 60-120s delay between titles, or "MIN-MAX" string + # Nesting a SERVICE TAG here provides defaults for that service's OWN command + # options (e.g. a service-defined --bitrate flag) -- NOT for the dl options + # above. To override dl options per service, use services..dl instead. EXAMPLE: bitrate: CBR @@ -624,12 +639,20 @@ services: # Other services or no service specified will still use random selection # NEW: Configuration overrides (can be combined with profiles and certificates) - # Override dl command defaults for this service + # Override dl command defaults for this service. ANY dl option works here, + # same key naming as the global dl section (v_lang, range_, list_, ...). + # These beat the global dl section; explicit CLI arguments beat both. dl: downloads: 4 # Limit concurrent track downloads (global default: 6) workers: 8 # Reduce workers per track (global default: 16) lang: ["en", "es-419"] # Different language priority for this service + v_lang: [en] # Video language differs from audio on this service + s_lang: [all] # Grab every subtitle language here + range: HDR10 # Prefer HDR10 on this service (CLI -r still wins) + quality: 720 # This service rate-limits 1080p; int or list both work sub_format: srt # Force SRT subtitle format + slow: true # Pace title downloads (60-120s); or "20-40" for a custom range + repack: false # Even if the global dl section sets repack: true # Override subtitle processing for this service subtitle: @@ -671,7 +694,8 @@ services: # - Only specified keys are overridden, others use global defaults # - Reserved keys (profiles, api_key, certificate, etc.) are NOT treated as overrides # - Any dict-type config option can be overridden (dl, subtitle, muxing, headers, etc.) - # - CLI arguments always take priority over service-specific config + # - Any dl option can be overridden; dashes become underscores (--v-lang -> v_lang) + # - Precedence: CLI arguments > service dl override > global dl section > built-in defaults # External proxy provider services proxy_providers: