Answer 400 for a request value of the wrong type

A value such as chapter_ids that is not a list of numbers gave a 500.

bindery/web.py

@@ -92,6 +92,46 @@ def _json() -> dict[str, Any]:
     return data if isinstance(data, dict) else {}
 
 
+# Values of a request body. A value of the wrong type is an error of the request, so it gets a 400.
+
+
+def _list(data: dict[str, Any], name: str) -> list[Any]:
+    value = data.get(name)
+    if value is None:
+        return []
+    if not isinstance(value, list):
+        raise ValueError(f"{name} must be a list")
+    return value
+
+
+def _int(value: Any, name: str) -> int:
+    try:
+        return int(value)
+    except (TypeError, ValueError):
+        raise ValueError(f"{name} must be a whole number") from None
+
+
+def _ids(data: dict[str, Any], name: str = "chapter_ids") -> list[int]:
+    return [_int(v, name) for v in _list(data, name)]
+
+
+def _rules(data: dict[str, Any]) -> list[dict[str, Any]]:
+    rules = _list(data, "rules")
+    if not all(isinstance(r, dict) for r in rules):
+        raise ValueError("rules must be a list of rules")
+    return rules
+
+
+def _names(data: dict[str, Any], name: str) -> list[str] | None:
+    """A list of text values, or None if the request leaves it out."""
+    if data.get(name) is None:
+        return None
+    values = _list(data, name)
+    if not all(isinstance(v, str) for v in values):
+        raise ValueError(f"{name} must be a list of names")
+    return values
+
+
 # Login gate and errors
 
 
@@ -259,9 +299,8 @@ def stories() -> Any:
 
 @app.post("/api/stories")
 def add_stories() -> Any:
-    urls = _json().get("urls", [])
-    if isinstance(urls, str):
-        urls = re.split(r"\s+", urls)
+    data = _json()
+    urls = re.split(r"\s+", data["urls"]) if isinstance(data.get("urls"), str) else _list(data, "urls")
     queued, errors = orchestrator.queue_urls([str(u) for u in urls])
     return {"jobs": queued, "errors": errors}
 
@@ -294,15 +333,19 @@ def download(story_id: int) -> Any:
     data = _json()
     # A rebuild that the user asks for also builds and sends a book that did not change.
     return orchestrator.queue_story(
-        story_id, data.get("mode", DownloadMode.NEW.value), data.get("chapter_ids"), data.get("force_fields"), True
+        story_id, str(data.get("mode", DownloadMode.NEW.value)), _ids(data), _names(data, "force_fields"), True
     )
 
 
 @app.put("/api/stories/<int:story_id>/settings")
 def story_settings(story_id: int) -> Any:
     data = _json()
+    days = data.get("recheck_days")
     orchestrator.update_story_settings(
-        story_id, data.get("locked_fields"), data.get("auto_update"), data.get("recheck_days")
+        story_id,
+        _names(data, "locked_fields"),
+        data.get("auto_update"),
+        None if days is None else _int(days, "recheck_days"),
     )
     return orchestrator.story_details(story_id)["story"]
 
@@ -330,23 +373,19 @@ def approve_upload(story_id: int) -> Any:
 @app.post("/api/stories/<int:story_id>/chapters")
 def chapters_included(story_id: int) -> Any:
     data = _json()
-    return orchestrator.set_chapters_included(
-        story_id, [int(c) for c in data.get("chapter_ids", [])], bool(data.get("included", True))
-    )
+    return orchestrator.set_chapters_included(story_id, _ids(data), bool(data.get("included", True)))
 
 
 @app.post("/api/stories/<int:story_id>/chapters/move")
 def move_chapters(story_id: int) -> Any:
     data = _json()
     after = data.get("after")
-    return orchestrator.move_chapters(
-        story_id, [int(c) for c in data.get("chapter_ids", [])], None if after is None else int(after)
-    )
+    return orchestrator.move_chapters(story_id, _ids(data), None if after is None else _int(after, "after"))
 
 
 @app.post("/api/stories/<int:story_id>/chapters/delete")
 def delete_chapters(story_id: int) -> Any:
-    return orchestrator.delete_chapters(story_id, [int(c) for c in _json().get("chapter_ids", [])])
+    return orchestrator.delete_chapters(story_id, _ids(_json()))
 
 
 @app.get("/api/stories/<int:story_id>/epub")
@@ -377,7 +416,7 @@ def story_cover(story_id: int) -> Any:
 def rules(story_id: int | None = None) -> Any:
     if request.method == "PUT":
         data = _json()
