From ff96430f534a70ad4be2c01b3ac2a86e755533b9 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 15 May 2026 18:03:48 +0200 Subject: [PATCH] fix(router): consolidate /device subrouter so PUT and DELETE actually resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #285's first fix (5f31616) registered the rename PUT inside a chi subrouter at `/streaming/account/{account}/device`, alongside the existing POST handlers. A *second* subrouter was already declared at `/streaming/account/{account}/device/{device}` for the per-device sub-resources (presets, recent, group, …). chi's radix tree treats those two registrations as overlapping prefixes and at request time prefers the more-specific `/device/{device}` subrouter — which had no root-level method handlers. A PUT to /device/X fell through to the [UNHANDLED] catch-all, got proxied to streaming.bose.com, came back as 401 from CloudFront. Speakers retried in a loop. The handlers-package regression test passed because the test router in `pkg/service/handlers/main_test.go` is flatter (one subrouter for device, no `/device/{device}` nested block). The route snapshot test passed because `chi.Walk` enumerates each subrouter's registrations independently — it doesn't simulate how the radix tree will resolve a runtime request when subrouters overlap. Reproduced against the actual production setupRouter in TestPUTRenameRoutesToLocalHandler (new in router_test.go). Before this commit: 404 / [UNHANDLED] / 401 proxy. After: 200 from HandleMargeUpdateDevice. Fix: collapse the two subrouters into one. All `/device` routes — the POST/PUT/DELETE on the device resource itself plus the GET/POST sub-resources — share a single `r.Route("/device", ...)` block with explicit `/{device}/...` paths inside. No radix-tree ambiguity. Knock-on: the `r.Delete("/device/{device}", server.HandleMargeRemoveDevice)` that lived at the outer `/account/{account}` level moves into the unified `/device` subrouter for symmetry. Its prior placement was also being shadowed by the radix overlap, which is why the route snapshot's first regeneration after this fix grew by exactly one DELETE line — that route was never resolvable at runtime under the old structure either. Refs #285. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/soundtouch-service/main.go | 37 ++++++++----- cmd/soundtouch-service/router_test.go | 55 +++++++++++++++++++ .../testdata/router_routes.txt | 1 + 3 files changed, 78 insertions(+), 15 deletions(-) diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 0211d9b..9bc2fce 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -936,6 +936,16 @@ func setupRouter(server *handlers.Server) *chi.Mux { r.Get("/presets/all", server.HandleMargeAccountPresets) r.Get("/provider_settings", server.HandleMargeProviderSettings) + // All `/device` routes share one chi subrouter. Two + // overlapping subrouters (`/device` + `/device/{device}`) + // caused chi's radix-tree resolver to bind a runtime + // request to the more-specific prefix even when only the + // less-specific subrouter had a matching method handler, + // producing the [UNHANDLED] → upstream-proxy fall-through + // behind issue #285's first-attempted fix. One subrouter + // keeps every device-scoped path resolvable; see + // TestPUTRenameRoutesToLocalHandler for the regression + // against the production router. r.Route("/device", func(r chi.Router) { r.Post("/", server.HandleMargeAddDevice) r.Post("/{device}", server.HandleMargeAddDevice) @@ -944,21 +954,20 @@ func setupRouter(server *handlers.Server) *chi.Mux { // when the user renames via Bose App or // `soundtouch-cli name set`. Issue #285. r.Put("/{device}", server.HandleMargeUpdateDevice) - }) + r.Delete("/{device}", server.HandleMargeRemoveDevice) - r.Route("/device/{device}", func(r chi.Router) { - r.Get("/presets", server.HandleMargePresets) - r.Post("/presets/{presetNumber}", server.HandleMargeUpdatePreset) - r.Put("/preset/{presetNumber}", server.HandleMargeUpdatePreset) - r.Delete("/preset/{presetNumber}", server.HandleMargeRemovePreset) - r.Get("/recent", server.HandleMargeRecents) - r.Get("/recents", server.HandleMargeRecents) - r.Post("/recent", server.HandleMargeAddRecent) + r.Get("/{device}/presets", server.HandleMargePresets) + r.Post("/{device}/presets/{presetNumber}", server.HandleMargeUpdatePreset) + r.Put("/{device}/preset/{presetNumber}", server.HandleMargeUpdatePreset) + r.Delete("/{device}/preset/{presetNumber}", server.HandleMargeRemovePreset) + r.Get("/{device}/recent", server.HandleMargeRecents) + r.Get("/{device}/recents", server.HandleMargeRecents) + r.Post("/{device}/recent", server.HandleMargeAddRecent) - r.Get("/group", server.HandleMargeDeviceGroup) - r.Get("/group/", server.HandleMargeDeviceGroup) - r.Get("/group/server", server.HandleMargeDeviceGroupServer) - r.Get("/group/member", server.HandleMargeDeviceGroupMember) + r.Get("/{device}/group", server.HandleMargeDeviceGroup) + r.Get("/{device}/group/", server.HandleMargeDeviceGroup) + r.Get("/{device}/group/server", server.HandleMargeDeviceGroupServer) + r.Get("/{device}/group/member", server.HandleMargeDeviceGroupMember) }) // Speakers POST to /group/ (with trailing slash) when forwarding @@ -968,8 +977,6 @@ func setupRouter(server *handlers.Server) *chi.Mux { r.Post("/group/", server.HandleMargeAddGroup) r.Post("/group/{groupId}", server.HandleMargeModifyGroup) r.Delete("/group/{groupId}", server.HandleMargeDeleteGroup) - - r.Delete("/device/{device}", server.HandleMargeRemoveDevice) }) r.Get("/device/{device}/streaming_token", server.HandleMargeStreamingToken) diff --git a/cmd/soundtouch-service/router_test.go b/cmd/soundtouch-service/router_test.go index d11db27..fc3a735 100644 --- a/cmd/soundtouch-service/router_test.go +++ b/cmd/soundtouch-service/router_test.go @@ -3,6 +3,7 @@ package main import ( "fmt" "net/http" + "net/http/httptest" "os" "reflect" "runtime" @@ -10,6 +11,7 @@ import ( "strings" "testing" + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" "github.com/gesellix/bose-soundtouch/pkg/service/handlers" "github.com/go-chi/chi/v5" ) @@ -102,3 +104,56 @@ func TestPrintRoutes(t *testing.T) { t.Errorf("Router routes changed! Diff the snapshot at %s with %s", snapshotPath, actualPath) } } + +// TestPUTRenameRoutesToLocalHandler reproduces the runtime routing +// behaviour the user saw on their deployed v0.80.0: a PUT to +// /streaming/account/{a}/device/{d} should land on +// HandleMargeUpdateDevice, not fall through to the [UNHANDLED] +// proxy. The handlers-package test (TestIssue285_*) uses a simplified +// router that doesn't have the overlapping `/device` and +// `/device/{device}` route groups, so it can't catch a chi radix- +// tree resolution that prefers the more-specific subrouter. +// +// This test exercises the actual production setupRouter so a +// regression in the route topology is caught against the same chi +// behaviour speakers will see. +func TestPUTRenameRoutesToLocalHandler(t *testing.T) { + tempDir, err := os.MkdirTemp("", "router-rename-") + if err != nil { + t.Fatalf("mkdir temp: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + server := handlers.NewServer(ds, nil, "http://localhost:8000", false, false, false) + r := setupRouter(server) + ts := httptest.NewServer(r) + defer ts.Close() + + body := `Sound MachinechenA81B6A536A98` + + req, err := http.NewRequest(http.MethodPut, + ts.URL+"/streaming/account/1111111/device/A81B6A536A98", + strings.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() }() + + // 200 means our local HandleMargeUpdateDevice handled it. + // 401 / 502 / anything else means the request fell through to + // the [UNHANDLED] proxy and got the upstream response — which + // is exactly the failure mode #285 was supposed to fix. + if resp.StatusCode != http.StatusOK { + t.Fatalf("PUT status = %d, want 200 (local handler). Anything else means the request fell through to [UNHANDLED] proxy — chi is routing to a different subrouter than the PUT registration intended.", resp.StatusCode) + } +} diff --git a/cmd/soundtouch-service/testdata/router_routes.txt b/cmd/soundtouch-service/testdata/router_routes.txt index 4995f77..a21fab3 100644 --- a/cmd/soundtouch-service/testdata/router_routes.txt +++ b/cmd/soundtouch-service/testdata/router_routes.txt @@ -8,6 +8,7 @@ DELETE /setup/dns-discoveries handlers.( DELETE /setup/interactions/sessions handlers.(*Server).HandleCleanupSessions-fm DELETE /setup/interactions/sessions/{session} handlers.(*Server).HandleDeleteSession-fm DELETE /setup/parity-mismatches handlers.(*Server).HandleClearParityMismatches-fm +DELETE /streaming/account/{account}/device/{device} handlers.(*Server).HandleMargeRemoveDevice-fm DELETE /streaming/account/{account}/device/{device}/preset/{presetNumber} handlers.(*Server).HandleMargeRemovePreset-fm DELETE /streaming/account/{account}/group/{groupId} handlers.(*Server).HandleMargeDeleteGroup-fm GET / handlers.(*Server).HandleRoot-fm