feat(marge): seed SPOTIFY/SpotifyConnectUserName placeholder + bind presets by exact (type, account)

When Spotify Connect is the active source on the speaker, a storePreset sent to marge carries source="SPOTIFY" sourceAccount="SpotifyConnectUserName" — the firmware-internal slug for "the user pushed playback from the Spotify app, no OAuth involved". marge.UpdatePreset's old fallback matched by SourceKeyType alone and collapsed every SPOTIFY preset onto whichever OAuth-brokered entry happened to exist (account=gesellix), rewriting the sourceAccount on save and quietly redirecting recall through the OAuth token path instead of the firmware's Connect path. UpdatePreset now: - parses Source and SourceAccount from the incoming XML; - prefers an exact (SourceKeyType, SourceKeyAccount) match before falling back to the type-only legacy path. Direct ID match still wins first. New marge.EnsurePlaceholderSources seeds the SPOTIFY/SpotifyConnectUserName placeholder (no credentials, Status=UNAVAILABLE, deterministic ID PLACEHOLDER_SPOTIFY_SpotifyConnectUserName). knownPlaceholderSources() collects future placeholders (UPnP, StoredMusic) in one slice. registerSpotifySourceForDevice in handlers/server.go calls EnsurePlaceholderSources after a successful AddSource so every priming run guarantees both the OAuth entry and the Connect placeholder exist. Tests in pkg/service/marge/placeholder_sources_test.go: - placeholder is seeded on first call, no-op on second (idempotent); - placeholder coexists with an OAuth source under the same account; - a Connect-style storePreset binds to the placeholder, not the OAuth source (regression); - an OAuth-style storePreset still binds to the OAuth source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-08-24 14:47:23 +00:00 · 2026-05-16 22:49:28 +02:00
parent b0d7e8aae2
commit 8a7f8010c9
3 changed files with 388 additions and 1 deletions
@@ -719,6 +719,15 @@ func (s *Server) registerSpotifySourceForDevice(deviceIP string, accounts []spot
 		registered = true
 	}

+	// Seed firmware-internal placeholder sources (e.g. SPOTIFY/SpotifyConnectUserName)
+	// so storePreset payloads originating from Spotify-Connect playback bind to the
+	// Connect placeholder rather than the OAuth-brokered entry above. Idempotent.
+	if registered && deviceID != "" {
+		if err := marge.EnsurePlaceholderSources(s.ds, accountID, deviceID); err != nil {
+			log.Printf("[Spotify Watchdog] Failed to seed placeholder sources for account %s device %s: %v", accountID, deviceID, err)
+		}
+	}
+
 	// Tell the speaker its sources list changed so it re-fetches from marge.
 	// Without this its on-device Sources.xml stays stale until something else
 	// triggers a sync — which leaves storePreset failing with
