fix(metadata): stop manual rematch from resurrecting recorded stale IDs (#276)
* fix(metadata): stop manual rematch from resurrecting recorded stale IDs The Apply Match flow (ModeIdentify) re-injected durable provider IDs into the identify request without checking stale_media_ids, so a known-dead tmdb ID rode along, 404ed again during the Phase-2 fetch, and was re-recorded with a fresh last_seen_at — the item never left the Stale External IDs list and jumped back to the top after every rematch. Filter recorded-stale IDs out of the injected durable set in prepareProcessRequest. Caller-supplied IDs are untouched, so an admin deliberately re-selecting a previously-stale ID still retries it (which is also why the ModeIdentify suppression guard in processInternal stays). Fixes #268 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(metadata): normalize provider-id keys so stale-ID suppression can't be bypassed by casing Review on PR #276 flagged that suppressRecordedStaleProviderIDs lowercases and trims the stored stale row's provider before looking it up in the incoming map, while the map keys are used verbatim, and that HandleApplyItemMatch passes req.ProviderIDs from the JSON body straight into metadata.Process without the normalization the search endpoint applies. A caller-supplied key like "TMDB" or " tmdb " therefore defeated the suppression. The same normalization gap was previously flagged on PR #182. Fix both layers: - HandleApplyItemMatch now runs req.ProviderIDs through normalizeMatchProviderIDs (same semantics as the search endpoint) and returns 400 when no non-blank entries remain, mirroring the existing empty-map rejection. - suppressRecordedStaleProviderIDs now indexes the incoming map by normalized key and deletes the matching original keys, so suppression is robust regardless of caller casing or padding. Adds regression tests at both layers: a metadata-level case where the durable row arrives as "TMDB " while the stale row records "tmdb", and handler-level cases asserting apply normalizes keys/values and rejects all-blank provider-id maps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Quick <31828688+Quick104@users.noreply.github.com>
This commit is contained in:
fluxis
committed by
GitHub
GitHub
co-authored by
Claude Fable 5
Quick
Claude Fable 5
Quick
parent
012006e7a7
commit
c75c519e3d
@@ -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,
|
||||
})
|
||||
|
||||
@@ -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{})
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user