diff --git a/internal/api/handlers/admin_match.go b/internal/api/handlers/admin_match.go index c1e85b44..3d1b6acd 100644 --- a/internal/api/handlers/admin_match.go +++ b/internal/api/handlers/admin_match.go @@ -161,7 +161,11 @@ func (h *AdminMatchHandler) HandleApplyItemMatch(w http.ResponseWriter, r *http. return } - if len(req.ProviderIDs) == 0 { + // Normalize keys and values the same way the search endpoint does so a + // caller-supplied key like "TMDB" or " tmdb " cannot bypass downstream + // provider-id handling (e.g. stale-ID suppression in metadata.Process). + providerIDs := normalizeMatchProviderIDs(req.ProviderIDs) + if len(providerIDs) == 0 { writeError(w, http.StatusBadRequest, "bad_request", "At least one provider ID is required") return } @@ -191,7 +195,7 @@ func (h *AdminMatchHandler) HandleApplyItemMatch(w http.ResponseWriter, r *http. result, err := h.metadata.Process(r.Context(), metadata.ProcessRequest{ ContentID: contentID, - ProviderIDs: req.ProviderIDs, + ProviderIDs: providerIDs, FolderID: folderIDStr, Mode: metadata.ModeIdentify, }) diff --git a/internal/api/handlers/admin_match_test.go b/internal/api/handlers/admin_match_test.go index fb480108..005f6251 100644 --- a/internal/api/handlers/admin_match_test.go +++ b/internal/api/handlers/admin_match_test.go @@ -326,6 +326,67 @@ func TestAdminMatchApply_RequiresProviderIDs(t *testing.T) { } } +func TestAdminMatchApply_NormalizesProviderIDKeys(t *testing.T) { + items := &fakeMatchItemLookup{ + items: map[string]*models.MediaItem{ + "item-5": {ContentID: "item-5", Title: "Test", Type: "movie"}, + }, + } + metaSvc := &fakeMatchMetadataService{ + processResult: &metadata.ProcessResult{ContentID: "item-5", Updated: true}, + } + h := NewAdminMatchHandler(items, nil, metaSvc) + router := buildMatchRouter(h) + + body, _ := json.Marshal(matchApplyRequest{ + ProviderIDs: map[string]string{ + " TMDB ": " 157336 ", + "blank": " ", + }, + }) + req := httptest.NewRequest(http.MethodPost, "/admin/items/item-5/match/apply", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if got := metaSvc.lastProcess.ProviderIDs["tmdb"]; got != "157336" { + t.Errorf("Process provider_ids[tmdb] = %q, want 157336 (keys/values must be normalized)", got) + } + if _, ok := metaSvc.lastProcess.ProviderIDs[" TMDB "]; ok { + t.Errorf("Process received raw key %q, want normalized keys only: %v", " TMDB ", metaSvc.lastProcess.ProviderIDs) + } + if _, ok := metaSvc.lastProcess.ProviderIDs["blank"]; ok { + t.Errorf("blank provider IDs should be dropped, got %v", metaSvc.lastProcess.ProviderIDs) + } +} + +func TestAdminMatchApply_RejectsAllBlankProviderIDs(t *testing.T) { + items := &fakeMatchItemLookup{ + items: map[string]*models.MediaItem{ + "item-6": {ContentID: "item-6", Title: "Test", Type: "movie"}, + }, + } + h := NewAdminMatchHandler(items, nil, &fakeMatchMetadataService{}) + router := buildMatchRouter(h) + + body, _ := json.Marshal(matchApplyRequest{ + ProviderIDs: map[string]string{"tmdb": " ", " ": "123"}, + }) + req := httptest.NewRequest(http.MethodPost, "/admin/items/item-6/match/apply", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for all-blank provider_ids, got %d: %s", rec.Code, rec.Body.String()) + } +} + func TestAdminMatchApply_ItemNotFound(t *testing.T) { items := &fakeMatchItemLookup{items: map[string]*models.MediaItem{}} h := NewAdminMatchHandler(items, nil, &fakeMatchMetadataService{}) diff --git a/internal/metadata/identify_stale_id_test.go b/internal/metadata/identify_stale_id_test.go new file mode 100644 index 00000000..6f4fbc3c --- /dev/null +++ b/internal/metadata/identify_stale_id_test.go @@ -0,0 +1,245 @@ +package metadata + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + + "github.com/Silo-Server/silo-server/internal/models" +) + +// notFoundMetadataProvider simulates a provider whose recorded external ID no +// longer exists upstream: any metadata fetch that reaches it 404s. +type notFoundMetadataProvider struct { + slug string + + mu sync.Mutex + called bool + reqIDs map[string]string +} + +func (p *notFoundMetadataProvider) Slug() string { return p.slug } + +func (p *notFoundMetadataProvider) Name() string { return p.slug } + +func (p *notFoundMetadataProvider) ForTypes() []string { return []string{"movie", "series"} } + +func (p *notFoundMetadataProvider) GetMetadata(_ context.Context, req MetadataRequest) (*MetadataResult, error) { + p.mu.Lock() + p.called = true + p.reqIDs = copyMap(req.ProviderIDs) + p.mu.Unlock() + return nil, errors.New(p.slug + ": HTTP 404: not found") +} + +// TestProcess_IdentifyDoesNotResurrectRecordedStaleProviderID reproduces +// issue #268: an item has a durable tmdb ID already recorded as stale; the +// admin rematches it to a different provider via the Apply Match flow +// (ModeIdentify). The stale tmdb ID must not be re-injected into the identify +// request, and after the successful rematch the item's stale rows must be +// cleared rather than re-recorded with a fresh last_seen. +func TestProcess_IdentifyDoesNotResurrectRecordedStaleProviderID(t *testing.T) { + h := newTestHarness() + ctx := context.Background() + + if err := h.itemRepo.Upsert(ctx, &models.MediaItem{ + ContentID: "existing-1", + Type: "series", + Title: "Formula 1", + Year: 2016, + Status: "matched", + Studios: []string{}, + Networks: []string{}, + Countries: []string{}, + Genres: []string{}, + }); err != nil { + t.Fatalf("upsert existing item: %v", err) + } + + providerRepo := newFakeProviderIDRepo() + providerRepo.set("existing-1", &models.MediaItemProviderID{ + ContentID: "existing-1", + ItemType: "series", + Provider: "tmdb", + ProviderID: "324880", + }) + h.service.providerIDRepo = providerRepo + + staleRepo := newFakeStaleIDRepo() + staleRepo.set("existing-1", &models.StaleMediaID{ + ContentID: "existing-1", + Provider: "tmdb", + ProviderID: "324880", + }) + h.service.staleIDRepo = staleRepo + + tmdb := ¬FoundMetadataProvider{slug: "tmdb"} + tvdb := &capturingMetadataProvider{ + response: &MetadataResult{ + HasMetadata: true, + Title: "Formula 1: Drive to Survive", + ProviderIDs: map[string]string{"tvdb": "417585"}, + }, + } + + result, err := h.service.ProcessWithProviders(ctx, ProcessRequest{ + ContentID: "existing-1", + ProviderIDs: map[string]string{"tvdb": "417585"}, + Language: "en", + Mode: ModeIdentify, + }, []Provider{tmdb, tvdb}) + if err != nil { + t.Fatalf("ProcessWithProviders: %v", err) + } + if result == nil || !result.Updated { + t.Fatalf("result = %#v, want Updated=true", result) + } + + req := tvdb.lastRequest() + if got := req.ProviderIDs["tmdb"]; got != "" { + t.Errorf("identify request tmdb id = %q, want empty (stale durable id must not be re-injected)", got) + } + if got := req.ProviderIDs["tvdb"]; got != "417585" { + t.Errorf("identify request tvdb id = %q, want 417585", got) + } + + stale, err := staleRepo.GetByContentID(ctx, "existing-1") + if err != nil { + t.Fatalf("get stale rows: %v", err) + } + if len(stale) != 0 { + t.Errorf("stale rows after successful rematch = %v, want none", stale) + } +} + +// TestProcess_IdentifySuppressesStaleIDDespiteKeyCasing guards the +// normalization layer: the stale row is recorded with the canonical +// lower-case provider slug, but the durable row (and therefore the injected +// provider-id map key) arrives with different casing and padding ("TMDB "). +// Suppression must still match the two and drop the stale ID. +func TestProcess_IdentifySuppressesStaleIDDespiteKeyCasing(t *testing.T) { + h := newTestHarness() + ctx := context.Background() + + if err := h.itemRepo.Upsert(ctx, &models.MediaItem{ + ContentID: "existing-1", + Type: "series", + Title: "Formula 1", + Year: 2016, + Status: "matched", + Studios: []string{}, + Networks: []string{}, + Countries: []string{}, + Genres: []string{}, + }); err != nil { + t.Fatalf("upsert existing item: %v", err) + } + + providerRepo := newFakeProviderIDRepo() + providerRepo.set("existing-1", &models.MediaItemProviderID{ + ContentID: "existing-1", + ItemType: "series", + Provider: "TMDB ", + ProviderID: "324880", + }) + h.service.providerIDRepo = providerRepo + + staleRepo := newFakeStaleIDRepo() + staleRepo.set("existing-1", &models.StaleMediaID{ + ContentID: "existing-1", + Provider: "tmdb", + ProviderID: "324880", + }) + h.service.staleIDRepo = staleRepo + + tmdb := ¬FoundMetadataProvider{slug: "tmdb"} + tvdb := &capturingMetadataProvider{ + response: &MetadataResult{ + HasMetadata: true, + Title: "Formula 1: Drive to Survive", + ProviderIDs: map[string]string{"tvdb": "417585"}, + }, + } + + result, err := h.service.ProcessWithProviders(ctx, ProcessRequest{ + ContentID: "existing-1", + ProviderIDs: map[string]string{"tvdb": "417585"}, + Language: "en", + Mode: ModeIdentify, + }, []Provider{tmdb, tvdb}) + if err != nil { + t.Fatalf("ProcessWithProviders: %v", err) + } + if result == nil || !result.Updated { + t.Fatalf("result = %#v, want Updated=true", result) + } + + req := tvdb.lastRequest() + for key, value := range req.ProviderIDs { + if strings.EqualFold(strings.TrimSpace(key), "tmdb") { + t.Errorf("identify request still carries %q=%q, want stale tmdb id suppressed despite key casing", key, value) + } + } + if got := req.ProviderIDs["tvdb"]; got != "417585" { + t.Errorf("identify request tvdb id = %q, want 417585", got) + } +} + +// TestProcess_IdentifyKeepsUserSuppliedIDEvenIfRecordedStale documents the +// deliberate exception: when the admin explicitly re-selects the very ID that +// was recorded stale (e.g. the provider has since fixed it), identify must +// retry that ID instead of silently suppressing it. +func TestProcess_IdentifyKeepsUserSuppliedIDEvenIfRecordedStale(t *testing.T) { + h := newTestHarness() + ctx := context.Background() + + if err := h.itemRepo.Upsert(ctx, &models.MediaItem{ + ContentID: "existing-1", + Type: "series", + Title: "Formula 1", + Year: 2016, + Status: "matched", + Studios: []string{}, + Networks: []string{}, + Countries: []string{}, + Genres: []string{}, + }); err != nil { + t.Fatalf("upsert existing item: %v", err) + } + + staleRepo := newFakeStaleIDRepo() + staleRepo.set("existing-1", &models.StaleMediaID{ + ContentID: "existing-1", + Provider: "tmdb", + ProviderID: "324880", + }) + h.service.staleIDRepo = staleRepo + + provider := &capturingMetadataProvider{ + response: &MetadataResult{ + HasMetadata: true, + Title: "Formula 1", + ProviderIDs: map[string]string{"tmdb": "324880"}, + }, + } + + result, err := h.service.ProcessWithProviders(ctx, ProcessRequest{ + ContentID: "existing-1", + ProviderIDs: map[string]string{"tmdb": "324880"}, + Language: "en", + Mode: ModeIdentify, + }, []Provider{provider}) + if err != nil { + t.Fatalf("ProcessWithProviders: %v", err) + } + if result == nil || !result.Updated { + t.Fatalf("result = %#v, want Updated=true", result) + } + + req := provider.lastRequest() + if got := req.ProviderIDs["tmdb"]; got != "324880" { + t.Errorf("identify request tmdb id = %q, want 324880 (user-supplied id must survive)", got) + } +} diff --git a/internal/metadata/service.go b/internal/metadata/service.go index b202b6e1..3b91eb3d 100644 --- a/internal/metadata/service.go +++ b/internal/metadata/service.go @@ -883,6 +883,20 @@ func (s *MetadataService) suppressRecordedStaleProviderIDs( if err != nil { return fmt.Errorf("loading stale provider ids for %s: %w", contentID, err) } + if len(staleIDs) == 0 { + return nil + } + // Index the incoming map by normalized provider key so suppression cannot + // be bypassed by casing or padding differences between the stored stale + // row and the caller-supplied map keys (e.g. "TMDB" vs "tmdb"). + keysByProvider := make(map[string][]string, len(providerIDs)) + for key := range providerIDs { + normalized := strings.ToLower(strings.TrimSpace(key)) + if normalized == "" { + continue + } + keysByProvider[normalized] = append(keysByProvider[normalized], key) + } for _, staleID := range staleIDs { if staleID == nil { continue @@ -891,10 +905,13 @@ func (s *MetadataService) suppressRecordedStaleProviderIDs( if provider == "" { continue } - if providerIDs[provider] != strings.TrimSpace(staleID.ProviderID) { - continue + staleValue := strings.TrimSpace(staleID.ProviderID) + for _, key := range keysByProvider[provider] { + if strings.TrimSpace(providerIDs[key]) != staleValue { + continue + } + delete(providerIDs, key) } - delete(providerIDs, provider) } return nil } @@ -918,6 +935,19 @@ func (s *MetadataService) prepareProcessRequest(ctx context.Context, req Process if len(durableIDs) == 0 { return req, nil } + // Strip durable IDs already recorded as stale before merging them into + // the request. Without this, a manual rematch (ModeIdentify) re-injects + // the known-dead ID, the Phase-2 fetch 404s again, and the item is + // re-recorded in stale_media_ids instead of leaving the stale list. + // Only the injected set is filtered: IDs the caller supplied explicitly + // in req.ProviderIDs stay untouched, so an admin deliberately + // re-selecting a previously-stale ID still retries it. + if err := s.suppressRecordedStaleProviderIDs(ctx, req.ContentID, durableIDs); err != nil { + return req, err + } + if len(durableIDs) == 0 { + return req, nil + } if req.ProviderIDs == nil { req.ProviderIDs = make(map[string]string, len(durableIDs)) }