@@ -1226,6 +1226,8 @@ func UpdatePreset(ds *datastore.DataStore, account, device string, presetNumber
 	var newPresetElem struct {
 		Name            string `xml:"name"`
 		SourceID        string `xml:"sourceid"`
+		Source          string `xml:"source"`
+		SourceAccount   string `xml:"sourceaccount"`
 		Location        string `xml:"location"`
 		ContentItemType string `xml:"contentItemType"`
 		ContainerArt    string `xml:"containerArt"`
@@ -1236,7 +1238,8 @@ func UpdatePreset(ds *datastore.DataStore, account, device string, presetNumber

 	var matchingSrc *models.ConfiguredSource

-	log.Printf("[Marge] Searching for source matching ID=%s in %d sources", newPresetElem.SourceID, len(sources))
+	log.Printf("[Marge] Searching for source matching ID=%s source=%s sourceAccount=%s in %d sources",
+		newPresetElem.SourceID, newPresetElem.Source, newPresetElem.SourceAccount, len(sources))

 	for i := range sources {
 		log.Printf("[Marge]   Source[%d]: ID=%s, Type=%s, SourceKeyType=%s, SourceKeyAccount=%s", i, sources[i].ID, sources[i].Type, sources[i].SourceKeyType, sources[i].SourceKeyAccount)
@@ -1247,6 +1250,21 @@ func UpdatePreset(ds *datastore.DataStore, account, device string, presetNumber
 		}
 	}

+	// Prefer an exact (SourceKeyType, SourceKeyAccount) match before falling
+	// back to type-only. This lets firmware-internal sourceAccounts (e.g.
+	// "SpotifyConnectUserName" for Connect-initiated playback) bind to their
+	// dedicated placeholder source rather than collapsing onto an unrelated
+	// OAuth-brokered entry with the same SourceKeyType. See
+	// marge.EnsurePlaceholderSources for the seeded placeholders.
+	if matchingSrc == nil && newPresetElem.Source != "" && newPresetElem.SourceAccount != "" {
+		for i := range sources {
+			if sources[i].SourceKeyType == newPresetElem.Source && sources[i].SourceKeyAccount == newPresetElem.SourceAccount {
+				matchingSrc = &sources[i]
+				break
+			}
+		}
+	}
+
 	if matchingSrc == nil {
 		if newPresetElem.SourceID == constants.ProviderInternetRadio || newPresetElem.SourceID == constants.ProviderTunein || newPresetElem.SourceID == constants.ProviderSpotify || newPresetElem.SourceID == constants.ProviderAmazon {
 			// Find by SourceKeyType instead of ID if it's a default source
@@ -2038,3 +2056,97 @@ func AddSource(ds *datastore.DataStore, account, username, providerID, secret, s

 	return sourceID, nil
 }
+
+// PlaceholderSpotifyConnectAccount is the firmware-internal sourceAccount
+// slug the speaker uses for Spotify-Connect-initiated playback (where the
+// speaker handles the Spotify session itself, independent of any OAuth
+// account brokered by AfterTouch). When the speaker stores a preset for
+// such playback it sends this exact string as sourceAccount.
+const PlaceholderSpotifyConnectAccount = "SpotifyConnectUserName"
+
+// placeholderSource describes a firmware-internal "identity" source. These
+// carry no credentials and exist purely as match anchors so storePreset
+// payloads naming a firmware-managed sourceAccount don't fall back to an
+// unrelated OAuth-brokered entry with the same SourceKeyType.
+type placeholderSource struct {
+	SourceKeyType    string
+	SourceKeyAccount string
+	SourceProviderID string
+}
+
+func knownPlaceholderSources() []placeholderSource {
+	return []placeholderSource{
+		{
+			SourceKeyType:    constants.ProviderSpotify,
+			SourceKeyAccount: PlaceholderSpotifyConnectAccount,
+			SourceProviderID: strconv.Itoa(constants.SpotifyProviderID),
+		},
+	}
+}
+
+// EnsurePlaceholderSources seeds the firmware-internal placeholder
+// ConfiguredSources for a single (account, device) pair. Idempotent — a
+// placeholder with the same (SourceKeyType, SourceKeyAccount) is left alone
+// if it already exists, so this is safe to call on every prime/discover
+// cycle.
+//
+// Placeholders are saved with empty credentials and Status="UNAVAILABLE" so
+// they don't masquerade as user-managed sources in the AfterTouch UI; their
+// only job is to give marge.UpdatePreset something concrete to match against
+// when the speaker sends e.g. sourceAccount="SpotifyConnectUserName".
+func EnsurePlaceholderSources(ds *datastore.DataStore, account, device string) error {
+	sources, err := ds.GetConfiguredSources(account, device)
+	if err != nil {
+		return err
+	}
+
+	changed := false
+
+	for _, p := range knownPlaceholderSources() {
+		if hasSourceWithKey(sources, p.SourceKeyType, p.SourceKeyAccount) {
+			continue
+		}
+
+		now := FormatTime(time.Now())
+
+		newSrc := models.ConfiguredSource{
+			ID:               "PLACEHOLDER_" + p.SourceKeyType + "_" + p.SourceKeyAccount,
+			SourceProviderID: p.SourceProviderID,
+			Username:         p.SourceKeyAccount,
+			Name:             p.SourceKeyAccount,
+			CreatedOn:        now,
+			UpdatedOn:        now,
+			Status:           "UNAVAILABLE",
+		}
+		newSrc.SourceKey.Type = p.SourceKeyType
+		newSrc.SourceKey.Account = p.SourceKeyAccount
+
+		PrepareConfiguredSource(&newSrc)
+
+		log.Printf("[Marge] Seeding placeholder source %s/%s for account=%s device=%s",
+			p.SourceKeyType, p.SourceKeyAccount, account, device)
+
+		sources = append(sources, newSrc)
+		changed = true
+	}
+
+	if !changed {
+		return nil
+	}
+
+	return ds.SaveConfiguredSources(account, device, sources)
+}
+
+func hasSourceWithKey(sources []models.ConfiguredSource, sourceKeyType, sourceKeyAccount string) bool {
+	for i := range sources {
+		// Match either the structured SourceKey or the legacy flat fields —
+		// PrepareConfiguredSource keeps the two in sync but older datastore
+		// records may have populated only one.
+		if (sources[i].SourceKey.Type == sourceKeyType && sources[i].SourceKey.Account == sourceKeyAccount) ||
+			(sources[i].SourceKeyType == sourceKeyType && sources[i].SourceKeyAccount == sourceKeyAccount) {
+			return true
+		}
+	}
+
+	return false
+}
@@ -0,0 +1,266 @@
+package marge
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/gesellix/bose-soundtouch/pkg/models"
+	"github.com/gesellix/bose-soundtouch/pkg/service/datastore"
+)
+
+// TestEnsurePlaceholderSources_SeedsSpotifyConnect verifies that the
+// SPOTIFY/SpotifyConnectUserName placeholder is created for a fresh device
+// and that subsequent calls are no-ops (idempotent).
+func TestEnsurePlaceholderSources_SeedsSpotifyConnect(t *testing.T) {
+	ds, account, device := newMargeTestDatastore(t)
+
+	if err := EnsurePlaceholderSources(ds, account, device); err != nil {
+		t.Fatalf("EnsurePlaceholderSources: %v", err)
+	}
+
+	sources, err := ds.GetConfiguredSources(account, device)
+	if err != nil {
+		t.Fatalf("GetConfiguredSources: %v", err)
+	}
+
+	if !hasSourceWithKey(sources, "SPOTIFY", PlaceholderSpotifyConnectAccount) {
+		t.Fatalf("expected SPOTIFY/%s placeholder, got %d sources: %+v",
+			PlaceholderSpotifyConnectAccount, len(sources), sourceSummary(sources))
+	}
+
+	// Find it and assert the cosmetic fields look right (UNAVAILABLE, no creds).
+	var placeholder *models.ConfiguredSource
+
+	for i := range sources {
+		if sources[i].SourceKey.Type == "SPOTIFY" && sources[i].SourceKey.Account == PlaceholderSpotifyConnectAccount {
+			placeholder = &sources[i]
+			break
+		}
+	}
+
+	if placeholder == nil {
+		t.Fatalf("could not locate placeholder source after seeding")
+	}
+
+	// No credentials — placeholders are pure match anchors. (Status carries
+	// xml:"-" so it doesn't round-trip through XML storage; not asserted.)
+	if placeholder.Secret != "" || placeholder.Credential.Value != "" {
+		t.Errorf("placeholder should carry no credential, got Secret=%q credential=%q",
+			placeholder.Secret, placeholder.Credential.Value)
+	}
+
+	if placeholder.SourceProviderID != "15" {
+		t.Errorf("placeholder SourceProviderID = %q, want 15", placeholder.SourceProviderID)
+	}
+
+	// Idempotency: second call must not add a duplicate.
+	if err := EnsurePlaceholderSources(ds, account, device); err != nil {
+		t.Fatalf("EnsurePlaceholderSources (second call): %v", err)
+	}
+
+	after, _ := ds.GetConfiguredSources(account, device)
+
+	connectCount := 0
+	for i := range after {
+		if after[i].SourceKey.Type == "SPOTIFY" && after[i].SourceKey.Account == PlaceholderSpotifyConnectAccount {
+			connectCount++
+		}
+	}
+
+	if connectCount != 1 {
+		t.Errorf("expected exactly 1 SPOTIFY/%s placeholder after second call, got %d",
+			PlaceholderSpotifyConnectAccount, connectCount)
+	}
+}
+
+// TestEnsurePlaceholderSources_CoexistsWithOAuth verifies that seeding the
+// placeholder does not collide with or overwrite an existing OAuth-brokered
+// SPOTIFY source under the same account.
+func TestEnsurePlaceholderSources_CoexistsWithOAuth(t *testing.T) {
+	ds, account, device := newMargeTestDatastore(t)
+
+	// Pre-populate the OAuth-brokered Spotify source (same as bridgeSpotifyToMarge would).
+	if _, err := AddSource(ds, account, "gesellix", "15", "bs-deadbeef", "token_version_3", "Gesell IX"); err != nil {
+		t.Fatalf("AddSource: %v", err)
+	}
+
+	if err := EnsurePlaceholderSources(ds, account, device); err != nil {
+		t.Fatalf("EnsurePlaceholderSources: %v", err)
+	}
+
+	sources, _ := ds.GetConfiguredSources(account, device)
+
+	// Both must be present.
+	if !hasSourceWithKey(sources, "SPOTIFY", "gesellix") {
+		t.Errorf("OAuth source SPOTIFY/gesellix missing after placeholder seeding")
+	}
+
+	if !hasSourceWithKey(sources, "SPOTIFY", PlaceholderSpotifyConnectAccount) {
+		t.Errorf("placeholder source SPOTIFY/%s missing after seeding", PlaceholderSpotifyConnectAccount)
+	}
+}
+
+// TestUpdatePreset_BindsToPlaceholderByExactAccount is the regression that
+// motivated this whole change: when the speaker sends storePreset with
+// source=SPOTIFY sourceAccount=SpotifyConnectUserName, marge must bind the
+// preset to the placeholder, NOT the OAuth-brokered "gesellix" entry which
+// the old type-only fallback would have picked.
+func TestUpdatePreset_BindsToPlaceholderByExactAccount(t *testing.T) {
+	ds, account, device := newMargeTestDatastore(t)
+
+	// OAuth-brokered Spotify (the speaker should NOT bind to this for a
+	// Connect-initiated preset).
+	if _, err := AddSource(ds, account, "gesellix", "15", "bs-deadbeef", "token_version_3", "Gesell IX"); err != nil {
+		t.Fatalf("AddSource: %v", err)
+	}
+
+	// Placeholder for Spotify Connect.
+	if err := EnsurePlaceholderSources(ds, account, device); err != nil {
+		t.Fatalf("EnsurePlaceholderSources: %v", err)
+	}
+
+	// Look up the IDs of the two SPOTIFY sources so we can match on them
+	// (the rendered preset XML doesn't echo Username back, so checking the
+	// embedded <source id=...> attribute is the reliable signal).
+	sources, _ := ds.GetConfiguredSources(account, device)
+
+	placeholderID := findSourceID(sources, "SPOTIFY", PlaceholderSpotifyConnectAccount)
+	oauthID := findSourceID(sources, "SPOTIFY", "gesellix")
+
+	if placeholderID == "" || oauthID == "" || placeholderID == oauthID {
+		t.Fatalf("test setup wrong: placeholderID=%q oauthID=%q", placeholderID, oauthID)
+	}
+
+	// Speaker payload modeled on what rhino sent: source/sourceAccount carry
+	// the firmware-internal slug, sourceid is absent (the speaker doesn't
+	// know our SRC_ IDs at storePreset time for a Connect-managed session).
+	body := []byte(`<?xml version="1.0" encoding="UTF-8"?>
+<preset>
+  <name>Sand Castle Tapes</name>
+  <source>SPOTIFY</source>
+  <sourceaccount>SpotifyConnectUserName</sourceaccount>
+  <location>/playback/container/c3BvdGlmeTphbGJ1bTo3M1I2YXlQS2VWWEV2SnR4UTR4SDNv</location>
+  <contentItemType>tracklisturl</contentItemType>
+  <containerArt>https://i.scdn.co/image/ab67616d0000b273</containerArt>
+</preset>`)
+
+	resp, err := UpdatePreset(ds, account, device, 1, body)
+	if err != nil {
+		t.Fatalf("UpdatePreset failed: %v", err)
+	}
+
+	xmlStr := string(resp)
+
+	if !strings.Contains(xmlStr, `id="`+placeholderID+`"`) {
+		t.Errorf("preset bound to wrong source: expected embedded <source id=%q>, got:\n%s", placeholderID, xmlStr)
+	}
+
+	if strings.Contains(xmlStr, `id="`+oauthID+`"`) {
+		t.Errorf("preset unexpectedly bound to OAuth source <id=%q>; should have hit placeholder.\nResponse: %s", oauthID, xmlStr)
+	}
+}
+
+// TestUpdatePreset_FallsBackToOAuthWhenNoPlaceholderMatch verifies the
+// existing type-only fallback still works for presets whose sourceAccount
+// matches the OAuth user (or is absent), so OAuth-driven presets keep
+// binding correctly.
+func TestUpdatePreset_FallsBackToOAuthWhenNoPlaceholderMatch(t *testing.T) {
+	ds, account, device := newMargeTestDatastore(t)
+
+	if _, err := AddSource(ds, account, "gesellix", "15", "bs-deadbeef", "token_version_3", "Gesell IX"); err != nil {
+		t.Fatalf("AddSource: %v", err)
+	}
+
+	if err := EnsurePlaceholderSources(ds, account, device); err != nil {
+		t.Fatalf("EnsurePlaceholderSources: %v", err)
+	}
+
+	sources, _ := ds.GetConfiguredSources(account, device)
+
+	placeholderID := findSourceID(sources, "SPOTIFY", PlaceholderSpotifyConnectAccount)
+	oauthID := findSourceID(sources, "SPOTIFY", "gesellix")
+
+	if placeholderID == "" || oauthID == "" || placeholderID == oauthID {
+		t.Fatalf("test setup wrong: placeholderID=%q oauthID=%q", placeholderID, oauthID)
+	}
+
+	body := []byte(`<?xml version="1.0" encoding="UTF-8"?>
+<preset>
+  <name>OAuth Album</name>
+  <source>SPOTIFY</source>
+  <sourceaccount>gesellix</sourceaccount>
+  <location>/playback/container/c3BvdGlmeTphbGJ1bTpvYXV0aA==</location>
+  <contentItemType>tracklisturl</contentItemType>
+</preset>`)
+
+	resp, err := UpdatePreset(ds, account, device, 2, body)
+	if err != nil {
+		t.Fatalf("UpdatePreset failed: %v", err)
+	}
+
+	xmlStr := string(resp)
+
+	if !strings.Contains(xmlStr, `id="`+oauthID+`"`) {
+		t.Errorf("preset did not bind to OAuth source <id=%q>; got:\n%s", oauthID, xmlStr)
+	}
+
+	if strings.Contains(xmlStr, `id="`+placeholderID+`"`) {
+		t.Errorf("preset unexpectedly bound to placeholder <id=%q>; OAuth preset collapsed wrong direction.\nResponse: %s", placeholderID, xmlStr)
+	}
+}
+
+func findSourceID(sources []models.ConfiguredSource, sourceKeyType, sourceKeyAccount string) string {
+	for i := range sources {
+		if (sources[i].SourceKey.Type == sourceKeyType && sources[i].SourceKey.Account == sourceKeyAccount) ||
+			(sources[i].SourceKeyType == sourceKeyType && sources[i].SourceKeyAccount == sourceKeyAccount) {
+			return sources[i].ID
+		}
+	}
+
+	return ""
+}
+
+func newMargeTestDatastore(t *testing.T) (*datastore.DataStore, string, string) {
+	t.Helper()
+
+	tempDir, err := os.MkdirTemp("", "marge-placeholder-test-*")
+	if err != nil {
+		t.Fatalf("MkdirTemp: %v", err)
+	}
+
+	t.Cleanup(func() { _ = os.RemoveAll(tempDir) })
+
+	ds := datastore.NewDataStore(tempDir)
+
+	const account = "1234567"
+
+	const device = "DEVPLACE"
+
+	info := &models.ServiceDeviceInfo{
+		DeviceID:  device,
+		AccountID: account,
+		Name:      "Test Speaker",
+	}
+	if err := ds.SaveDeviceInfo(account, device, info); err != nil {
+		t.Fatalf("SaveDeviceInfo: %v", err)
+	}
+
+	// marge.AddSource walks accounts/{account}/devices and needs the
+	// per-device dir present to write configuredsources.xml.
+	if err := os.MkdirAll(filepath.Join(ds.AccountDevicesDir(account), device), 0o755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+
+	return ds, account, device
+}
+
+func sourceSummary(sources []models.ConfiguredSource) []string {
+	out := make([]string, 0, len(sources))
+	for i := range sources {
+		out = append(out, sources[i].SourceKey.Type+"/"+sources[i].SourceKey.Account+"#"+sources[i].ID)
+	}
+
+	return out
+}