From aaf93fd2a1499bc13cb25cc09272ee5efa34ed6a Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:22:12 -0400 Subject: [PATCH] feat(subtitles): add external provider capability probe (#618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deployment with no external subtitle providers configured answers POST /api/v1/subtitles/search with 200 {"results": null} — byte-identical to a search that ran and matched nothing. Clients had no way to tell "this server cannot do that" from "nothing found for this file", so they offered an in-player search entry point that could never succeed. That reached us as a bug report against the clients for a feature that was simply not enabled here. Add GET /api/v1/subtitles/providers/status, following the per-subsystem capability convention already used by /subtitles/ai/status and /items/trailers/capability: {"schema_version": 1, "enabled": true, "providers": ["opensubtitles"]} Provider names are safe for any authenticated viewer — they already travel in every SubtitleResult.provider and DownloadedSubtitle.provider. The credentials behind them stay in the admin-only provider config. Two details worth noting for review: The whole /subtitles group is conditional on DB + S3 + repo, so on a storage-less deployment the probe would 404 — leaving clients to interpret exactly the ambiguous signal the probe exists to replace. An else branch mounts the probe alone, answering enabled:false. Only one of the two groups registers per boot. The path is two segments on purpose: a bare /providers would shadow the one-segment /{media_file_id} route, while /providers/status never competes with it in chi. Search and download behavior are unchanged. Co-authored-by: Claude Opus 5 (1M context) --- internal/api/handlers/subtitle_search.go | 56 ++++++++++ internal/api/handlers/subtitle_search_test.go | 103 ++++++++++++++++++ internal/api/router.go | 16 +++ internal/subtitles/manager.go | 18 +++ internal/subtitles/manager_test.go | 47 ++++++++ 5 files changed, 240 insertions(+) diff --git a/internal/api/handlers/subtitle_search.go b/internal/api/handlers/subtitle_search.go index 5b9a1137..08f53033 100644 --- a/internal/api/handlers/subtitle_search.go +++ b/internal/api/handlers/subtitle_search.go @@ -98,6 +98,62 @@ func (h *SubtitleSearchHandler) authorizeMediaFile(w http.ResponseWriter, r *htt return authorizeMediaFileAccess(w, r, h.FileAuthorizer, fileID) } +// subtitleProviderStatusResponse tells a client whether this deployment can +// search external subtitle providers at all, following the per-subsystem +// capability convention (/subtitles/ai/status, /items/trailers/capability). +// +// Without it a search on a server with no providers configured answers exactly +// like a search that ran and matched nothing, so a player has no way to tell +// "this server cannot do that" from "nothing found for this file" and ends up +// offering an entry point that can never succeed. A client that finds enabled +// false should disable the search action and say why rather than let the user +// run a query that is guaranteed to come back empty. +// +// Provider names are safe to return to any authenticated viewer: they already +// travel in every SubtitleResult.provider and DownloadedSubtitle.provider. The +// credentials behind them stay in the admin-only provider config. +type subtitleProviderStatusResponse struct { + SchemaVersion int `json:"schema_version"` + // Enabled reports that at least one provider is registered, so + // POST /subtitles/search can actually reach an upstream. + Enabled bool `json:"enabled"` + // Providers is the registered provider identifiers, sorted. Always a + // list — empty rather than null — so clients can iterate it unguarded. + Providers []string `json:"providers"` +} + +// HandleProviderStatus reports whether external subtitle search is available +// here, so the player can show or hide the entry point. +// GET /api/v1/subtitles/providers/status +// +// It answers even when the subtitle subsystem is unwired, because enabled:false +// is the answer in that case; the router registers a fallback so a client never +// has to interpret a 404 on the probe itself. +func (h *SubtitleSearchHandler) HandleProviderStatus(w http.ResponseWriter, _ *http.Request) { + providers := []string{} + if h != nil && h.manager != nil { + providers = h.manager.ProviderNames() + } + writeJSON(w, http.StatusOK, subtitleProviderStatusResponse{ + SchemaVersion: 1, + Enabled: len(providers) > 0, + Providers: providers, + }) +} + +// WriteSubtitleProvidersDisabledStatus answers the subtitle provider capability +// probe with a 200 {"enabled": false, "providers": []} when no subtitle handler +// is wired, so the client gets a clean negative instead of a 404 (the 2-segment +// /providers/status path is not shadowed by the 1-segment /{media_file_id} +// route — they never compete in chi's router). +func WriteSubtitleProvidersDisabledStatus(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, subtitleProviderStatusResponse{ + SchemaVersion: 1, + Enabled: false, + Providers: []string{}, + }) +} + // HandleSearch handles POST /api/v1/subtitles/search func (h *SubtitleSearchHandler) HandleSearch(w http.ResponseWriter, r *http.Request) { var req searchSubtitlesRequest diff --git a/internal/api/handlers/subtitle_search_test.go b/internal/api/handlers/subtitle_search_test.go index 984c8fc6..9a977047 100644 --- a/internal/api/handlers/subtitle_search_test.go +++ b/internal/api/handlers/subtitle_search_test.go @@ -235,6 +235,109 @@ func TestHandleDeleteRequiresAccessToMediaFile(t *testing.T) { } } +// stubSubtitleProvider is a registerable no-op provider; only its name matters +// to the capability probe. +type stubSubtitleProvider struct { + name string +} + +func (s stubSubtitleProvider) Name() string { return s.name } + +func (s stubSubtitleProvider) Search(context.Context, subtitles.SearchRequest) ([]subtitles.SubtitleResult, error) { + return nil, nil +} + +func (s stubSubtitleProvider) Download(context.Context, string) ([]byte, subtitles.SubtitleFormat, error) { + return nil, subtitles.FormatSRT, nil +} + +func decodeProviderStatus(t *testing.T, rr *httptest.ResponseRecorder) struct { + SchemaVersion int `json:"schema_version"` + Enabled bool `json:"enabled"` + Providers []string `json:"providers"` +} { + t.Helper() + + var resp struct { + SchemaVersion int `json:"schema_version"` + Enabled bool `json:"enabled"` + Providers []string `json:"providers"` + } + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.SchemaVersion != 1 { + t.Fatalf("schema_version = %d, want 1", resp.SchemaVersion) + } + return resp +} + +func TestHandleProviderStatusWithoutProviders(t *testing.T) { + repo := newMockSubtitleRepoForHandler() + manager := subtitles.NewManager(repo, newMockS3ClientForHandler(), "test-bucket") + handler := NewSubtitleSearchHandler(manager, repo, stubSubtitleMediaResolver{}) + + rr := httptest.NewRecorder() + handler.HandleProviderStatus(rr, newSubtitleAuthRequest(http.MethodGet, "/subtitles/providers/status", nil)) + + resp := decodeProviderStatus(t, rr) + if resp.Enabled { + t.Fatal("enabled = true, want false with no providers registered") + } + if len(resp.Providers) != 0 { + t.Fatalf("providers = %v, want empty", resp.Providers) + } + // The empty list must reach the wire as [] — a null would read to a + // client as a missing field rather than "no providers here". + if !bytes.Contains(rr.Body.Bytes(), []byte(`"providers":[]`)) { + t.Fatalf("body = %s, want providers serialized as []", rr.Body.String()) + } +} + +func TestHandleProviderStatusWithProviders(t *testing.T) { + repo := newMockSubtitleRepoForHandler() + manager := subtitles.NewManager(repo, newMockS3ClientForHandler(), "test-bucket") + manager.RegisterProvider(stubSubtitleProvider{name: "subdl"}) + manager.RegisterProvider(stubSubtitleProvider{name: "opensubtitles"}) + handler := NewSubtitleSearchHandler(manager, repo, stubSubtitleMediaResolver{}) + + rr := httptest.NewRecorder() + handler.HandleProviderStatus(rr, newSubtitleAuthRequest(http.MethodGet, "/subtitles/providers/status", nil)) + + resp := decodeProviderStatus(t, rr) + if !resp.Enabled { + t.Fatal("enabled = false, want true with providers registered") + } + want := []string{"opensubtitles", "subdl"} + if len(resp.Providers) != len(want) { + t.Fatalf("providers = %v, want %v", resp.Providers, want) + } + for i, name := range want { + if resp.Providers[i] != name { + t.Fatalf("providers = %v, want %v (sorted)", resp.Providers, want) + } + } +} + +func TestWriteSubtitleProvidersDisabledStatus(t *testing.T) { + rr := httptest.NewRecorder() + WriteSubtitleProvidersDisabledStatus(rr, newSubtitleAuthRequest(http.MethodGet, "/subtitles/providers/status", nil)) + + resp := decodeProviderStatus(t, rr) + if resp.Enabled { + t.Fatal("enabled = true, want false from the disabled fallback") + } + if len(resp.Providers) != 0 { + t.Fatalf("providers = %v, want empty", resp.Providers) + } + if !bytes.Contains(rr.Body.Bytes(), []byte(`"providers":[]`)) { + t.Fatalf("body = %s, want providers serialized as []", rr.Body.String()) + } +} + type handlerMockSubtitleRepo struct { subtitles map[int]*subtitles.DownloadedSubtitle nextID int diff --git a/internal/api/router.go b/internal/api/router.go index cc413867..118bf93a 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -2575,6 +2575,11 @@ func NewRouter(deps Dependencies) chi.Router { } } r.Route("/subtitles", func(r chi.Router) { + // Capability probe for external subtitle search. Two + // segments on purpose: a bare /providers would shadow + // the /{media_file_id} route below, while + // /providers/status never competes with it in chi. + r.Get("/providers/status", subtitleSearchHandler.HandleProviderStatus) r.Post("/search", subtitleSearchHandler.HandleSearch) r.Post("/download", subtitleSearchHandler.HandleDownload) r.Post("/upload", subtitleSearchHandler.HandleUpload) @@ -2595,6 +2600,17 @@ func NewRouter(deps Dependencies) chi.Router { r.Get("/{media_file_id}", subtitleSearchHandler.HandleList) r.Delete("/{id}", subtitleSearchHandler.HandleDelete) }) + } else { + // The whole group above is conditional (it needs the DB, + // S3 and the subtitle repo), so on a storage-less + // deployment the capability probe would 404 — leaving a + // client to interpret the same ambiguous status the probe + // exists to replace. Mount the probe alone, answering + // enabled:false, so feature detection always gets a real + // answer. + r.Route("/subtitles", func(r chi.Router) { + r.Get("/providers/status", handlers.WriteSubtitleProvidersDisabledStatus) + }) } // Playback routes. diff --git a/internal/subtitles/manager.go b/internal/subtitles/manager.go index a6f61336..138997d7 100644 --- a/internal/subtitles/manager.go +++ b/internal/subtitles/manager.go @@ -52,6 +52,24 @@ func (m *Manager) RemoveProvider(name string) { delete(m.providers, name) } +// ProviderNames returns the names of every currently registered provider, +// sorted for a stable response. Empty when none are configured. +// +// The slice is always non-nil so callers can hand it straight to a JSON +// response without it marshalling as null — clients feature-detecting subtitle +// search read an empty list as "no providers here", not as a missing field. +func (m *Manager) ProviderNames() []string { + m.mu.RLock() + names := make([]string, 0, len(m.providers)) + for name := range m.providers { + names = append(names, name) + } + m.mu.RUnlock() + + sort.Strings(names) + return names +} + // Search fans out to all registered providers concurrently. func (m *Manager) Search(ctx context.Context, req SearchRequest) (*SearchResponse, error) { m.mu.RLock() diff --git a/internal/subtitles/manager_test.go b/internal/subtitles/manager_test.go index a03c64c8..e549e380 100644 --- a/internal/subtitles/manager_test.go +++ b/internal/subtitles/manager_test.go @@ -116,6 +116,53 @@ func (m *mockS3Client) DeleteObject(_ context.Context, _, key string) error { return nil } +type stubProvider struct { + name string +} + +func (s stubProvider) Name() string { return s.name } + +func (s stubProvider) Search(context.Context, SearchRequest) ([]SubtitleResult, error) { + return nil, nil +} + +func (s stubProvider) Download(context.Context, string) ([]byte, SubtitleFormat, error) { + return nil, FormatSRT, nil +} + +func TestManagerProviderNames(t *testing.T) { + manager := NewManager(newMockSubtitleRepo(), newMockS3Client(), "test-bucket") + + names := manager.ProviderNames() + if names == nil { + t.Fatal("ProviderNames() = nil, want non-nil empty slice") + } + if len(names) != 0 { + t.Fatalf("ProviderNames() = %v, want empty", names) + } + + manager.RegisterProvider(stubProvider{name: "subdl"}) + manager.RegisterProvider(stubProvider{name: "opensubtitles"}) + manager.RegisterProvider(stubProvider{name: "subsource"}) + + want := []string{"opensubtitles", "subdl", "subsource"} + names = manager.ProviderNames() + if len(names) != len(want) { + t.Fatalf("ProviderNames() = %v, want %v", names, want) + } + for i, name := range want { + if names[i] != name { + t.Fatalf("ProviderNames() = %v, want %v (sorted)", names, want) + } + } + + manager.RemoveProvider("subdl") + names = manager.ProviderNames() + if len(names) != 2 || names[0] != "opensubtitles" || names[1] != "subsource" { + t.Fatalf("ProviderNames() after removal = %v, want [opensubtitles subsource]", names) + } +} + func TestManagerUploadStoresSubtitle(t *testing.T) { repo := newMockSubtitleRepo() s3 := newMockS3Client()