diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index 62c2ee3..ddfa76f 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -832,16 +832,28 @@ func mapPresetsToFullResponse(presets []models.ServicePreset, sources []models.C } var matchedSource *models.ConfiguredSource - // 1. Try exact ID match first + // 1. Exact ID match — but only if the source's type is + // compatible with what the preset claims. GH-343: a numeric + // SourceID collision between e.g. a TuneIn preset and a + // RADIOPLAYER source in the configured-sources list used to + // silently rewrite the preset's source attribute to + // RADIOPLAYER on the next /full sync. Strict-match refuses + // the bind when the types disagree; the fallback below then + // finds the right source (or synthesise/skip kicks in). if p.SourceID != "" { for j := range sources { - if sources[j].ID == p.SourceID { + if sources[j].ID == p.SourceID && sourceTypeCompatible(p.Source, sources[j].SourceKeyType) { copySource := sources[j] PrepareConfiguredSource(©Source) matchedSource = ©Source break } + + if sources[j].ID == p.SourceID && !sourceTypeCompatible(p.Source, sources[j].SourceKeyType) { + log.Printf("[Marge] /full: refusing cross-type bind for preset %s — preset.Source=%q but configured source id=%s has type %q; falling through to type-based match", + p.ButtonNumber, p.Source, sources[j].ID, sources[j].SourceKeyType) + } } } @@ -953,17 +965,24 @@ func synthesiseDefaultSourceByType(sourceKeyType string) (models.FullResponseSou } // findMatchingSourceForRecent resolves the configured source a recent -// references. Tries exact SourceID match first, then SourceKeyType+account. -// Returns nil when nothing matches — caller falls back to synthesise/skip. +// references. Tries exact SourceID match (but only if the matched source's +// type is compatible with the recent's claimed Source — see GH-343), then +// SourceKeyType+account. Returns nil when nothing matches; caller falls +// back to synthesise/skip. func findMatchingSourceForRecent(r *models.ServiceRecent, sources []models.ConfiguredSource) *models.ConfiguredSource { if r.SourceID != "" { for j := range sources { - if sources[j].ID == r.SourceID { + if sources[j].ID == r.SourceID && sourceTypeCompatible(r.Source, sources[j].SourceKeyType) { copySource := sources[j] PrepareConfiguredSource(©Source) return ©Source } + + if sources[j].ID == r.SourceID && !sourceTypeCompatible(r.Source, sources[j].SourceKeyType) { + log.Printf("[Marge] /full: refusing cross-type bind for recent id=%s — recent.Source=%q but configured source id=%s has type %q; falling through to type-based match", + r.ID, r.Source, sources[j].ID, sources[j].SourceKeyType) + } } } @@ -980,6 +999,20 @@ func findMatchingSourceForRecent(r *models.ServiceRecent, sources []models.Confi return nil } +// sourceTypeCompatible decides whether a preset/recent whose original +// source is `claimed` can be bound to a configured source of type +// `configured`. Strict-match policy: identical types compatible; either +// side empty is treated as "no info, allow" (matches pre-fix behaviour for +// records that lost their Source attribute). Refuse when both are set and +// disagree — that's the GH-343 cross-type rewrite the speaker reflects. +func sourceTypeCompatible(claimed, configured string) bool { + if claimed == "" || configured == "" { + return true + } + + return claimed == configured +} + func mapRecentsToFullResponse(recents []models.ServiceRecent, sources []models.ConfiguredSource) []models.FullResponseRecent { var fullRecents []models.FullResponseRecent diff --git a/pkg/service/marge/presets_unresolved_source_regression_test.go b/pkg/service/marge/presets_unresolved_source_regression_test.go index 49eb0fb..d34554f 100644 --- a/pkg/service/marge/presets_unresolved_source_regression_test.go +++ b/pkg/service/marge/presets_unresolved_source_regression_test.go @@ -258,6 +258,85 @@ func TestMapRecentsToFullResponse_UnresolvedSource(t *testing.T) { } } +// TestMapPresetsToFullResponse_StrictTypeMatch_GH343 is the regression for +// GH-343: a preset stored with Source=TUNEIN used to come back from /full +// rebound to RADIOPLAYER because the numeric SourceID collided with a +// RADIOPLAYER source in the configured-sources list. The speaker, trusting +// /full as ground truth, then locally re-attributed the preset's source. +// +// Strict-match refuses to bind a TUNEIN preset to a RADIOPLAYER source, +// even when the IDs match. The fallback type-search then finds the right +// TUNEIN source. +func TestMapPresetsToFullResponse_StrictTypeMatch_GH343(t *testing.T) { + // Two configured sources with the same numeric ID. In real life they'd + // be from different devices/accounts; the bug surfaced when a stale + // migration left a RADIOPLAYER source with the same ID a TUNEIN preset + // referenced. Order matters here — RADIOPLAYER comes first so the + // strict-match has to actively skip it to reach the TUNEIN entry. + configured := []models.ConfiguredSource{ + { + ID: "10004", + Type: "Audio", + SourceKeyType: "RADIOPLAYER", + SourceProviderID: "35", + }, + { + ID: "10004", + Type: "Audio", + SourceKeyType: constants.ProviderTunein, + SourceProviderID: "25", + }, + } + + presets := []models.ServicePreset{ + { + ServiceContentItem: models.ServiceContentItem{ + Name: "MDR JUMP", + Source: constants.ProviderTunein, + SourceID: "10004", + Location: "/v1/playback/station/s6634", + ContentItemType: "stationurl", + }, + ButtonNumber: "1", + }, + } + + got := mapPresetsToFullResponse(presets, configured) + + if len(got) != 1 { + t.Fatalf("expected 1 emitted preset, got %d", len(got)) + } + + if got[0].Source.SourceProviderID != "25" { + t.Errorf("strict-match should have bound to TuneIn (sourceproviderid=25), got %q — cross-type bind not refused?", got[0].Source.SourceProviderID) + } +} + +// TestSourceTypeCompatible pins the strict-match policy: +// - identical types compatible +// - either side empty treated as "no info, allow" +// - both set and different — refused +func TestSourceTypeCompatible(t *testing.T) { + cases := []struct { + claimed, configured string + want bool + }{ + {"TUNEIN", "TUNEIN", true}, + {"TUNEIN", "RADIOPLAYER", false}, + {"", "TUNEIN", true}, + {"TUNEIN", "", true}, + {"", "", true}, + {"SPOTIFY", "SPOTIFY", true}, + {"SPOTIFY", "AMAZON", false}, + } + + for _, tc := range cases { + if got := sourceTypeCompatible(tc.claimed, tc.configured); got != tc.want { + t.Errorf("sourceTypeCompatible(%q, %q) = %v, want %v", tc.claimed, tc.configured, got, tc.want) + } + } +} + // TestCanonicalDefaultsByType pins the type → (id, providerid) mapping // against the canonical IDs the speaker firmware ships with. // canonicalProviderIDByID (the inverse) is already tested implicitly via