fix(api): check and cast download job service options like a session does

A bad value now gets 400 INVALID_PARAMETERS at submit and the worker casts each option; a flat dl key such as quality or lang no longer fills a service option of the same name.

docs/dev/rest-api/endpoints.md

@@ -492,9 +492,11 @@ Make a download job. It requires `service` and `title_id`. Every other field is
 
 **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. 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
+reads service options only from it, and every flat key is a `dl` value. Without it, a flat key that
+is also a `dl` option, such as `quality` or `lang`, stays 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
+run. The server checks each service option against that option's type, and a value it refuses gets
+`400 INVALID_PARAMETERS`. `profile` at the top level always means the credential
 profile, never a service's own `--profile` option.
 
 === "Request"

tests/remote/unit/test_api_service_dl_overrides.py

@@ -4,10 +4,12 @@ import asyncio
 from datetime import datetime
 from types import SimpleNamespace
 
+import click
 import pytest
 
 from unshackle.core.api import download_manager as dm
 from unshackle.core.api import handlers
+from unshackle.core.api.errors import APIError, APIErrorCode
 
 pytestmark = pytest.mark.unit
 
@@ -106,8 +108,6 @@ def test_client_sent_keys_without_stamp_counts_all_keys():
 
 
 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")
@@ -119,3 +119,40 @@ def test_service_defaults_and_options_travel_nested(env, monkeypatch):
     assert params["profile"] == "work"
     assert params["service_params"] == {"profile": 2, "device": "chrome"}
     assert env(body={"device": "tv"})["service_params"] == {"profile": 1, "device": "tv"}
+
+
+def use_cli(monkeypatch, *options):
+    """Make the loaded service declare ``options`` on its cli."""
+    cli = click.Command("svc", params=list(options))
+    monkeypatch.setattr(handlers.Services, "load", lambda service: SimpleNamespace(cli=cli))
+
+
+def test_a_service_option_its_type_refuses_is_a_400(env, monkeypatch):
+    use_cli(monkeypatch, click.Option(["--episodes"], type=click.IntRange(1, 3), default=1))
+    with pytest.raises(APIError) as exc_info:
+        env(body={"service_params": {"episodes": 9}})
+    assert exc_info.value.error_code == APIErrorCode.INVALID_PARAMETERS
+    assert "episodes" in exc_info.value.message
+    assert env(body={"service_params": {"episodes": "2"}})["service_params"]["episodes"] == "2"
+
+
+@pytest.mark.parametrize(
+    ("name", "value"),
+    [("episodes", {"a": 1}), ("episodes", [1]), ("movie", 2), ("movie", [1]), ("device", {"a": 1}), ("device", [1])],
+)
+def test_a_json_shape_click_cannot_convert_is_a_400(env, monkeypatch, name, value):
+    use_cli(
+        monkeypatch,
+        click.Option(["--episodes"], type=int),
+        click.Option(["--movie"], is_flag=True),
+        click.Option(["--device"]),
+    )
+    with pytest.raises(APIError) as exc_info:
+        env(body={"service_params": {name: value}})
+    assert exc_info.value.error_code == APIErrorCode.INVALID_PARAMETERS
+    assert name in exc_info.value.message
+
+
+def test_a_flat_dl_key_never_fills_a_service_option_of_the_same_name(env, monkeypatch):
+    use_cli(monkeypatch, click.Option(["--quality"], type=click.Choice(["SD", "HD"]), default="HD"))
+    assert env(body={"quality": [1080]})["service_params"] == {"quality": "HD"}

tests/remote/unit/test_download_manager.py

