mirror of
https://github.com/gesellix/Bose-SoundTouch.git
synced 2026-08-18 08:36:13 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
Tobias Gesellchen
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
401a546482
commit
f899dbaa89
@@ -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 <name/> or <name></name> or fallback name, got %s", string(fullXML2))
|
||||
}
|
||||
}
|
||||
|
||||
// extractSourceFragment returns the raw `<source id="id" ...>...</source>`
|
||||
// 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 := `<source id="` + id + `"`
|
||||
|
||||
start := strings.Index(doc, marker)
|
||||
if start < 0 {
|
||||
t.Fatalf("source id=%q not found in document:\n%s", id, doc)
|
||||
}
|
||||
|
||||
end := strings.Index(doc[start:], "</source>")
|
||||
if end < 0 {
|
||||
t.Fatalf("source id=%q has no closing </source>:\n%s", id, doc)
|
||||
}
|
||||
|
||||
return doc[start : start+end+len("</source>")]
|
||||
}
|
||||
|
||||
// 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 <source> 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 <source> 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])
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 = `<?xml version="1.0" encoding="UTF-8" ?>
|
||||
<ListMediaServersResponse>
|
||||
<media_server id="uuid:fa095ecc-uuid" ip="198.51.100.10" manufacturer="AVM" model_name="FRITZ!Mediaserver" friendly_name="FRITZ!Mediaserver"/>
|
||||
</ListMediaServersResponse>`
|
||||
|
||||
// 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": `<?xml version="1.0" encoding="UTF-8" ?>
|
||||
<ListMediaServersResponse>
|
||||
<media_server id="uuid:other-udn" ip="198.51.100.20" manufacturer="Synology" model_name="DS220" friendly_name="NAS"/>
|
||||
</ListMediaServersResponse>`,
|
||||
})
|
||||
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": `<?xml version="1.0" encoding="UTF-8" ?>
|
||||
<ListMediaServersResponse>
|
||||
<media_server id="fa095ecc-uuid" ip="198.51.100.10" manufacturer="AVM" model_name="FRITZ!Mediaserver" friendly_name="FRITZ!Mediaserver"/>
|
||||
</ListMediaServersResponse>`,
|
||||
})
|
||||
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])
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user