refactor(remote)!: send service options only in service_params

A service option named like a dl option (e.g. --profile) is no longer dropped or mixed with the dl value; the server casts nested values and keeps cli defaults nested.

docs/dev/rest-api/endpoints.md

@@ -205,7 +205,8 @@ Get the list of titles behind a title ID, for example episodes or a movie, witho
 | `cdm_type` | string | no | Preferred CDM type. |
 | `credentials`, `cookies`, `cache` | - | no | Your own login material, on a `--remote-only` server only. See [Client cache on a remote-only server](#client-cache-on-a-remote-only-server). |
 
-You can also pass service-specific CLI options as additional keys.
+You can also pass a service's own CLI options, in a nested `service_params` object or as
+additional keys. When the body has `service_params`, the server reads service options only from it.
 
 #### Logins that need the user
 
@@ -490,9 +491,10 @@ Make a download job. It requires `service` and `title_id`. Every other field is
 **Validation.** Invalid values return `400 INVALID_PARAMETERS`. `vcodec`, `acodec` and `sub_format` take the same spellings as the CLI options of the same name (`unshackle dl --help` prints each list). The API and the CLI read one list, so they cannot disagree. `range` must be one of SDR/HDR10/HDR10P/DV/HLG/HYBRID, and `HDR10+` is also valid. The bitrate, download worker, and download counts must be positive integers. You may set at most one of the `*_only` flags. You cannot combine `no_subs` with `subs_only`, or `no_audio` with `audio_only`.
 
 **Service options.** A service's own CLI options travel in the same body. Send them in a nested
-`service_params` object, or as flat top-level keys. `service_params` wins over a flat key of the
-same name. An option you leave out takes the default declared on the service's `cli` command, so
-the job matches the equivalent `dl` run. `profile` at the top level always means the credential
+`service_params` object, or as flat top-level keys. When the body has `service_params`, the server
+reads service options only from it, and every flat key is a `dl` value. An option you leave out
+takes the default declared on the service's `cli` command, so the job matches the equivalent `dl`
+run. `profile` at the top level always means the credential
 profile, never a service's own `--profile` option.
 
 === "Request"
@@ -969,7 +971,7 @@ Make a remote session for a service and title. Authentication runs asynchronousl
 
 `dl_params` is an object with the `dl` track selection: `lang`, `v_lang`, `a_lang` and `s_lang` (arrays of language strings), `acodec` (an array of audio codec names or `dl -a` aliases such as `ddp`), and `forced_subs` and `exact_lang` (booleans). The service reads these values from `ctx.parent.params`. The server uses the `dl` default for each absent or malformed value, and drops an unknown codec name.
 
-Service CLI options also travel in a nested `service_params` object, which wins over a flat key with the same name. `profile` at the top level always means the credentials profile, never a service's own `--profile` option.
+Service CLI options also travel in a nested `service_params` object. When the body has `service_params`, the server reads service options only from it, and a flat key with the same name stays a `dl` value. `profile` at the top level always means the credentials profile, never a service's own `--profile` option.
 
 The `proxy` value must be a full proxy URI, unless the operator gives your API key `server_proxy` in the `serve.users` config. Without `server_proxy`, the server does not resolve country codes with its own proxy providers. It rejects the request with `INVALID_PROXY` when no `proxy` is set and the reported `client_region` differs from the server's region. A request that reports no `client_region` is not blocked. Pass `proxy` with your own proxy, or `no_proxy` to accept the server's own connection. With `server_proxy`, the server resolves a country code and picks a proxy for your `client_region` itself. A Control D resolver, `controld://<resolver>@dns.controld.com`, counts as a full proxy URI: the server runs a forwarder for it and needs no Control D configuration of its own.
 

docs/dev/rest-api/remote-sessions.md

@@ -282,9 +282,9 @@ There are two ways DRM keys get resolved, chosen by the client's `server_cdm` fl
       `server_cdm: false` when it creates the remote session, and the server builds the
       service on your device even when the API key has the server CDM.
     - A service option that the service lists in `SERVER_DEVICE_OPTIONS` keeps the server
-      CDM. The server reads the option from `service_params`, then from the top level of
-      the request. The create response gives the result in `server_cdm`, and the client
-      shows a warning when the server did not apply its choice.
+      CDM. The server reads the option from `service_params`, or from the top level of
+      a request that has no `service_params`. The create response gives the result in
+      `server_cdm`, and the client shows a warning when the server did not apply its choice.
     - The service on the server sees your device, not the server's: its DRM system,
       security level, system ID and device type. A CDM call that the service makes
       (for example a session-key exchange at login) goes to your client as a `cdm_call`,

tests/remote/unit/test_api_service_dl_overrides.py

@@ -103,3 +103,19 @@ def test_client_sent_keys_uses_the_stamp():
 
 def test_client_sent_keys_without_stamp_counts_all_keys():
     assert dm.client_sent_keys({"workers": 1, "quality": None}) == {"workers", "quality"}
+
+
+def test_service_defaults_and_options_travel_nested(env, monkeypatch):
+    import click
+
+    @click.command()
+    @click.option("--profile", type=int, default=1)
+    @click.option("--device", default="chrome")
+    def cli(**kwargs):
+        pass
+
+    monkeypatch.setattr(handlers.Services, "load", lambda service: SimpleNamespace(cli=cli))
+    params = env(body={"profile": "work", "service_params": {"profile": 2}})
+    assert params["profile"] == "work"
+    assert params["service_params"] == {"profile": 2, "device": "chrome"}
+    assert env(body={"device": "tv"})["service_params"] == {"profile": 1, "device": "tv"}

tests/remote/unit/test_remote_relay.py

@@ -136,6 +136,12 @@ def test_make_remote_service_passes_only_cli_options(remote_config, monkeypatch)
     assert built["args"][5]["EXAMPLE"] == {"x": 1}
 
 
+def test_a_service_option_named_like_a_dl_value_passes_only_nested():
+    info = {"cli_params": [{"kind": "option", "name": "profile"}]}
+    assert remote_service.remote_service_params(info, {"service_params": {"profile": "QC"}}) == {"profile": "QC"}
+    assert remote_service.remote_service_params(info, {"profile": "QC"}) == {}
+
+
 def test_remote_error_carries_its_reason_and_exits_a_cli_run():
     err = remote_service.RemoteError("Server error [SESSION_NOT_FOUND]: gone", "SESSION_NOT_FOUND")
     assert isinstance(err, SystemExit) and err.code == 1 and err.kind == "SESSION_NOT_FOUND"

tests/remote/unit/test_remote_service_helpers.py

@@ -645,9 +645,15 @@ def test_dl_selection_travels_nested_and_a_service_option_cannot_override_it() -
         "forced_subs": True,
         "exact_lang": True,
     }
