diff --git a/docs/dev/rest-api/remote-sessions.md b/docs/dev/rest-api/remote-sessions.md index d57d3fb2..b7951a9e 100644 --- a/docs/dev/rest-api/remote-sessions.md +++ b/docs/dev/rest-api/remote-sessions.md @@ -87,18 +87,37 @@ remote_services: If you configure exactly one server, unshackle uses it implicitly: ```bash -unshackle dl EXAMPLE 0ABCDEF +unshackle dl --remote EXAMPLE 0ABCDEF ``` If you configure **more than one** server, you must pick one with `--server`: ```bash -unshackle dl --server my_server EXAMPLE 0ABCDEF +unshackle dl --remote --server my_server EXAMPLE 0ABCDEF ``` With no `remote_services` configured at all, unshackle raises a clear error telling you to add the block shown above. +### Making a service remote by default + +To run a service on the server without `--remote`, set `remote` in the +[`services`](../../reference/configuration/services.md#services) block, not in the +`services` sub-dict of `remote_services`. Add `server` to select one of several servers: + +```yaml title="unshackle.yaml (client)" +services: + EXAMPLE: + dl: + remote: true + server: my_server +``` + +`unshackle dl EXAMPLE 0ABCDEF` and `unshackle search EXAMPLE "query"` then run on +`my_server`. A `--server` on the command line overrides the configured server. The global +`dl: remote: true` makes every `dl` run remote, but it has no effect on `search`. +The REST API ignores both settings: a job runs remote only when its request asks for it. + --- ## What happens during a download diff --git a/docs/reference/configuration/services.md b/docs/reference/configuration/services.md index 7c91a4ac..0d426bf4 100644 --- a/docs/reference/configuration/services.md +++ b/docs/reference/configuration/services.md @@ -13,7 +13,10 @@ Per-service configuration, keyed by the canonical service tag (for example `EXAM - **`title_map`**: an exact-match rename map applied to fetched titles (source name → desired name), so a service that names a title differently from your library still matches. - **`dl`**: per-service download defaults, using the same keys as the global - [`dl`](download.md#dl) block. + [`dl`](download.md#dl) block. Set `remote: true` to send the `dl` and `search` runs of that + service to a [remote server](#remote_services) without `--remote`. Add `server` to select one of + several servers. A `--server` on the command line overrides it. A REST API job ignores both keys. + See [making a service remote by default](../../dev/rest-api/remote-sessions.md#making-a-service-remote-by-default). Individual services may read any additional keys they define. unshackle hands the merged result (the service's own `config.yaml` with this block layered on top) to the service as `self.config`. diff --git a/tests/core/test_service_dl_overrides.py b/tests/core/test_service_dl_overrides.py index 19d46a61..c0054cd7 100644 --- a/tests/core/test_service_dl_overrides.py +++ b/tests/core/test_service_dl_overrides.py @@ -131,6 +131,15 @@ def test_hand_built_ctx_client_value_wins(): assert ctx.params["repack"] is False +def test_hand_built_ctx_skips_routing_params(caplog): + """An API job routes from its request, so the server's service config must not flip it, or warn.""" + ctx = hand_built_ctx({}) + with caplog.at_level(logging.WARNING, logger=log.name): + apply_service_dl_overrides(ctx, {"remote": True, "server": "box"}, log) + assert ctx.params == {"repack": False} + assert not caplog.records + + 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({}) diff --git a/tests/core/test_service_remote_config.py b/tests/core/test_service_remote_config.py new file mode 100644 index 00000000..54d02839 --- /dev/null +++ b/tests/core/test_service_remote_config.py @@ -0,0 +1,106 @@ +"""``services..dl`` ``remote`` and ``server`` route a service without ``--remote``. Explicit CLI values win.""" + +from __future__ import annotations + +from typing import Any, Optional, cast + +import click +import pytest + +from unshackle.commands.dl import dl +from unshackle.commands.search import search +from unshackle.core.config import config +from unshackle.core.services import Services + + +def make_ctx( + command: click.Command, args: Optional[list[str]] = None, default_map: Optional[dict[str, Any]] = None +) -> click.Context: + """Parse the group options of a real ``Services`` command, without running its callback.""" + return command.make_context( + command.name or "dl", [*(args or []), "EXAMPLE", "title"], default_map=default_map or {} + ) + + +def group(ctx: click.Context) -> Services: + return cast(Services, ctx.command) + + +@pytest.fixture +def routed(monkeypatch: pytest.MonkeyPatch) -> list[tuple[str, Optional[str]]]: + """Record the path get_command takes and the server it passes, without loading a service or calling a server.""" + calls: list[tuple[str, Optional[str]]] = [] + + def remote(name: str, server_name: Optional[str]) -> click.Command: + calls.append(("remote", server_name)) + return click.Command(name) + + def local(tag: str) -> Any: + calls.append(("local", None)) + return type("FakeService", (), {"cli": click.Command(tag)}) + + monkeypatch.setattr(Services, "make_remote_command", staticmethod(remote)) + monkeypatch.setattr(Services, "load", staticmethod(local)) + return calls + + +@pytest.mark.parametrize("command", [dl.cli, search]) +def test_service_remote_routes_to_remote_command( + monkeypatch: pytest.MonkeyPatch, routed: list[tuple[str, Optional[str]]], command: click.Command +) -> None: + """The typed name matches the config key in any case, as a service only the server has keeps the typed case.""" + monkeypatch.setattr(config, "services", {"EXAMPLE": {"dl": {"remote": True, "server": "box"}}}) + ctx = make_ctx(command) + group(ctx).resolve_command(ctx, ["example", "title"]) + assert routed == [("remote", "box")] + assert ctx.params["remote"] is True + assert ctx.params["server"] == "box" + + +@pytest.mark.parametrize( + ("services", "args", "default_map", "expected"), + [ + ({"OTHER": {"dl": {"remote": True}}}, [], None, ("local", None)), + ({"EXAMPLE": {"dl": {"remote": True, "server": "box"}}}, ["--server", "other"], None, ("remote", "other")), + ({"EXAMPLE": {"dl": {"remote": False}}}, [], {"remote": True}, ("local", None)), + ({"EXAMPLE": {"dl": {"remote": "false"}}}, [], None, ("local", None)), + ], + ids=["other-service", "cli-server-wins", "service-false-beats-global", "quoted-false-is-cast"], +) +def test_routing_precedence( + monkeypatch: pytest.MonkeyPatch, + routed: list, + services: dict, + args: list[str], + default_map: Optional[dict[str, Any]], + expected: tuple[str, Optional[str]], +) -> None: + monkeypatch.setattr(config, "services", services) + ctx = make_ctx(dl.cli, args, default_map) + group(ctx).get_command(ctx, "EXAMPLE") + assert routed == [expected] + + +def test_invalid_value_names_the_config_key(monkeypatch: pytest.MonkeyPatch, routed: list) -> None: + monkeypatch.setattr(config, "services", {"EXAMPLE": {"dl": {"remote": "maybe"}}}) + ctx = make_ctx(dl.cli) + with pytest.raises(click.ClickException, match=r"services\.EXAMPLE\.dl\.remote"): + group(ctx).get_command(ctx, "EXAMPLE") + + +def test_resolved_service_does_not_route_the_next(monkeypatch: pytest.MonkeyPatch, routed: list) -> None: + """Help and shell completion call get_command for every service on the context that resolved one.""" + monkeypatch.setattr(config, "services", {"EXAMPLE": {"dl": {"remote": True}}}) + ctx = make_ctx(dl.cli) + group(ctx).resolve_command(ctx, ["EXAMPLE", "title"]) + assert ctx.params["remote"] is True + group(ctx).get_command(ctx, "OTHER") + assert routed == [("remote", None), ("local", None)] + + +def test_import_ignores_service_remote(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(config, "services", {"EXAMPLE": {"dl": {"remote": True}}}) + monkeypatch.setattr(Services, "make_import_command", staticmethod(lambda tag, ctx: click.Command(tag))) + ctx = make_ctx(dl.cli, ["--import", "export.json"]) + group(ctx).resolve_command(ctx, ["EXAMPLE", "title"]) + assert ctx.params["remote"] is False diff --git a/tests/remote/unit/test_remote_command_unknown_tag.py b/tests/remote/unit/test_remote_command_unknown_tag.py index 277543fa..3f4c5034 100644 --- a/tests/remote/unit/test_remote_command_unknown_tag.py +++ b/tests/remote/unit/test_remote_command_unknown_tag.py @@ -22,7 +22,7 @@ SERVED = [ def _make(name: str) -> click.Command: - return Services.make_remote_command(name, ctx=click.Context(click.Command("dl"))) + return Services.make_remote_command(name, None) def test_unknown_name_raises_with_typed_name_and_available_list() -> None: diff --git a/unshackle/commands/dl.py b/unshackle/commands/dl.py index 99e6256b..6b404396 100644 --- a/unshackle/commands/dl.py +++ b/unshackle/commands/dl.py @@ -70,7 +70,7 @@ from unshackle.core.proxies.resolve import ( split_proxy_query, ) from unshackle.core.service import Service, grow_session_pool -from unshackle.core.services import Services +from unshackle.core.services import ROUTING_PARAMS, Services from unshackle.core.temp import with_task_temp from unshackle.core.title_cacher import get_account_hash from unshackle.core.titles import Movie, Movies, Series, Song, Title_T @@ -461,9 +461,13 @@ def cut_music_sample(title: Any, window: tuple[int, int], log: logging.Logger) - 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. - The config replaces defaults and global ``dl:`` default_map values.""" + The config replaces defaults and global ``dl:`` default_map values. + ``remote`` and ``server`` are skipped: they pick the code that runs, so ``Services.resolve_command`` sets + them before the service loads, and an API job takes them from its request.""" 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(): + if name in ROUTING_PARAMS: + continue param = params_by_name.get(name) if param is None: log.warning(f"Ignoring unknown dl option '{name}' in service config") @@ -1280,7 +1284,7 @@ class dl: self.service = Services.get_tag(ctx.invoked_subcommand) self.vault_service = Services.get_vault_tag(self.service) - apply_service_dl_overrides(ctx, config.services.get(self.service, {}).get("dl", {}), self.log) + apply_service_dl_overrides(ctx, (ci_get(config.services, self.service) or {}).get("dl") or {}, self.log) # Refresh locals Click bound before the overrides ran. no_proxy = ctx.params.get("no_proxy", no_proxy) @@ -1458,9 +1462,9 @@ class dl: ) except KeyError: pass - merge_dict(config.services.get(self.service), self.service_config) + merge_dict(ci_get(config.services, self.service), self.service_config) - service_config = config.services.get(self.service, {}) + service_config = ci_get(config.services, self.service, {}) if service_config: reserved_keys = { "profiles", diff --git a/unshackle/commands/search.py b/unshackle/commands/search.py index bea3f599..86902e48 100644 --- a/unshackle/commands/search.py +++ b/unshackle/commands/search.py @@ -20,7 +20,7 @@ from unshackle.core.proxies.resolve import REGION, describe_proxy, load_proxy_pr from unshackle.core.service import Service from unshackle.core.services import Services from unshackle.core.utils.click_types import ContextData -from unshackle.core.utils.collections import merge_dict +from unshackle.core.utils.collections import ci_get, merge_dict from unshackle.core.utils.redact import mask_proxy @@ -80,7 +80,7 @@ def search( if service_config_path.exists(): service_config = yaml.safe_load(service_config_path.read_text(encoding="utf8")) log.info("Service Config loaded") - merge_dict(config.services.get(service), service_config) + merge_dict(ci_get(config.services, service), service_config) proxy_providers: list[Proxy] = [] if no_proxy: diff --git a/unshackle/core/services.py b/unshackle/core/services.py index 9350b347..2fddd021 100644 --- a/unshackle/core/services.py +++ b/unshackle/core/services.py @@ -12,6 +12,7 @@ from pathlib import Path from typing import Any, Iterable import click +from click.core import ParameterSource from unshackle.core import binaries from unshackle.core.config import config @@ -19,6 +20,7 @@ from unshackle.core.constants import DEFAULT_SAMPLE from unshackle.core.service import Service from unshackle.core.service_repo import DirtyServiceRepo, head, is_repo_spec, refresh_repo, resolve_service_repo from unshackle.core.utilities import import_module_by_path +from unshackle.core.utils.collections import ci_get from unshackle.core.utils.redact import redact_path log = logging.getLogger("services") @@ -348,6 +350,9 @@ def check_load_errors() -> None: raise click.ClickException(f"Failed to load {len(LOAD_ERRORS)} service(s):\n{joined}") +ROUTING_PARAMS = ("remote", "server") + + class Services(click.Group): """Lazy-loaded command group of project services.""" @@ -384,7 +389,7 @@ class Services(click.Group): """ remote = ctx.params.get("remote") or (ctx.parent and ctx.parent.params.get("remote")) if remote: - remote_services = Services.fetch_remote_services(ctx) + remote_services = Services.fetch_remote_services(ctx.params.get("server")) if remote_services is not None: return [s["tag"] for s in remote_services] tags = Services.get_tags() @@ -401,13 +406,13 @@ class Services(click.Group): check_load_errors() tag = Services.get_tag(name) - import_file = ctx.params.get("import_file") or (ctx.parent and ctx.parent.params.get("import_file")) - if import_file: + if Services.is_import(ctx): return Services.make_import_command(tag, ctx) - remote = ctx.params.get("remote") or (ctx.parent and ctx.parent.params.get("remote")) + routing = Services.routing(ctx, tag) + remote = routing.get("remote") or (ctx.parent and ctx.parent.params.get("remote")) if remote: - return Services.make_remote_command(name, ctx) + return Services.make_remote_command(name, routing.get("server")) try: service = Services.load(tag) @@ -429,6 +434,49 @@ class Services(click.Group): raise click.ClickException(f"Service '{tag}' has no 'cli' method configured.") + def resolve_command( + self, ctx: click.Context, args: list[str] + ) -> tuple[str | None, click.Command | None, list[str]]: + """Resolve the service, then store its routing in ``ctx.params`` for the callback. + + ``get_command`` has already computed it, but help and shell completion call ``get_command`` on its own, + so only this path may store it. + """ + cmd_name, cmd, rest = super().resolve_command(ctx, args) + if cmd_name: + ctx.params.update(Services.routing(ctx, Services.get_tag(cmd_name))) + return cmd_name, cmd, rest + + @staticmethod + def is_import(ctx: click.Context) -> bool: + return bool(ctx.params.get("import_file") or (ctx.parent and ctx.parent.params.get("import_file"))) + + @staticmethod + def routing(ctx: click.Context, tag: str) -> dict[str, Any]: + """The ``remote`` and ``server`` values for one service: CLI/env, then ``services..dl``, then defaults. + + Built from the parameter sources, never read back from ``ctx.params``: help and shell completion resolve + every service on one context, so a value stored for one service must not route the next one. + An import never routes. + """ + if Services.is_import(ctx): + return {} + service_dl = (ci_get(config.services, tag) or {}).get("dl") or {} + routing: dict[str, Any] = {} + for param in ctx.command.params: + name = param.name + if name not in ROUTING_PARAMS or name not in ctx.params: + continue + if ctx.get_parameter_source(name) not in (ParameterSource.DEFAULT, ParameterSource.DEFAULT_MAP): + routing[name] = ctx.params[name] + continue + value = service_dl.get(name) + try: + routing[name] = param.type_cast_value(ctx, param.get_default(ctx) if value is None else value) + except click.BadParameter as e: + raise click.ClickException(f"services.{tag}.dl.{name}: {e.format_message()}") from e + return routing + @staticmethod def docstring_help(doc: str) -> str: """Format a service docstring for Click help, one \\b per paragraph so Click keeps the layout. @@ -443,20 +491,19 @@ class Services(click.Group): return "\n\n".join([first] + [f"\b\n{p}" for p in rest]) @staticmethod - def fetch_remote_services(ctx: click.Context) -> list[dict] | None: + def fetch_remote_services(server_name: str | None) -> list[dict] | None: """Fetch the service list from the remote server (cached); None when it is unreachable.""" from unshackle.core.remote_service import remote_service_list, resolve_server - server_name = ctx.params.get("server") resolve_server(server_name) # a bad --server raises its ClickException, not "unreachable" return remote_service_list(server_name) @staticmethod - def make_remote_command(name: str, ctx: click.Context) -> click.Command: + def make_remote_command(name: str, server_name: str | None) -> click.Command: """Make a Click command for a remote service, with the options the remote server gives.""" # A failed fetch (None) means the remote server was unreachable, not that the name is # invalid - fall through to a bare stub so `-h` keeps working offline. - services = Services.fetch_remote_services(ctx) + services = Services.fetch_remote_services(server_name) if services is None: svc_info = None tag = Services.get_tag(name) @@ -492,7 +539,6 @@ class Services(click.Group): def remote_cli(ctx: click.Context, title: str, **kwargs: object) -> object: from unshackle.core.remote_service import make_remote_service - server_name = ctx.parent.params.get("server") if ctx.parent else None return make_remote_service(ctx, server_name, tag, title, {"service_params": kwargs}) if cli_params: