fix(subs): ask OpenSubtitles for SRT at download

A search entry often carries no `format`, so `format_of(None)` produced
`Other("")`, that was stashed at search time and handed back as the
download's format, and conversion had no parser to pick:

    4073669 could not be converted from  to SRT: no parser for ""

`POST /download` takes `sub_format`, and the API converts on its side, so
asking for srt makes the answer srt whatever the uploader posted. The
format guessed from the search entry is dead weight and goes with it.

The download mock matched only `file_id`, so it never exercised what the
real API returns.

Closes #272

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

crates/arr-subs/src/opensubtitles.rs

@@ -117,7 +117,6 @@ impl OpenSubtitlesConfig {
 #[derive(Clone, Debug)]
 struct CandidateMeta {
     language: Language,
-    format: SubtitleFormat,
 }
 
 /// The OpenSubtitles.com client behind the [`Provider`] trait.
@@ -251,7 +250,12 @@ impl OpenSubtitles {
             .header("Api-Key", &self.config.api_key)
             .header("User-Agent", USER_AGENT);
         if let Some(query) = query {
-            request = request.query(query);
+            // The API answers a 301 when the parameters are not in
+            // alphabetical order, so sort before sending rather than trust a
+            // redirect to carry the request intact (#270).
+            let mut query = query.to_vec();
+            query.sort_by(|(left, _), (right, _)| left.cmp(right));
+            request = request.query(&query);
         }
         if let Some(json) = json {
             request = request.json(json);
@@ -282,12 +286,20 @@ impl OpenSubtitles {
                 provider: self.id.clone(),
                 detail: format!("bad login path: {err}"),
             })?;
+        // The credentials travel as a JSON body, not as HTTP basic auth
+        // (#271). Basic auth is answered 401, which reads as "your username
+        // and password are wrong" and is what this reported for every
+        // download.
+        let credentials = serde_json::json!({
+            "username": username,
+            "password": password,
+        });
         let response = self
             .http
             .post(url)
-            .basic_auth(username, Some(password))
             .header("Api-Key", &self.config.api_key)
             .header("User-Agent", USER_AGENT)
+            .json(&credentials)
             .send()
             .await
             .map_err(|err| Error::Transport {
@@ -343,25 +355,7 @@ impl OpenSubtitles {
                 source,
             })?;
 
-        let mut params: Vec<(String, String)> =
-            vec![("languages".to_owned(), languages_param(&request.languages))];
-        match request.file.media {
-            MediaRef::Movie { tmdb_id } => {
-                params.push(("tmdb_movie_id".to_owned(), tmdb_id.to_string()));
-            }
-            MediaRef::Episode {
-                tmdb_id,
-                season,
-                episode,
-            } => {
-                params.push(("tmdb_series_id".to_owned(), tmdb_id.to_string()));
-                params.push(("season_number".to_owned(), season.to_string()));
-                params.push(("episode_number".to_owned(), episode.to_string()));
-            }
-        }
-        if let Some(hash) = hash.as_deref() {
-            params.push(("moviehash".to_owned(), hash.to_owned()));
-        }
+        let params = search_params(request, hash.as_deref());
 
         let response = self
             .send(Method::GET, "subtitles", Some(&params), None, None)
@@ -385,7 +379,6 @@ impl OpenSubtitles {
                 offered.candidate.id.clone(),
                 CandidateMeta {
                     language: offered.candidate.language.clone(),
-                    format: offered.format.clone(),
                 },
             )
         }));
@@ -416,7 +409,11 @@ impl OpenSubtitles {
         })?;
 
         let token = self.current_token().await?;
-        let body = serde_json::json!({ "file_id": file_id });
+        // Ask for SRT rather than take whatever the uploader posted (#272).
+        // A search entry often carries no `format` at all, which left the
+        // download labelled `Other("")` and the conversion with no parser to
+        // pick. The API converts on its side, so the answer is always SRT.
+        let body = serde_json::json!({ "file_id": file_id, "sub_format": "srt" });
 
         let response = self
             .send(Method::POST, "download", None, Some(&body), Some(&token))