-    assert data["lang"] == 3
+    assert "lang" not in data and "a_lang" not in data
     assert data["service_params"] == {"lang": 3, "a_lang": "x"}
 
 
+def test_a_service_profile_option_travels_nested_and_keeps_the_credential_profile() -> None:
+    data = _sent_create_data({"profile": "work"}, {"profile": "QC"})
+    assert data["profile"] == "work"
+    assert data["service_params"] == {"profile": "QC"}
+
+
 def test_no_dl_params_when_the_ctx_has_no_selection() -> None:
     assert "dl_params" not in _sent_create_data({}, {})

tests/remote/unit/test_server_cdm_max_height.py

@@ -2,6 +2,7 @@
 
 import json
 import logging
+from datetime import datetime
 from types import SimpleNamespace
 from uuid import UUID
 
@@ -97,8 +98,8 @@ def test_a_server_device_option_keeps_the_server_cdm_against_the_clients_choice(
 
     nested = {"cdm_type": "widevine", "server_cdm": False, "service_params": {"server_side": True}}
     assert handlers.choose_session_cdm(request("flat"), "EXAMPLE1", nested) == (True, 1080)
-    # service_params wins over the flat key, as it does when the service is built
-    nested["server_side"], nested["service_params"] = True, {"server_side": False}
+    # with service_params present a flat key is a dl value, as it is when the service is built
+    nested["server_side"], nested["service_params"] = True, {}
     assert handlers.choose_session_cdm(request("flat"), "EXAMPLE1", nested) == (False, None)
 
 
@@ -318,8 +319,8 @@ def test_a_capped_download_job_cannot_ask_for_hybrid(hdr_range):
     handlers.enforce_download_gates({"service": "EXAMPLE1", "quality": [1080], "range": ["HDR10"]}, request("flat"))
 
 
-def test_the_download_gate_reads_the_service_click_defaults(monkeypatch):
-    """A service's own click default overrides the serve default, and the gate sees what the job runs with."""
+def test_a_service_click_default_stays_out_of_the_download_gate(monkeypatch):
+    """A service's own option is not the dl option of the same name, so its default never trips the gate."""
     import asyncio
 
     from unshackle.core.api import download_manager as dm
@@ -329,15 +330,21 @@ def test_the_download_gate_reads_the_service_click_defaults(monkeypatch):
     monkeypatch.setattr(
         handlers.Services, "load", lambda service: SimpleNamespace(cli=SimpleNamespace(params=[option]))
     )
+    jobs = []
 
-    def no_job():
-        raise AssertionError("a refused job must not reach the download manager")
+    class Manager:
+        async def start_workers(self):
+            pass
 
-    monkeypatch.setattr(dm, "get_download_manager", no_job)
+        def create_job(self, service, title_id, owner_key=None, **params):
+            jobs.append(params)
+            return SimpleNamespace(job_id="j", status=SimpleNamespace(value="queued"), created_time=datetime.now())
+
+    monkeypatch.setattr(dm, "get_download_manager", Manager)
     body = {"service": "EXAMPLE1", "title_id": "t1", "quality": [1080]}
-    with pytest.raises(APIError) as ei:
-        asyncio.run(handlers.download_handler(body, request("flat")))
-    assert ei.value.error_code is APIErrorCode.SERVER_CDM_CAPPED
+    asyncio.run(handlers.download_handler(body, request("flat")))
+    assert jobs[0]["service_params"] == {"best_available": True}
+    assert not jobs[0].get("best_available")
 
 
 @pytest.fixture

tests/remote/unit/test_session_log.py

@@ -121,6 +121,31 @@ def test_transport_keys_block_profile_and_service_params() -> None:
     assert "service_params" in LIST_HANDLER_TRANSPORT_KEYS
 
 
+def test_service_params_are_the_only_service_options_when_present() -> None:
+    import click
+
+    from unshackle.core.api.errors import APIError
+    from unshackle.core.api.handlers import SESSION_TRANSPORT_KEYS, instantiate_service
+
+    @click.command()
+    @click.argument("title")
+    @click.option("--profile", default=None)
+    @click.option("--device", default=None)
+    @click.option("--count", type=int, default=1)
+    def cli(title: str, **kwargs: object) -> dict:
+        return kwargs
+
+    def build(data: dict) -> dict:
+        return instantiate_service(click.Context(click.Command("dl")), module, "t", data, SESSION_TRANSPORT_KEYS)
+
+    module = type("Svc", (), {"cli": cli})
+    nested = {"profile": "work", "device": "tv", "service_params": {"profile": "QC", "count": "2"}}
+    assert build(nested) == {"profile": "QC", "device": None, "count": 2}
+    assert build({"profile": "work", "device": "tv"}) == {"profile": None, "device": "tv", "count": 1}
+    with pytest.raises(APIError):
+        build({"service_params": {"count": "x"}})
+
+
 def test_drain_server_logs_survives_client_systemexit() -> None:
     """RemoteClient raises SystemExit on transport errors; the drain must swallow it."""
     from unshackle.core.remote_service import RemoteService

unshackle/core/api/download_manager.py

@@ -21,6 +21,7 @@ from unshackle.core.api.events import bus, publish_service_event
 from unshackle.core.api.sanitize import sanitize_log
 from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, VIDEO_CODEC_LIST
 from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_path, redact_text
+from unshackle.core.utils.service_params import service_options
 
 log = logging.getLogger("download_manager")
 
@@ -574,14 +575,7 @@ def perform_download(
             if "title" in service_init_params:
                 service_kwargs["title"] = title_id
 
-            for key, value in params.items():
-                if key in service_init_params and key not in ["service", "title_id", "profile", "service_params"]:
-                    service_kwargs[key] = value
-            service_params = params.get("service_params")
-            if isinstance(service_params, dict):
-                for key, value in service_params.items():
-                    if key in service_init_params and key != "title":
-                        service_kwargs[key] = value
+            service_kwargs.update(service_options(params, service_init_params.keys() - {"title"}))
 
             for param_name, param_info in service_init_params.items():
                 if param_name not in service_kwargs and param_name not in ["self", "ctx"]:

unshackle/core/api/handlers.py

@@ -15,7 +15,7 @@ from datetime import date as date_
 from functools import partial
 from http.cookiejar import CookieJar
 from pathlib import Path
-from typing import Any, Callable, Dict, Iterable, List, Optional, Set, cast
+from typing import AbstractSet, Any, Callable, Dict, Iterable, List, Optional, Set, cast
 
 import click
 from aiohttp import web
@@ -45,6 +45,7 @@ from unshackle.core.utils.click_types import AUDIO_CODEC_LIST, QUALITY_LIST, SUB
 from unshackle.core.utils.collections import ci_get
 from unshackle.core.utils.hdr10plus import probed_tracks
 from unshackle.core.utils.redact import REDACTED, URL_USERINFO_RE, redact_all, redact_secrets, redact_text
+from unshackle.core.utils.service_params import service_options
 
 log = logging.getLogger("api")
 
@@ -246,30 +247,26 @@ def instantiate_service(
     service_module: Any,
     title: str,
     data: Optional[Dict[str, Any]] = None,
-    transport_keys: Optional[set] = None,
+    transport_keys: AbstractSet[str] = frozenset(),
 ) -> Any:
-    """Instantiate a service by invoking its click cli through Click.
+    """Instantiate a service by invoking its click cli with the ``service_options`` in ``data``.
 
-    Click fills option defaults through ``param.get_default()`` and runs type coercion,
-    so we no longer have to inspect ``__init__`` or stitch defaults by hand. This function
-    pulls extra kwargs from the ``data`` entries whose names match a cli option name
-    and are not in the transport-key blocklist.
+    Click fills the option defaults, but ``ctx.invoke`` passes the given kwargs uncast, so each value
+    goes through its option's type here: JSON or a remote client's str-typed stub can send "1" for an int.
     """
     cli_params = getattr(getattr(service_module, "cli", None), "params", []) or []
-    cli_param_names = {p.name for p in cli_params if hasattr(p, "name") and p.name}
-    transport_keys = transport_keys or set()
+    params_by_name = {p.name: p for p in cli_params if getattr(p, "name", None) and p.name != "title"}
     extras: Dict[str, Any] = {}
     if data:
-        for k, v in data.items():
-            if k in cli_param_names and k not in transport_keys and k != "title":
-                extras[k] = v
-        service_params = data.get("service_params")
-        if isinstance(service_params, dict):
-            for k, v in service_params.items():
-                if k in cli_param_names and k != "title":
-                    extras[k] = v
-        elif service_params is None:
-            for k in cli_param_names & transport_keys & set(data):
+        for k, v in service_options(data, params_by_name, transport_keys).items():
+            try:
+                extras[k] = params_by_name[k].type_cast_value(parent_ctx, v)
+            except click.BadParameter as e:
+                raise APIError(
+                    APIErrorCode.INVALID_PARAMETERS, f"Invalid service option '{sanitize_log(k)}': {e.message}"
+                ) from e
+        if data.get("service_params") is None:
+            for k in params_by_name.keys() & transport_keys & data.keys():
                 if k not in ("service", "title_id", "proxy", "no_proxy") and data.get(k) is not None:
                     log.warning(
                         f"Ignoring flat '{sanitize_log(k)}' as a service option, send it under 'service_params'"
@@ -1044,9 +1041,7 @@ def choose_session_cdm(
         return False, None
     if data.get("server_cdm") is False and data.get("cdm_type"):
         server_device_options = getattr(Services.load(service), "SERVER_DEVICE_OPTIONS", ())
-        service_params = data.get("service_params")
-        options = {**data, **service_params} if isinstance(service_params, dict) else data
-        if not any(options.get(option) for option in server_device_options):
+        if not any(service_options(data, server_device_options).values()):
             return False, None
     cap = server_cdm_max_height(request, service)
     wanted = requested_height(data.get("quality"))
@@ -2456,16 +2451,19 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request]
 
     try:
         service_module = None if server else Services.load(normalized_service)
-        service_specific_defaults = {}
+        service_params: Optional[Dict[str, Any]] = None
 
-        # Skip None defaults here: this dict overlays into job params; injecting
-        # None for keys like `profile` would clobber serve-config overrides.
-        # Missing required __init__ params are handled in download_manager.perform_download.
+        # Service defaults and options travel nested: flat, a service default would overwrite the dl value of
+        # the same name. Missing required __init__ params are handled in download_manager.perform_download.
         # A remote service applies its own defaults on the remote server.
-        if hasattr(service_module, "cli") and hasattr(service_module.cli, "params"):
-            for param in service_module.cli.params:
-                if hasattr(param, "name") and param.default is not None and not isinstance(param.default, enum.Enum):
-                    service_specific_defaults[param.name] = param.default
+        cli_params = getattr(getattr(service_module, "cli", None), "params", None)
+        if cli_params is not None:
+            defaults = {
+                param.name: param.default
+                for param in cli_params
+                if param.name and param.default is not None and not isinstance(param.default, enum.Enum)
+            }
+            service_params = {**defaults, **service_options(data, {param.name for param in cli_params})}
 
         filtered_params = {k: v for k, v in data.items() if k not in ["service", "title_id"]}
         # Overlay any dl-relevant keys from `serve:` config (e.g. downloads, workers) so the API
@@ -2478,10 +2476,11 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request]
         params_with_defaults = {
             **DEFAULT_DOWNLOAD_PARAMS,
             **serve_overrides,
-            **service_specific_defaults,
             **service_dl,
             **filtered_params,
         }
+        if service_params is not None:
+            params_with_defaults["service_params"] = service_params
         await asyncio.to_thread(enforce_download_gates, {**params_with_defaults, "service": service_tag}, request)
 
         manager = get_download_manager()

unshackle/core/api/routes.py

@@ -1958,8 +1958,9 @@ async def session_create(request: web.Request) -> web.Response:
                 description: |
                   False when the user picked the local CDM. With `cdm_type`, the server then builds
                   the service on the client's device, also for an API key with the server CDM.
-                  A request that sets one of the service's `SERVER_DEVICE_OPTIONS`, in
-                  `service_params` or at the top level, keeps the server CDM
+                  A request that sets one of the service's `SERVER_DEVICE_OPTIONS` keeps the server
+                  CDM. The server reads it from `service_params`, or from the top level when
+                  the request has no `service_params`
               cdm_type:
                 type: string
                 enum: [widevine, playready]

unshackle/core/remote_service.py

@@ -55,6 +55,7 @@ from unshackle.core.tunnel import TunnelClient
 from unshackle.core.utils import run_proxy
 from unshackle.core.utils.collections import ci_get
 from unshackle.core.utils.redact import redact_path, redact_text, safe_display_url
+from unshackle.core.utils.service_params import service_options
 from unshackle.core.vault import Vault
 
 log = logging.getLogger("remote_service")
@@ -873,15 +874,9 @@ def find_remote_service(services: list[dict], name: str) -> Optional[dict]:
 
 
 def remote_service_params(info: dict, data: Dict[str, Any]) -> Dict[str, Any]:
-    """The service options to send: ``data["service_params"]``, else the ``data`` keys naming a cli option.
-
-    Only the remote service's cli option names pass, either way: RemoteService merges them into the
-    session-create body, where a key such as ``service`` or ``proxy`` would override the checked value.
-    """
+    """The remote service's cli options in ``data``, read by ``service_options``."""
     names = {p.get("name") for p in info.get("cli_params") or [] if p.get("kind") == "option"}
-    names -= {"title", "service", "title_id", "profile", "proxy", "no_proxy"}
-    source = data.get("service_params") if isinstance(data.get("service_params"), dict) else data
-    return {k: v for k, v in source.items() if k in names and v is not None and v is not False}
+    return {k: v for k, v in service_options(data, names).items() if v is not None and v is not False}
 
 
 def make_remote_service(
@@ -1385,9 +1380,8 @@ class RemoteService:
             if dl_params:
                 create_data["dl_params"] = dl_params
 
-        if self._service_params:
-            create_data.update(self._service_params)
-            create_data["service_params"] = self._service_params
+        # sent even empty: without it the server reads service options from the flat dl keys
+        create_data["service_params"] = self._service_params
 
         # sent with the server CDM too: a capped server hands the tracks above its cap to this device
         cdm = self.local_cdm

unshackle/core/utils/service_params.py

@@ -0,0 +1,18 @@
+from typing import Any, Collection, Dict, Iterable
+
+FLAT_DL_KEYS = frozenset({"title", "service", "title_id", "profile", "proxy", "no_proxy"})
+
+
+def service_options(
+    data: Dict[str, Any], names: Collection[str], flat_excluded: Iterable[str] = FLAT_DL_KEYS
+) -> Dict[str, Any]:
+    """A request's service options named in ``names``: from its ``service_params`` object, else its flat keys.
+
+    With ``service_params`` present every flat key is a dl value, so it never fills a service option of the same name.
+    Without it, the flat keys in ``flat_excluded`` stay dl values.
+    """
+    nested = data.get("service_params")
+    if isinstance(nested, dict):
+        return {k: v for k, v in nested.items() if k in names}
+    excluded = set(flat_excluded)
+    return {k: v for k, v in data.items() if k in names and k not in excluded}