-        return orchestrator.save_rules(story_id, data.get("rules", []), data.get("paragraphs", ...))
+        return orchestrator.save_rules(story_id, _rules(data), data.get("paragraphs", ...))
     return orchestrator.get_rules(story_id)
 
 
@@ -387,8 +426,8 @@ def rules_preview(story_id: int | None = None) -> Any:
     data = _json()
     return orchestrator.preview_rules(
         story_id,
-        data.get("rules", []),
-        int(data.get("chapter_id", 0)),
+        _rules(data),
+        _int(data.get("chapter_id", 0), "chapter_id"),
         bool(data.get("source")),
         data.get("paragraphs"),
     )
@@ -406,7 +445,7 @@ def chapter_diff(chapter_id: int) -> Any:
 
 @app.post("/api/chapters/<int:chapter_id>/revision")
 def chapter_revision(chapter_id: int) -> Any:
-    return orchestrator.set_chapter_revision(chapter_id, int(_json().get("rev", 0)))
+    return orchestrator.set_chapter_revision(chapter_id, _int(_json().get("rev", 0), "rev"))
 
 
 @app.delete("/api/chapters/<int:chapter_id>/revisions/<int:rev>")
@@ -430,7 +469,7 @@ def library_search() -> Any:
 @app.post("/api/library/import")
 def library_import() -> Any:
     data = _json()
-    books = [b for b in data.get("books", []) if isinstance(b, dict) and "id" in b]
+    books = [{**b, "id": _int(b["id"], "id")} for b in _list(data, "books") if isinstance(b, dict) and "id" in b]
     return orchestrator.queue_import(books, bool(data.get("check", True)))
 
 

tests/test_api_flows.py

@@ -152,3 +152,37 @@ def test_unexpected_backend_error_returns_json(client, monkeypatch, caplog):
     assert response.status_code == 500
     assert response.json == {"error": "Bindery hit an unexpected error. The log has the details."}
     assert "RuntimeError: broken" in caplog.text
+
+
+@pytest.mark.parametrize(
+    ("method", "path", "body", "error"),
+    [
+        ("post", "stories", {"urls": 5}, "urls must be a list"),
+        ("post", "stories/{sid}/chapters", {"chapter_ids": 5}, "chapter_ids must be a list"),
+        ("post", "stories/{sid}/chapters/move", {"chapter_ids": [[1]]}, "chapter_ids must be a whole number"),
+        ("post", "stories/{sid}/chapters/move", {"chapter_ids": [1], "after": [2]}, "after must be a whole number"),
+        ("post", "stories/{sid}/chapters/delete", {"chapter_ids": "all"}, "chapter_ids must be a list"),
+        ("post", "stories/{sid}/download", {"mode": "recheck", "chapter_ids": ["x"]}, "chapter_ids must be a whole"),
+        ("post", "stories/{sid}/download", {"force_fields": "title"}, "force_fields must be a list"),
+        ("post", "stories/{sid}/download", {"force_fields": [["title"]]}, "force_fields must be a list of names"),
+        ("put", "stories/{sid}/settings", {"locked_fields": [{}]}, "locked_fields must be a list of names"),
+        ("put", "stories/{sid}/settings", {"recheck_days": [7]}, "recheck_days must be a whole number"),
+        ("put", "rules", {"rules": ["a"]}, "rules must be a list of rules"),
+        ("post", "rules/preview", {"rules": [], "chapter_id": None}, "chapter_id must be a whole number"),
+        ("post", "chapters/{cid}/revision", {"rev": {}}, "rev must be a whole number"),
+        ("post", "library/import", {"books": 7}, "books must be a list"),
+        ("post", "library/import", {"books": [{"id": "x"}]}, "id must be a whole number"),
+    ],
+)
+def test_a_value_of_the_wrong_type_is_a_bad_request(client, story, method, path, body, error):
+    sid, cid = story
+    response = getattr(client, method)("/api/" + path.format(sid=sid, cid=cid), json=body, headers=H)
+    assert response.status_code == 400
+    assert error in response.json["error"]
+
+
+def test_chapter_requests_without_ids_name_what_to_select(client, story):
+    sid, _ = story
+    for body in ({}, {"chapter_ids": None}):
+        response = client.post(f"/api/stories/{sid}/chapters/delete", json=body, headers=H)
+        assert (response.status_code, response.json["error"]) == (400, "Select the chapters to delete")