diff options
| author | Paul Buetow <paul@buetow.org> | 2026-05-19 19:23:50 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-05-19 19:23:50 +0300 |
| commit | 3779053951f338076fcc448daf9bee9ada18cf54 (patch) | |
| tree | 50bbb2ccb68d9eab90b0ffdd5897560daec0bd17 /player-server | |
| parent | 3ce06a482fafd884013c9a7a827d9ccb183a05da (diff) | |
Remove nil streamer fallback in api.serveFileResult (DIP)
The serveFileResult handler used to fabricate a default MediaStreamer at
request time when s.streamer was nil. That silently masked wiring
mistakes and violated the Dependency Inversion Principle — the handler
was inventing its own dependency instead of demanding one from the
caller. Removed the fallback so the handler now uses s.streamer directly.
Construction-time validation of deps.MediaStreamer was added to
api.NewServerWithLogger in commit 4215db6, which makes the runtime nil
case unreachable. Mirrors the explicit-deps pattern set in commits
622827c (http.Client in podcast service) and 92edb83 (TokenManager in
auth service).
Test helpers updated to inject a service.NewMediaStreamer(nil) when one
isn't otherwise supplied so the existing suite still constructs Servers
through the validating constructor.
Refs agent task 6a.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Diffstat (limited to 'player-server')
| -rw-r--r-- | player-server/internal/api/handlers.go | 8 | ||||
| -rw-r--r-- | player-server/internal/api/handlers_playback_test.go | 3 | ||||
| -rw-r--r-- | player-server/internal/api/handlers_podcast_test.go | 3 | ||||
| -rw-r--r-- | player-server/internal/api/handlers_test.go | 14 | ||||
| -rw-r--r-- | player-server/internal/api/integration_test.go | 3 |
5 files changed, 22 insertions, 9 deletions
diff --git a/player-server/internal/api/handlers.go b/player-server/internal/api/handlers.go index a9c832f..f04ec19 100644 --- a/player-server/internal/api/handlers.go +++ b/player-server/internal/api/handlers.go @@ -162,10 +162,12 @@ func (s *Server) serveDetach(w http.ResponseWriter, r *http.Request) { // ------------------------------------------------------------------ func (s *Server) serveFileResult(w http.ResponseWriter, r *http.Request, res *service.FileResult, attachment bool) { + // s.streamer is required at construction time (see NewServerWithLogger), + // so it is guaranteed non-nil here. We previously fell back to a default + // streamer when nil, which silently hid wiring mistakes and violated the + // Dependency Inversion Principle by letting the handler decide its own + // dependency. streamer := s.streamer - if streamer == nil { - streamer = service.NewMediaStreamer(nil) - } stream, err := streamer.Open(r.Context(), res, attachment) if err != nil { diff --git a/player-server/internal/api/handlers_playback_test.go b/player-server/internal/api/handlers_playback_test.go index 7febab0..869b0d1 100644 --- a/player-server/internal/api/handlers_playback_test.go +++ b/player-server/internal/api/handlers_playback_test.go @@ -38,7 +38,8 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session Auth: authSvc, PlaybackHints: hintSvc, }, - StaticFS: fs, + StaticFS: fs, + MediaStreamer: service.NewMediaStreamer(nil), }) if err != nil { t.Fatalf("NewServer: %v", err) diff --git a/player-server/internal/api/handlers_podcast_test.go b/player-server/internal/api/handlers_podcast_test.go index 4138d97..7dcdfe6 100644 --- a/player-server/internal/api/handlers_podcast_test.go +++ b/player-server/internal/api/handlers_podcast_test.go @@ -68,7 +68,8 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash Auth: authSvc, Podcast: podcastSvc, }, - StaticFS: fs, + StaticFS: fs, + MediaStreamer: service.NewMediaStreamer(nil), }) if err != nil { t.Fatalf("NewServer: %v", err) diff --git a/player-server/internal/api/handlers_test.go b/player-server/internal/api/handlers_test.go index 6279895..5fbd4c9 100644 --- a/player-server/internal/api/handlers_test.go +++ b/player-server/internal/api/handlers_test.go @@ -65,9 +65,17 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm if len(streamer) > 0 { mediaStreamer = streamer[0] } - // NewServer now returns an error when required deps (e.g. Config) are - // missing. Tests always pass a non-nil Config, so a failure here indicates - // a programming mistake in the test setup itself. + if mediaStreamer == nil { + // NewServer now requires a non-nil MediaStreamer at construction + // (see api.NewServerWithLogger). Tests that don't exercise streaming + // still need one, so we fall back to the default in-process streamer + // (nil remuxer means remux requests will error, which is fine for + // non-streaming tests). + mediaStreamer = service.NewMediaStreamer(nil) + } + // NewServer now returns an error when required deps (e.g. Config, + // MediaStreamer) are missing. Tests always pass non-nil values, so a + // failure here indicates a programming mistake in the test setup itself. srv, err := NewServer(ServerDeps{ Store: store, Hasher: hasher, diff --git a/player-server/internal/api/integration_test.go b/player-server/internal/api/integration_test.go index 2f51bd2..9344552 100644 --- a/player-server/internal/api/integration_test.go +++ b/player-server/internal/api/integration_test.go @@ -82,7 +82,8 @@ func newIntegrationServer(t *testing.T) *integrationEnv { Auth: authSvc, Podcast: &integrationPodcastService{}, }, - StaticFS: http.FS(staticFS), + StaticFS: http.FS(staticFS), + MediaStreamer: service.NewMediaStreamer(nil), }) if err != nil { t.Fatalf("NewServer: %v", err) |
