From f899dbaa89cd2d489f0cca8525ec77e030e095c8 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Mon, 10 Aug 2026 14:19:16 +0200 Subject: [PATCH] test(marge): guard source XML shape; feat(library): merge speaker-side media server discovery Comparing against JRpersonal/streborn#587 surfaced two gaps: no test pinned that a newly added source type renders the same element shape as a known-good default (the firmware rejects the whole account document if one source entry omits an expected element), and our DLNA discovery only swept SSDP from the service host, missing servers only visible from a paired speaker's own LAN segment. Adds TestSourceXMLShapeConsistencyAcrossTypes in pkg/service/marge, and has HandleDiscoverLibraryServers merge results from each paired speaker's own /listMediaServers alongside the existing SSDP sweep, deduped by UDN, with unreachable speakers skipped silently. Co-Authored-By: Claude Sonnet 5 --- pkg/service/marge/marge_test.go | 171 ++++++++++++++++++ pkg/service/soundtouchweb/handler_library.go | 115 +++++++++++- .../soundtouchweb/handler_library_test.go | 115 ++++++++++++ 3 files changed, 395 insertions(+), 6 deletions(-) diff --git a/pkg/service/marge/marge_test.go b/pkg/service/marge/marge_test.go index 3521a8e..ef94829 100644 --- a/pkg/service/marge/marge_test.go +++ b/pkg/service/marge/marge_test.go @@ -1,6 +1,7 @@ package marge import ( + "encoding/xml" "os" "path/filepath" "strconv" @@ -9,6 +10,7 @@ import ( "time" "github.com/gesellix/bose-soundtouch/pkg/models" + "github.com/gesellix/bose-soundtouch/pkg/service/constants" "github.com/gesellix/bose-soundtouch/pkg/service/datastore" ) @@ -784,3 +786,172 @@ func TestAccountFullToXML_WithBackupStructure(t *testing.T) { t.Errorf("Expected or or fallback name, got %s", string(fullXML2)) } } + +// extractSourceFragment returns the raw `...` +// substring for one source out of a rendered account document. Tests need the +// raw wire text, not an unmarshaled struct, because Go's XML decoder can't +// distinguish "element present but empty" from "element absent" — and that +// distinction is exactly what has broken parsing on real speakers before +// (issue #195, #334). +func extractSourceFragment(t *testing.T, doc, id string) string { + t.Helper() + + marker := `") + if end < 0 { + t.Fatalf("source id=%q has no closing :\n%s", id, doc) + } + + return doc[start : start+end+len("")] +} + +// xmlElementNames returns the ordered sequence of start-tag element names in +// an XML fragment (attributes and closing tags are ignored). Used to compare +// the "shape" of two rendered entries without caring about their +// differing content. +func xmlElementNames(fragment string) []string { + var out []string + + for _, part := range strings.Split(fragment, "<") { + if i := strings.IndexAny(part, " >/"); i > 0 { + out = append(out, part[:i]) + } + } + + return out +} + +// TestSourceXMLShapeConsistencyAcrossTypes guards against a known Bose +// firmware failure mode: a entry that omits an element the firmware +// expects makes the speaker reject the *whole* account document, not just +// that entry (see the AccountFullToXML sourceproviderid comment above, and +// issues #195/#334). A newly added source type (here STORED_MUSIC, as used by +// the DLNA/UPnP media-library feature) must render with the exact same +// element set, in the same order, as an existing known-good default source — +// content may legitimately differ, element names may not. +func TestSourceXMLShapeConsistencyAcrossTypes(t *testing.T) { + tempDir, err := os.MkdirTemp("", "marge-test-shape-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer func() { _ = os.RemoveAll(tempDir) }() + + ds := datastore.NewDataStore(tempDir) + account := "7654321" + device := "AABBCCDDEE0B" + + info := &models.ServiceDeviceInfo{ + DeviceID: device, + Name: "Office SoundTouch", + } + _ = ds.SaveDeviceInfo(account, device, info) + _ = ds.SavePresets(account, device, []models.ServicePreset{}) + _ = ds.SaveRecents(account, device, []models.ServiceRecent{}) + + // A STORED_MUSIC entry as HandleAddLibraryServer's registration flow would + // produce it: no SourceProviderID set explicitly, so PrepareConfiguredSource + // must resolve it via constants.StaticProviders at render time, exactly like + // a freshly registered DLNA media server would. + stored := models.ConfiguredSource{ + ID: "20001", + DisplayName: "FRITZ!Mediaserver", + Type: "Audio", + Name: "FRITZ!Mediaserver", + SourceName: constants.ProviderStoredMusic, + Username: "fa095ecc-uuid/0", + } + stored.SourceKey.Type = constants.ProviderStoredMusic + stored.SourceKey.Account = "fa095ecc-uuid/0" + stored.SourceKeyType = constants.ProviderStoredMusic + stored.SourceKeyAccount = "fa095ecc-uuid/0" + + if err := ds.SaveConfiguredSources(account, device, []models.ConfiguredSource{stored}); err != nil { + t.Fatalf("SaveConfiguredSources: %v", err) + } + + tunein := strconv.Itoa(constants.TuneinProviderID) + storedMusicID := strconv.Itoa(constants.StoredMusicProviderID) + + cases := []struct { + name string + render func() ([]byte, error) + }{ + {"AccountFullToXML", func() ([]byte, error) { return AccountFullToXML(ds, account) }}, + {"AccountSourcesToXML", func() ([]byte, error) { return AccountSourcesToXML(ds, account) }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + data, err := tc.render() + if err != nil { + t.Fatalf("%s failed: %v", tc.name, err) + } + + var sources []models.FullResponseSource + + switch tc.name { + case "AccountFullToXML": + var resp models.AccountFullResponse + if uerr := xml.Unmarshal(data, &resp); uerr != nil { + t.Fatalf("%s: whole-document unmarshal failed: %v\n%s", tc.name, uerr, data) + } + sources = resp.Sources + case "AccountSourcesToXML": + var resp models.AccountSourcesResponse + if uerr := xml.Unmarshal(data, &resp); uerr != nil { + t.Fatalf("%s: whole-document unmarshal failed: %v\n%s", tc.name, uerr, data) + } + sources = resp.Sources + } + + // Defaults minus AUX (filtered out of /full and /sources on purpose, + // see the getAccountSources comment) plus our one extra STORED_MUSIC entry. + wantCount := len(ds.GetInitialSources()) - 1 + 1 + if len(sources) != wantCount { + t.Fatalf("%s: got %d sources, want %d:\n%s", tc.name, len(sources), wantCount, data) + } + + var tuneinSource, storedMusicSource *models.FullResponseSource + for i := range sources { + switch sources[i].SourceProviderID { + case tunein: + tuneinSource = &sources[i] + case storedMusicID: + storedMusicSource = &sources[i] + } + } + + if tuneinSource == nil { + t.Fatalf("%s: TUNEIN source missing from rendered document:\n%s", tc.name, data) + } + if storedMusicSource == nil { + t.Fatalf("%s: STORED_MUSIC source missing, or its sourceproviderid did not resolve to %q:\n%s", tc.name, storedMusicID, data) + } + if storedMusicSource.ID == tuneinSource.ID { + t.Errorf("%s: STORED_MUSIC source id %q collides with a default source id", tc.name, storedMusicSource.ID) + } + + tuneinFragment := extractSourceFragment(t, string(data), tuneinSource.ID) + storedFragment := extractSourceFragment(t, string(data), storedMusicSource.ID) + + wantShape := xmlElementNames(tuneinFragment) + gotShape := xmlElementNames(storedFragment) + + if len(wantShape) != len(gotShape) { + t.Fatalf("%s: element count differs from a known-good default source:\n default (TUNEIN): %v\n STORED_MUSIC: %v", tc.name, wantShape, gotShape) + } + + for i := range wantShape { + if wantShape[i] != gotShape[i] { + t.Errorf("%s: element %d differs: default (TUNEIN) %q, STORED_MUSIC %q", tc.name, i, wantShape[i], gotShape[i]) + } + } + }) + } +} diff --git a/pkg/service/soundtouchweb/handler_library.go b/pkg/service/soundtouchweb/handler_library.go index 7339ac9..9ac1798 100644 --- a/pkg/service/soundtouchweb/handler_library.go +++ b/pkg/service/soundtouchweb/handler_library.go @@ -6,8 +6,10 @@ import ( "net/url" "strconv" "strings" + "sync" "time" + "github.com/gesellix/bose-soundtouch/pkg/client" "github.com/gesellix/bose-soundtouch/pkg/discovery" "github.com/gesellix/bose-soundtouch/pkg/models" "github.com/gesellix/bose-soundtouch/pkg/service/soundtouchweb/webtypes" @@ -55,10 +57,17 @@ func normalizeUDN(s string) string { } // HandleDiscoverLibraryServers performs a LAN-wide SSDP sweep for DLNA media -// servers and returns them as a JSON array. An optional ?timeout= query -// parameter (in seconds, integer) overrides the default 5-second budget. +// servers, plus a query to every paired speaker's own /listMediaServers, and +// returns the merged set as a JSON array. An optional ?timeout= query +// parameter (in seconds, integer) overrides the default 5-second SSDP budget. // This handler is global (not device-scoped) and lives under // /api/control/providers/library/servers. +// +// The two sources see different networks: our SSDP sweep runs from the +// AfterTouch service host, while each speaker's /listMediaServers reflects +// what that speaker sees on its own LAN segment. They can disagree when the +// service isn't co-located with the speaker (different subnet/VLAN), so a +// server invisible to one path may still be visible via the other. func (app *WebApp) HandleDiscoverLibraryServers(w http.ResponseWriter, r *http.Request) { timeout := 5 * time.Second @@ -74,15 +83,44 @@ func (app *WebApp) HandleDiscoverLibraryServers(w http.ResponseWriter, r *http.R return } - out := make([]libraryServer, 0, len(servers)) + byUDN := make(map[string]libraryServer, len(servers)) + order := make([]string, 0, len(servers)) + for _, s := range servers { - out = append(out, libraryServer{ - UDN: normalizeUDN(s.UDN), + udn := normalizeUDN(s.UDN) + byUDN[udn] = libraryServer{ + UDN: udn, Name: s.FriendlyName, Manufacturer: s.Manufacturer, Model: s.ModelName, CDSControlURL: s.CDSControlURL, - }) + } + order = append(order, udn) + } + + deviceFound := app.discoverDeviceMediaServers() + for i := range deviceFound { + found := &deviceFound[i] + + udn := normalizeUDN(found.ID) + if _, exists := byUDN[udn]; exists { + // Already found via SSDP, which carries CDSControlURL; keep that + // entry since registration itself only needs the UDN and name. + continue + } + + byUDN[udn] = libraryServer{ + UDN: udn, + Name: found.FriendlyName, + Manufacturer: found.Manufacturer, + Model: found.ModelName, + } + order = append(order, udn) + } + + out := make([]libraryServer, 0, len(order)) + for _, udn := range order { + out = append(out, byUDN[udn]) } w.Header().Set("Content-Type", "application/json") @@ -92,6 +130,71 @@ func (app *WebApp) HandleDiscoverLibraryServers(w http.ResponseWriter, r *http.R } } +// discoverDeviceMediaServers queries every paired speaker's own +// /listMediaServers endpoint concurrently and returns the union of what they +// report, deduplicated by UDN. A speaker that is offline, times out, or runs +// firmware without the endpoint is skipped silently: this is a best-effort +// second discovery path, and one unreachable speaker must not fail or delay +// the overall response. +func (app *WebApp) discoverDeviceMediaServers() []models.MediaServerInfo { + devices := app.DeviceSnapshot() + + type result struct { + servers []models.MediaServerInfo + } + + results := make(chan result, len(devices)) + + var wg sync.WaitGroup + + for _, entry := range devices { + deviceClient := entry.Device.Client + if deviceClient == nil { + continue + } + + wg.Add(1) + + go func(c *client.Client) { + defer wg.Done() + + resp, err := c.ListMediaServers() + if err != nil || resp == nil { + results <- result{} + return + } + + results <- result{servers: resp.MediaServers} + }(deviceClient) + } + + go func() { + wg.Wait() + close(results) + }() + + seen := make(map[string]bool) + + out := make([]models.MediaServerInfo, 0, len(devices)) + + for r := range results { + for i := range r.servers { + s := &r.servers[i] + + udn := normalizeUDN(s.ID) + if udn == "" || seen[udn] { + continue + } + + seen[udn] = true + + out = append(out, *s) + } + } + + return out +} + // HandleDeviceLibraryServers returns the STORED_MUSIC sources currently // registered on a specific speaker. Each source corresponds to one DLNA // server that has been paired with that device. diff --git a/pkg/service/soundtouchweb/handler_library_test.go b/pkg/service/soundtouchweb/handler_library_test.go index 17d1f4b..f24a30b 100644 --- a/pkg/service/soundtouchweb/handler_library_test.go +++ b/pkg/service/soundtouchweb/handler_library_test.go @@ -733,3 +733,118 @@ func TestHandleAddLibraryServer_AlreadyRegistered(t *testing.T) { t.Errorf("expected success=true when error contains 1024, got error=%s", resp.Error) } } + +// ---- discoverDeviceMediaServers (speaker-side /listMediaServers) -------- + +// cannedListMediaServersResponse is a minimal /listMediaServers XML response +// with one server, used to exercise the speaker-side discovery merge path +// that complements our own SSDP sweep. +const cannedListMediaServersResponse = ` + + +` + +// newMediaServerTestDevice registers a device backed by speakerURL under id, +// mirroring newLibraryTestApp's setup but for tests that need more than one +// device on the same WebApp. +func newMediaServerTestDevice(app *WebApp, id, speakerURL string) { + c := client.NewClient(&client.Config{Host: speakerURL}) + info := &models.DeviceInfo{DeviceID: id} + app.AddDevice(id, webtypes.NewDeviceConnection(c, info)) +} + +// TestDiscoverDeviceMediaServers_QueriesEveryPairedSpeaker verifies that +// discoverDeviceMediaServers calls /listMediaServers on every paired device +// and returns the union of what they report. This is the path that lets +// discovery see a server the AfterTouch service's own SSDP sweep might miss +// because it isn't co-located with the speaker's LAN segment. +func TestDiscoverDeviceMediaServers_QueriesEveryPairedSpeaker(t *testing.T) { + speakerA, _ := setupSpeakerMock(t, map[string]string{ + "/listMediaServers": cannedListMediaServersResponse, + }) + defer speakerA.Close() + + speakerB, _ := setupSpeakerMock(t, map[string]string{ + "/listMediaServers": ` + + +`, + }) + defer speakerB.Close() + + app := NewWebApp() + newMediaServerTestDevice(app, "dev-a", speakerA.URL) + newMediaServerTestDevice(app, "dev-b", speakerB.URL) + + got := app.discoverDeviceMediaServers() + + if len(got) != 2 { + t.Fatalf("expected 2 servers across both speakers, got %d: %+v", len(got), got) + } + + udns := map[string]bool{} + for _, s := range got { + udns[normalizeUDN(s.ID)] = true + } + + if !udns["fa095ecc-uuid"] || !udns["other-udn"] { + t.Errorf("expected both UDNs present, got %+v", udns) + } +} + +// TestDiscoverDeviceMediaServers_DedupesAcrossSpeakers verifies that the same +// server reported by two speakers (e.g. two boxes on the same LAN both seeing +// one NAS) is returned only once, keyed by normalized UDN, even when the two +// speakers report the UDN in different forms (with/without "uuid:" prefix). +func TestDiscoverDeviceMediaServers_DedupesAcrossSpeakers(t *testing.T) { + speakerA, _ := setupSpeakerMock(t, map[string]string{ + "/listMediaServers": cannedListMediaServersResponse, + }) + defer speakerA.Close() + + speakerB, _ := setupSpeakerMock(t, map[string]string{ + "/listMediaServers": ` + + +`, + }) + defer speakerB.Close() + + app := NewWebApp() + newMediaServerTestDevice(app, "dev-a", speakerA.URL) + newMediaServerTestDevice(app, "dev-b", speakerB.URL) + + got := app.discoverDeviceMediaServers() + + if len(got) != 1 { + t.Fatalf("expected 1 deduplicated server, got %d: %+v", len(got), got) + } +} + +// TestDiscoverDeviceMediaServers_UnreachableSpeakerSkippedSilently verifies +// that a speaker whose /listMediaServers call fails (offline, old firmware +// without the endpoint) does not prevent results from other, reachable +// speakers, and does not error the overall call. +func TestDiscoverDeviceMediaServers_UnreachableSpeakerSkippedSilently(t *testing.T) { + speakerA, _ := setupSpeakerMock(t, map[string]string{ + "/listMediaServers": cannedListMediaServersResponse, + }) + defer speakerA.Close() + + speakerB, _ := setupSpeakerMock(t, nil) + speakerB.Close() // closed before use: every request to it fails outright. + + app := NewWebApp() + newMediaServerTestDevice(app, "dev-a", speakerA.URL) + newMediaServerTestDevice(app, "dev-b", speakerB.URL) + + got := app.discoverDeviceMediaServers() + + if len(got) != 1 { + t.Fatalf("expected 1 server from the reachable speaker, got %d: %+v", len(got), got) + } + + if normalizeUDN(got[0].ID) != "fa095ecc-uuid" { + t.Errorf("expected the reachable speaker's server, got %+v", got[0]) + } +}