From 554fa78c0b90e60609590454e2264f7aef03625e Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 15 May 2026 16:41:55 +0200 Subject: [PATCH] fix(marge): handle the rename PUT speakers fire at /streaming/account/.../device/{id} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes issue #285. When the user renames an ST10 via the Bose App or via `soundtouch-cli name set`, the speaker fires: PUT http://:8000/streaming/account/{accountID}/device/{deviceID} Content-Type: application/xml NEW… The router only had POST registered for that path; PUT fell through to chi's default handling and the speaker observed HTTP 502 (captured verbatim in _/i285/Rename.log:38: "SimpleURLFetcher: retry needed, Curl 0, http 502, retries remaining 0"). The speaker's SimpleURLFetcher retried the PUT on a 15-second timer, the Bose App showed the rename spinning indefinitely, and the device's display name never updated on the AfterTouch side. Implementation reuses marge.AddDeviceToAccount, which is already an upsert via ds.SaveDeviceInfo — there's no semantic difference between "add" and "update" at the persistence layer. The new handler HandleMargeUpdateDevice differs from HandleMargeAddDevice only in the HTTP envelope: - 200 OK (not 201 Created — this is an update, not a fresh resource) - no Location header (the resource already lives at the URL the speaker is PUT-ing to) - deviceID in the body must match the URL's {device} segment; mismatch is a 400 rather than a silent re-key Registered as `r.Put("/{device}", server.HandleMargeUpdateDevice)` inside the existing `/streaming/account/{account}/device/` route group in both cmd/soundtouch-service/main.go and the handlers-package test router. Router-routes snapshot regenerated. Test coverage in pkg/service/handlers/issue285_regression_test.go: - TestIssue285_RenamePutAcceptedAndPersisted seeds the datastore with a device under its original name, replays the literal log payload from _/i285/Rename.log:36 against the real router, and asserts 200 OK + new name in response body + new name persisted on disk. testdata/issue285/rename_request.xml is the captured payload byte-for-byte (accountID 3981561, deviceID 884AEAEEBD27, rename to "Wohnzimmer SB" — same as the reporter). - TestIssue285_RenamePutRejectsMismatchedDeviceID pins the safety check: body deviceid != URL {device} → 400. Closes #285. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/soundtouch-service/main.go | 5 + .../testdata/router_routes.txt | 1 + pkg/service/handlers/handlers_marge.go | 56 +++++ .../handlers/issue285_regression_test.go | 208 ++++++++++++++++++ pkg/service/handlers/main_test.go | 2 + .../testdata/issue285/rename_request.xml | 1 + 6 files changed, 273 insertions(+) create mode 100644 pkg/service/handlers/issue285_regression_test.go create mode 100644 pkg/service/handlers/testdata/issue285/rename_request.xml diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 7f6579c..0211d9b 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -939,6 +939,11 @@ func setupRouter(server *handlers.Server) *chi.Mux { r.Route("/device", func(r chi.Router) { r.Post("/", server.HandleMargeAddDevice) r.Post("/{device}", server.HandleMargeAddDevice) + // PUT is the rename / update path — speakers fire + // this against PUT /streaming/account/{a}/device/{d} + // when the user renames via Bose App or + // `soundtouch-cli name set`. Issue #285. + r.Put("/{device}", server.HandleMargeUpdateDevice) }) r.Route("/device/{device}", func(r chi.Router) { diff --git a/cmd/soundtouch-service/testdata/router_routes.txt b/cmd/soundtouch-service/testdata/router_routes.txt index ad937ac..4995f77 100644 --- a/cmd/soundtouch-service/testdata/router_routes.txt +++ b/cmd/soundtouch-service/testdata/router_routes.txt @@ -155,5 +155,6 @@ POST /streaming/support/power_on handlers.( POST /v1/scmudc/{deviceId} handlers.(*Server).HandleAppEvents-fm POST /v1/stapp/{deviceId} handlers.(*Server).HandleAppEvents-fm PUT /oauth/* handlers.(*Server).HandleBoseProxy-fm +PUT /streaming/account/{account}/device/{device} handlers.(*Server).HandleMargeUpdateDevice-fm PUT /streaming/account/{account}/device/{device}/preset/{presetNumber} handlers.(*Server).HandleMargeUpdatePreset-fm TRACE /oauth/* handlers.(*Server).HandleBoseProxy-fm diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index c7162ee..b3d2da4 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -600,6 +600,62 @@ func (s *Server) HandleMargeAddDevice(w http.ResponseWriter, r *http.Request) { _, _ = w.Write(data) } +// HandleMargeUpdateDevice handles the speaker's rename PUT against +// /streaming/account/{account}/device/{device}. The speaker fires +// this whenever the user renames it via the Bose App or via +// `soundtouch-cli name set`; before this handler existed AfterTouch +// returned 502, the speaker retried in a loop, and the App showed +// the rename hanging indefinitely (issue #285). +// +// The expected payload mirrors the POST shape: +// +// NEWDEVID +// +// AddDeviceToAccount is already an upsert via ds.SaveDeviceInfo, so +// rather than introduce a parallel UpdateDevice function we route +// the PUT through the same persistence path. The semantic delta is +// purely in the HTTP envelope: 200 (not 201), no Location header, +// and the deviceID in the body has to match the URL — a mismatch +// means the speaker is targeting the wrong record and we refuse +// rather than silently re-key. +func (s *Server) HandleMargeUpdateDevice(w http.ResponseWriter, r *http.Request) { + account := chi.URLParam(r, "account") + if !validatePathID(account) { + http.Error(w, "Invalid account ID", http.StatusBadRequest) + return + } + + device := chi.URLParam(r, "device") + if !validatePathID(device) { + http.Error(w, "Invalid device ID", http.StatusBadRequest) + return + } + + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "Failed to read body", http.StatusInternalServerError) + return + } + + bodyDeviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + if bodyDeviceID != device { + http.Error(w, + fmt.Sprintf("device ID in body (%q) does not match URL (%q)", bodyDeviceID, device), + http.StatusBadRequest) + + return + } + + w.Header().Set("Content-Type", "application/vnd.bose.streaming-v1.2+xml") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(data) +} + // HandleMargeRemovePreset removes a preset for the specified account and device. func (s *Server) HandleMargeRemovePreset(w http.ResponseWriter, r *http.Request) { account := chi.URLParam(r, "account") diff --git a/pkg/service/handlers/issue285_regression_test.go b/pkg/service/handlers/issue285_regression_test.go new file mode 100644 index 0000000..63c538a --- /dev/null +++ b/pkg/service/handlers/issue285_regression_test.go @@ -0,0 +1,208 @@ +package handlers + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/models" + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +// TestIssue285_RenamePutAcceptedAndPersisted reproduces the rename +// loop documented in issue #285: +// +// https://github.com/gesellix/Bose-SoundTouch/issues/285 +// +// When a user renames an ST10 via the Bose App or via +// `soundtouch-cli name set`, the speaker fires PUT +// /streaming/account/{accountID}/device/{deviceID} with a body of +// the form: +// +// NEW… +// +// Before this commit the router only registered POST for that path; +// PUT fell through to the chi router's default handling and the +// speaker observed HTTP 502 (captured verbatim in +// _/i285/Rename.log:38: "SimpleURLFetcher: retry needed, Curl 0, +// http 502, retries remaining 0"). The speaker retried in a loop +// and the Bose App showed the rename spinning indefinitely. +// +// The fixture at testdata/issue285/rename_request.xml is the exact +// payload from the log (line 36) — `deviceid="884AEAEEBD27"`, +// `Wohnzimmer SB`. The test: +// +// 1. Pre-seeds the datastore with a device record under the +// reporter's accountID + deviceID so the PUT is updating, not +// creating. +// 2. Replays the rename PUT. +// 3. Asserts: +// - HTTP 200 (NOT 201; this is an update, not a create — speakers +// observed 502 before, so any 2xx is the headline fix, but +// pinning 200 protects against accidentally returning 201 +// which would change the Location-header contract). +// - Response body carries the new name verbatim. +// - Persisted Sources/DeviceInfo on disk reflects the new name. +// +// When future work decides to preserve `createdOn` across updates +// (currently AddDeviceToAccount rewrites both timestamps), update +// the test to also assert that — the rename request from the log +// does NOT carry a createdOn, so any value our marge response +// emits is purely our choice and should be stable. +func TestIssue285_RenamePutAcceptedAndPersisted(t *testing.T) { + tempDir, err := os.MkdirTemp("", "issue285-") + if err != nil { + t.Fatalf("mkdir temp: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + const ( + accountID = "3981561" + deviceID = "884AEAEEBD27" + oldName = "Wohnzimmer" + newName = "Wohnzimmer SB" + ) + + // 1. Seed datastore with the device under its original name — + // modelling a pre-existing paired device the user is now + // renaming. + if err := ds.SaveDeviceInfo(accountID, deviceID, &models.ServiceDeviceInfo{ + DeviceID: deviceID, + AccountID: accountID, + Name: oldName, + IPAddress: "192.168.0.109", + }); err != nil { + t.Fatalf("seed datastore: %v", err) + } + + // 2. Spin up the router and replay the captured rename PUT. + r, _ := setupRouter("http://localhost:8001", ds) + ts := httptest.NewServer(r) + + t.Cleanup(ts.Close) + + body, err := os.ReadFile(filepath.Join("testdata", "issue285", "rename_request.xml")) + if err != nil { + t.Fatalf("read fixture: %v", err) + } + + // Sanity-check the fixture before trusting any downstream + // assertion against it. + if !bytes.Contains(body, []byte(`deviceid="`+deviceID+`"`)) { + t.Fatalf("fixture missing expected deviceid=%q; got:\n%s", deviceID, body) + } + + if !bytes.Contains(body, []byte(``+newName+``)) { + t.Fatalf("fixture missing expected new name %q; got:\n%s", newName, body) + } + + req, err := http.NewRequest(http.MethodPut, + ts.URL+"/streaming/account/"+accountID+"/device/"+deviceID, + bytes.NewReader(body)) + if err != nil { + t.Fatalf("build request: %v", err) + } + + req.Header.Set("Content-Type", "application/xml") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + // 3. Headline assertion: the speaker observed 502 before — any + // 2xx fixes the loop. Pin 200 specifically so we don't drift + // into 201/Created (which would change the Location-header + // contract POST gets). + if resp.StatusCode != http.StatusOK { + respBody, _ := io.ReadAll(resp.Body) + t.Fatalf("PUT status = %d, want 200; body:\n%s", resp.StatusCode, respBody) + } + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read response: %v", err) + } + + // Response shape: NEW… + if !bytes.Contains(respBody, []byte(`deviceid="`+deviceID+`"`)) { + t.Errorf("response missing deviceid=%q; body:\n%s", deviceID, respBody) + } + + if !bytes.Contains(respBody, []byte(``+newName+``)) { + t.Errorf("response missing new name %q; body:\n%s", newName, respBody) + } + + if strings.Contains(string(respBody), ``+oldName+``) { + t.Errorf("response still carries old name %q; body:\n%s", oldName, respBody) + } + + // 4. Persistence assertion: the datastore now reflects the new + // name. This is what the Bose App reads back on its next + // /streaming/account/.../full poll, which is what closes the + // visible rename loop. + persisted, err := ds.GetDeviceInfo(accountID, deviceID) + if err != nil { + t.Fatalf("read persisted device info: %v", err) + } + + if persisted.Name != newName { + t.Errorf("persisted Name = %q, want %q", persisted.Name, newName) + } +} + +// TestIssue285_RenamePutRejectsMismatchedDeviceID pins the safety +// check: if the speaker (or a bug elsewhere) ever sends a PUT with +// a body whose `deviceid="…"` doesn't match the URL's `{device}` +// segment, we refuse with 400 rather than silently re-key the +// persisted record under the wrong account/device. +func TestIssue285_RenamePutRejectsMismatchedDeviceID(t *testing.T) { + tempDir, err := os.MkdirTemp("", "issue285-mismatch-") + if err != nil { + t.Fatalf("mkdir temp: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + r, _ := setupRouter("http://localhost:8001", ds) + ts := httptest.NewServer(r) + + t.Cleanup(ts.Close) + + const urlDeviceID = "884AEAEEBD27" + + // Body claims a different deviceID than the URL. + body := []byte(`` + + `RogueDEADBEEFCAFE`) + + req, err := http.NewRequest(http.MethodPut, + ts.URL+"/streaming/account/3981561/device/"+urlDeviceID, + bytes.NewReader(body)) + if err != nil { + t.Fatalf("build request: %v", err) + } + + req.Header.Set("Content-Type", "application/xml") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusBadRequest { + respBody, _ := io.ReadAll(resp.Body) + t.Fatalf("PUT status = %d, want 400; body:\n%s", resp.StatusCode, respBody) + } +} diff --git a/pkg/service/handlers/main_test.go b/pkg/service/handlers/main_test.go index 87bcb19..373c16f 100644 --- a/pkg/service/handlers/main_test.go +++ b/pkg/service/handlers/main_test.go @@ -47,6 +47,8 @@ func setupRouter(targetURL string, ds *datastore.DataStore) (*chi.Mux, *Server) r.Route("/account/{account}/device", func(r chi.Router) { r.Post("/", server.HandleMargeAddDevice) r.Post("/{device}", server.HandleMargeAddDevice) + // Rename PUT — mirrors the production router. Issue #285. + r.Put("/{device}", server.HandleMargeUpdateDevice) }) r.Get("/account/{account}/device/{device}/recent", server.HandleMargeRecents) r.Post("/account/{account}/device/{device}/recent", server.HandleMargeAddRecent) diff --git a/pkg/service/handlers/testdata/issue285/rename_request.xml b/pkg/service/handlers/testdata/issue285/rename_request.xml new file mode 100644 index 0000000..19a720e --- /dev/null +++ b/pkg/service/handlers/testdata/issue285/rename_request.xml @@ -0,0 +1 @@ +Wohnzimmer SB884AEAEEBD27