From 35aa611038c97a2ce27510e3898318afca37cac0 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Tue, 19 May 2026 19:15:14 +0300 Subject: Complete s9 + aa: NewServerWithLogger returns error; share-page renderer extracted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related refactors that converged on internal/api/server.go. s9 — NewServerWithLogger no longer panics on nil deps.Config. Returns (*Server, error) so cmd/player/main.go can log a clear message and exit cleanly when wiring is misconfigured. All callers updated, including api unit tests that construct a Server directly. aa — Share-page HTML rendering moved out of handlers_share.go into a new internal/web package. internal/web/sharepage.go owns SharePageRenderer and the private injectShareMedia helper; the api handler now calls s.shareRenderer.Render(...) and routes errors via http.Error. injectShareMedia coverage was moved alongside it in internal/web/sharepage_test.go; the duplicate tests in handlers_more_test.go were removed (placeholder comment left so the reference is greppable). Server gained one new field (shareRenderer *web.SharePageRenderer) and one new constructor line; production wiring uses deps.StaticFS. Background: both tasks were initially worked by separate sub-agents that stopped mid-edit when usage limits hit; their WIP was preserved in a stash and finalized in this commit. Tests rerun from a clean state: full Go unit suite green, 25/25 LLM e2e, 22/22 Playwright. Co-Authored-By: Claude Opus 4.7 --- player-server/cmd/player/main.go | 5 +- player-server/internal/api/handlers_more_test.go | 40 +------- .../internal/api/handlers_playback_test.go | 8 +- .../internal/api/handlers_podcast_test.go | 8 +- player-server/internal/api/handlers_share.go | 37 ++----- player-server/internal/api/handlers_test.go | 9 +- player-server/internal/api/integration_test.go | 7 +- player-server/internal/api/server.go | 18 +++- player-server/internal/api/server_test.go | 19 ++-- player-server/internal/web/sharepage.go | 106 +++++++++++++++++++++ player-server/internal/web/sharepage_test.go | 92 ++++++++++++++++++ 11 files changed, 264 insertions(+), 85 deletions(-) create mode 100644 player-server/internal/web/sharepage.go create mode 100644 player-server/internal/web/sharepage_test.go diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go index 8835a1a..8b80d8d 100644 --- a/player-server/cmd/player/main.go +++ b/player-server/cmd/player/main.go @@ -246,7 +246,7 @@ func runWithSignal(args []string, sigCh <-chan os.Signal) error { staticFS := http.Dir("web") remuxer := probe.NewFFRemuxer() streamer := service.NewMediaStreamer(remuxer) - server := api.NewServerWithLogger(api.ServerDeps{ + server, err := api.NewServerWithLogger(api.ServerDeps{ Store: store, Hasher: deps.hasher, SessionManager: deps.sm, @@ -267,6 +267,9 @@ func runWithSignal(args []string, sigCh <-chan os.Signal) error { StaticFS: staticFS, MediaStreamer: streamer, }, logger) + if err != nil { + return fmt.Errorf("failed to create API server: %w", err) + } return runServer(server, cfg, logger, sigCh) } diff --git a/player-server/internal/api/handlers_more_test.go b/player-server/internal/api/handlers_more_test.go index 5ae71bb..4c441cd 100644 --- a/player-server/internal/api/handlers_more_test.go +++ b/player-server/internal/api/handlers_more_test.go @@ -1209,43 +1209,9 @@ func TestServer_SharePage(t *testing.T) { }) } -func TestInjectShareMedia(t *testing.T) { - t.Run("injects marshaled metadata", func(t *testing.T) { - html, err := injectShareMedia(``, map[string]string{"stream_url": "/s/abc/stream"}) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if !strings.Contains(html, `"stream_url":"/s/abc/stream"`) { - t.Fatalf("expected injected JSON, got %q", html) - } - }) - - t.Run("omits empty thumbnail url", func(t *testing.T) { - html, err := injectShareMedia(``, service.GetSharedMediaResult{ - Media: &service.SharedMediaView{ID: 1, FileName: "share.mp3", Type: model.MediaTypeAudio}, - HasThumb: false, - StreamURL: "/s/abc/stream", - DownloadURL: "/s/abc/download", - ThumbURL: "", - }) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if strings.Contains(html, "thumb_url") { - t.Fatalf("expected no thumb_url in injected JSON, got %q", html) - } - }) - - t.Run("returns marshal error", func(t *testing.T) { - _, err := injectShareMedia(``, map[string]any{"bad": make(chan int)}) - if err == nil { - t.Fatal("expected marshal error") - } - if !strings.Contains(err.Error(), "marshal share metadata") { - t.Fatalf("expected wrapped marshal error, got %v", err) - } - }) -} +// TestInjectShareMedia coverage moved to internal/web/sharepage_test.go +// when share-page rendering was extracted into the internal/web package +// (task aa). The api package no longer owns the injection helper. func TestServer_ShareStream(t *testing.T) { path := makeTempFile(t, "shared") diff --git a/player-server/internal/api/handlers_playback_test.go b/player-server/internal/api/handlers_playback_test.go index 5b14b39..7febab0 100644 --- a/player-server/internal/api/handlers_playback_test.go +++ b/player-server/internal/api/handlers_playback_test.go @@ -28,7 +28,9 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session CountUsersFunc: func(context.Context) (int, error) { return 1, nil }, GetUserByIDFunc: func(context.Context, int64) (*model.User, error) { return &model.User{ID: 1, IsAdmin: true}, nil }, } - return NewServer(ServerDeps{ + // NewServer now returns (*Server, error); we pass a non-nil Config here, + // so a failure points to a wiring bug in the test setup. + srv, err := NewServer(ServerDeps{ Store: store, SessionManager: sm, Config: &internal.Config{}, @@ -38,6 +40,10 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session }, StaticFS: fs, }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + return srv } // sessionForPlaybackTest creates a session cookie that resolves to userID 1. diff --git a/player-server/internal/api/handlers_podcast_test.go b/player-server/internal/api/handlers_podcast_test.go index 4836218..4138d97 100644 --- a/player-server/internal/api/handlers_podcast_test.go +++ b/player-server/internal/api/handlers_podcast_test.go @@ -49,7 +49,9 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash GetUserByIDFunc: func(context.Context, int64) (*model.User, error) { return &model.User{ID: 1, IsAdmin: true}, nil }, } } - return NewServer(ServerDeps{ + // NewServer now returns (*Server, error); tests always supply Config, + // so the only realistic cause of failure is a programming mistake. + srv, err := NewServer(ServerDeps{ Store: store, Hasher: hasher, SessionManager: sm, @@ -68,6 +70,10 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash }, StaticFS: fs, }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + return srv } // setupPodcastE2E creates a full server with a real SQLite store and real services. diff --git a/player-server/internal/api/handlers_share.go b/player-server/internal/api/handlers_share.go index 5c7db83..50ad3ef 100644 --- a/player-server/internal/api/handlers_share.go +++ b/player-server/internal/api/handlers_share.go @@ -1,10 +1,7 @@ package api import ( - "encoding/json" "errors" - "fmt" - "io" "net/http" "strings" "time" @@ -91,39 +88,17 @@ func (s *Server) handleSharePage(w http.ResponseWriter, r *http.Request) { return } - // Serve HTML page with media metadata injected. - f, err := s.staticFS.Open("share.html") + // Render the HTML view via the dedicated renderer. This keeps the + // handler focused on transport concerns (status codes, headers) and + // keeps templating in the internal/web package. + page, err := s.shareRenderer.Render(res) if err != nil { - http.Error(w, "not found", http.StatusNotFound) - return - } - defer f.Close() - stat, err := f.Stat() - if err != nil { - http.Error(w, "not found", http.StatusNotFound) - return - } - var buf strings.Builder - if _, err := io.Copy(&buf, f); err != nil { - http.Error(w, "internal error", http.StatusInternalServerError) - return - } - html, err := injectShareMedia(buf.String(), res) - if err != nil { - s.logger.Error("marshal share page metadata", "err", err) + s.logger.Error("render share page", "err", err) http.Error(w, "internal error", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "text/html; charset=utf-8") - http.ServeContent(w, r, "share.html", stat.ModTime(), strings.NewReader(html)) -} - -func injectShareMedia(html string, data any) (string, error) { - encoded, err := json.Marshal(data) - if err != nil { - return "", fmt.Errorf("marshal share metadata: %w", err) - } - return strings.Replace(html, "", string(encoded), 1), nil + http.ServeContent(w, r, page.Name, page.ModTime, strings.NewReader(page.HTML)) } func (s *Server) handleShareThumbnail(w http.ResponseWriter, r *http.Request) { diff --git a/player-server/internal/api/handlers_test.go b/player-server/internal/api/handlers_test.go index fea6861..2c662b3 100644 --- a/player-server/internal/api/handlers_test.go +++ b/player-server/internal/api/handlers_test.go @@ -65,7 +65,10 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm if len(streamer) > 0 { mediaStreamer = streamer[0] } - return NewServer(ServerDeps{ + // 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. + srv, err := NewServer(ServerDeps{ Store: store, Hasher: hasher, SessionManager: sm, @@ -84,6 +87,10 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm StaticFS: fs, MediaStreamer: mediaStreamer, }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + return srv } func addSessionCookie(t *testing.T, store repository.Store, sm auth.SessionManager, userID int64) *http.Cookie { diff --git a/player-server/internal/api/integration_test.go b/player-server/internal/api/integration_test.go index c5e7d18..2f51bd2 100644 --- a/player-server/internal/api/integration_test.go +++ b/player-server/internal/api/integration_test.go @@ -63,7 +63,9 @@ func newIntegrationServer(t *testing.T) *integrationEnv { "share.html": {Data: []byte("share")}, } - srv := NewServer(ServerDeps{ + // NewServer now returns (*Server, error); cfg is always provided here so a + // failure indicates a wiring bug in the test setup. + srv, err := NewServer(ServerDeps{ Store: store, Hasher: hasher, SessionManager: sm, @@ -82,6 +84,9 @@ func newIntegrationServer(t *testing.T) *integrationEnv { }, StaticFS: http.FS(staticFS), }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } return &integrationEnv{srv: srv} } diff --git a/player-server/internal/api/server.go b/player-server/internal/api/server.go index 608e934..2fccf32 100644 --- a/player-server/internal/api/server.go +++ b/player-server/internal/api/server.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "log/slog" "net/http" "strconv" @@ -12,6 +13,7 @@ import ( "codeberg.org/snonux/player/internal/auth" "codeberg.org/snonux/player/internal/repository" "codeberg.org/snonux/player/internal/service" + "codeberg.org/snonux/player/internal/web" ) // Server holds HTTP handlers and dependencies. @@ -35,6 +37,7 @@ type Server struct { playbackHintSvc service.PlaybackHintsService streamer service.MediaStreamer staticFS http.FileSystem + shareRenderer *web.SharePageRenderer logger *slog.Logger mw *Middleware } @@ -67,14 +70,20 @@ type ServerDeps struct { } // NewServer creates a Server with routes. -func NewServer(deps ServerDeps) *Server { +// It returns an error if required dependencies (e.g. Config) are missing +// so callers can handle invalid input gracefully instead of crashing. +func NewServer(deps ServerDeps) (*Server, error) { return NewServerWithLogger(deps, slog.Default()) } // NewServerWithLogger creates a Server with routes and an injected logger. -func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) *Server { +// It returns an error if deps.Config is nil; previously this case panicked, +// but returning an error lets the caller (e.g. cmd/player/main.go) report +// the failure cleanly and exit with a useful message rather than crashing +// deep in the wiring code. +func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) (*Server, error) { if deps.Config == nil { - panic("api.NewServerWithLogger: Config is nil") + return nil, errors.New("api.NewServerWithLogger: Config is nil") } if deps.StaticFS == nil { deps.StaticFS = http.Dir("web") @@ -101,12 +110,13 @@ func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) *Server { playbackHintSvc: deps.Services.PlaybackHints, streamer: deps.MediaStreamer, staticFS: deps.StaticFS, + shareRenderer: web.NewSharePageRenderer(deps.StaticFS), logger: logger, mw: NewMiddleware(deps.Services.Auth, deps.SessionManager), } s.routes() s.handler = withCORS(s.cfg.CORSAllowedOrigins, s.mw.BootstrapRedirect(s.mux)) - return s + return s, nil } // ServeHTTP implements http.Handler. diff --git a/player-server/internal/api/server_test.go b/player-server/internal/api/server_test.go index a3e84a7..91f793d 100644 --- a/player-server/internal/api/server_test.go +++ b/player-server/internal/api/server_test.go @@ -5,15 +5,18 @@ import ( "testing" ) -func TestNewServerWithLogger_PanicsOnNilConfig(t *testing.T) { - defer func() { - if r := recover(); r == nil { - t.Fatal("expected panic for nil Config, got none") - } - }() - - NewServerWithLogger(ServerDeps{ +// TestNewServerWithLogger_ErrorsOnNilConfig verifies that the constructor +// returns an error (rather than panicking) when deps.Config is nil. The +// caller in cmd/player/main.go relies on this to fail gracefully. +func TestNewServerWithLogger_ErrorsOnNilConfig(t *testing.T) { + srv, err := NewServerWithLogger(ServerDeps{ Config: nil, StaticFS: nil, }, slog.Default()) + if err == nil { + t.Fatal("expected error for nil Config, got nil") + } + if srv != nil { + t.Fatalf("expected nil Server on error, got %v", srv) + } } diff --git a/player-server/internal/web/sharepage.go b/player-server/internal/web/sharepage.go new file mode 100644 index 0000000..37844d8 --- /dev/null +++ b/player-server/internal/web/sharepage.go @@ -0,0 +1,106 @@ +// Package web renders HTML pages for the player-server. +// +// The share-page renderer encapsulates the templating concern that used +// to live inline in the api package: opening the static share.html file, +// replacing the SHARE_MEDIA placeholder with marshaled JSON metadata, +// and reporting the file's ModTime for cache validators. +// +// Keeping this logic here lets HTTP handlers stay focused on routing and +// error translation (Separation of Concerns) and stops them reaching +// through a file-system abstraction (Law of Demeter). +package web + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "time" +) + +// ShareMediaPlaceholder is the HTML comment that gets substituted with +// the JSON-encoded share metadata inside share.html. +const ShareMediaPlaceholder = "" + +// shareTemplateName is the filename looked up in the static FS. +const shareTemplateName = "share.html" + +// SharePageRenderer renders the public share landing page by inlining +// share metadata into a static HTML template. +// +// A renderer captures the file system and the placeholder it works with +// so callers (HTTP handlers) only need to pass the data to inject. +type SharePageRenderer struct { + fs http.FileSystem + template string + placeholder string +} + +// NewSharePageRenderer builds a renderer backed by the given file system. +// The file system must contain share.html. The placeholder defaults to +// ShareMediaPlaceholder. +func NewSharePageRenderer(fs http.FileSystem) *SharePageRenderer { + return &SharePageRenderer{ + fs: fs, + template: shareTemplateName, + placeholder: ShareMediaPlaceholder, + } +} + +// RenderedPage carries the bytes to serve along with the source template's +// modification time (used for HTTP cache validators in ServeContent). +type RenderedPage struct { + HTML string + ModTime time.Time + Name string +} + +// Render reads the share template from the file system, injects the +// JSON-encoded data in place of the placeholder, and returns the result. +// +// The caller (an HTTP handler) is responsible for turning errors into +// appropriate HTTP status codes; this package stays transport-agnostic. +func (r *SharePageRenderer) Render(data any) (RenderedPage, error) { + if r == nil || r.fs == nil { + return RenderedPage{}, fmt.Errorf("share renderer not configured") + } + + f, err := r.fs.Open(r.template) + if err != nil { + return RenderedPage{}, fmt.Errorf("open share template: %w", err) + } + defer f.Close() + + stat, err := f.Stat() + if err != nil { + return RenderedPage{}, fmt.Errorf("stat share template: %w", err) + } + + var buf strings.Builder + if _, err := io.Copy(&buf, f); err != nil { + return RenderedPage{}, fmt.Errorf("read share template: %w", err) + } + + html, err := injectShareMedia(buf.String(), r.placeholder, data) + if err != nil { + return RenderedPage{}, err + } + + return RenderedPage{ + HTML: html, + ModTime: stat.ModTime(), + Name: r.template, + }, nil +} + +// injectShareMedia replaces placeholder with the JSON-encoded form of +// data, returning the new HTML. It is private to keep this package's +// surface small: callers are expected to go through SharePageRenderer. +func injectShareMedia(html, placeholder string, data any) (string, error) { + encoded, err := json.Marshal(data) + if err != nil { + return "", fmt.Errorf("marshal share metadata: %w", err) + } + return strings.Replace(html, placeholder, string(encoded), 1), nil +} diff --git a/player-server/internal/web/sharepage_test.go b/player-server/internal/web/sharepage_test.go new file mode 100644 index 0000000..7a89131 --- /dev/null +++ b/player-server/internal/web/sharepage_test.go @@ -0,0 +1,92 @@ +package web + +import ( + "errors" + "io" + "net/http" + "os" + "strings" + "testing" + "testing/fstest" +) + +func TestSharePageRenderer_Render(t *testing.T) { + t.Run("injects marshaled metadata", func(t *testing.T) { + fs := fstest.MapFS{ + "share.html": {Data: []byte(``)}, + } + r := NewSharePageRenderer(http.FS(fs)) + page, err := r.Render(map[string]string{"stream_url": "/s/abc/stream"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(page.HTML, `"stream_url":"/s/abc/stream"`) { + t.Fatalf("expected injected JSON, got %q", page.HTML) + } + if page.Name != "share.html" { + t.Fatalf("expected name share.html, got %q", page.Name) + } + }) + + t.Run("returns marshal error", func(t *testing.T) { + fs := fstest.MapFS{ + "share.html": {Data: []byte(``)}, + } + r := NewSharePageRenderer(http.FS(fs)) + _, err := r.Render(map[string]any{"bad": make(chan int)}) + if err == nil { + t.Fatal("expected marshal error") + } + if !strings.Contains(err.Error(), "marshal share metadata") { + t.Fatalf("expected wrapped marshal error, got %v", err) + } + }) + + t.Run("nil renderer", func(t *testing.T) { + var r *SharePageRenderer + if _, err := r.Render(nil); err == nil { + t.Fatal("expected error from nil renderer") + } + }) + + t.Run("missing template", func(t *testing.T) { + r := NewSharePageRenderer(http.FS(fstest.MapFS{})) + _, err := r.Render(nil) + if err == nil { + t.Fatal("expected open error") + } + }) + + t.Run("stat error propagates", func(t *testing.T) { + r := NewSharePageRenderer(statErrorFS{}) + _, err := r.Render(nil) + if err == nil { + t.Fatal("expected stat error") + } + }) +} + +func TestInjectShareMedia(t *testing.T) { + html, err := injectShareMedia(`a b`, "", "v") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if html != `a "v" b` { + t.Fatalf("unexpected result: %q", html) + } +} + +// statErrorFS returns a file whose Stat() fails — used to cover the +// rare error path in Render where the template is openable but cannot +// be stat'd. +type statErrorFS struct{} + +func (statErrorFS) Open(string) (http.File, error) { return statErrorFile{}, nil } + +type statErrorFile struct{} + +func (statErrorFile) Close() error { return nil } +func (statErrorFile) Read([]byte) (int, error) { return 0, io.EOF } +func (statErrorFile) Seek(int64, int) (int64, error) { return 0, nil } +func (statErrorFile) Readdir(int) ([]os.FileInfo, error) { return nil, nil } +func (statErrorFile) Stat() (os.FileInfo, error) { return nil, errors.New("stat failed") } -- cgit v1.2.3