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]) + } +}