@@ -268,6 +268,40 @@ def test_perform_download_passes_latest_episodes_to_dl_result(tmp_path: Path, mo
     assert exc.value.args[0]["latest_episodes"] == 3
 
 
+def test_perform_download_casts_service_options(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+    import click
+
+    import unshackle.commands.dl as dl_module
+    from unshackle.core.api import download_manager, handlers
+    from unshackle.core.services import Services
+
+    class FakeDl:
+        cli = click.Command("dl")
+
+        def __init__(self, ctx: click.Context, **kwargs: object) -> None:
+            pass
+
+    class FakeService:
+        cli = click.Command(
+            "svc", params=[click.Option(["--episodes"], type=int), click.Option(["--tag"], multiple=True)]
+        )
+
+        def __init__(self, ctx: click.Context, episodes: object = None, tag: object = ()) -> None:
+            raise _CapturedCtx(episodes, tag)
+
+    monkeypatch.setattr(dl_module, "dl", FakeDl)
+    monkeypatch.setattr(Services, "get_path", staticmethod(lambda s: tmp_path))
+    monkeypatch.setattr(Services, "load", staticmethod(lambda s: FakeService))
+    monkeypatch.setattr(handlers, "load_full_cdm", lambda *a: None)
+
+    with pytest.raises(_CapturedCtx) as exc:
+        download_manager.perform_download(
+            "job-1", "EXAMPLE", "t1", {"service_params": {"episodes": "2", "tag": ["a", "b"]}}
+        )
+
+    assert exc.value.args == (2, ("a", "b"))
+
+
 def test_worker_relays_a_prompt_and_reads_the_answer(monkeypatch: pytest.MonkeyPatch) -> None:
     import json
     import os

unshackle/core/api/download_manager.py

@@ -21,7 +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
+from unshackle.core.utils.service_params import cast_service_options, service_options
 
 if TYPE_CHECKING:
     from unshackle.core.service import Service
@@ -579,7 +579,9 @@ def perform_download(
             if "title" in service_init_params:
                 service_kwargs["title"] = title_id
 
-            service_kwargs.update(service_options(params, service_init_params.keys() - {"title"}))
+            cli_params = getattr(getattr(service_module, "cli", None), "params", None) or []
+            options = service_options(params, service_init_params.keys() - {"title"})
+            service_kwargs.update(cast_service_options(cli_params, service_ctx, options))
 
             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

@@ -56,7 +56,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
+from unshackle.core.utils.service_params import FLAT_DL_KEYS, cast_service_options, service_options
 
 log = logging.getLogger("api")
 
@@ -253,6 +253,17 @@ def build_parent_ctx(
     return parent
 
 
+def cast_or_refuse(params: Iterable[click.Parameter], ctx: Any, options: Dict[str, Any]) -> Dict[str, Any]:
+    """``cast_service_options``, with a refused value as ``400 INVALID_PARAMETERS``."""
+    try:
+        return cast_service_options(params, ctx, options)
+    except click.BadParameter as e:
+        name = e.param.name if e.param and e.param.name else "?"
+        raise APIError(
+            APIErrorCode.INVALID_PARAMETERS, f"Invalid service option '{sanitize_log(name)}': {sanitize_log(e.message)}"
+        ) from e
+
+
 def instantiate_service(
     parent_ctx: Any,
     service_module: Any,
@@ -266,22 +277,13 @@ def instantiate_service(
     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 []
-    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 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'"
-                    )
+    names = {p.name for p in cli_params if getattr(p, "name", None) and p.name != "title"}
+    data = data or {}
+    extras = cast_or_refuse(cli_params, parent_ctx, service_options(data, names, transport_keys))
+    if data and data.get("service_params") is None:
+        for k in names & 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'")
     return parent_ctx.invoke(service_module.cli, title=title, **extras)
 
 
@@ -2491,7 +2493,12 @@ async def download_handler(data: Dict[str, Any], request: Optional[web.Request]
                 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})}
+            # A flat dl key (quality, lang) stays a dl value, as in instantiate_service.
+            sent = service_options(
+                data, {param.name for param in cli_params}, FLAT_DL_KEYS | DEFAULT_DOWNLOAD_PARAMS.keys()
+            )
+            cast_or_refuse(cli_params, click.Context(click.Command(normalized_service)), sent)
+            service_params = {**defaults, **sent}
 
         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

unshackle/core/utils/service_params.py

@@ -1,5 +1,7 @@
 from typing import Any, Collection, Dict, Iterable
 
+import click
+
 FLAT_DL_KEYS = frozenset({"title", "service", "title_id", "profile", "proxy", "no_proxy"})
 
 
@@ -16,3 +18,25 @@ def service_options(
         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}
+
+
+def cast_service_options(params: Iterable[click.Parameter], ctx: Any, options: Dict[str, Any]) -> Dict[str, Any]:
+    """``options`` with each value cast by the click parameter of the same name; raises ``click.BadParameter``.
+
+    A name that no parameter declares keeps its value. A JSON object, or an array for a single-value option,
+    is refused before the cast, since click would stringify it.
+    """
+    by_name = {p.name: p for p in params if p.name}
+    cast: Dict[str, Any] = {}
+    for k, v in options.items():
+        param = by_name.get(k)
+        if param is None:
+            cast[k] = v
+            continue
+        if isinstance(v, dict) or (isinstance(v, list) and not param.multiple and param.nargs == 1):
+            raise click.BadParameter("expected a single value, not a JSON object or array", ctx=ctx, param=param)
+        try:
+            cast[k] = param.type_cast_value(ctx, v)
+        except (TypeError, ValueError, AttributeError) as e:  # convert() only promises BadParameter for strings
+            raise click.BadParameter(f"not a valid {param.type.name} value", ctx=ctx, param=param) from e
+    return cast