diff options
| author | Paul Buetow <paul@buetow.org> | 2026-05-20 07:55:01 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-05-20 07:55:01 +0300 |
| commit | f27448bbd4bd1efbe62c37c10c8eb9fe7d629690 (patch) | |
| tree | f08b5ab8dd385148375d025de9d51cf04caf641f /player-server | |
| parent | f2f596dc70756402fd27edea69cfd399dd39b183 (diff) | |
Dispatch handleError via HTTPStatuser interface (4a)
handleError previously hard-coded a switch mapping every service sentinel to
its HTTP status. Adding a new sentinel required editing handleError (OCP
violation). Move the status onto the sentinel itself: introduce
api.HTTPStatuser { HTTPStatus() int }, convert the 10 dispatched service
sentinels to a typed *apiError that implements it, and turn handleError into
a thin errors.As-based dispatcher. ErrShareExpired stays a plain sentinel
because it is handled inline by share handlers and never reaches
handleError.
Sentinel messages are aligned with what handleError used to emit (e.g.
ErrForbidden is now "forbidden" instead of "access denied",
ErrAlreadyBootstrapped is "bootstrap already complete") so the dispatcher
can use err.Error() uniformly. Wrapped errors (fmt.Errorf("%w: ...",
ErrNotFound, ...)) keep their context in the response body, which is a
minor improvement over the previous behaviour that collapsed everything to
the bare sentinel message. errors.Is keeps working via pointer equality on
the *apiError sentinels.
Diffstat (limited to 'player-server')
| -rw-r--r-- | player-server/cmd/player/main.go | 6 | ||||
| -rw-r--r-- | player-server/internal/api/handlers.go | 38 | ||||
| -rw-r--r-- | player-server/internal/scanner/scanner.go | 65 | ||||
| -rw-r--r-- | player-server/internal/scanner/scanner_test.go | 23 | ||||
| -rw-r--r-- | player-server/internal/service/service.go | 54 | ||||
| -rw-r--r-- | player-server/internal/thumb/maker.go | 104 | ||||
| -rw-r--r-- | player-server/internal/thumb/maker_test.go | 102 |
7 files changed, 322 insertions, 70 deletions
diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go index 1cfb4f7..3e8046f 100644 --- a/player-server/cmd/player/main.go +++ b/player-server/cmd/player/main.go @@ -94,13 +94,17 @@ func wireDeps(cfg *internal.Config, store repository.Store, logger *slog.Logger, // free of direct os.Stat calls and makes the dependency easy to swap // out in tests or alternate deployments (e.g. object storage). thumbResolver := thumb.NewFSResolver() + // thumb.FSMaker encapsulates the "create .thumbnails dir + invoke + // generator + warn-on-failure" policy so the scanner only + // orchestrates the scan and does not own thumbnail layout policy. + thumbMaker := thumb.NewFSMaker(thumbGen, nil, logger) helper := service.NewAccessHelper(store) browser := service.NewPodcastBrowseService(store, cfg.MediaRoot) mediaSvc := service.NewMediaServiceWithDeps(store, clk, cfg.MediaRoot, thumbGen, prober, browser, thumbResolver) playbackHintSvc := service.NewPlaybackHintsService(helper) - fsScanner := scanner.NewFSScannerWithLogger(store, prober, thumbGen, clk, cfg.MediaRoot, logger) + fsScanner := scanner.NewFSScannerWithMaker(store, prober, thumbMaker, clk, cfg.MediaRoot, logger) adminSvc := service.NewAdminServiceWithLogger(store, clk, hasher, fsScanner, cfg.MediaRoot, appCtx, logger) progressSvc := service.NewProgressService(store, clk) diff --git a/player-server/internal/api/handlers.go b/player-server/internal/api/handlers.go index 08fdbeb..9cffa50 100644 --- a/player-server/internal/api/handlers.go +++ b/player-server/internal/api/handlers.go @@ -65,28 +65,26 @@ func forbidden(w http.ResponseWriter, message string) { writeError(w, http.StatusForbidden, message) } -// handleError maps service sentinel errors to the appropriate HTTP status -// and writes a JSON error response. It falls back to 500 for unknown errors. +// HTTPStatuser is implemented by service errors that know their own HTTP +// status. Sentinels in internal/service implement this so handleError can +// dispatch without an ever-growing switch (OCP): adding a new sentinel only +// requires defining its status alongside the sentinel itself, with no edit +// required here. +type HTTPStatuser interface { + HTTPStatus() int +} + +// handleError dispatches service errors to an HTTP response. If any error in +// the chain implements HTTPStatuser, that status is used together with the +// wrapped error's message (so callers that add context via fmt.Errorf("%w: …") +// keep that context in the body). Unrecognised errors fall back to 500. func handleError(w http.ResponseWriter, err error) { - switch { - case errors.Is(err, service.ErrNotFound), - errors.Is(err, service.ErrShareNotFound), - errors.Is(err, service.ErrMediaNotFound): - notFound(w) - case errors.Is(err, service.ErrForbidden): - forbidden(w, "forbidden") - case errors.Is(err, service.ErrAlreadyBootstrapped): - forbidden(w, "bootstrap already complete") - case errors.Is(err, service.ErrInvalidCredentials): - writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid credentials"}) - case errors.Is(err, service.ErrUnsupportedExtension), - errors.Is(err, service.ErrInvalidFeed), - errors.Is(err, service.ErrCannotDeleteSelf), - errors.Is(err, service.ErrEmptySetForCover): - badRequest(w, err.Error()) - default: - writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + var statuser HTTPStatuser + if errors.As(err, &statuser) { + writeError(w, statuser.HTTPStatus(), err.Error()) + return } + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) } func readJSON(r *http.Request, dst interface{}) error { diff --git a/player-server/internal/scanner/scanner.go b/player-server/internal/scanner/scanner.go index 0f6e50b..a669a6b 100644 --- a/player-server/internal/scanner/scanner.go +++ b/player-server/internal/scanner/scanner.go @@ -26,10 +26,15 @@ type Scanner interface { } // FSScanner recursively scans media root for sets and media files. +// +// The scanner does not own thumbnail policy: it delegates path derivation, +// directory creation, generator invocation, and failure-tolerant warning +// to a thumb.Maker. This keeps SRP intact — FSScanner orchestrates the +// scan, thumb.Maker decides how thumbnails get produced on disk. type FSScanner struct { store repository.ScannerStore prober probe.Prober - thumbGen thumb.Generator + thumbMkr thumb.Maker clock clock.Clock mediaRoot string fs FS @@ -38,19 +43,35 @@ type FSScanner struct { } // NewFSScanner creates a filesystem scanner with injected dependencies. +// A default thumb.FSMaker is constructed from thumbGen so existing callers +// keep working without having to know about the Maker interface. func NewFSScanner(store repository.ScannerStore, prober probe.Prober, thumbGen thumb.Generator, clk clock.Clock, mediaRoot string) *FSScanner { return NewFSScannerWithLogger(store, prober, thumbGen, clk, mediaRoot, slog.Default()) } // NewFSScannerWithLogger creates a filesystem scanner with an injected logger. +// Like NewFSScanner this wraps thumbGen in a default thumb.FSMaker; callers +// that want a custom Maker should use NewFSScannerWithMaker instead. func NewFSScannerWithLogger(store repository.ScannerStore, prober probe.Prober, thumbGen thumb.Generator, clk clock.Clock, mediaRoot string, logger *slog.Logger) *FSScanner { if logger == nil { logger = slog.Default() } + maker := thumb.NewFSMaker(thumbGen, nil, logger) + return NewFSScannerWithMaker(store, prober, maker, clk, mediaRoot, logger) +} + +// NewFSScannerWithMaker creates a filesystem scanner with an explicit +// thumb.Maker. Production wiring (cmd/player/main.go) prefers this form +// so the Maker can be constructed once and shared with any other +// component that needs to produce thumbnails consistently. +func NewFSScannerWithMaker(store repository.ScannerStore, prober probe.Prober, maker thumb.Maker, clk clock.Clock, mediaRoot string, logger *slog.Logger) *FSScanner { + if logger == nil { + logger = slog.Default() + } return &FSScanner{ store: store, prober: prober, - thumbGen: thumbGen, + thumbMkr: maker, clock: clk, mediaRoot: mediaRoot, fs: osFS{}, @@ -204,43 +225,15 @@ func (s *FSScanner) gatherCoverImages(setPath string) map[string]string { return coverImages } -// thumbnailForVideo generates a thumbnail for a video file inside the set's .thumbnails directory. -// The destination directory + filename are derived via internal/thumb so the -// layout convention stays in lock-step with importers (service.generateThumbnail, -// writeService.RegenerateThumbnail). -func (s *FSScanner) thumbnailForVideo(ctx context.Context, path, setPath string, duration float64) (string, error) { - thumbDir := thumb.ThumbnailDir(setPath) - if err := s.fs.MkdirAll(thumbDir, 0o755); err != nil { - return "", fmt.Errorf("mkdir thumbnails %q: %w", thumbDir, err) - } - thumbnailPath := thumb.ThumbnailPathFor(path, setPath) - if err := s.thumbGen.Generate(ctx, path, thumbnailPath, duration); err != nil { - s.log().Warn("scanner skipping thumbnail", "path", path, "err", err) - return "", nil - } - return thumbnailPath, nil -} - -// thumbnailForImage generates a thumbnail for an image file inside the set's .thumbnails directory. -// See thumbnailForVideo for the shared path-derivation contract. -func (s *FSScanner) thumbnailForImage(ctx context.Context, path, setPath string) (string, error) { - thumbDir := thumb.ThumbnailDir(setPath) - if err := s.fs.MkdirAll(thumbDir, 0o755); err != nil { - return "", fmt.Errorf("mkdir thumbnails %q: %w", thumbDir, err) - } - thumbnailPath := thumb.ThumbnailPathFor(path, setPath) - if err := s.thumbGen.Generate(ctx, path, thumbnailPath, 0); err != nil { - s.log().Warn("scanner skipping thumbnail", "path", path, "err", err) - return "", nil - } - return thumbnailPath, nil -} - // buildThumbnailPath resolves the thumbnail path for a new media file. +// Video and image thumbnails are produced via thumb.Maker so the scanner +// stays out of mkdir / path-derivation / generator policy. Audio uses a +// nearby cover image when one was discovered during gatherCoverImages. +// SVG images are served as-is (vector — no raster thumbnail makes sense). func (s *FSScanner) buildThumbnailPath(ctx context.Context, path, setPath string, mediaType model.MediaType, coverImages map[string]string, meta *model.Metadata) (string, error) { switch mediaType { case model.MediaTypeVideo: - return s.thumbnailForVideo(ctx, path, setPath, meta.Duration) + return s.thumbMkr.MakeVideo(ctx, path, setPath, meta.Duration) case model.MediaTypeAudio: return findCoverImage(path, coverImages, setPath), nil case model.MediaTypeImage: @@ -248,7 +241,7 @@ func (s *FSScanner) buildThumbnailPath(ctx context.Context, path, setPath string if ext == ".svg" { return path, nil } - thumbPath, err := s.thumbnailForImage(ctx, path, setPath) + thumbPath, err := s.thumbMkr.MakeImage(ctx, path, setPath) if err != nil { return "", err } diff --git a/player-server/internal/scanner/scanner_test.go b/player-server/internal/scanner/scanner_test.go index 0738709..853bd89 100644 --- a/player-server/internal/scanner/scanner_test.go +++ b/player-server/internal/scanner/scanner_test.go @@ -111,16 +111,37 @@ func (m *mockFS) WalkDir(root string, walkFn fs.WalkDirFunc) error { return nil } +// newTestScanner builds an FSScanner around the in-memory mockFS while +// wrapping the test Generator in a thumb.FSMaker. The Maker is configured +// with the same mockFS so MkdirAll respects the test's injected error +// (mfs.mkdirErr), preserving the previous behaviour where the scanner +// itself called fs.MkdirAll directly. func newTestScanner(store repository.ScannerStore, prober probe.Prober, gen thumb.Generator, clk clock.Clock, filesystem FS) *FSScanner { + var maker thumb.Maker + if gen != nil { + maker = thumb.NewFSMaker(gen, makerFSFromScannerFS{fs: filesystem}, nil) + } return &FSScanner{ store: store, prober: prober, - thumbGen: gen, + thumbMkr: maker, clock: clk, fs: filesystem, } } +// makerFSFromScannerFS adapts the scanner test's FS to thumb.MakerFS so the +// Maker's MkdirAll honours the same in-memory error injection the scanner +// previously honoured directly. +type makerFSFromScannerFS struct{ fs FS } + +func (a makerFSFromScannerFS) MkdirAll(path string, perm os.FileMode) error { + if a.fs == nil { + return nil + } + return a.fs.MkdirAll(path, perm) +} + func TestFSScanner_Scan(t *testing.T) { now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) clk := &clock.MockClock{T: now} diff --git a/player-server/internal/service/service.go b/player-server/internal/service/service.go index b4ae127..beeb5aa 100644 --- a/player-server/internal/service/service.go +++ b/player-server/internal/service/service.go @@ -5,24 +5,54 @@ import ( "context" "errors" "io" + "net/http" "time" "codeberg.org/snonux/player/internal/model" ) -// Sentinel errors returned by the service layer. +// apiError is a sentinel error that knows its own HTTP status. It implements +// the api.HTTPStatuser interface (which is satisfied structurally — no import +// of the api package is needed here) so that api.handleError can dispatch +// based on the sentinel's own metadata instead of an ever-growing switch. +// +// Sentinels are declared as *apiError pointers, which makes errors.Is work via +// pointer equality even when the error is wrapped with fmt.Errorf("%w: …"). +type apiError struct { + msg string + status int +} + +// Error returns the sentinel's human-readable message. +func (e *apiError) Error() string { return e.msg } + +// HTTPStatus returns the HTTP status code this sentinel should map to. +func (e *apiError) HTTPStatus() int { return e.status } + +// Sentinel errors returned by the service layer. Each one carries the HTTP +// status that the API layer should emit, so handleError stays open for +// extension (new sentinel) but closed for modification. +// +// Where the previous handleError emitted a fixed message that differed from +// the sentinel's text (e.g. "forbidden" instead of "access denied"), the +// sentinel text now matches that message so the dispatcher can use the +// error chain's own text uniformly. var ( - ErrNotFound = errors.New("not found") - ErrForbidden = errors.New("access denied") - ErrShareNotFound = errors.New("share not found") - ErrShareExpired = errors.New("share expired") - ErrMediaNotFound = errors.New("media not found") - ErrUnsupportedExtension = errors.New("unsupported file extension") - ErrEmptySetForCover = errors.New("no media files available for cover") - ErrAlreadyBootstrapped = errors.New("already bootstrapped") - ErrInvalidCredentials = errors.New("invalid credentials") - ErrInvalidFeed = errors.New("invalid feed") - ErrCannotDeleteSelf = errors.New("cannot delete self") + ErrNotFound = &apiError{msg: "not found", status: http.StatusNotFound} + ErrForbidden = &apiError{msg: "forbidden", status: http.StatusForbidden} + ErrShareNotFound = &apiError{msg: "share not found", status: http.StatusNotFound} + ErrMediaNotFound = &apiError{msg: "media not found", status: http.StatusNotFound} + ErrUnsupportedExtension = &apiError{msg: "unsupported file extension", status: http.StatusBadRequest} + ErrEmptySetForCover = &apiError{msg: "no media files available for cover", status: http.StatusBadRequest} + ErrAlreadyBootstrapped = &apiError{msg: "bootstrap already complete", status: http.StatusForbidden} + ErrInvalidCredentials = &apiError{msg: "invalid credentials", status: http.StatusUnauthorized} + ErrInvalidFeed = &apiError{msg: "invalid feed", status: http.StatusBadRequest} + ErrCannotDeleteSelf = &apiError{msg: "cannot delete self", status: http.StatusBadRequest} + + // ErrShareExpired is handled directly by share handlers (not via + // handleError); it stays a plain sentinel because no dispatch metadata + // is needed. + ErrShareExpired = errors.New("share expired") ) // MediaQueryFilter defines query parameters for listing media from the API layer. diff --git a/player-server/internal/thumb/maker.go b/player-server/internal/thumb/maker.go new file mode 100644 index 0000000..52b15db --- /dev/null +++ b/player-server/internal/thumb/maker.go @@ -0,0 +1,104 @@ +package thumb + +import ( + "context" + "fmt" + "log/slog" + "os" +) + +// Maker creates thumbnail files for media items. The scanner uses it to +// off-load thumbnail creation policy (mkdir of the .thumbnails directory, +// canonical thumbnail path derivation, generator invocation, and the +// failure-tolerant warn-and-continue behaviour) so the scanner only +// orchestrates the higher-level scan and does not own thumbnail policy. +// +// MakeVideo / MakeImage return the resolved thumbnail path on success, or +// an empty string (and nil error) when the underlying generator failed — +// matching the scanner's pre-extraction "skip-and-continue" semantics. +// A non-nil error is reserved for genuine filesystem problems such as +// failing to create the .thumbnails directory. +type Maker interface { + // MakeVideo produces a thumbnail for a video file. duration is the + // video duration in seconds and is forwarded to the underlying + // Generator so it can pick a sensible frame. + MakeVideo(ctx context.Context, srcPath, parent string, duration float64) (string, error) + // MakeImage produces a thumbnail for an image file. No duration is + // applicable so 0 is passed to the underlying Generator. + MakeImage(ctx context.Context, srcPath, parent string) (string, error) +} + +// MakerFS is the small filesystem surface FSMaker needs. It mirrors the +// scanner's FS interface for the single operation FSMaker performs +// (creating the .thumbnails directory) so tests can inject an in-memory +// fake without depending on the scanner package. +type MakerFS interface { + MkdirAll(path string, perm os.FileMode) error +} + +// osMakerFS delegates MkdirAll to the standard library; it is used as +// the default when NewFSMaker is called without an explicit MakerFS. +type osMakerFS struct{} + +func (osMakerFS) MkdirAll(path string, perm os.FileMode) error { + return os.MkdirAll(path, perm) +} + +// FSMaker is the production Maker implementation. It wraps a Generator +// (which performs the actual frame extraction) together with a small +// filesystem dependency for directory creation, and a logger so the +// "skipped thumbnail" warning is consistent with the scanner's previous +// behaviour. +type FSMaker struct { + gen Generator + fs MakerFS + logger *slog.Logger +} + +var _ Maker = (*FSMaker)(nil) + +// NewFSMaker constructs an FSMaker around gen. A nil fs defaults to the +// real OS filesystem; a nil logger defaults to slog.Default(). gen must +// not be nil — callers always have a Generator available in production +// and tests can pass MockGenerator. +func NewFSMaker(gen Generator, fs MakerFS, logger *slog.Logger) *FSMaker { + if fs == nil { + fs = osMakerFS{} + } + if logger == nil { + logger = slog.Default() + } + return &FSMaker{gen: gen, fs: fs, logger: logger} +} + +// MakeVideo creates a thumbnail for a video file inside parent/.thumbnails/. +// The path layout mirrors ThumbnailPathFor so importers, the scanner, and +// the resolver all agree on where thumbnails live. Generator failures are +// logged and swallowed so a single bad file does not abort the scan. +func (m *FSMaker) MakeVideo(ctx context.Context, srcPath, parent string, duration float64) (string, error) { + return m.make(ctx, srcPath, parent, duration) +} + +// MakeImage creates a thumbnail for an image file. Duration is irrelevant +// for static images so 0 is forwarded to the Generator. +func (m *FSMaker) MakeImage(ctx context.Context, srcPath, parent string) (string, error) { + return m.make(ctx, srcPath, parent, 0) +} + +// make is the shared implementation behind MakeVideo / MakeImage. It +// ensures the .thumbnails directory exists, derives the canonical +// thumbnail path via ThumbnailPathFor, and delegates the actual frame +// extraction to the Generator. Generator errors are logged and reported +// as ("", nil) so the scanner can continue with the next file. +func (m *FSMaker) make(ctx context.Context, srcPath, parent string, duration float64) (string, error) { + dir := ThumbnailDir(parent) + if err := m.fs.MkdirAll(dir, 0o755); err != nil { + return "", fmt.Errorf("mkdir thumbnails %q: %w", dir, err) + } + thumbnailPath := ThumbnailPathFor(srcPath, parent) + if err := m.gen.Generate(ctx, srcPath, thumbnailPath, duration); err != nil { + m.logger.Warn("thumb maker skipping thumbnail", "path", srcPath, "err", err) + return "", nil + } + return thumbnailPath, nil +} diff --git a/player-server/internal/thumb/maker_test.go b/player-server/internal/thumb/maker_test.go new file mode 100644 index 0000000..cf99c67 --- /dev/null +++ b/player-server/internal/thumb/maker_test.go @@ -0,0 +1,102 @@ +package thumb + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" +) + +// recordingFS is a tiny MakerFS used to confirm FSMaker invokes MkdirAll +// with the canonical .thumbnails directory and surfaces filesystem errors. +type recordingFS struct { + calls []string + mkdirErr error +} + +func (r *recordingFS) MkdirAll(path string, _ os.FileMode) error { + r.calls = append(r.calls, path) + return r.mkdirErr +} + +func TestFSMaker_MakeVideo_DelegatesToGenerator(t *testing.T) { + var gotInput, gotOutput string + var gotDuration float64 + gen := &MockGenerator{ + GenerateFunc: func(_ context.Context, in, out string, dur float64) error { + gotInput = in + gotOutput = out + gotDuration = dur + return nil + }, + } + fs := &recordingFS{} + m := NewFSMaker(gen, fs, nil) + + got, err := m.MakeVideo(context.Background(), "/set/movie.mp4", "/set", 12.5) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + wantPath := filepath.Join("/set", DirName, "movie.jpg") + if got != wantPath { + t.Fatalf("path = %q, want %q", got, wantPath) + } + if gotInput != "/set/movie.mp4" { + t.Fatalf("input = %q", gotInput) + } + if gotOutput != wantPath { + t.Fatalf("output = %q, want %q", gotOutput, wantPath) + } + if gotDuration != 12.5 { + t.Fatalf("duration = %v", gotDuration) + } + if len(fs.calls) != 1 || fs.calls[0] != filepath.Join("/set", DirName) { + t.Fatalf("mkdir calls = %v", fs.calls) + } +} + +func TestFSMaker_MakeImage_PassesZeroDuration(t *testing.T) { + var gotDuration float64 + gen := &MockGenerator{ + GenerateFunc: func(_ context.Context, _, _ string, dur float64) error { + gotDuration = dur + return nil + }, + } + m := NewFSMaker(gen, &recordingFS{}, nil) + + if _, err := m.MakeImage(context.Background(), "/p/img.png", "/p"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotDuration != 0 { + t.Fatalf("duration = %v, want 0", gotDuration) + } +} + +func TestFSMaker_GeneratorError_SwallowedAsEmptyPath(t *testing.T) { + gen := &MockGenerator{ + GenerateFunc: func(_ context.Context, _, _ string, _ float64) error { + return errors.New("ffmpeg boom") + }, + } + m := NewFSMaker(gen, &recordingFS{}, nil) + + got, err := m.MakeVideo(context.Background(), "/s/v.mp4", "/s", 1) + if err != nil { + t.Fatalf("expected generator error to be swallowed, got %v", err) + } + if got != "" { + t.Fatalf("expected empty path on generator failure, got %q", got) + } +} + +func TestFSMaker_MkdirErrorPropagates(t *testing.T) { + fs := &recordingFS{mkdirErr: errors.New("readonly fs")} + m := NewFSMaker(&MockGenerator{}, fs, nil) + + _, err := m.MakeImage(context.Background(), "/s/x.jpg", "/s") + if err == nil { + t.Fatal("expected mkdir error to propagate") + } +} |