@@ -436,7 +433,7 @@ impl OpenSubtitles {
         Ok(Fetched {
             id: id.clone(),
             language: meta.language,
-            format: meta.format,
+            format: SubtitleFormat::Srt,
             content,
         })
     }
@@ -501,7 +498,6 @@ fn truncate(body: &str) -> String {
 /// One API entry turned into arr's terms, plus the facts download will need.
 struct Offered {
     candidate: Candidate,
-    format: SubtitleFormat,
 }
 
 /// The wire shape of `POST /login`: the user token everything that touches
@@ -536,8 +532,6 @@ impl SubtitleEntry {
         let file_id = attributes.files.first()?.file_id;
         let language = language_of(attributes.language.as_deref()?)?;
         let release_name = attributes.release.filter(|release| !release.is_empty());
-        let format = format_of(attributes.format.as_deref());
-
         Some(Offered {
             candidate: Candidate {
                 provider: ProviderId::new(PROVIDER_NAME),
@@ -552,7 +546,6 @@ impl SubtitleEntry {
                 sdh: attributes.hearing_impaired,
                 release_name,
             },
-            format,
         })
     }
 }
@@ -570,7 +563,6 @@ struct SubtitleAttributes {
     hearing_impaired: bool,
     #[serde(default)]
     foreign_parts_only: bool,
-    format: Option<String>,
     files: Vec<SubtitleFileRef>,
 }
 
@@ -579,6 +571,39 @@ struct SubtitleFileRef {
     file_id: u64,
 }
 
+/// The query a search sends, in the parameter names the API documents.
+///
+/// `tmdb_movie_id` and `tmdb_series_id` — what this sent until #270 — are not
+/// parameters of this API. Unknown ones are ignored rather than refused, so
+/// the id lane contributed nothing and every search came back as whatever the
+/// `moviehash` alone matched: one result for a popular release, none at all
+/// for anything the hash database has never seen.
+fn search_params(request: &SearchRequest, hash: Option<&str>) -> Vec<(String, String)> {
+    let mut params: Vec<(String, String)> =
+        vec![("languages".to_owned(), languages_param(&request.languages))];
+    match request.file.media {
+        MediaRef::Movie { tmdb_id } => {
+            params.push(("tmdb_id".to_owned(), tmdb_id.to_string()));
+        }
+        MediaRef::Episode {
+            tmdb_id,
+            season,
+            episode,
+        } => {
+            params.push(("parent_tmdb_id".to_owned(), tmdb_id.to_string()));
+            params.push(("season_number".to_owned(), season.to_string()));
+            params.push(("episode_number".to_owned(), episode.to_string()));
+        }
+    }
+    // Sent alongside the id rather than instead of it: the API answers the id
+    // search and flags the entries the hash also matched, which is what
+    // ranking reads.
+    if let Some(hash) = hash {
+        params.push(("moviehash".to_owned(), hash.to_owned()));
+    }
+    params
+}
+
 /// The API spells languages `pt-PT`, `pt-BR`, `en`; accept `_` too, since
 /// tooling around this API uses it.
 fn language_of(code: &str) -> Option<Language> {
@@ -594,16 +619,21 @@ fn language_of(code: &str) -> Option<Language> {
 /// The `languages` query parameter: comma-separated codes, pt-PT and pt-BR
 /// kept apart — keeping them apart is why this is the primary provider.
 fn languages_param(languages: &[Language]) -> String {
-    languages
+    // Lowercase and sorted, both of which the API requires to answer rather
+    // than redirect (#270). Case is not what keeps pt-PT and pt-BR apart --
+    // the region subtag is -- so lowercasing costs nothing.
+    let mut codes: Vec<String> = languages
         .iter()
         .map(|language| match language {
-            Language::PortuguesePortugal => "pt-PT",
-            Language::PortugueseBrazil => "pt-BR",
-            Language::PortugueseUnverified => "pt",
-            Language::Other(tag) => tag.as_str(),
+            Language::PortuguesePortugal => "pt-pt".to_owned(),
+            Language::PortugueseBrazil => "pt-br".to_owned(),
+            Language::PortugueseUnverified => "pt".to_owned(),
+            Language::Other(tag) => tag.to_ascii_lowercase(),
         })
-        .collect::<Vec<_>>()
-        .join(",")
+        .collect();
+    codes.sort();
+    codes.dedup();
+    codes.join(",")
 }
 
 /// The release group of the release a subtitle was timed against, read off
@@ -616,15 +646,6 @@ fn source_of_release(release: &str) -> Option<arr_core::Source> {
     arr_parse::parse(release).source.map(Into::into)
 }
 
-fn format_of(format: Option<&str>) -> SubtitleFormat {
-    match format.unwrap_or_default().to_ascii_lowercase().as_str() {
-        "srt" | "subrip" => SubtitleFormat::Srt,
-        "ass" | "ssa" => SubtitleFormat::Ass,
-        "vtt" | "webvtt" => SubtitleFormat::Vtt,
-        other => SubtitleFormat::Other(other.to_owned()),
-    }
-}
-
 /// Order the candidates best first through the pure ranker (#185), building
 /// the target from the same facts the search ran on: the file's own hash and
 /// whatever the release name claims about group and source.
@@ -658,8 +679,9 @@ fn rank_candidates(
 
 #[cfg(test)]
 mod tests {
-    use super::{format_of, language_of, moviehash, rank_candidates, PROVIDER_NAME};
-    use crate::{Candidate, CandidateId, MediaFile, MediaRef, ProviderId, SubtitleFormat};
+    use super::{language_of, moviehash, rank_candidates, search_params, PROVIDER_NAME};
+    use crate::SearchRequest;
+    use crate::{Candidate, CandidateId, MediaFile, MediaRef, ProviderId};
     use arr_core::Language;
 
     #[test]
@@ -718,21 +740,69 @@ mod tests {
                 Language::PortugueseUnverified,
                 Language::Other("en".to_owned()),
             ]),
-            "pt-PT,pt-BR,pt,en"
+            "en,pt,pt-br,pt-pt"
+        );
+    }
+
+    fn request(media: MediaRef, languages: Vec<Language>) -> SearchRequest {
+        SearchRequest {
+            file: MediaFile {
+                path: std::path::PathBuf::from("/media/x.mkv"),
+                size: 1,
+                release_name: None,
+                media,
+            },
+            languages,
+        }
+    }
+
+    /// #270: the id lane sent parameter names this API does not have. It
+    /// ignores unknown ones, so every search silently degraded to a
+    /// `moviehash` lookup and answered nothing for a release no one has
+    /// hashed.
+    #[test]
+    fn a_movie_search_asks_by_tmdb_id() {
+        let params = search_params(
+            &request(
+                MediaRef::Movie { tmdb_id: 272 },
+                vec![Language::Other("en".to_owned())],
+            ),
+            Some("abc123"),
+        );
+
+        assert_eq!(
+            params,
+            vec![
+                ("languages".to_owned(), "en".to_owned()),
+                ("tmdb_id".to_owned(), "272".to_owned()),
+                ("moviehash".to_owned(), "abc123".to_owned()),
+            ]
         );
     }
 
     #[test]
-    fn formats_spell_the_way_providers_do() {
-        assert_eq!(format_of(Some("srt")), SubtitleFormat::Srt);
-        assert_eq!(format_of(Some("subrip")), SubtitleFormat::Srt);
-        assert_eq!(format_of(Some("ass")), SubtitleFormat::Ass);
-        assert_eq!(format_of(Some("vtt")), SubtitleFormat::Vtt);
-        assert_eq!(
-            format_of(Some("idx")),
-            SubtitleFormat::Other("idx".to_owned())
+    fn an_episode_search_asks_by_the_series_tmdb_id_and_the_numbers() {
+        let params = search_params(
+            &request(
+                MediaRef::Episode {
+                    tmdb_id: 60625,
+                    season: 2,
+                    episode: 4,
+                },
+                vec![Language::PortuguesePortugal],
+            ),
+            None,
+        );
+
+        assert_eq!(
+            params,
+            vec![
+                ("languages".to_owned(), "pt-pt".to_owned()),
+                ("parent_tmdb_id".to_owned(), "60625".to_owned()),
+                ("season_number".to_owned(), "2".to_owned()),
+                ("episode_number".to_owned(), "4".to_owned()),
+            ]
         );
-        assert_eq!(format_of(None), SubtitleFormat::Other(String::new()));
     }
 
     fn candidate(id: u64, hash_match: bool, downloads: u64) -> Candidate {

crates/arr-subs/tests/opensubtitles.rs

@@ -92,9 +92,9 @@ async fn movie_search_sends_both_lanes_and_ranks_hash_match_first() {
     Mock::given(method("GET"))
         .and(path("/api/v1/subtitles"))
         .and(header("Api-Key", "test-api-key"))
-        .and(query_param("tmdb_movie_id", "693134"))
+        .and(query_param("tmdb_id", "693134"))
         .and(query_param("moviehash", "0000000000040000"))
-        .and(query_param("languages", "pt-PT,pt-BR,en"))
+        .and(query_param("languages", "en,pt-br,pt-pt"))
         .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_MOVIE))
         .mount(&server)
         .await;
@@ -139,7 +139,7 @@ async fn episode_search_addresses_the_series_not_the_movie_lane() {
 
     Mock::given(method("GET"))
         .and(path("/api/v1/subtitles"))
-        .and(query_param("tmdb_series_id", "94605"))
+        .and(query_param("parent_tmdb_id", "94605"))
         .and(query_param("season_number", "3"))
         .and(query_param("episode_number", "7"))
         .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_EPISODE))
@@ -182,7 +182,7 @@ async fn a_file_too_small_to_hash_searches_without_the_hash_lane() {
 
     Mock::given(method("GET"))
         .and(path("/api/v1/subtitles"))
-        .and(query_param("tmdb_movie_id", "42"))
+        .and(query_param("tmdb_id", "42"))
         .and(QueryParamMissing("moviehash"))
         .respond_with(ResponseTemplate::new(200).set_body_string(SEARCH_EPISODE))
         .mount(&server)
@@ -250,6 +250,12 @@ async fn an_expired_key_is_unauthorized_and_a_cap_is_rate_limited() {
 async fn mount_login(server: &MockServer, token: &'static str) {
     Mock::given(method("POST"))
         .and(path("/api/v1/login"))
+        // The credentials are a JSON body. Sent as HTTP basic auth — what
+        // this did until #271 — the API answers 401 and every download
+        // reports the username and password as refused.
+        .and(body_partial_json(
+            serde_json::json!({ "username": "user", "password": "pass" }),
+        ))
         .respond_with(
             ResponseTemplate::new(200).set_body_json(serde_json::json!({ "token": token })),
         )
@@ -261,7 +267,12 @@ async fn mount_download_link(server: &MockServer, token: &'static str, file_id:
     Mock::given(method("POST"))
         .and(path("/api/v1/download"))
         .and(header("Authorization", format!("Bearer {token}")))
-        .and(body_partial_json(serde_json::json!({ "file_id": file_id })))
+        // `sub_format` is what makes the answer SRT (#272). A search entry
+        // often carries no format at all, so without it the download was
+        // labelled `Other("")` and no parser matched.
+        .and(body_partial_json(
+            serde_json::json!({ "file_id": file_id, "sub_format": "srt" }),
+        ))
         .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
             "link": format!("{}/file/{file_id}.srt", server.uri()),
             "file_name": "subtitle.srt",