From 758e64c9996e6aa01e8428fb587292873f9f181f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 2 Oct 2026 05:05:32 -0400 Subject: [PATCH 1/4] feat(scanner): per-library PID configuration (#6252) * feat(model): add per-library PID config columns * refactor(metadata): pass PID config to ToMediaFile and add spec validation * feat(scanner): rescan only libraries whose PID config changed * feat(server): validate library PID config and rescan on change * feat(ui): edit per-library PID config * fix(ui): label the PID mode selects * fix: tighten per-library PID rescan edge cases An interrupted PID rescan no longer upgrades every library to a full scan, a save that loses the race for the scanner logs at debug, the confirm dialog only shows when the effective PID spec changes, and it now gets translation keys. * refactor(metadata): pass the library to ToMediaFile ToMediaFile and core.Inspect took the library ID and its PID config as separate arguments, so a caller could mix values from two libraries. They now take the model.Library and resolve the effective PID config from it. * chore: tidy per-library PID comments, PropTypes and migration Trim comments that restated the code, add PropTypes to the new UI components, and recreate the migration with make migration-sql. * fix(ui): show the PID spec help under its input * feat(cmd): make inspect use the file's library PID config inspect always used the global PID config, so it showed different IDs than the scanner for files in a library with an override. It now finds the file's library in the DB and uses its effective config, falling back to the global config when there is no DB or the file is outside every library. It never creates a DB. The library path matcher moves from core/playlists to model so both can use it. * refactor: simplify per-library PID code Share the DB-file check between CLI commands, move ErrAlreadyScanning to model so core no longer imports scanner, read the libraries once for insights, and let ValidatePIDSpec accept an empty spec and look tags up directly. In the scanner, use FullScanInProgress instead of a second flag, and skip recomputing album IDs when the album spec did not change. In the UI, share the PID inputs between Create and Edit, and use docsUrl. * feat(ui): add section titles to Library Create and pre-fill Custom PID specs Custom now starts from the global spec, so admins edit a working spec instead of typing one from scratch. * fix(inspect): map files with the library-relative path the scanner uses Inspect gave metadata the file's directory as typed, so folder-based PIDs never matched the DB. It now uses the path relative to the library root, through the scanner's helper, which moves to model. * fix(scanner): say when a PID rescan only covers target folders * fix: reject tag aliases in album PID specs and match root libraries Tags are stored under canonical names, so an alias in a spec always reads as empty. In an album spec that gives every album the same ID, so album specs now require the tag name. Track specs keep accepting aliases, since the default one uses them. LibraryMatcher now matches paths under a library at the filesystem root. * refactor(model): move the tag alias lookup to tag_mappings.go * test: run the library matcher and inspect tests on Windows Build test paths with filepath instead of Unix literals, so they use the OS separator like filepath.Abs output, and drop the Windows skips. * feat(ui): add pt-BR translations for per-library PID settings --- adapters/gotaglib/end_to_end_test.go | 2 +- cmd/inspect.go | 46 +++- cmd/inspect_test.go | 61 +++++ cmd/root.go | 28 ++- cmd/root_test.go | 30 +++ cmd/utils.go | 13 +- consts/consts.go | 2 - core/inspect.go | 16 +- core/inspect_test.go | 45 ++++ core/library.go | 54 ++++- core/library_test.go | 73 ++++++ core/metrics/insights.go | 9 +- core/playlists/import.go | 4 +- core/playlists/parse_m3u.go | 70 +----- core/playlists/parse_m3u_test.go | 181 -------------- ...20260929221042_add_library_pid_columns.sql | 18 ++ model/library.go | 37 +++ model/library_matcher.go | 57 +++++ model/library_matcher_test.go | 91 +++++++ model/library_test.go | 73 ++++++ model/metadata/map_mediafile.go | 10 +- model/metadata/map_mediafile_test.go | 16 +- model/metadata/map_participants_test.go | 2 +- model/metadata/metadata_test.go | 2 +- model/metadata/persistent_ids.go | 55 ++++- model/metadata/persistent_ids_test.go | 54 ++++- model/scanner.go | 3 + model/tag_mappings.go | 22 ++ model/tag_mappings_test.go | 19 ++ persistence/library_repository.go | 11 + persistence/library_repository_test.go | 32 +++ resources/i18n/pt-br.json | 19 +- scanner/controller.go | 8 +- scanner/controller_test.go | 12 +- scanner/phase_1_folders.go | 73 +++--- scanner/scanner.go | 55 ++--- scanner/scanner_internal_test.go | 39 --- scanner/scanner_multilibrary_test.go | 166 +++++++++++++ server/nativeapi/inspect.go | 7 +- server/serve_index.go | 2 + server/serve_index_test.go | 2 + tests/mock_library_repo.go | 11 + ui/src/config.js | 2 + ui/src/i18n/en.json | 19 +- ui/src/library/LibraryCreate.jsx | 31 +++ ui/src/library/LibraryEdit.jsx | 222 +++++++++++------- ui/src/library/LibraryEdit.test.jsx | 125 ++++++++++ ui/src/library/PIDInput.jsx | 114 +++++++++ ui/src/library/pidPresets.js | 33 +++ ui/src/library/pidPresets.test.js | 65 +++++ 50 files changed, 1626 insertions(+), 515 deletions(-) create mode 100644 cmd/inspect_test.go create mode 100644 core/inspect_test.go create mode 100644 db/migrations/20260929221042_add_library_pid_columns.sql create mode 100644 model/library_matcher.go create mode 100644 model/library_matcher_test.go create mode 100644 model/library_test.go create mode 100644 ui/src/library/LibraryEdit.test.jsx create mode 100644 ui/src/library/PIDInput.jsx create mode 100644 ui/src/library/pidPresets.js create mode 100644 ui/src/library/pidPresets.test.js diff --git a/adapters/gotaglib/end_to_end_test.go b/adapters/gotaglib/end_to_end_test.go index e7dd18ac1..0f9a90d94 100644 --- a/adapters/gotaglib/end_to_end_test.go +++ b/adapters/gotaglib/end_to_end_test.go @@ -90,7 +90,7 @@ var _ = Describe("Extractor", func() { info.FileInfo = testFileInfo{FileInfo: fileInfo} metadata := metadata.New(path, info) - return new(metadata.ToMediaFile(1, "folderID")) + return new(metadata.ToMediaFile(model.Library{ID: 1}, "folderID")) } BeforeEach(func() { diff --git a/cmd/inspect.go b/cmd/inspect.go index 5e88793cc..05f569f3e 100644 --- a/cmd/inspect.go +++ b/cmd/inspect.go @@ -1,13 +1,17 @@ package cmd import ( + "context" "encoding/json" "fmt" + "path/filepath" "strings" "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/persistence" "github.com/pelletier/go-toml/v2" "github.com/spf13/cobra" "gopkg.in/yaml.v3" @@ -28,7 +32,7 @@ var inspectCmd = &cobra.Command{ Long: "Show file tags as seen by Navidrome", Args: cobra.MinimumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - runInspector(args) + runInspector(cmd.Context(), args) }, } @@ -55,18 +59,24 @@ func prettyMarshal(v any) ([]byte, error) { return []byte(res.String()), nil } -func runInspector(args []string) { +func runInspector(ctx context.Context, args []string) { marshal := marshalers[format] if marshal == nil { log.Fatal("Invalid format", "format", format) } + libs := loadLibraries(ctx) + matcher := model.NewLibraryMatcher(libs) var out []core.InspectOutput for _, filePath := range args { if !model.IsAudioFile(filePath) { log.Warn("Not an audio file", "file", filePath) continue } - output, err := core.Inspect(filePath, 1, "") + lib, ok := libraryForFile(matcher, filePath) + if !ok && len(libs) > 0 { + log.Warn("File is not in any library, using the global PID config", "file", filePath) + } + output, err := core.Inspect(filePath, lib, "") if err != nil { log.Warn("Unable to process file", "file", filePath, "error", err) continue @@ -77,3 +87,33 @@ func runInspector(args []string) { data, _ := marshal(out) fmt.Println(string(data)) } + +// loadLibraries reads the libraries, so each file gets its library's PID config. It never creates a DB. +func loadLibraries(ctx context.Context) model.Libraries { + if dbFile, ok := existingDBFile(); !ok { + log.Warn(ctx, "No database found, using the global PID config", "path", dbFile) + return nil + } + defer db.Init(ctx)() + libs, err := persistence.New(db.Db()).Library().GetAll(ctx) + if err != nil { + log.Warn(ctx, "Could not load libraries, using the global PID config", err) + return nil + } + for i := range libs { + if absPath, err := filepath.Abs(libs[i].Path); err == nil { + libs[i].Path = absPath + } + } + return libs +} + +// libraryForFile falls back to the default library with no overrides, which uses the global PID config. +func libraryForFile(matcher *model.LibraryMatcher, filePath string) (model.Library, bool) { + if absPath, err := filepath.Abs(filePath); err == nil { + if lib, ok := matcher.FindLibrary(absPath); ok { + return lib, true + } + } + return model.Library{ID: model.DefaultLibraryID}, false +} diff --git a/cmd/inspect_test.go b/cmd/inspect_test.go new file mode 100644 index 000000000..728dc8770 --- /dev/null +++ b/cmd/inspect_test.go @@ -0,0 +1,61 @@ +package cmd + +import ( + "os" + "path/filepath" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("inspect", func() { + Describe("libraryForFile", func() { + var matcher *model.LibraryMatcher + var root string + + BeforeEach(func() { + root = GinkgoT().TempDir() + cwd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + matcher = model.NewLibraryMatcher(model.Libraries{ + {ID: 1, Path: filepath.Join(root, "music")}, + {ID: 2, Path: filepath.Join(cwd, "loose"), PIDAlbum: "folder"}, + }) + }) + + It("returns the library that contains an absolute path", func() { + lib, ok := libraryForFile(matcher, filepath.Join(root, "music", "album", "track.mp3")) + Expect(ok).To(BeTrue()) + Expect(lib.ID).To(Equal(1)) + }) + + It("resolves a relative path against the working directory", func() { + lib, ok := libraryForFile(matcher, filepath.Join("loose", "track.mp3")) + Expect(ok).To(BeTrue()) + Expect(lib.PIDAlbum).To(Equal("folder")) + }) + + It("falls back to the default library without overrides", func() { + lib, ok := libraryForFile(matcher, filepath.Join(root, "elsewhere", "track.mp3")) + Expect(ok).To(BeFalse()) + Expect(lib).To(Equal(model.Library{ID: model.DefaultLibraryID})) + }) + }) + + Describe("loadLibraries", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("does not create a database when there is none", func() { + dbFile := filepath.Join(GinkgoT().TempDir(), "navidrome.db") + conf.Server.DbPath = dbFile + "?_journal_mode=WAL" + + Expect(loadLibraries(GinkgoT().Context())).To(BeNil()) + Expect(dbFile).ToNot(BeAnExistingFile()) + }) + }) +}) diff --git a/cmd/root.go b/cmd/root.go index 089f09472..e39c55365 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -190,16 +190,20 @@ func schedulePeriodicScan(ctx context.Context) func() error { } } -func pidHashChanged(ds model.DataStore) (bool, error) { - pidAlbum, err := ds.Property().DefaultGet(context.Background(), consts.PIDAlbumKey, "") +// librariesWithChangedPID returns the names of the libraries whose effective PID config differs from +// the one used by their last finished scan +func librariesWithChangedPID(ctx context.Context, ds model.DataStore) ([]string, error) { + libs, err := ds.Library().GetAll(ctx) if err != nil { - return false, err + return nil, err } - pidTrack, err := ds.Property().DefaultGet(context.Background(), consts.PIDTrackKey, "") - if err != nil { - return false, err + var names []string + for _, lib := range libs { + if lib.PIDChanged() { + names = append(names, lib.Name) + } } - return !strings.EqualFold(pidAlbum, conf.Server.PID.Album) || !strings.EqualFold(pidTrack, conf.Server.PID.Track), nil + return names, nil } // runInitialScan runs an initial scan of the music library if needed. @@ -214,12 +218,12 @@ func runInitialScan(ctx context.Context) func() error { if err != nil { return err } - pidHasChanged, err := pidHashChanged(ds) + pidChangedLibs, err := librariesWithChangedPID(ctx, ds) if err != nil { return err } scanOnStartup := conf.Server.Scanner.Enabled && conf.Server.Scanner.ScanOnStartup - scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || pidHasChanged + scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || len(pidChangedLibs) > 0 time.Sleep(2 * time.Second) // Wait 2 seconds before the initial scan if scanNeeded { s := CreateScanner(ctx) @@ -227,9 +231,9 @@ func runInitialScan(ctx context.Context) func() error { case fullScanRequired == "1": log.Warn(ctx, "Full scan required after migration") _ = ds.Property().Delete(ctx, consts.FullScanAfterMigrationFlagKey) - case pidHasChanged: - log.Warn(ctx, "PID config changed, performing full scan") - fullScanRequired = "1" + case len(pidChangedLibs) > 0: + // Includes never-scanned libraries. The scanner rescans in full only the ones that need it + log.Warn(ctx, "Libraries with a new or changed PID config, scanning", "libraries", pidChangedLibs) case inProgress: log.Warn(ctx, "Resuming interrupted scan") default: diff --git a/cmd/root_test.go b/cmd/root_test.go index af8d44e7e..423cd2a8f 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "net/http" "net/http/httptest" "path" @@ -9,6 +10,8 @@ import ( "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -44,3 +47,30 @@ var _ = Describe("profilerHandler", func() { Entry("with a trailing-slash BasePath", "/music/"), ) }) + +var _ = Describe("librariesWithChangedPID", func() { + var ds *tests.MockDataStore + var libs *tests.MockLibraryRepo + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + libs = &tests.MockLibraryRepo{} + ds = &tests.MockDataStore{MockedLibrary: libs} + }) + + It("returns only the libraries whose PID config changed", func() { + pid := model.Library{}.EffectivePID() + libs.SetData(model.Libraries{ + {ID: 1, Name: "Same", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 2, Name: "Changed", PIDAlbum: "folder", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 3, Name: "Never scanned"}, + }) + Expect(librariesWithChangedPID(GinkgoT().Context(), ds)).To(ConsistOf("Changed", "Never scanned")) + }) + + It("returns the error from the repository", func() { + libs.Err = errors.New("db down") + _, err := librariesWithChangedPID(GinkgoT().Context(), ds) + Expect(err).To(MatchError("db down")) + }) +}) diff --git a/cmd/utils.go b/cmd/utils.go index 72ec67f90..f35a31fb1 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -18,11 +18,16 @@ import ( "github.com/navidrome/navidrome/persistence" ) -// requireExistingDB aborts the command when the database file (DbPath minus DSN -// params) does not exist. -func requireExistingDB() { +// existingDBFile returns the database file (DbPath minus DSN params), and whether it exists. +func existingDBFile() (string, bool) { path, _, _ := strings.Cut(conf.Server.DbPath, "?") - if _, err := os.Stat(path); os.IsNotExist(err) { + _, err := os.Stat(path) + return path, err == nil +} + +// requireExistingDB aborts the command when the database file does not exist. +func requireExistingDB() { + if path, ok := existingDBFile(); !ok { log.Fatal("No existing database", "path", path) } } diff --git a/consts/consts.go b/consts/consts.go index 9bdac9125..42e9ec42f 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -156,8 +156,6 @@ const ( //DefaultAlbumPID = "album_legacy" DefaultAlbumPID = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" DefaultTrackPID = "musicbrainz_trackid|albumid,discnumber,tracknumber,title" - PIDAlbumKey = "PIDAlbum" - PIDTrackKey = "PIDTrack" ) const ( diff --git a/core/inspect.go b/core/inspect.go index 01ec33760..c60459b88 100644 --- a/core/inspect.go +++ b/core/inspect.go @@ -15,7 +15,7 @@ type InspectOutput struct { MappedTags *model.MediaFile `json:"mappedTags,omitempty"` } -func Inspect(filePath string, libraryId int, folderId string) (*InspectOutput, error) { +func Inspect(filePath string, lib model.Library, folderId string) (*InspectOutput, error) { path, file := filepath.Split(filePath) s, err := storage.For(path) @@ -39,12 +39,22 @@ func Inspect(filePath string, libraryId int, folderId string) (*InspectOutput, e return nil, model.ErrNotFound } - md := metadata.New(path, tag) + md := metadata.New(scannerPath(lib, filePath), tag) result := &InspectOutput{ File: filePath, RawTags: tags[file].Tags, - MappedTags: new(md.ToMediaFile(libraryId, folderId)), + MappedTags: new(md.ToMediaFile(lib, folderId)), } return result, nil } + +// scannerPath returns the path the scanner uses for the file (relative to its library), so +// folder-based PIDs match the DB. Files outside the library keep their absolute path. +func scannerPath(lib model.Library, filePath string) string { + absPath, err := filepath.Abs(filePath) + if err != nil || lib.Path == "" { + return filePath + } + return model.LibraryRelativePath(lib.Path, absPath) +} diff --git a/core/inspect_test.go b/core/inspect_test.go new file mode 100644 index 000000000..0ac90990c --- /dev/null +++ b/core/inspect_test.go @@ -0,0 +1,45 @@ +package core_test + +import ( + "path/filepath" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Inspect", func() { + var fixtures string + + BeforeEach(func() { + var err error + fixtures, err = filepath.Abs(filepath.Join("tests", "fixtures")) + Expect(err).ToNot(HaveOccurred()) + }) + + It("maps the file with the library-relative path the scanner uses", func() { + lib := model.Library{ID: 2, Path: filepath.Dir(fixtures), PIDAlbum: "folder"} + out, err := core.Inspect(filepath.Join(fixtures, "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + Expect(out.MappedTags.Path).To(Equal("fixtures/test.mp3")) + Expect(out.MappedTags.LibraryID).To(Equal(2)) + }) + + It("gives the same IDs for relative and absolute paths", func() { + lib := model.Library{ID: 2, Path: filepath.Dir(fixtures), PIDAlbum: "folder"} + abs, err := core.Inspect(filepath.Join(fixtures, "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + rel, err := core.Inspect(filepath.Join("tests", "fixtures", "test.mp3"), lib, "") + Expect(err).ToNot(HaveOccurred()) + Expect(rel.MappedTags.AlbumID).To(Equal(abs.MappedTags.AlbumID)) + Expect(rel.MappedTags.PID).To(Equal(abs.MappedTags.PID)) + }) + + It("keeps the given path for a file outside the library", func() { + filePath := filepath.Join(fixtures, "test.mp3") + out, err := core.Inspect(filePath, model.Library{ID: model.DefaultLibraryID}, "") + Expect(err).ToNot(HaveOccurred()) + Expect(out.MappedTags.Path).To(Equal(filePath)) + }) +}) diff --git a/core/library.go b/core/library.go index 628ee4b7b..f1153da26 100644 --- a/core/library.go +++ b/core/library.go @@ -2,10 +2,12 @@ package core import ( "context" + "errors" "fmt" "io/fs" "os" "path/filepath" + "slices" "strconv" "strings" "time" @@ -15,6 +17,7 @@ import ( "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/metadata" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/slice" @@ -200,23 +203,22 @@ func (r *libraryRepositoryWrapper) Update(ctx context.Context, id string, entity } pathChanged := originalLib.Path != lib.Path + pidChanged := (updatesColumn(cols, "pidAlbum") && originalLib.PIDAlbum != lib.PIDAlbum) || + (updatesColumn(cols, "pidTrack") && originalLib.PIDTrack != lib.PIDTrack) err = r.LibraryRepository.Put(ctx, lib, cols...) if err != nil { return r.mapError(err) } - // Restart watcher and trigger scan if path was updated - if pathChanged { - if r.watcher != nil { - if err := r.watcher.Watch(ctx, lib); err != nil { - log.Warn(ctx, "Failed to restart watcher for updated library", "libraryID", lib.ID, "name", lib.Name, "path", lib.Path, err) - } + if pathChanged && r.watcher != nil { + if err := r.watcher.Watch(ctx, lib); err != nil { + log.Warn(ctx, "Failed to restart watcher for updated library", "libraryID", lib.ID, "name", lib.Name, "path", lib.Path, err) } + } - if r.scanner != nil { - go r.triggerScan(ctx, lib, "updated") - } + if (pathChanged || pidChanged) && r.scanner != nil { + go r.triggerScan(ctx, lib, "updated") } // Send library refresh event to all clients @@ -325,6 +327,15 @@ func (r *libraryRepositoryWrapper) validateLibrary(ctx context.Context, library } } + library.PIDAlbum = strings.TrimSpace(library.PIDAlbum) + library.PIDTrack = strings.TrimSpace(library.PIDTrack) + if err := metadata.ValidatePIDSpec(library.PIDAlbum, true); err != nil { + validationErrors["pidAlbum"] = err.Error() + } + if err := metadata.ValidatePIDSpec(library.PIDTrack, false); err != nil { + validationErrors["pidTrack"] = err.Error() + } + if len(validationErrors) > 0 { return &rest.ValidationError{Errors: validationErrors} } @@ -332,6 +343,11 @@ func (r *libraryRepositoryWrapper) validateLibrary(ctx context.Context, library return nil } +// updatesColumn reports whether an update with these columns writes col. No columns means all of them. +func updatesColumn(cols []string, col string) bool { + return len(cols) == 0 || slices.Contains(cols, col) +} + func (r *libraryRepositoryWrapper) validateLibraryPath(ctx context.Context, library *model.Library) error { // Validate path format if !filepath.IsAbs(library.Path) { @@ -399,11 +415,27 @@ func (s *libraryService) validateLibraryIDs(ctx context.Context, libraryIDs []in return nil } +var scanWaitInterval = time.Second + func (r *libraryRepositoryWrapper) triggerScan(ctx context.Context, lib *model.Library, action string) { + // Runs in its own goroutine and outlives the HTTP request + ctx = context.WithoutCancel(ctx) + + // A running scan loaded the libraries before this change, and would reject a new request + for { + status, err := r.scanner.Status(ctx) + if err != nil || !status.Scanning { + break + } + time.Sleep(scanWaitInterval) + } + log.Info(ctx, fmt.Sprintf("Triggering scan for %s library", action), "libraryID", lib.ID, "name", lib.Name, "path", lib.Path) start := time.Now() - warnings, err := r.scanner.ScanAll(ctx, false) // Quick scan for new library - if err != nil { + warnings, err := r.scanner.ScanAll(ctx, false) // Quick scan: the scanner rescans libraries with a changed PID config in full + if errors.Is(err, model.ErrAlreadyScanning) { + log.Debug(ctx, "Scan already running, it covers this change", "libraryID", lib.ID, "name", lib.Name) + } else if err != nil { log.Error(ctx, fmt.Sprintf("Error scanning %s library", action), "libraryID", lib.ID, "name", lib.Name, err) } else { log.Info(ctx, fmt.Sprintf("Scan completed for %s library", action), "libraryID", lib.ID, "name", lib.Name, "warnings", len(warnings), "elapsed", time.Since(start)) diff --git a/core/library_test.go b/core/library_test.go index 5402eac22..e6ebb1974 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -322,6 +322,37 @@ var _ = Describe("Library Service", func() { }) }) + Describe("PID validation", func() { + pidError := func(err error, field string) string { + var validationErr *rest.ValidationError + Expect(errors.As(err, &validationErr)).To(BeTrue()) + return validationErr.Errors[field] + } + + It("rejects an unknown attribute in the album PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: "albmversion"}) + Expect(pidError(err, "pidAlbum")).To(ContainSubstring(`unknown attribute "albmversion"`)) + }) + + It("rejects albumid in the album PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: "albumid"}) + Expect(pidError(err, "pidAlbum")).To(ContainSubstring("albumid")) + }) + + It("rejects an unknown attribute in the track PID", func() { + _, err := repo.Save(ctx, &model.Library{Name: "Lib", Path: tempDir, PIDTrack: "nosuchtag"}) + Expect(pidError(err, "pidTrack")).To(ContainSubstring(`unknown attribute "nosuchtag"`)) + }) + + It("trims spaces", func() { + library := &model.Library{Name: "Lib", Path: tempDir, PIDAlbum: " folder ", PIDTrack: " "} + _, err := repo.Save(ctx, library) + Expect(err).ToNot(HaveOccurred()) + Expect(library.PIDAlbum).To(Equal("folder")) + Expect(library.PIDTrack).To(BeEmpty()) + }) + }) + Describe("Path Validation", func() { Context("Create operation", func() { It("fails when path is not absolute", func() { @@ -679,6 +710,48 @@ var _ = Describe("Library Service", func() { }, "100ms", "10ms").Should(Equal(0)) }) + It("triggers scan when updating the library PID config", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir}}) + + library := model.Library{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"} + Expect(repo.Update(ctx, "1", library)).To(Succeed()) + + Eventually(func() int { + return scanner.GetScanAllCallCount() + }, "1s", "10ms").Should(Equal(1)) + // A quick scan: the scanner itself rescans this library in full + Expect(scanner.GetScanAllCalls()[0].FullScan).To(BeFalse()) + }) + + It("does not trigger scan when the PID fields were not sent", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"}}) + + // The REST layer decodes a missing pidAlbum as "". Only the sent fields count. + library := model.Library{ID: 1, Name: "Renamed", Path: tempDir} + Expect(repo.Update(ctx, "1", library, "name", "path")).To(Succeed()) + + Consistently(func() int { + return scanner.GetScanAllCallCount() + }, "100ms", "10ms").Should(Equal(0)) + }) + + It("waits for a running scan before triggering a new one", func() { + libraryRepo.SetData(model.Libraries{{ID: 1, Name: "Library", Path: tempDir}}) + scanner.SetScanning(true) + + library := model.Library{ID: 1, Name: "Library", Path: tempDir, PIDAlbum: "folder"} + Expect(repo.Update(ctx, "1", library)).To(Succeed()) + + Consistently(func() int { + return scanner.GetScanAllCallCount() + }, "200ms", "20ms").Should(Equal(0)) + + scanner.SetScanning(false) + Eventually(func() int { + return scanner.GetScanAllCallCount() + }, "3s", "20ms").Should(Equal(1)) + }) + It("does not trigger scan when library creation fails", func() { // Try to create library with invalid data (empty name) library := &model.Library{Path: tempDir} diff --git a/core/metrics/insights.go b/core/metrics/insights.go index 4a78a7f3f..1d2ff5df4 100644 --- a/core/metrics/insights.go +++ b/core/metrics/insights.go @@ -10,6 +10,7 @@ import ( "path/filepath" "runtime" "runtime/debug" + "slices" "strings" "sync" "sync/atomic" @@ -269,9 +270,13 @@ func (c *insightsCollector) collect(ctx context.Context) []byte { if err != nil { log.Trace(ctx, "Error reading radios count", err) } - data.Library.Libraries, err = c.ds.Library().CountAll(ctx) + libs, err := c.ds.Library().GetAll(ctx) if err != nil { - log.Trace(ctx, "Error reading libraries count", err) + log.Trace(ctx, "Error reading libraries", err) + } + data.Library.Libraries = int64(len(libs)) + if slices.ContainsFunc(libs, func(lib model.Library) bool { return lib.PIDAlbum != "" || lib.PIDTrack != "" }) { + data.Config.HasCustomPID = true } data.Library.ActiveUsers, err = c.ds.User().CountAll(ctx, model.QueryOptions{ Filters: squirrel.Gt{"last_access_at": time.Now().Add(-7 * 24 * time.Hour)}, diff --git a/core/playlists/import.go b/core/playlists/import.go index 658bd92dc..b5991b095 100644 --- a/core/playlists/import.go +++ b/core/playlists/import.go @@ -78,8 +78,8 @@ func (s *playlists) resolveFolder(ctx context.Context, dir string) (*model.Folde if err != nil { return nil, err } - matcher := newLibraryMatcher(libs) - lib, ok := matcher.findLibrary(dir) + matcher := model.NewLibraryMatcher(libs) + lib, ok := matcher.FindLibrary(dir) if !ok { return nil, fmt.Errorf("%w: %s", errNotInLibrary, dir) } diff --git a/core/playlists/parse_m3u.go b/core/playlists/parse_m3u.go index 9610e9dbb..ab95b8850 100644 --- a/core/playlists/parse_m3u.go +++ b/core/playlists/parse_m3u.go @@ -1,13 +1,11 @@ package playlists import ( - "cmp" "context" "fmt" "io" "net/url" "path/filepath" - "slices" "strings" "time" @@ -156,61 +154,9 @@ func (r pathResolution) ToQualifiedString() (string, error) { return fmt.Sprintf("%d:%s", r.libraryID, filepath.ToSlash(relativePath)), nil } -// libraryMatcher holds sorted libraries with cleaned paths for efficient path matching. -type libraryMatcher struct { - libraries model.Libraries - cleanedPaths []string -} - -// findLibraryForPath finds which library contains the given absolute path. -// Returns library ID and path, or 0 and empty string if not found. -func (lm *libraryMatcher) findLibraryForPath(absolutePath string) (int, string) { - lib, ok := lm.findLibrary(absolutePath) - if !ok { - return 0, "" - } - return lib.ID, filepath.Clean(lib.Path) -} - -// findLibrary checks if the absolute path is under any of the library paths. -func (lm *libraryMatcher) findLibrary(absolutePath string) (model.Library, bool) { - // Check sorted libraries (longest path first) to find the best match - for i, cleanLibPath := range lm.cleanedPaths { - // Check if absolutePath is under this library path - if strings.HasPrefix(absolutePath, cleanLibPath) { - // Ensure it's a proper path boundary (not just a prefix) - if len(absolutePath) == len(cleanLibPath) || absolutePath[len(cleanLibPath)] == filepath.Separator { - return lm.libraries[i], true - } - } - } - return model.Library{}, false -} - -// newLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). -// This ensures correct matching when library paths are prefixes of each other. -// Example: /music-classical must be checked before /music -// Otherwise, /music-classical/track.mp3 would match /music instead of /music-classical -func newLibraryMatcher(libs model.Libraries) *libraryMatcher { - // Sort libraries by path length (descending) to ensure longest paths match first. - slices.SortFunc(libs, func(i, j model.Library) int { - return cmp.Compare(len(j.Path), len(i.Path)) // Reverse order for descending - }) - - // Pre-clean all library paths once for efficient matching - cleanedPaths := make([]string, len(libs)) - for i, lib := range libs { - cleanedPaths[i] = filepath.Clean(lib.Path) - } - return &libraryMatcher{ - libraries: libs, - cleanedPaths: cleanedPaths, - } -} - // pathResolver handles path resolution logic for playlist imports. type pathResolver struct { - matcher *libraryMatcher + matcher *model.LibraryMatcher } // newPathResolver creates a pathResolver with libraries loaded from the datastore. @@ -219,7 +165,7 @@ func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, er if err != nil { return nil, err } - matcher := newLibraryMatcher(libs) + matcher := model.NewLibraryMatcher(libs) return &pathResolver{matcher: matcher}, nil } @@ -246,14 +192,14 @@ func (r *pathResolver) resolvePath(line string, folder *model.Folder) pathResolu // a pathResolution with the library information. Returns an invalid resolution if // the path is not found in any library. func (r *pathResolver) findInLibraries(absolutePath string) pathResolution { - libID, libPath := r.matcher.findLibraryForPath(absolutePath) - if libID == 0 { + lib, ok := r.matcher.FindLibrary(absolutePath) + if !ok { return pathResolution{valid: false} } return pathResolution{ absolutePath: absolutePath, - libraryPath: libPath, - libraryID: libID, + libraryPath: filepath.Clean(lib.Path), + libraryID: lib.ID, valid: true, } } @@ -288,7 +234,7 @@ func (r *pathResolver) resolvePaths(ctx context.Context, folder *model.Folder, l // HTTP(S) URLs are stored as-is (gated by EnableM3UExternalAlbumArt). // Local paths (file://, absolute, or relative) are resolved to an absolute path // and validated against known library boundaries via matcher. -func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher, owner model.User) string { +func resolveImageURL(value string, folder *model.Folder, matcher *model.LibraryMatcher, owner model.User) string { value = strings.TrimSpace(value) if value == "" { return "" @@ -308,7 +254,7 @@ func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher return "" } - lib, ok := matcher.findLibrary(localPath) + lib, ok := matcher.FindLibrary(localPath) // A playlist without a folder (API upload, or CLI import from outside all libraries) may only use the owner's libraries. if !ok || (folder == nil && !owner.HasLibraryAccess(lib.ID)) { return "" diff --git a/core/playlists/parse_m3u_test.go b/core/playlists/parse_m3u_test.go index b6a3a96f9..ced6c2b16 100644 --- a/core/playlists/parse_m3u_test.go +++ b/core/playlists/parse_m3u_test.go @@ -9,187 +9,6 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("libraryMatcher", func() { - var ds *tests.MockDataStore - var mockLibRepo *tests.MockLibraryRepo - ctx := context.Background() - - BeforeEach(func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-playlists)") - mockLibRepo = &tests.MockLibraryRepo{} - ds = &tests.MockDataStore{ - MockedLibrary: mockLibRepo, - } - }) - - // Helper function to create a libraryMatcher from the mock datastore - createMatcher := func(ds model.DataStore) *libraryMatcher { - libs, err := ds.Library().GetAll(ctx) - Expect(err).ToNot(HaveOccurred()) - return newLibraryMatcher(libs) - } - - Describe("Longest library path matching", func() { - It("matches the longest library path when multiple libraries share a prefix", func() { - // Setup libraries with prefix conflicts - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - {ID: 2, Path: "/music-classical"}, - {ID: 3, Path: "/music-classical/opera"}, - }) - - matcher := createMatcher(ds) - - // Test that longest path matches first and returns correct library ID - testCases := []struct { - path string - expectedLibID int - expectedLibPath string - }{ - {"/music-classical/opera/track.mp3", 3, "/music-classical/opera"}, - {"/music-classical/track.mp3", 2, "/music-classical"}, - {"/music/track.mp3", 1, "/music"}, - {"/music-classical/opera/subdir/file.mp3", 3, "/music-classical/opera"}, - } - - for _, tc := range testCases { - libID, libPath := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d, but got %d", tc.path, tc.expectedLibID, libID) - Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s, but got %s", tc.path, tc.expectedLibPath, libPath) - } - }) - - It("handles libraries with similar prefixes but different structures", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/home/user/music"}, - {ID: 2, Path: "/home/user/music-backup"}, - }) - - matcher := createMatcher(ds) - - // Test that music-backup library is matched correctly - libID, libPath := matcher.findLibraryForPath("/home/user/music-backup/track.mp3") - Expect(libID).To(Equal(2)) - Expect(libPath).To(Equal("/home/user/music-backup")) - - // Test that music library is still matched correctly - libID, libPath = matcher.findLibraryForPath("/home/user/music/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/home/user/music")) - }) - - It("matches path that is exactly the library root", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - {ID: 2, Path: "/music-classical"}, - }) - - matcher := createMatcher(ds) - - // Exact library path should match - libID, libPath := matcher.findLibraryForPath("/music-classical") - Expect(libID).To(Equal(2)) - Expect(libPath).To(Equal("/music-classical")) - }) - - It("handles complex nested library structures", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/media"}, - {ID: 2, Path: "/media/audio"}, - {ID: 3, Path: "/media/audio/classical"}, - {ID: 4, Path: "/media/audio/classical/baroque"}, - }) - - matcher := createMatcher(ds) - - testCases := []struct { - path string - expectedLibID int - expectedLibPath string - }{ - {"/media/audio/classical/baroque/bach/track.mp3", 4, "/media/audio/classical/baroque"}, - {"/media/audio/classical/mozart/track.mp3", 3, "/media/audio/classical"}, - {"/media/audio/rock/track.mp3", 2, "/media/audio"}, - {"/media/video/movie.mp4", 1, "/media"}, - } - - for _, tc := range testCases { - libID, libPath := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) - Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s", tc.path, tc.expectedLibPath) - } - }) - }) - - Describe("Edge cases", func() { - It("handles empty library list", func() { - mockLibRepo.SetData([]model.Library{}) - - matcher := createMatcher(ds) - Expect(matcher).ToNot(BeNil()) - - // Should not match anything - libID, libPath := matcher.findLibraryForPath("/music/track.mp3") - Expect(libID).To(Equal(0)) - Expect(libPath).To(BeEmpty()) - }) - - It("handles single library", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music"}, - }) - - matcher := createMatcher(ds) - - libID, libPath := matcher.findLibraryForPath("/music/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/music")) - }) - - It("handles libraries with special characters in paths", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/music[test]"}, - {ID: 2, Path: "/music(backup)"}, - }) - - matcher := createMatcher(ds) - Expect(matcher).ToNot(BeNil()) - - // Special characters should match literally - libID, libPath := matcher.findLibraryForPath("/music[test]/track.mp3") - Expect(libID).To(Equal(1)) - Expect(libPath).To(Equal("/music[test]")) - }) - }) - - Describe("Path matching order", func() { - It("ensures longest paths match first", func() { - mockLibRepo.SetData([]model.Library{ - {ID: 1, Path: "/a"}, - {ID: 2, Path: "/ab"}, - {ID: 3, Path: "/abc"}, - }) - - matcher := createMatcher(ds) - - // Verify that longer paths match correctly (not cut off by shorter prefix) - testCases := []struct { - path string - expectedLibID int - }{ - {"/abc/file.mp3", 3}, - {"/ab/file.mp3", 2}, - {"/a/file.mp3", 1}, - } - - for _, tc := range testCases { - libID, _ := matcher.findLibraryForPath(tc.path) - Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) - } - }) - }) -}) - var _ = Describe("pathResolver", func() { var ds *tests.MockDataStore var mockLibRepo *tests.MockLibraryRepo diff --git a/db/migrations/20260929221042_add_library_pid_columns.sql b/db/migrations/20260929221042_add_library_pid_columns.sql new file mode 100644 index 000000000..487512287 --- /dev/null +++ b/db/migrations/20260929221042_add_library_pid_columns.sql @@ -0,0 +1,18 @@ +-- +goose Up +-- +goose StatementBegin +alter table library add column pid_album varchar default '' not null; +alter table library add column pid_track varchar default '' not null; +alter table library add column scanned_pid_album varchar default '' not null; +alter table library add column scanned_pid_track varchar default '' not null; + +-- Every library was scanned with the global PID config, so seed it as their scanned config. +-- This way the upgrade does not trigger a full rescan. +update library set + scanned_pid_album = coalesce((select value from property where id = 'PIDAlbum'), ''), + scanned_pid_track = coalesce((select value from property where id = 'PIDTrack'), ''); + +delete from property where id in ('PIDAlbum', 'PIDTrack'); +-- +goose StatementEnd + +-- +goose Down +SELECT 1; diff --git a/model/library.go b/model/library.go index 1e33222ac..e80d22c89 100644 --- a/model/library.go +++ b/model/library.go @@ -1,10 +1,13 @@ package model import ( + "cmp" "context" + "strings" "time" "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/utils/slice" ) @@ -27,6 +30,38 @@ type Library struct { TotalSize int64 `json:"totalSize" db:"total_size"` TotalDuration float64 `json:"totalDuration" db:"total_duration"` DefaultNewUsers bool `json:"defaultNewUsers" db:"default_new_users"` + PIDAlbum string `json:"pidAlbum" db:"pid_album"` + PIDTrack string `json:"pidTrack" db:"pid_track"` + ScannedPIDAlbum string `json:"-" db:"scanned_pid_album"` + ScannedPIDTrack string `json:"-" db:"scanned_pid_track"` +} + +// PIDConfig holds the persistent ID specs used to compute track and album IDs. +type PIDConfig struct { + Track string + Album string +} + +// EffectivePID returns the PID specs in effect for this library: its own overrides, falling back to +// the global config. +func (l Library) EffectivePID() PIDConfig { + return PIDConfig{ + Track: cmp.Or(l.PIDTrack, conf.Server.PID.Track), + Album: cmp.Or(l.PIDAlbum, conf.Server.PID.Album), + } +} + +// PIDChanged reports whether the effective PID specs differ from the ones used by the last finished +// scan of this library. A library that was never scanned counts as changed. +func (l Library) PIDChanged() bool { + pid := l.EffectivePID() + return !strings.EqualFold(l.ScannedPIDAlbum, pid.Album) || !strings.EqualFold(l.ScannedPIDTrack, pid.Track) +} + +// NeedsPIDRescan reports whether the library has content imported with an old PID config, so it must be +// rescanned in full. A library that never finished a scan has nothing to regroup. +func (l Library) NeedsPIDRescan() bool { + return !l.LastScanAt.IsZero() && l.PIDChanged() } const ( @@ -59,6 +94,8 @@ type LibraryRepository interface { // TODO These methods should be moved to a core service ScanBegin(ctx context.Context, id int, fullScan bool) error ScanEnd(ctx context.Context, id int) error + // SetScannedPID records the PID specs used by the last finished scan of the library + SetScannedPID(ctx context.Context, id int, pid PIDConfig) error ScanInProgress(ctx context.Context) (bool, error) RefreshStats(ctx context.Context, id int) error } diff --git a/model/library_matcher.go b/model/library_matcher.go new file mode 100644 index 000000000..83af96f9f --- /dev/null +++ b/model/library_matcher.go @@ -0,0 +1,57 @@ +package model + +import ( + "cmp" + "path/filepath" + "slices" + "strings" +) + +// LibraryMatcher finds the library that contains an absolute path. +type LibraryMatcher struct { + libraries Libraries + cleanedPaths []string +} + +// NewLibraryMatcher sorts the libraries longest path first, so /music-classical is checked before /music. +func NewLibraryMatcher(libs Libraries) *LibraryMatcher { + libs = slices.Clone(libs) + slices.SortFunc(libs, func(i, j Library) int { + return cmp.Compare(len(j.Path), len(i.Path)) + }) + cleanedPaths := make([]string, len(libs)) + for i, lib := range libs { + cleanedPaths[i] = filepath.Clean(lib.Path) + } + return &LibraryMatcher{libraries: libs, cleanedPaths: cleanedPaths} +} + +// FindLibrary returns the library whose path contains absolutePath. +func (lm *LibraryMatcher) FindLibrary(absolutePath string) (Library, bool) { + for i, libPath := range lm.cleanedPaths { + // A cleaned path only ends with a separator when it is a filesystem root + if strings.HasPrefix(absolutePath, libPath) && (len(absolutePath) == len(libPath) || + absolutePath[len(libPath)] == filepath.Separator || strings.HasSuffix(libPath, string(filepath.Separator))) { + return lm.libraries[i], true + } + } + return Library{}, false +} + +// LibraryRelativePath rebases an absolute path onto the library root, as the scanner's io/fs sees it +// (forward slashes). Relative paths, and absolute paths outside the library root, are returned unchanged. +func LibraryRelativePath(libPath, path string) string { + if !filepath.IsAbs(path) { + return path + } + // The library root may be relative (e.g. the default "./music"); it resolves against the same cwd + absLib, err := filepath.Abs(libPath) + if err != nil { + return path + } + rel, err := filepath.Rel(absLib, path) + if err != nil || !filepath.IsLocal(rel) { + return path + } + return filepath.ToSlash(rel) +} diff --git a/model/library_matcher_test.go b/model/library_matcher_test.go new file mode 100644 index 000000000..09e6f7e33 --- /dev/null +++ b/model/library_matcher_test.go @@ -0,0 +1,91 @@ +package model_test + +import ( + "os" + "path/filepath" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("LibraryMatcher", func() { + // Paths are written Unix-style and converted, so they use the OS separator, as filepath.Abs output does + find := func(libs model.Libraries, path string) int { + for i := range libs { + libs[i].Path = filepath.FromSlash(libs[i].Path) + } + lib, ok := model.NewLibraryMatcher(libs).FindLibrary(filepath.FromSlash(path)) + if !ok { + return 0 + } + return lib.ID + } + + DescribeTable("matches the longest library path", + func(libs model.Libraries, path string, expectedID int) { + Expect(find(libs, path)).To(Equal(expectedID)) + }, + Entry("nested library", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}, {ID: 3, Path: "/music-classical/opera"}}, "/music-classical/opera/subdir/track.mp3", 3), + Entry("sibling with a shared prefix", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music-classical/track.mp3", 2), + Entry("shorter library", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music/track.mp3", 1), + Entry("exact library root", model.Libraries{{ID: 1, Path: "/music"}, {ID: 2, Path: "/music-classical"}}, "/music-classical", 2), + Entry("deeply nested libraries", model.Libraries{{ID: 1, Path: "/media"}, {ID: 2, Path: "/media/audio"}, {ID: 3, Path: "/media/audio/classical"}, {ID: 4, Path: "/media/audio/classical/baroque"}}, "/media/audio/classical/mozart/track.mp3", 3), + Entry("prefix that is not a path boundary", model.Libraries{{ID: 1, Path: "/a"}, {ID: 2, Path: "/ab"}, {ID: 3, Path: "/abc"}}, "/ab/file.mp3", 2), + Entry("special characters match literally", model.Libraries{{ID: 1, Path: "/music[test]"}, {ID: 2, Path: "/music(backup)"}}, "/music[test]/track.mp3", 1), + Entry("library path with a trailing slash", model.Libraries{{ID: 1, Path: "/music/"}}, "/music/track.mp3", 1), + Entry("library at the filesystem root", model.Libraries{{ID: 1, Path: "/"}}, "/music/track.mp3", 1), + Entry("nested library under a root library", model.Libraries{{ID: 1, Path: "/"}, {ID: 2, Path: "/music"}}, "/music/track.mp3", 2), + ) + + It("does not match a path outside every library", func() { + Expect(find(model.Libraries{{ID: 1, Path: "/music"}}, "/music-backup/track.mp3")).To(BeZero()) + }) + + It("does not match anything without libraries", func() { + Expect(find(nil, "/music/track.mp3")).To(BeZero()) + }) + + It("does not reorder the caller's libraries", func() { + libs := model.Libraries{{ID: 1, Path: "/a"}, {ID: 2, Path: "/abc"}} + model.NewLibraryMatcher(libs) + Expect(libs.IDs()).To(Equal([]int{1, 2})) + }) +}) + +var _ = Describe("LibraryRelativePath", func() { + // Paths are built with filepath so the "absolute" cases stay absolute on every OS + // (a Unix-style "/foo" is not absolute on Windows). + libRoot, _ := filepath.Abs(filepath.Join("jukebox", "collection")) + outside, _ := filepath.Abs(filepath.Join("somewhere", "else")) + + It("returns a relative path unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, "_Collection")).To(Equal("_Collection")) + }) + + It("rebases an absolute target when the library root is relative", func() { + cwd, err := os.Getwd() + Expect(err).ToNot(HaveOccurred()) + Expect(model.LibraryRelativePath(filepath.Join("music", "library"), filepath.Join(cwd, "music", "library", "rock"))).To(Equal("rock")) + }) + + It("rebases an absolute path that equals the library root to '.'", func() { + Expect(model.LibraryRelativePath(libRoot, libRoot)).To(Equal(".")) + }) + + It("rebases an absolute path under the library root", func() { + Expect(model.LibraryRelativePath(libRoot, filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) + }) + + It("handles a trailing slash on the library path", func() { + Expect(model.LibraryRelativePath(libRoot+string(filepath.Separator), filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) + }) + + It("leaves an absolute path outside the library root unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, outside)).To(Equal(outside)) + }) + + It("returns an empty path unchanged", func() { + Expect(model.LibraryRelativePath(libRoot, "")).To(Equal("")) + }) +}) diff --git a/model/library_test.go b/model/library_test.go new file mode 100644 index 000000000..4e799e13f --- /dev/null +++ b/model/library_test.go @@ -0,0 +1,73 @@ +package model_test + +import ( + "encoding/json" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Library PID config", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.PID.Album = "global_album" + conf.Server.PID.Track = "global_track" + }) + + Describe("EffectivePID", func() { + It("falls back to the global config", func() { + Expect(model.Library{}.EffectivePID()).To(Equal(model.PIDConfig{Track: "global_track", Album: "global_album"})) + }) + It("uses the library overrides", func() { + lib := model.Library{PIDAlbum: "folder", PIDTrack: "title"} + Expect(lib.EffectivePID()).To(Equal(model.PIDConfig{Track: "title", Album: "folder"})) + }) + }) + + Describe("PIDChanged", func() { + It("is false when the scanned specs match, ignoring case", func() { + lib := model.Library{ScannedPIDAlbum: "GLOBAL_ALBUM", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeFalse()) + }) + It("is true when the album override differs from the scanned spec", func() { + lib := model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true when only the track spec changed", func() { + lib := model.Library{PIDTrack: "title", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true when the global config changed for a library without overrides", func() { + lib := model.Library{ScannedPIDAlbum: "old_album", ScannedPIDTrack: "global_track"} + Expect(lib.PIDChanged()).To(BeTrue()) + }) + It("is true for a library that was never scanned", func() { + Expect(model.Library{}.PIDChanged()).To(BeTrue()) + }) + }) + + Describe("NeedsPIDRescan", func() { + It("is false for a library that never finished a scan", func() { + Expect(model.Library{PIDAlbum: "folder"}.NeedsPIDRescan()).To(BeFalse()) + }) + It("is true for a scanned library whose PID config changed", func() { + lib := model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track", LastScanAt: time.Now()} + Expect(lib.NeedsPIDRescan()).To(BeTrue()) + }) + It("is false for a scanned library whose PID config did not change", func() { + lib := model.Library{ScannedPIDAlbum: "global_album", ScannedPIDTrack: "global_track", LastScanAt: time.Now()} + Expect(lib.NeedsPIDRescan()).To(BeFalse()) + }) + }) + + It("does not expose the scanned specs in JSON", func() { + data, err := json.Marshal(model.Library{PIDAlbum: "folder", ScannedPIDAlbum: "secret_album", ScannedPIDTrack: "secret_track"}) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(ContainSubstring(`"pidAlbum":"folder"`)) + Expect(string(data)).ToNot(ContainSubstring("secret_")) + }) +}) diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go index 6d12feba9..2135824d2 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -8,15 +8,14 @@ import ( "math" "strconv" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/str" ) -func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { +func (md Metadata) ToMediaFile(lib model.Library, folderID string) model.MediaFile { mf := model.MediaFile{ - LibraryID: libID, + LibraryID: lib.ID, FolderID: folderID, Tags: maps.Clone(md.tags), } @@ -84,8 +83,9 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf.AlbumArtist = md.mapDisplayAlbumArtist(mf) // Persistent IDs - mf.PID = md.trackPID(mf) - mf.AlbumID = md.albumID(mf, conf.Server.PID.Album) + pid := lib.EffectivePID() + mf.PID = md.trackPID(mf, pid) + mf.AlbumID = md.albumID(mf, pid.Album) // BFR These IDs will go away once the UI handle multiple participants. // BFR For Legacy Subsonic compatibility, we will set them in the API handlers diff --git a/model/metadata/map_mediafile_test.go b/model/metadata/map_mediafile_test.go index baaf8fab5..c19398841 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -30,9 +30,23 @@ var _ = Describe("ToMediaFile", func() { var toMediaFile = func(tags model.RawTags) model.MediaFile { props.Tags = tags md = metadata.New("filepath", props) - return md.ToMediaFile(1, "folderID") + return md.ToMediaFile(model.Library{ID: 1}, "folderID") } + Describe("Persistent IDs", func() { + It("uses the library PID config for the album ID and for albumid in the track spec", func() { + props.Tags = model.RawTags{"ALBUM": {"Kind of Blue"}, "TITLE": {"So What"}} + md = metadata.New("Jazz/Loose/01.mp3", props) + + byTags := md.ToMediaFile(model.Library{ID: 1, PIDAlbum: "album", PIDTrack: "albumid,title"}, "folderID") + byFolder := md.ToMediaFile(model.Library{ID: 1, PIDAlbum: "folder", PIDTrack: "albumid,title"}, "folderID") + + Expect(byFolder.AlbumID).ToNot(Equal(byTags.AlbumID)) + Expect(byFolder.AlbumID).To(Equal(md.AlbumID(byFolder, "folder"))) + Expect(byFolder.PID).ToNot(Equal(byTags.PID)) + }) + }) + Describe("Dates", func() { It("should parse properly tagged dates ", func() { mf = toMediaFile(model.RawTags{ diff --git a/model/metadata/map_participants_test.go b/model/metadata/map_participants_test.go index ec66e12b9..db652fb8b 100644 --- a/model/metadata/map_participants_test.go +++ b/model/metadata/map_participants_test.go @@ -38,7 +38,7 @@ var _ = Describe("Participants", func() { var toMediaFile = func(tags model.RawTags) model.MediaFile { props.Tags = tags md = metadata.New("filepath", props) - return md.ToMediaFile(1, "folderID") + return md.ToMediaFile(model.Library{ID: 1}, "folderID") } Describe("ARTIST(S) tags", func() { diff --git a/model/metadata/metadata_test.go b/model/metadata/metadata_test.go index c84d93981..a1a675006 100644 --- a/model/metadata/metadata_test.go +++ b/model/metadata/metadata_test.go @@ -323,7 +323,7 @@ var _ = Describe("Metadata", func() { tag: {tagValue}, } md = metadata.New(filePath, props) - return md.ToMediaFile(0, "0") + return md.ToMediaFile(model.Library{}, "0") } DescribeTable("Gain", diff --git a/model/metadata/persistent_ids.go b/model/metadata/persistent_ids.go index db315dc6b..b66ce824a 100644 --- a/model/metadata/persistent_ids.go +++ b/model/metadata/persistent_ids.go @@ -2,11 +2,11 @@ package metadata import ( "cmp" + "errors" "fmt" "path/filepath" "strings" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -22,12 +22,13 @@ type hashFunc = func(...string) string // attributes. Attributes can be either tags or processed values like folder, // albumid, albumartistid, etc. For each field, it gets all its attribute values // and concatenates them, then hashes the result. If a field is empty, it is -// skipped and the function looks for the next field. +// skipped and the function looks for the next field. albumSpec is the album PID +// spec used to resolve the `albumid` attribute. // // Taking hash as a parameter (instead of closing over it in a factory) keeps // mf on the stack: closing over mf would force the whole ~1KB MediaFile to the // heap on every call. -func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, hash hashFunc) string { +func computePID(mf model.MediaFile, md Metadata, spec, albumSpec string, prependLibId bool, hash hashFunc) string { switch spec { case "track_legacy": return legacyTrackID(mf, prependLibId) @@ -41,7 +42,7 @@ func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, values := make([]string, len(attributes)) hasValue := false for i, attr := range attributes { - v := getPIDAttr(mf, md, attr, prependLibId, spec, hash) + v := getPIDAttr(mf, md, attr, prependLibId, spec, albumSpec, hash) if v != "" { hasValue = true } @@ -58,15 +59,15 @@ func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, return hash(pid) } -func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec string, hash hashFunc) string { +func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec, albumSpec string, hash hashFunc) string { attr = strings.TrimSpace(strings.ToLower(attr)) switch attr { case "albumid": - if spec == conf.Server.PID.Album { + if spec == albumSpec { log.Error("Recursive PID definition detected, ignoring `albumid`", "spec", spec) return "" } - return computePID(mf, md, conf.Server.PID.Album, prependLibId, hash) + return computePID(mf, md, albumSpec, albumSpec, prependLibId, hash) case "folder": return filepath.Dir(mf.Path) case "albumartistid": @@ -79,18 +80,50 @@ func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, return md.String(model.TagName(attr)) } -func (md Metadata) trackPID(mf model.MediaFile) string { - return computePID(mf, md, conf.Server.PID.Track, true, id.NewHash) +// ValidatePIDSpec checks a PID override before it is stored; empty means "use the global config". +// Aliases resolve to empty at scan time: accepted only in track specs, because the default one uses them. +func ValidatePIDSpec(spec string, isAlbum bool) error { + switch { + case spec == "", isAlbum && spec == "album_legacy", !isAlbum && spec == "track_legacy": + return nil + } + for field := range strings.SplitSeq(spec, "|") { + for attr := range strings.SplitSeq(field, ",") { + attr = strings.TrimSpace(strings.ToLower(attr)) + switch attr { + case "": + return fmt.Errorf("empty attribute in %q", spec) + case "albumid": + if isAlbum { + return errors.New("albumid cannot be used in an album PID") + } + case "folder", "albumartistid": + default: + name, ok := model.CanonicalTagName(attr) + if !ok { + return fmt.Errorf("unknown attribute %q", attr) + } + if isAlbum && string(name) != attr { + return fmt.Errorf("use the tag name %q instead of its alias %q", name, attr) + } + } + } + } + return nil +} + +func (md Metadata) trackPID(mf model.MediaFile, pid model.PIDConfig) string { + return computePID(mf, md, pid.Track, pid.Album, true, id.NewHash) } func (md Metadata) albumID(mf model.MediaFile, pidConf string) string { - return computePID(mf, md, pidConf, true, id.NewHash) + return computePID(mf, md, pidConf, pidConf, true, id.NewHash) } // BFR Must be configurable? func (md Metadata) artistID(name string) string { mf := model.MediaFile{AlbumArtist: name} - return computePID(mf, md, "albumartistid", false, id.NewHash) + return computePID(mf, md, "albumartistid", "", false, id.NewHash) } func (md Metadata) mapTrackTitle() string { diff --git a/model/metadata/persistent_ids_test.go b/model/metadata/persistent_ids_test.go index 8e38bbd42..9f6eaf1f4 100644 --- a/model/metadata/persistent_ids_test.go +++ b/model/metadata/persistent_ids_test.go @@ -3,8 +3,7 @@ package metadata import ( "strings" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -13,16 +12,18 @@ import ( var _ = Describe("getPID", func() { var ( - md Metadata - mf model.MediaFile - sum hashFunc + md Metadata + mf model.MediaFile + sum hashFunc + albumSpec string ) getPID := func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string { - return computePID(mf, md, spec, prependLibId, sum) + return computePID(mf, md, spec, albumSpec, prependLibId, sum) } BeforeEach(func() { sum = func(s ...string) string { return "(" + strings.Join(s, ",") + ")" } + albumSpec = consts.DefaultAlbumPID }) Context("attributes are tags", func() { @@ -66,8 +67,7 @@ var _ = Describe("getPID", func() { Context("calculated attributes", func() { BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.PID.Album = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" + albumSpec = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" }) When("field is title", func() { It("should return the pid", func() { @@ -121,8 +121,8 @@ var _ = Describe("getPID", func() { When("albumid configuration refers to albumid recursively", func() { It("should avoid infinite recursion", func() { // Reproduce the issue from #4920 - conf.Server.PID.Album = "albumid,album,albumversion,releasedate" - spec := conf.Server.PID.Album + albumSpec = "albumid,album,albumversion,releasedate" + spec := albumSpec md.tags = map[model.TagName][]string{ "album": {"Album Name"}, "albumversion": {"Version"}, @@ -205,8 +205,7 @@ var _ = Describe("getPID", func() { }) When("prependLibId is true with nested albumid", func() { It("should handle nested albumid calls correctly", func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.PID.Album = "album" + albumSpec = "album" spec := "albumid" md.tags = map[model.TagName][]string{"album": {"Test Album"}} mf.AlbumArtist = "Test Artist" @@ -306,3 +305,34 @@ var _ = Describe("getPID", func() { }) }) }) + +var _ = Describe("ValidatePIDSpec", func() { + DescribeTable("accepts valid specs", + func(spec string, isAlbum bool) { + Expect(ValidatePIDSpec(spec, isAlbum)).To(Succeed()) + }, + Entry("empty, meaning the global config", "", true), + Entry("default album spec", consts.DefaultAlbumPID, true), + Entry("default track spec, which uses tag aliases", consts.DefaultTrackPID, false), + Entry("folder", "folder", true), + Entry("album legacy", "album_legacy", true), + Entry("track legacy", "track_legacy", false), + Entry("computed attributes", "albumartistid,album|title", true), + Entry("albumid in a track spec", "albumid,title", false), + Entry("spaces and mixed case", "MusicBrainz_AlbumID | Folder", true), + ) + + DescribeTable("rejects invalid specs", + func(spec string, isAlbum bool, msg string) { + Expect(ValidatePIDSpec(spec, isAlbum)).To(MatchError(ContainSubstring(msg))) + }, + Entry("unknown tag", "albmversion", true, `unknown attribute "albmversion"`), + Entry("empty field", "album||title", true, "empty attribute"), + Entry("empty attribute", "album,,title", true, "empty attribute"), + Entry("trailing separator", "album|", true, "empty attribute"), + Entry("albumid in an album spec", "albumid,album", true, "albumid"), + Entry("tag alias in an album spec", "talb", true, `use the tag name "album" instead of its alias "talb"`), + Entry("track legacy in an album spec", "track_legacy", true, `unknown attribute "track_legacy"`), + Entry("album legacy in a track spec", "album_legacy", false, `unknown attribute "album_legacy"`), + ) +}) diff --git a/model/scanner.go b/model/scanner.go index 36c9007fb..d22c3d0d6 100644 --- a/model/scanner.go +++ b/model/scanner.go @@ -2,12 +2,15 @@ package model import ( "context" + "errors" "fmt" "strconv" "strings" "time" ) +var ErrAlreadyScanning = errors.New("already scanning") + // ScanTarget represents a specific folder within a library to be scanned. // NOTE: This struct is used as a map key, so it should only contain comparable types. type ScanTarget struct { diff --git a/model/tag_mappings.go b/model/tag_mappings.go index ce7d2f37b..5a8168754 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -195,6 +195,28 @@ func TagMappings() map[TagName]TagConf { return mappings } +// CanonicalTagName returns the mapped tag that name is, or is an alias of. Tags are stored under this name. +func CanonicalTagName(name string) (TagName, bool) { + tagName, ok := tagNameIndex()[TagName(name).ToLower()] + return tagName, ok +} + +// tagNameIndex maps every tag name and alias to its tag name. Names are added last, so they win over aliases +// (musicbrainz_trackid is a tag and also an alias of musicbrainz_recordingid). +var tagNameIndex = sync.OnceValue(func() map[TagName]TagName { + mappings := TagMappings() + index := make(map[TagName]TagName, len(mappings)) + for name, tag := range mappings { + for _, alias := range tag.Aliases { + index[TagName(alias)] = name + } + } + for name := range mappings { + index[name] = name + } + return index +}) + func TagRolesConf() TagConf { _, cfg := parseMappings() return cfg.Roles diff --git a/model/tag_mappings_test.go b/model/tag_mappings_test.go index e582c3f2f..91e54e5d4 100644 --- a/model/tag_mappings_test.go +++ b/model/tag_mappings_test.go @@ -192,3 +192,22 @@ var _ = Describe("TagConf", func() { }) }) }) + +var _ = Describe("CanonicalTagName", func() { + DescribeTable("resolves tag names and aliases", + func(name string, expected TagName) { + tagName, ok := CanonicalTagName(name) + Expect(ok).To(BeTrue()) + Expect(tagName).To(Equal(expected)) + }, + Entry("tag name", "album", TagAlbum), + Entry("alias", "talb", TagAlbum), + Entry("mixed case alias", "TALB", TagAlbum), + Entry("tag name that is also an alias of another tag", "musicbrainz_trackid", TagMusicBrainzTrackID), + ) + + It("does not resolve an unknown name", func() { + _, ok := CanonicalTagName("nosuchtag") + Expect(ok).To(BeFalse()) + }) +}) diff --git a/persistence/library_repository.go b/persistence/library_repository.go index bf6b8995e..85da65cbc 100644 --- a/persistence/library_repository.go +++ b/persistence/library_repository.go @@ -93,6 +93,8 @@ func (r *libraryRepository) Put(ctx context.Context, l *model.Library, colsToUpd "path": l.Path, "remote_path": l.RemotePath, "default_new_users": l.DefaultNewUsers, + "pid_album": l.PIDAlbum, + "pid_track": l.PIDTrack, }, colsToUpdate...) cols["updated_at"] = l.UpdatedAt sq := Update(r.tableName).SetMap(cols).Where(Eq{"id": l.ID}) @@ -176,6 +178,15 @@ func (r *libraryRepository) ScanEnd(ctx context.Context, id int) error { return err } +func (r *libraryRepository) SetScannedPID(ctx context.Context, id int, pid model.PIDConfig) error { + sq := Update(r.tableName). + Set("scanned_pid_album", pid.Album). + Set("scanned_pid_track", pid.Track). + Where(Eq{"id": id}) + _, err := r.executeSQL(ctx, sq) + return err +} + func (r *libraryRepository) ScanInProgress(ctx context.Context) (bool, error) { query := r.newSelect(ctx).Where(NotEq{"last_scan_started_at": time.Time{}}) count, err := r.count(ctx, query) diff --git a/persistence/library_repository_test.go b/persistence/library_repository_test.go index 0ff470861..bf485a06f 100644 --- a/persistence/library_repository_test.go +++ b/persistence/library_repository_test.go @@ -270,6 +270,38 @@ var _ = Describe("LibraryRepository", func() { }) }) + Describe("PID config", func() { + It("stores the overrides, and Put never touches the scanned specs", func() { + lib := &model.Library{Name: "PID Library", Path: "/music/pid", PIDAlbum: "folder", PIDTrack: "title"} + Expect(repo.Put(ctx, lib)).To(Succeed()) + Expect(repo.SetScannedPID(ctx, lib.ID, model.PIDConfig{Album: "folder", Track: "title"})).To(Succeed()) + + // An update coming from the REST API has no scanned specs. It must not clear them + update := &model.Library{ID: lib.ID, Name: "PID Library", Path: "/music/pid", PIDTrack: "title"} + Expect(repo.Put(ctx, update)).To(Succeed()) + + saved, err := repo.Get(ctx, lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.PIDAlbum).To(BeEmpty()) + Expect(saved.PIDTrack).To(Equal("title")) + Expect(saved.ScannedPIDAlbum).To(Equal("folder")) + Expect(saved.ScannedPIDTrack).To(Equal("title")) + }) + + It("keeps the overrides when a partial update does not send them", func() { + lib := &model.Library{Name: "Partial", Path: "/music/partial", PIDAlbum: "folder", PIDTrack: "title"} + Expect(repo.Put(ctx, lib)).To(Succeed()) + + Expect(repo.Put(ctx, &model.Library{ID: lib.ID, Name: "Renamed"}, "name")).To(Succeed()) + + saved, err := repo.Get(ctx, lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(saved.Name).To(Equal("Renamed")) + Expect(saved.PIDAlbum).To(Equal("folder")) + Expect(saved.PIDTrack).To(Equal("title")) + }) + }) + Describe("Delete", func() { var adminRepo model.LibraryRepository var artistRepo model.ArtistRepository diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index f0d8a0e06..238919259 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -328,11 +328,22 @@ "totalDuration": "Duração", "defaultNewUsers": "Padrão para Novos Usuários", "createdAt": "Data de Criação", - "updatedAt": "Últ. Atualização" + "updatedAt": "Últ. Atualização", + "pidAlbum": "Agrupamento de álbuns", + "pidTrack": "Identificação das faixas" }, "sections": { "basic": "Informações Básicas", - "statistics": "Estatísticas" + "statistics": "Estatísticas", + "pid": "IDs Persistentes" + }, + "pid": { + "global": "Usar configuração global (%{value})", + "folder": "Pasta (um álbum por pasta)", + "custom": "Personalizado", + "spec": "Especificação do PID", + "help": "Tags e atributos que identificam um item. Consulte a sintaxe na documentação:", + "docs": "IDs Persistentes" }, "actions": { "scan": "Scanear Biblioteca", @@ -362,7 +373,9 @@ "messages": { "deleteConfirm": "Tem certeza que deseja excluir esta biblioteca? Isso removerá todos os dados associados.", "scanInProgress": "Scan em progresso...", - "noLibrariesAssigned": "Nenhuma biblioteca atribuída a este usuário" + "noLibrariesAssigned": "Nenhuma biblioteca atribuída a este usuário", + "pidChangeTitle": "Alterar os IDs persistentes?", + "pidChangeConfirm": "Ao salvar, os álbuns desta biblioteca serão reagrupados e as faixas serão identificadas novamente. Um scan completo da biblioteca começará imediatamente. As marcações como favoritas, as classificações e as contagens de reprodução das faixas serão mantidas. Os favoritos e as classificações dos álbuns serão transferidos para os novos álbuns quando um álbum antigo corresponder a um novo." } }, "plugin": { diff --git a/scanner/controller.go b/scanner/controller.go index 5eed6c58d..1b13c1846 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -25,7 +25,7 @@ import ( ) var ( - ErrAlreadyScanning = errors.New("already scanning") + ErrAlreadyScanning = model.ErrAlreadyScanning ) func New(rootCtx context.Context, ds model.DataStore, broker events.Broker, @@ -304,14 +304,14 @@ func LockForMaintenance() (func(), bool) { return scanMaintenanceMux.Unlock, true } -// EffectiveFullScan reports whether a scan was requested as full or will resume an interrupted -// full scan in one of the included libraries. +// EffectiveFullScan reports whether a scan was requested as full, will resume an interrupted full scan, +// or will rescan a library in full because its PID config changed, in one of the included libraries. func EffectiveFullScan(ctx context.Context, ds model.DataStore, fullScan bool, targets []model.ScanTarget) bool { if fullScan { return true } return anyIncludedLibrary(ctx, ds, targets, func(library model.Library) bool { - return library.FullScanInProgress + return library.FullScanInProgress || library.NeedsPIDRescan() }) } diff --git a/scanner/controller_test.go b/scanner/controller_test.go index bdcb99eda..974540e32 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -2,6 +2,7 @@ package scanner_test import ( "context" + "time" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -70,14 +71,21 @@ var _ = Describe("EffectiveFullScan", func() { var ds *tests.MockDataStore BeforeEach(func() { + pid := model.Library{}.EffectivePID() libraries := &tests.MockLibraryRepo{} libraries.SetData(model.Libraries{ - {ID: 1, FullScanInProgress: true}, - {ID: 2}, + {ID: 1, FullScanInProgress: true, ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 2, ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, + {ID: 3, LastScanAt: time.Now(), PIDAlbum: "folder", ScannedPIDAlbum: pid.Album, ScannedPIDTrack: pid.Track}, }) ds = &tests.MockDataStore{MockedLibrary: libraries} }) + It("detects a library that needs a full rescan for a PID change", func() { + targets := []model.ScanTarget{{LibraryID: 3, FolderPath: "."}} + Expect(scanner.EffectiveFullScan(GinkgoT().Context(), ds, false, targets)).To(BeTrue()) + }) + It("detects an interrupted full scan in a targeted library", func() { targets := []model.ScanTarget{{LibraryID: 1, FolderPath: "."}} Expect(scanner.EffectiveFullScan(context.Background(), ds, false, targets)).To(BeTrue()) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 7b6a6b097..4edaecacd 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -40,6 +40,7 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor if err != nil { log.Error(ctx, "Scanner: Error creating scan context", "lib", lib.Name, err) state.sendError(err) + state.markFailed(lib.ID) continue } jobs = append(jobs, job) @@ -51,12 +52,13 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor } type scanJob struct { - lib model.Library - fs storage.MusicFS - lastUpdates map[string]model.FolderUpdateInfo // Holds last update info for all (DB) folders in this library - targetFolders []string // Specific folders to scan (including all descendants) - lock sync.Mutex - numFolders atomic.Int64 + lib model.Library + fs storage.MusicFS + lastUpdates map[string]model.FolderUpdateInfo // Holds last update info for all (DB) folders in this library + targetFolders []string // Specific folders to scan (including all descendants) + prevAlbumPIDConf string // Album PID spec of the last finished scan, only when it differs from the current one + lock sync.Mutex + numFolders atomic.Int64 } func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, fullScan bool, targetFolders []string) (*scanJob, error) { @@ -77,16 +79,32 @@ func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, full return nil, fmt.Errorf("getting fs for library: %w", err) } + pid := lib.EffectivePID() + if lib.NeedsPIDRescan() { + msg := "Scanner: PID config changed, rescanning library in full" + if len(targetFolders) > 0 { + msg = "Scanner: PID config changed, rescanning target folders in full" + } + log.Info(ctx, msg, "lib", lib.Name, "targetFolders", targetFolders, + "album", pid.Album, "track", pid.Track, "scannedAlbum", lib.ScannedPIDAlbum, "scannedTrack", lib.ScannedPIDTrack) + fullScan = true + } + var prevAlbumPIDConf string + if lib.ScannedPIDAlbum != pid.Album { + prevAlbumPIDConf = lib.ScannedPIDAlbum + } + // Ensure FullScanInProgress reflects the current scan request. // This is important when resuming an interrupted quick scan as a full scan: // the DB may have FullScanInProgress=false, but we need it true for isOutdated() to work correctly. lib.FullScanInProgress = lib.FullScanInProgress || fullScan return &scanJob{ - lib: lib, - fs: fsys, - lastUpdates: lastUpdates, - targetFolders: targetFolders, + lib: lib, + fs: fsys, + lastUpdates: lastUpdates, + targetFolders: targetFolders, + prevAlbumPIDConf: prevAlbumPIDConf, }, nil } @@ -122,14 +140,13 @@ func (j *scanJob) createFolderEntry(path string) *folderEntry { // The phaseFolders struct implements the phase interface, providing methods to produce // folder entries, process folders, persist changes to the database, and log the results. type phaseFolders struct { - jobs []*scanJob - ds model.DataStore - ctx context.Context //nolint:containedctx // phase runs under a single scan ctx - walkCtx context.Context //nolint:containedctx // cancelled when a folder fails to persist, so the walk stops early - stopWalk context.CancelCauseFunc - state *scanState - prevAlbumPIDConf string - imageChanges *imageChangeCollector + jobs []*scanJob + ds model.DataStore + ctx context.Context //nolint:containedctx // phase runs under a single scan ctx + walkCtx context.Context //nolint:containedctx // cancelled when a folder fails to persist, so the walk stops early + stopWalk context.CancelCauseFunc + state *scanState + imageChanges *imageChangeCollector } func (p *phaseFolders) description() string { @@ -138,12 +155,6 @@ func (p *phaseFolders) description() string { func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { return ppl.NewProducer(func(put func(entry *folderEntry)) error { - var err error - p.prevAlbumPIDConf, err = p.ds.Property().DefaultGet(p.ctx, consts.PIDAlbumKey, "") - if err != nil { - return fmt.Errorf("getting album PID conf: %w", err) - } - // TODO Parallelize multiple job when we have multiple libraries var total int64 var totalChanged int64 @@ -173,7 +184,7 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { // Check if folder is outdated if folder.isOutdated() { - if !p.state.fullScan { + if !folder.job.lib.FullScanInProgress { // Ancestor folders need a row even with no files of their own: artwork // resolution climbs them, and an image added later needs a state to diff. if folder.isEmpty() && folder.isNew() { @@ -239,7 +250,7 @@ func (p *phaseFolders) processFolder(entry *folderEntry) (*folderEntry, error) { for afPath, af := range entry.audioFiles { fullPath := path.Join(entry.path, afPath) dbTrack, foundInDB := dbTracks[fullPath] - if !foundInDB || p.state.fullScan { + if !foundInDB || entry.job.lib.FullScanInProgress { filesToImport[fullPath] = dbTrack } else { info, err := af.Info() @@ -289,18 +300,18 @@ func (p *phaseFolders) loadTagsFromFiles(entry *folderEntry, toImport map[string } for filePath, info := range allInfo { md := metadata.New(filePath, info) - track := md.ToMediaFile(entry.job.lib.ID, entry.id) + track := md.ToMediaFile(entry.job.lib, entry.id) tracks = append(tracks, track) for _, t := range track.Tags.FlattenAll() { uniqueTags[t.ID] = t } // Keep track of any album ID changes, to reassign annotations later - prevAlbumID := "" + prevAlbumID := track.AlbumID if prev := toImport[filePath]; prev != nil { prevAlbumID = prev.AlbumID - } else { - prevAlbumID = md.AlbumID(track, p.prevAlbumPIDConf) + } else if entry.job.prevAlbumPIDConf != "" { + prevAlbumID = md.AlbumID(track, entry.job.prevAlbumPIDConf) } _, ok := entry.albumIDMap[track.AlbumID] if prevAlbumID != track.AlbumID && !ok { @@ -453,7 +464,7 @@ func (p *phaseFolders) persistFolder(ctx context.Context, tx model.DataStore, en if len(queueItems) > 0 { queue := tx.ArtworkQueue() enqueue := queue.Enqueue - if p.state.fullScan { + if entry.job.lib.FullScanInProgress { enqueue = queue.EnqueueIfMissing } if err := enqueue(ctx, queueItems...); err != nil { diff --git a/scanner/scanner.go b/scanner/scanner.go index cd2fe3c8d..305c443f4 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -4,13 +4,11 @@ import ( "context" "fmt" "maps" - "path/filepath" "slices" "sync/atomic" "time" ppl "github.com/google/go-pipeline/pkg/pipeline" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" @@ -32,6 +30,7 @@ type scanState struct { libraries model.Libraries // Store libraries list for consistency across phases targets map[int][]string // Optional: map[libraryID][]folderPaths for selective scans totalLibraryCount int // Total number of libraries (unfiltered), for cross-library move detection + failedLibs map[int]bool // Libraries that could not be scanned in this run } func (s *scanState) sendProgress(info *ProgressInfo) { @@ -48,29 +47,15 @@ func (s *scanState) sendWarning(msg string) { s.sendProgress(&ProgressInfo{Warning: msg}) } -func (s *scanState) sendError(err error) { - s.sendProgress(&ProgressInfo{Error: err.Error()}) +func (s *scanState) markFailed(libID int) { + if s.failedLibs == nil { + s.failedLibs = map[int]bool{} + } + s.failedLibs[libID] = true } -// libraryRelativePath rebases an absolute scan target path onto the library root, since the -// scanner's fs.FS only accepts paths relative to it. Relative paths, and absolute paths outside -// the library root, are returned unchanged. -func libraryRelativePath(libPath, folderPath string) string { - if !filepath.IsAbs(folderPath) { - return folderPath - } - // The library root may be relative (e.g. the default "./music"); it must be made absolute - // to match against an absolute target, and it resolves against the same cwd as the scanner's fs. - absLib, err := filepath.Abs(libPath) - if err != nil { - return folderPath - } - rel, err := filepath.Rel(absLib, folderPath) - if err != nil || !filepath.IsLocal(rel) { - return folderPath - } - // The scanner's fs.FS is an io/fs, which always uses forward slashes. - return filepath.ToSlash(rel) +func (s *scanState) sendError(err error) { + s.sendProgress(&ProgressInfo{Error: err.Error()}) } func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets []model.ScanTarget, progress chan<- *ProgressInfo) { @@ -104,7 +89,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] }) for _, target := range targets { - folderPath := libraryRelativePath(libPaths[target.LibraryID], target.FolderPath) + folderPath := model.LibraryRelativePath(libPaths[target.LibraryID], target.FolderPath) if folderPath == "" { folderPath = "." } @@ -137,6 +122,10 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] // if there was a full scan in progress, force a full scan if !state.fullScan { for _, lib := range state.libraries { + // A pending PID rescan already restarts in full through its own job + if lib.NeedsPIDRescan() { + continue + } if lib.FullScanInProgress { log.Info(ctx, "Scanner: Interrupted full scan detected", "lib", lib.Name) state.fullScan = true @@ -215,10 +204,13 @@ func (s *scannerImpl) prepareLibrariesForScan(ctx context.Context, state *scanSt var successfulLibs []model.Library for _, lib := range state.libraries { - if lib.LastScanStartedAt.IsZero() { + // A library with a changed PID config restarts its scan: resuming would skip the folders that + // the interrupted scan already processed with the old config + pidRescan := lib.NeedsPIDRescan() + if lib.LastScanStartedAt.IsZero() || pidRescan { // This is a new scan - mark it as started err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { - return tx.Library().ScanBegin(ctx, lib.ID, state.fullScan) + return tx.Library().ScanBegin(ctx, lib.ID, state.fullScan || pidRescan) }, "scanner: begin library scan") if err != nil { log.Error(ctx, "Scanner: Error marking scan start", "lib", lib.Name, err) @@ -340,11 +332,12 @@ func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) if err := tx.Library().ScanEnd(ctx, lib.ID); err != nil { return fmt.Errorf("updating last scan completed for %s: %w", lib.Name, err) } - if err := tx.Property().Put(ctx, consts.PIDTrackKey, conf.Server.PID.Track); err != nil { - return fmt.Errorf("updating track PID conf: %w", err) - } - if err := tx.Property().Put(ctx, consts.PIDAlbumKey, conf.Server.PID.Album); err != nil { - return fmt.Errorf("updating album PID conf: %w", err) + // A selective scan covers only part of the library, so the rest may still use the old PID + // config. A library that could not be scanned did not apply it either. + if !state.isSelectiveScan() && !state.failedLibs[lib.ID] { + if err := tx.Library().SetScannedPID(ctx, lib.ID, lib.EffectivePID()); err != nil { + return fmt.Errorf("updating PID conf for %s: %w", lib.Name, err) + } } if state.changesDetected.Load() { log.Debug(ctx, "Scanner: Refreshing library stats", "lib", lib.Name) diff --git a/scanner/scanner_internal_test.go b/scanner/scanner_internal_test.go index 0778bd6ec..e8abb7c7d 100644 --- a/scanner/scanner_internal_test.go +++ b/scanner/scanner_internal_test.go @@ -4,8 +4,6 @@ package scanner import ( "context" "errors" - "os" - "path/filepath" "sync/atomic" ppl "github.com/google/go-pipeline/pkg/pipeline" @@ -13,43 +11,6 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("libraryRelativePath", func() { - // Paths are built with filepath so the "absolute" cases stay absolute on every OS - // (a Unix-style "/foo" is not absolute on Windows). - libRoot, _ := filepath.Abs(filepath.Join("jukebox", "collection")) - outside, _ := filepath.Abs(filepath.Join("somewhere", "else")) - - It("returns a relative path unchanged", func() { - Expect(libraryRelativePath(libRoot, "_Collection")).To(Equal("_Collection")) - }) - - It("rebases an absolute target when the library root is relative", func() { - cwd, err := os.Getwd() - Expect(err).ToNot(HaveOccurred()) - Expect(libraryRelativePath(filepath.Join("music", "library"), filepath.Join(cwd, "music", "library", "rock"))).To(Equal("rock")) - }) - - It("rebases an absolute path that equals the library root to '.'", func() { - Expect(libraryRelativePath(libRoot, libRoot)).To(Equal(".")) - }) - - It("rebases an absolute path under the library root", func() { - Expect(libraryRelativePath(libRoot, filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) - }) - - It("handles a trailing slash on the library path", func() { - Expect(libraryRelativePath(libRoot+string(filepath.Separator), filepath.Join(libRoot, "_Collection"))).To(Equal("_Collection")) - }) - - It("leaves an absolute path outside the library root unchanged", func() { - Expect(libraryRelativePath(libRoot, outside)).To(Equal(outside)) - }) - - It("returns an empty path unchanged", func() { - Expect(libraryRelativePath(libRoot, "")).To(Equal("")) - }) -}) - type mockPhase struct { num int produceFunc func() ppl.Producer[int] diff --git a/scanner/scanner_multilibrary_test.go b/scanner/scanner_multilibrary_test.go index 546baf756..f6634c875 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -835,4 +835,170 @@ var _ = Describe("Scanner - Multi-Library", Ordered, func() { Expect(lastError).To(BeEmpty()) }) }) + + Context("Per-library PID config", func() { + albumsOf := func(libID int) model.Albums { + // The mock datastore's GC is a no-op, so run the real one to purge the albums left + // empty by a regroup, as the scanner does in production + Expect(ds.RealDS.GC(ctx)).To(Succeed()) + albums, err := ds.Album().GetAll(ctx, model.QueryOptions{ + Filters: squirrel.Eq{"library_id": libID, "missing": false}, + Sort: "name", + }) + Expect(err).ToNot(HaveOccurred()) + return albums + } + trackByTitle := func(libID int, title string) model.MediaFile { + mfs, err := ds.MediaFile().GetAll(ctx, model.QueryOptions{ + Filters: squirrel.Eq{"library_id": libID, "title": title}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(1)) + return mfs[0] + } + rockTitles := func() []string { + mfs, err := ds.MediaFile().GetAll(ctx, model.QueryOptions{Filters: squirrel.Eq{"library_id": lib1.ID}}) + Expect(err).ToNot(HaveOccurred()) + return slice.Map(mfs, func(mf model.MediaFile) string { return mf.Title }) + } + // changeRockInDB edits the rock track in the DB only. A full rescan of the rock library would + // restore the title from the file tags, a quick scan leaves it alone. + changeRockInDB := func() { + _, err := db.Db().ExecContext(ctx, "update media_file set title = 'Changed In DB' where library_id = ?", lib1.ID) + Expect(err).ToNot(HaveOccurred()) + } + // changeBlueTrainInDB does the same for one jazz track + changeBlueTrainInDB := func() { + _, err := db.Db().ExecContext(ctx, "update media_file set title = 'Blue Train In DB' where library_id = ? and title = 'Blue Train'", lib2.ID) + Expect(err).ToNot(HaveOccurred()) + } + + BeforeEach(func() { + beatles := template(_t{"albumartist": "The Beatles", "album": "Abbey Road", "year": 1969}) + _ = createFS("rock", fstest.MapFS{ + "The Beatles/Abbey Road/01 - Come Together.mp3": beatles(track(1, "Come Together")), + }) + + miles := template(_t{"albumartist": "Miles Davis", "album": "Kind of Blue", "year": 1959}) + coltrane := template(_t{"albumartist": "John Coltrane", "album": "Giant Steps", "year": 1960}) + blueTrain := template(_t{"albumartist": "John Coltrane", "album": "Blue Train", "year": 1957}) + _ = createFS("jazz", fstest.MapFS{ + "Loose/01 - So What.mp3": miles(track(1, "So What")), + "Loose/02 - Giant Steps.mp3": coltrane(track(1, "Giant Steps")), + "Coltrane/Blue Train/01 - Blue Train.mp3": blueTrain(track(1, "Blue Train")), + }) + }) + + It("regroups only the library whose PID config changed, keeping annotations", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + Expect(albumsOf(lib2.ID)).To(HaveLen(3)) + + // Star Blue Train, to check the star follows the album to its new ID + oldBlueTrain := trackByTitle(lib2.ID, "Blue Train") + Expect(ds.Album().SetStar(ctx, true, oldBlueTrain.AlbumID)).To(Succeed()) + changeRockInDB() + + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Jazz is grouped by folder now: "Loose" is one album + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + Expect(trackByTitle(lib2.ID, "So What").AlbumID).To(Equal(trackByTitle(lib2.ID, "Giant Steps").AlbumID)) + + newBlueTrain := trackByTitle(lib2.ID, "Blue Train") + Expect(newBlueTrain.AlbumID).ToNot(Equal(oldBlueTrain.AlbumID)) + album, err := ds.Album().Get(ctx, newBlueTrain.AlbumID) + Expect(err).ToNot(HaveOccurred()) + Expect(album.Starred).To(BeTrue()) + + // Rock only got a quick scan + Expect(rockTitles()).To(ConsistOf("Changed In DB")) + + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.ScannedPIDAlbum).To(Equal("folder")) + Expect(jazz.PIDChanged()).To(BeFalse()) + rock, err := ds.Library().Get(ctx, lib1.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(rock.PIDChanged()).To(BeFalse()) + }) + + It("rescans only libraries that follow the global config", func() { + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, true)).To(Succeed()) + changeRockInDB() + changeBlueTrainInDB() + + conf.Server.PID.Album = "album" + Expect(runScanner(ctx, false)).To(Succeed()) + + // Rock follows the global config, so it was rescanned in full and its title restored + Expect(rockTitles()).To(ConsistOf("Come Together")) + // Jazz has its own override, so it only got a quick scan + trackByTitle(lib2.ID, "Blue Train In DB") + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.ScannedPIDAlbum).To(Equal("folder")) + }) + + It("restarts an interrupted scan when the PID config changed meanwhile", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + + // Simulate a quick scan of jazz that was interrupted after it had processed every folder: + // the folders were updated after the (old) scan start time + _, err := db.Db().ExecContext(ctx, "update library set last_scan_started_at = ?, full_scan_in_progress = false where id = ?", + time.Now().Add(-time.Hour), lib2.ID) + Expect(err).ToNot(HaveOccurred()) + + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Every folder was revisited with the new config + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + }) + + It("does not turn an interrupted PID rescan into a full scan of every library", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + changeRockInDB() + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + + // Simulate a PID full scan of jazz that was interrupted + Expect(ds.Library().ScanBegin(ctx, lib2.ID, true)).To(Succeed()) + Expect(runScanner(ctx, false)).To(Succeed()) + + // Rock only got a quick scan, jazz was rescanned with the new config + Expect(rockTitles()).To(ConsistOf("Changed In DB")) + Expect(albumsOf(lib2.ID)).To(HaveLen(2)) + }) + + It("does not record the PID config for a library that could not be scanned", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + broken := model.Library{Name: "Broken", Path: "unregistered:///music", PIDAlbum: "folder"} + Expect(ds.Library().Put(ctx, &broken)).To(Succeed()) + + // The scan reports an error for the broken library, and still finishes the others + _ = runScanner(ctx, false) + + reloaded, err := ds.Library().Get(ctx, broken.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(reloaded.PIDChanged()).To(BeTrue()) + }) + + It("does not record the PID config after a selective scan", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + lib2.PIDAlbum = "folder" + Expect(ds.Library().Put(ctx, &lib2)).To(Succeed()) + + _, err := s.ScanFolders(ctx, false, []model.ScanTarget{{LibraryID: lib2.ID, FolderPath: "Loose"}}) + Expect(err).ToNot(HaveOccurred()) + + jazz, err := ds.Library().Get(ctx, lib2.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(jazz.PIDChanged()).To(BeTrue()) + }) + }) }) diff --git a/server/nativeapi/inspect.go b/server/nativeapi/inspect.go index f1e6c4539..61013dd5a 100644 --- a/server/nativeapi/inspect.go +++ b/server/nativeapi/inspect.go @@ -22,7 +22,12 @@ func doInspect(ctx context.Context, ds model.DataStore, id string) (*core.Inspec return nil, model.ErrNotFound } - return core.Inspect(file.AbsolutePath(), file.LibraryID, file.FolderID) + lib, err := ds.Library().Get(ctx, file.LibraryID) + if err != nil { + return nil, err + } + + return core.Inspect(file.AbsolutePath(), *lib, file.FolderID) } func inspect(ds model.DataStore) http.HandlerFunc { diff --git a/server/serve_index.go b/server/serve_index.go index 4b093b953..167197403 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -58,6 +58,8 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl "uiSearchDebounceMs": conf.Server.UISearchDebounceMs, "uiCoverArtSize": conf.Server.UICoverArtSize, "enableCoverAnimation": conf.Server.EnableCoverAnimation, + "pidAlbum": conf.Server.PID.Album, + "pidTrack": conf.Server.PID.Track, "enableNowPlaying": conf.Server.EnableNowPlaying, "playbackReportIntervalMs": conf.Server.UIPlaybackReportInterval.Milliseconds(), "gaTrackingId": conf.Server.GATrackingID, diff --git a/server/serve_index_test.go b/server/serve_index_test.go index e2df55c4b..277513768 100644 --- a/server/serve_index_test.go +++ b/server/serve_index_test.go @@ -89,6 +89,8 @@ var _ = Describe("serveIndex", func() { Entry("uiSearchDebounceMs", func() { conf.Server.UISearchDebounceMs = 500 }, "uiSearchDebounceMs", float64(500)), Entry("uiCoverArtSize", func() { conf.Server.UICoverArtSize = 300 }, "uiCoverArtSize", float64(300)), Entry("enableCoverAnimation", func() { conf.Server.EnableCoverAnimation = true }, "enableCoverAnimation", true), + Entry("pidAlbum", func() { conf.Server.PID.Album = "folder" }, "pidAlbum", "folder"), + Entry("pidTrack", func() { conf.Server.PID.Track = "title" }, "pidTrack", "title"), Entry("enableNowPlaying", func() { conf.Server.EnableNowPlaying = true }, "enableNowPlaying", true), Entry("gaTrackingId", func() { conf.Server.GATrackingID = "UA-12345" }, "gaTrackingId", "UA-12345"), Entry("defaultDownloadableShare", func() { conf.Server.DefaultDownloadableShare = true }, "defaultDownloadableShare", true), diff --git a/tests/mock_library_repo.go b/tests/mock_library_repo.go index e21dcccce..6de2b9265 100644 --- a/tests/mock_library_repo.go +++ b/tests/mock_library_repo.go @@ -145,6 +145,17 @@ func (m *MockLibraryRepo) ScanEnd(_ context.Context, id int) error { return nil } +func (m *MockLibraryRepo) SetScannedPID(_ context.Context, id int, pid model.PIDConfig) error { + if m.Err != nil { + return m.Err + } + if lib, ok := m.Data[id]; ok { + lib.ScannedPIDAlbum, lib.ScannedPIDTrack = pid.Album, pid.Track + m.Data[id] = lib + } + return nil +} + func (m *MockLibraryRepo) ScanInProgress(_ context.Context) (bool, error) { if m.Err != nil { return false, m.Err diff --git a/ui/src/config.js b/ui/src/config.js index e406e47cf..62b3cb822 100644 --- a/ui/src/config.js +++ b/ui/src/config.js @@ -32,6 +32,8 @@ const defaultConfig = { listenBrainzEnabled: true, enableExternalServices: true, enableCoverAnimation: true, + pidAlbum: 'musicbrainz_albumid|albumartistid,album,albumversion,releasedate', // See consts.DefaultAlbumPID + pidTrack: 'musicbrainz_trackid|albumid,discnumber,tracknumber,title', // See consts.DefaultTrackPID enableNowPlaying: true, playbackReportIntervalMs: 60000, devShowArtistPage: true, diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index a04b6e311..f694ea75f 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -331,11 +331,22 @@ "totalDuration": "Duration", "defaultNewUsers": "Default for New Users", "createdAt": "Created", - "updatedAt": "Updated" + "updatedAt": "Updated", + "pidAlbum": "Album grouping", + "pidTrack": "Track identity" }, "sections": { "basic": "Basic Information", - "statistics": "Statistics" + "statistics": "Statistics", + "pid": "Persistent IDs" + }, + "pid": { + "global": "Use global setting (%{value})", + "folder": "Folder (one album per folder)", + "custom": "Custom", + "spec": "PID spec", + "help": "Tags and attributes that identify an item. See the documentation for the syntax:", + "docs": "Persistent IDs" }, "actions": { "scan": "Scan Library", @@ -365,7 +376,9 @@ "messages": { "deleteConfirm": "Are you sure you want to delete this library? This will remove all associated data and user access.", "scanInProgress": "Scan in progress...", - "noLibrariesAssigned": "No libraries assigned to this user" + "noLibrariesAssigned": "No libraries assigned to this user", + "pidChangeTitle": "Change persistent IDs?", + "pidChangeConfirm": "This regroups albums and tracks in this library. A full rescan of this library starts now. Track stars, ratings and play counts are kept. Album stars and ratings move to the new albums where an old album maps to a new one." } }, "plugin": { diff --git a/ui/src/library/LibraryCreate.jsx b/ui/src/library/LibraryCreate.jsx index 0e69964b6..8166bb2f3 100644 --- a/ui/src/library/LibraryCreate.jsx +++ b/ui/src/library/LibraryCreate.jsx @@ -1,4 +1,5 @@ import React, { useCallback } from 'react' +import PropTypes from 'prop-types' import { Create, SimpleForm, @@ -10,7 +11,34 @@ import { useNotify, useRedirect, } from 'react-admin' +import { Typography } from '@material-ui/core' +import { makeStyles } from '@material-ui/core/styles' import { Title } from '../common' +import { PIDInputs } from './PIDInput' + +const useStyles = makeStyles((theme) => ({ + spaced: { marginTop: theme.spacing(3) }, +})) + +// SimpleForm passes form props (variant, record, ...) to its children, so Typography can't be used directly +const SectionTitle = ({ label, spaced }) => { + const translate = useTranslate() + const classes = useStyles() + return ( + + {translate(label)} + + ) +} + +SectionTitle.propTypes = { + label: PropTypes.string.isRequired, + spaced: PropTypes.bool, +} const LibraryCreate = (props) => { const translate = useTranslate() @@ -73,9 +101,12 @@ const LibraryCreate = (props) => { return ( } {...props}> + + + ) diff --git a/ui/src/library/LibraryEdit.jsx b/ui/src/library/LibraryEdit.jsx index 53d17ac7f..c42c7ac4b 100644 --- a/ui/src/library/LibraryEdit.jsx +++ b/ui/src/library/LibraryEdit.jsx @@ -1,9 +1,11 @@ -import React, { useCallback } from 'react' +import React, { useCallback, useState } from 'react' +import PropTypes from 'prop-types' import { Edit, FormWithRedirect, TextInput, BooleanInput, + Confirm, required, SaveButton, useTranslate, @@ -22,6 +24,9 @@ import { ReadOnlySizeField, Title, } from '../common' +import config from '../config' +import { PIDInputs } from './PIDInput' +import { pidConfigChanged } from './pidPresets' const useStyles = makeStyles({ toolbar: { @@ -53,8 +58,131 @@ const CustomToolbar = ({ showDelete, ...props }) => ( ) -const LibraryEdit = (props) => { +export const LibraryEditForm = ({ formProps, canEditPath, canDelete }) => { const translate = useTranslate() + const [confirmOpen, setConfirmOpen] = useState(false) + + // Every submit path (Save button and Enter key) goes through here, so a PID change always asks first + const submit = () => { + if ( + pidConfigChanged( + formProps.form.getState().values, + formProps.record, + config, + ) + ) { + setConfirmOpen(true) + return + } + formProps.handleSubmit() + } + + const handleConfirm = () => { + setConfirmOpen(false) + formProps.handleSubmit() + } + + return ( +
{ + event.preventDefault() + submit() + }} + > + + + + {/* Basic Information */} + + {translate('resources.library.sections.basic')} + + + + + + + + + {translate('resources.library.sections.pid')} + + + + + + {/* Statistics - Two Column Layout */} + + {translate('resources.library.sections.statistics')} + + + + + + + + + + + + + + + + + + + + + setConfirmOpen(false)} + /> + + ) +} + +LibraryEditForm.propTypes = { + formProps: PropTypes.object.isRequired, + canEditPath: PropTypes.bool, + canDelete: PropTypes.bool, +} + +const LibraryEdit = (props) => { const [mutate] = useMutation() const notify = useNotify() const redirect = useRedirect() @@ -93,91 +221,11 @@ const LibraryEdit = (props) => { {...props} save={save} render={(formProps) => ( -
- - - - {/* Basic Information */} - - {translate('resources.library.sections.basic')} - - - - - - - - - {/* Statistics - Two Column Layout */} - - {translate('resources.library.sections.statistics')} - - - - - - - - - - - - - - - - - - - - - + )} /> diff --git a/ui/src/library/LibraryEdit.test.jsx b/ui/src/library/LibraryEdit.test.jsx new file mode 100644 index 000000000..926adc839 --- /dev/null +++ b/ui/src/library/LibraryEdit.test.jsx @@ -0,0 +1,125 @@ +import * as React from 'react' +import { TestContext } from 'ra-test' +import { + FormWithRedirect, + RecordContextProvider, + SaveContextProvider, +} from 'react-admin' +import { + cleanup, + fireEvent, + render, + screen, + waitFor, + within, +} from '@testing-library/react' +import { describe, it, expect, vi, afterEach } from 'vitest' +import { LibraryEditForm } from './LibraryEdit' +import config from '../config' + +const record = { + id: '2', + name: 'Jazz', + path: '/music/jazz', + pidAlbum: '', + pidTrack: '', +} + +// Edit provides a save context in the app. SaveButton only reads these setters from it +const saveContext = { + save: vi.fn(), + setOnSuccess: vi.fn(), + setOnFailure: vi.fn(), + setTransform: vi.fn(), +} + +const renderForm = (save) => + render( + + + + ( + + )} + /> + + + , + ) + +const chooseAlbumGrouping = (optionText) => { + fireEvent.mouseDown( + screen.getByLabelText('resources.library.fields.pidAlbum'), + ) + fireEvent.click(within(screen.getByRole('listbox')).getByText(optionText)) +} + +const dialogTitle = 'resources.library.messages.pidChangeTitle' + +describe('LibraryEditForm', () => { + afterEach(cleanup) + + it('saves directly when the PID config did not change', async () => { + const save = vi.fn() + renderForm(save) + fireEvent.change(screen.getByLabelText(/resources.library.fields.name/), { + target: { value: 'Jazz Renamed' }, + }) + fireEvent.click(screen.getByText('ra.action.save')) + await waitFor(() => expect(save).toHaveBeenCalled()) + expect(screen.queryByText(dialogTitle)).not.toBeInTheDocument() + }) + + it('asks before saving a PID change, and Cancel keeps the edits', async () => { + const save = vi.fn() + renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.click(screen.getByText('ra.action.save')) + + expect(await screen.findByText(dialogTitle)).toBeInTheDocument() + expect(save).not.toHaveBeenCalled() + + fireEvent.click(screen.getByText('ra.action.cancel')) + await waitFor(() => + expect(screen.queryByText(dialogTitle)).not.toBeInTheDocument(), + ) + expect(save).not.toHaveBeenCalled() + expect(screen.getByText('resources.library.pid.folder')).toBeInTheDocument() + }) + + it('saves the PID change after Confirm', async () => { + const save = vi.fn() + renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.click(screen.getByText('ra.action.save')) + fireEvent.click(await screen.findByText('ra.action.confirm')) + + await waitFor(() => expect(save).toHaveBeenCalled()) + expect(save.mock.calls[0][0]).toMatchObject({ pidAlbum: 'folder' }) + }) + + it('pre-fills a Custom spec with the global spec', () => { + renderForm(vi.fn()) + chooseAlbumGrouping('resources.library.pid.custom') + expect(screen.getByLabelText(/resources.library.pid.spec/)).toHaveValue( + config.pidAlbum, + ) + }) + + it('asks before saving when the form is submitted with Enter', async () => { + const save = vi.fn() + const { container } = renderForm(save) + chooseAlbumGrouping('resources.library.pid.folder') + fireEvent.submit(container.querySelector('form')) + + expect(await screen.findByText(dialogTitle)).toBeInTheDocument() + expect(save).not.toHaveBeenCalled() + }) +}) diff --git a/ui/src/library/PIDInput.jsx b/ui/src/library/PIDInput.jsx new file mode 100644 index 000000000..6481dc392 --- /dev/null +++ b/ui/src/library/PIDInput.jsx @@ -0,0 +1,114 @@ +import React, { useState } from 'react' +import PropTypes from 'prop-types' +import { TextInput, required, useTranslate } from 'react-admin' +import { useField } from 'react-final-form' +import { FormHelperText, Link, MenuItem, TextField } from '@material-ui/core' +import { makeStyles } from '@material-ui/core/styles' +import { + PID_CUSTOM, + PID_FOLDER, + PID_GLOBAL, + pidModeFromValue, + pidValueForMode, +} from './pidPresets' +import config from '../config' +import { docsUrl } from '../utils' + +const PID_DOCS_URL = docsUrl('/docs/usage/pids/') + +const useStyles = makeStyles((theme) => ({ + help: { marginBottom: theme.spacing(1) }, +})) + +// PIDInput edits a library PID override: use the global setting, a preset, or a custom spec +export const PIDInput = ({ source, label, globalValue, allowFolder }) => { + const translate = useTranslate() + const classes = useStyles() + const { input } = useField(source) + // Local state, so choosing Custom shows the text box before anything is typed + const [mode, setMode] = useState(() => + pidModeFromValue(input.value, allowFolder), + ) + + const choices = [ + { + id: PID_GLOBAL, + name: translate('resources.library.pid.global', { value: globalValue }), + }, + ...(allowFolder + ? [{ id: PID_FOLDER, name: translate('resources.library.pid.folder') }] + : []), + { id: PID_CUSTOM, name: translate('resources.library.pid.custom') }, + ] + + const handleModeChange = (event) => { + const newMode = event.target.value + setMode(newMode) + input.onChange(pidValueForMode(newMode, globalValue)) + } + + return ( + <> + + {choices.map((choice) => ( + + {choice.name} + + ))} + + {mode === PID_CUSTOM && ( + <> + + + {translate('resources.library.pid.help')}{' '} + + {translate('resources.library.pid.docs')} + + + + )} + + ) +} + +PIDInput.propTypes = { + source: PropTypes.string.isRequired, + label: PropTypes.string.isRequired, + globalValue: PropTypes.string, + allowFolder: PropTypes.bool, +} + +export const PIDInputs = () => { + const translate = useTranslate() + return ( + <> + + + + ) +} diff --git a/ui/src/library/pidPresets.js b/ui/src/library/pidPresets.js new file mode 100644 index 000000000..0483fc691 --- /dev/null +++ b/ui/src/library/pidPresets.js @@ -0,0 +1,33 @@ +export const PID_GLOBAL = 'global' +export const PID_FOLDER = 'folder' +export const PID_CUSTOM = 'custom' + +export const pidModeFromValue = (value, allowFolder) => { + const v = (value || '').trim() + if (v === '') return PID_GLOBAL + if (allowFolder && v === PID_FOLDER) return PID_FOLDER + return PID_CUSTOM +} + +// Returns the value to store for a dropdown choice. Custom starts from the global spec +export const pidValueForMode = (mode, globalValue) => { + switch (mode) { + case PID_GLOBAL: + return '' + case PID_FOLDER: + return PID_FOLDER + default: + return globalValue || '' + } +} + +// Reports whether the form values change the effective PID spec of the saved record. Like the +// server, it trims, treats empty as the global value and compares case-insensitively +export const pidConfigChanged = (values, record, globals) => { + const effective = (value, field) => + ((value || '').trim() || globals[field] || '').toLowerCase() + return ['pidAlbum', 'pidTrack'].some( + (field) => + effective(values[field], field) !== effective(record[field], field), + ) +} diff --git a/ui/src/library/pidPresets.test.js b/ui/src/library/pidPresets.test.js new file mode 100644 index 000000000..dab1c82e2 --- /dev/null +++ b/ui/src/library/pidPresets.test.js @@ -0,0 +1,65 @@ +import { describe, it, expect } from 'vitest' +import { + PID_CUSTOM, + PID_FOLDER, + PID_GLOBAL, + pidConfigChanged, + pidModeFromValue, + pidValueForMode, +} from './pidPresets' + +describe('pidModeFromValue', () => { + it('maps an empty value to the global setting', () => { + expect(pidModeFromValue('', true)).toBe(PID_GLOBAL) + expect(pidModeFromValue(undefined, true)).toBe(PID_GLOBAL) + }) + it('maps folder to the Folder preset when allowed', () => { + expect(pidModeFromValue('folder', true)).toBe(PID_FOLDER) + }) + it('maps folder to Custom when the Folder preset is not offered', () => { + expect(pidModeFromValue('folder', false)).toBe(PID_CUSTOM) + }) + it('maps any other value to Custom', () => { + expect(pidModeFromValue('album|title', true)).toBe(PID_CUSTOM) + }) +}) + +describe('pidValueForMode', () => { + it('stores an empty value for the global setting', () => { + expect(pidValueForMode(PID_GLOBAL, 'album')).toBe('') + }) + it('stores folder for the Folder preset', () => { + expect(pidValueForMode(PID_FOLDER, '')).toBe('folder') + }) + it('starts Custom from the global spec', () => { + expect(pidValueForMode(PID_CUSTOM, 'album|title')).toBe('album|title') + expect(pidValueForMode(PID_CUSTOM, undefined)).toBe('') + }) +}) + +describe('pidConfigChanged', () => { + const record = { pidAlbum: 'folder', pidTrack: '' } + const globals = { + pidAlbum: 'musicbrainz_albumid|albumartistid,album', + pidTrack: 'musicbrainz_trackid|albumid,discnumber,tracknumber,title', + } + it.each([ + ['nothing changed', { pidAlbum: 'folder', pidTrack: '' }, false], + ['a missing value equals an empty one', { pidAlbum: 'folder' }, false], + [ + 'Custom set to the global value', + { pidAlbum: 'folder', pidTrack: globals.pidTrack }, + false, + ], + ['a case-only change', { pidAlbum: 'FOLDER', pidTrack: '' }, false], + [ + 'a whitespace-only change', + { pidAlbum: ' folder ', pidTrack: ' ' }, + false, + ], + ['the album PID changed', { pidAlbum: '', pidTrack: '' }, true], + ['the track PID changed', { pidAlbum: 'folder', pidTrack: 'title' }, true], + ])('%s', (_, values, expected) => { + expect(pidConfigChanged(values, record, globals)).toBe(expected) + }) +}) From 95f67d2c4ef391967327f0c7dccd5d0d8b02f62e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 3 Oct 2026 08:58:54 -0700 Subject: [PATCH 2/4] fix(share): reuse cached transcodes for share streams and zip downloads (#6262) * fix(share): reuse cached transcodes when streaming from share links Public share streams built the stream request with only the share's format and bit rate, leaving sample rate, bit depth and channels at zero. Regular playback resolves those through the transcode decider (e.g. 48000 Hz for Opus), and they are part of the transcoding cache key, so a track already transcoded during normal playback was transcoded again into a separate, identical cache entry when played through a share link. The public router now resolves share stream requests with the same TranscodeDecider.ResolveRequest used by the Subsonic stream endpoint, so both paths produce the same request and share cache entries. Fixes #6261 * fix(archiver): reuse cached transcodes when zipping downloads Zip downloads (album, artist, playlist and share) built the stream request with only the format and bit rate, leaving sample rate, bit depth and channels at zero. Those are part of the transcoding cache key, so a track already transcoded for playback was transcoded again into a separate cache entry when downloaded in a zip, and vice versa. The archiver now resolves each request with TranscodeDecider.ResolveRequest, the same as single-song downloads and streams. This also applies the decider's defaults, so a zip requested without a bit rate uses the target format's default bit rate instead of leaving it to ffmpeg. * fix(archiver): name zip entries after the resolved transcoding format The transcode decider can pick a different format than the one requested (for example a player's forced transcoding, or a fallback to the default downsampling format when the requested one can't be produced). Zip entry names and the playlist M3U were still built from the requested format, so an entry could end in .mp3 or .flac while holding Opus data. Each track's request is now resolved before its entry name is built, and the name uses the resolved format. --- cmd/wire_gen.go | 9 +-- core/archiver.go | 30 ++++++---- core/archiver_test.go | 58 +++++++++++++++++++- server/public/handle_streams.go | 5 +- server/public/handle_streams_test.go | 17 ++++-- server/public/public.go | 5 +- server/subsonic/e2e/subsonic_artwork_test.go | 2 +- 7 files changed, 101 insertions(+), 25 deletions(-) diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 19f92d9d5..2a396689d 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -95,8 +95,9 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) + transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) share := core.NewShare(dataStore) - archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) + archiver := core.NewArchiver(mediaStreamer, transcodeDecider, dataStore, share, artworkArtwork) players := core.NewPlayers(dataStore) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) @@ -110,7 +111,6 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) - transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics, lyricsLyrics, transcodeDecider, sonicSonic) return router @@ -159,9 +159,10 @@ func CreatePublicRouter() *public.Router { artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) + transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) share := core.NewShare(dataStore) - archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) - router := public.New(dataStore, artworkArtwork, mediaStreamer, share, archiver) + archiver := core.NewArchiver(mediaStreamer, transcodeDecider, dataStore, share, artworkArtwork) + router := public.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, share, archiver) return router } diff --git a/core/archiver.go b/core/archiver.go index 6f362322a..60eb44858 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -35,13 +35,14 @@ type Archiver interface { ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error } -func NewArchiver(ms stream.MediaStreamer, ds model.DataStore, shares Share, artwork artwork.Artwork) Archiver { - return &archiver{ds: ds, ms: ms, shares: shares, artwork: artwork} +func NewArchiver(ms stream.MediaStreamer, decider stream.TranscodeDecider, ds model.DataStore, shares Share, artwork artwork.Artwork) Archiver { + return &archiver{ds: ds, ms: ms, decider: decider, shares: shares, artwork: artwork} } type archiver struct { ds model.DataStore ms stream.MediaStreamer + decider stream.TranscodeDecider shares Share artwork artwork.Artwork } @@ -78,8 +79,9 @@ func (a *archiver) zipAlbums(ctx context.Context, id string, format string, bitr log.Debug(ctx, "Zipping album", "name", album[0].Album, "artist", album[0].AlbumArtist, "folder", folder, "format", format, "bitrate", bitrate, "isMultiDisc", isMultiDisc, "numTracks", len(album)) for _, mf := range album { - file := a.albumFilename(mf, format, isMultiDisc, folder) - if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { + req := a.resolveRequest(ctx, &mf, format, bitrate) + file := a.albumFilename(mf, req.Format, isMultiDisc, folder) + if addErr := a.addFileToZip(ctx, z, mf, req, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { // Stop iterating: continuing would just rack up more // rejections from the limiter. Close finalises whatever // tracks were already written; the rejected one is not @@ -204,8 +206,9 @@ func (a *archiver) zipMediaFiles(ctx context.Context, id, name string, format st zippedMfs := make(model.MediaFiles, len(mfs)) for idx, mf := range mfs { - file := a.playlistFilename(mf, format, idx) - if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { + req := a.resolveRequest(ctx, &mf, format, bitrate) + file := a.playlistFilename(mf, req.Format, idx) + if addErr := a.addFileToZip(ctx, z, mf, req, file); errors.Is(addErr, stream.ErrTooManyTranscodes) { // Abort the whole archive: continuing would silently emit // empty zip entries since the headers are already written. _ = z.Close() @@ -251,7 +254,14 @@ func (a *archiver) playlistFilename(mf model.MediaFile, format string, idx int) return fmt.Sprintf("%02d - %s - %s.%s", idx+1, str.SanitizeFilename(mf.Artist), str.SanitizeFilename(mf.Title), ext) } -func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.MediaFile, format string, bitrate int, filename string) error { +func (a *archiver) resolveRequest(ctx context.Context, mf *model.MediaFile, format string, bitrate int) stream.Request { + if format == "" || format == "raw" { + return stream.Request{Format: "raw"} + } + return a.decider.ResolveRequest(ctx, mf, format, bitrate, 0) +} + +func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.MediaFile, req stream.Request, filename string) error { path := mf.AbsolutePath() // Open the source before writing the zip entry header so a rejection @@ -259,13 +269,13 @@ func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.Med // archive. var r io.ReadCloser var err error - if format != "raw" && format != "" { - r, err = a.ms.NewStream(ctx, &mf, stream.Request{Format: format, BitRate: bitrate}) + if req.Format != "raw" { + r, err = a.ms.NewStream(ctx, &mf, req) } else { r, err = os.Open(path) } if err != nil { - log.Error(ctx, "Error opening file for zipping", "file", path, "format", format, err) + log.Error(ctx, "Error opening file for zipping", "file", path, "format", req.Format, err) return err } defer func() { diff --git a/core/archiver_test.go b/core/archiver_test.go index 4e00ce78c..178d1b6b9 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -26,6 +26,7 @@ var _ = Describe("Archiver", func() { var ( arch core.Archiver ms *mockMediaStreamer + dc *fakeDecider ds *mockDataStore sh *mockShare ca *mockCoverArt @@ -33,10 +34,11 @@ var _ = Describe("Archiver", func() { BeforeEach(func() { ms = &mockMediaStreamer{} + dc = &fakeDecider{} sh = &mockShare{} ds = &mockDataStore{} ca = &mockCoverArt{images: map[string][]byte{}} - arch = core.NewArchiver(ms, ds, sh, ca) + arch = core.NewArchiver(ms, dc, ds, sh, ca) }) Context("ZipAlbum", func() { @@ -66,6 +68,23 @@ var _ = Describe("Archiver", func() { Expect(zr.File[0].Name).To(Equal("Album_Promo/01 - track1.mp3")) Expect(zr.File[1].Name).To(Equal("Album_Promo/02 - track2.mp3")) }) + + It("streams the request resolved by the transcode decider and names the entry after its format", func() { + mfRepo := &mockMediaFileRepository{} + mfRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{{Path: "test_data/01 - track1.flac", Suffix: "flac", AlbumID: "1"}}, nil) + ds.On("MediaFile").Return(mfRepo) + resolved := stream.Request{Format: "opus", BitRate: 128, SampleRate: 48000, Channels: 2} + dc.resolved = &resolved + ms.On("NewStream", mock.Anything, mock.Anything, resolved).Return(io.NopCloser(strings.NewReader("test")), nil).Once() + + out := new(bytes.Buffer) + Expect(arch.ZipAlbum(GinkgoT().Context(), "1", "mp3", 128, out)).To(Succeed()) + ms.AssertExpectations(GinkgoT()) + + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) + Expect(err).ToNot(HaveOccurred()) + Expect(zr.File[0].Name).To(HaveSuffix("01 - track1.opus")) + }) }) Context("ZipArtist", func() { @@ -296,6 +315,30 @@ var _ = Describe("Archiver", func() { }) Context("ZipPlaylist", func() { + It("names the entries and the M3U lines after the resolved format", func() { + pls := &model.Playlist{ID: "1", Name: "Test Playlist", Tracks: []model.PlaylistTrack{ + {MediaFile: model.MediaFile{Path: "test_data/01 - track1.flac", Suffix: "flac", Artist: "Artist 1", Title: "track1"}}, + }} + plRepo := &mockPlaylistRepository{} + plRepo.On("GetWithTracks", "1", true, false).Return(pls, nil) + ds.On("Playlist").Return(plRepo) + dc.resolved = &stream.Request{Format: "opus", BitRate: 128} + ms.On("NewStream", mock.Anything, mock.Anything, *dc.resolved).Return(io.NopCloser(strings.NewReader("test")), nil) + + out := new(bytes.Buffer) + Expect(arch.ZipPlaylist(GinkgoT().Context(), "1", "mp3", 128, out)).To(Succeed()) + + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) + Expect(err).ToNot(HaveOccurred()) + Expect(zr.File[0].Name).To(Equal("01 - Artist 1 - track1.opus")) + m3u, err := zr.File[1].Open() + Expect(err).ToNot(HaveOccurred()) + defer m3u.Close() + content, err := io.ReadAll(m3u) + Expect(err).ToNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring("01 - Artist 1 - track1.opus")) + }) + It("zips a playlist correctly", func() { tracks := []model.PlaylistTrack{ {MediaFile: model.MediaFile{Path: "test_data/01 - track1.mp3", Suffix: "mp3", AlbumID: "1", Album: "Album 1", DiscNumber: 1, Artist: "AC/DC", Title: "track1"}}, @@ -571,6 +614,19 @@ func (m *mockMediaStreamer) NewStream(ctx context.Context, mf *model.MediaFile, return &stream.Stream{ReadCloser: args.Get(0).(io.ReadCloser)}, nil } +// fakeDecider echoes the legacy format/bitrate unless a resolved request is set. +type fakeDecider struct { + stream.TranscodeDecider + resolved *stream.Request +} + +func (f *fakeDecider) ResolveRequest(_ context.Context, _ *model.MediaFile, format string, bitRate int, offset int) stream.Request { + if f.resolved != nil { + return *f.resolved + } + return stream.Request{Format: format, BitRate: bitRate, Offset: offset} +} + type mockShare struct { mock.Mock core.Share diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index 37ae56c2b..46c7ca210 100644 --- a/server/public/handle_streams.go +++ b/server/public/handle_streams.go @@ -60,9 +60,8 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { return } - stream, err := pub.streamer.NewStream(ctx, mf, streampkg.Request{ - Format: info.format, BitRate: info.bitrate, - }) + streamReq := pub.decider.ResolveRequest(ctx, mf, info.format, info.bitrate, 0) + stream, err := pub.streamer.NewStream(ctx, mf, streamReq) if err != nil { if errors.Is(err, streampkg.ErrTooManyTranscodes) { w.Header().Set("Retry-After", strconv.Itoa(streampkg.RetryAfterSeconds)) diff --git a/server/public/handle_streams_test.go b/server/public/handle_streams_test.go index 4b4a3545b..965bc7e05 100644 --- a/server/public/handle_streams_test.go +++ b/server/public/handle_streams_test.go @@ -116,11 +116,11 @@ var _ = Describe("handleStream", func() { BeforeEach(func() { ctx = GinkgoT().Context() auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) - ds = &tests.MockDataStore{} + ds = &tests.MockDataStore{MockedTranscoding: &tests.MockTranscodingRepo{}} shareRepo = &tests.MockShareRepo{} ds.MockedShare = shareRepo streamer = &mockStreamer{} - pub = &Router{ds: ds, streamer: streamer} + pub = &Router{ds: ds, streamer: streamer, decider: stream.NewTranscodeDecider(ds, tests.NewMockFFmpeg(""))} }) makeRequest := func(token string) *httptest.ResponseRecorder { @@ -152,8 +152,17 @@ var _ = Describe("handleStream", func() { makeRequest(token) Expect(streamer.called).To(BeTrue()) - Expect(streamer.req.Format).To(Equal("mp3")) - Expect(streamer.req.BitRate).To(Equal(192)) + }) + + It("resolves the full stream request like the Subsonic endpoint, so transcodes share the cache", func() { + mf := model.MediaFile{ID: "mf-123", Suffix: "flac", BitRate: 1500, SampleRate: 44100, BitDepth: new(24), Channels: 2} + shareOwnedBy(model.User{ID: "owner1", UserName: "owner1", IsAdmin: true}, mf) + + claims := auth.Claims{ID: "mf-123", Format: "opus", BitRate: 128, ShareID: "share123"} + token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims) + makeRequest(token) + + Expect(streamer.req).To(Equal(stream.Request{Format: "opus", BitRate: 128, SampleRate: 48000, Channels: 2})) }) It("returns 404 when the track is outside the share owner's libraries", func() { diff --git a/server/public/public.go b/server/public/public.go index 142c474bd..8239ef927 100644 --- a/server/public/public.go +++ b/server/public/public.go @@ -21,14 +21,15 @@ type Router struct { http.Handler artwork artwork.Artwork streamer stream.MediaStreamer + decider stream.TranscodeDecider archiver core.Archiver share core.Share assetsHandler http.Handler ds model.DataStore } -func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, share core.Share, archiver core.Archiver) *Router { - p := &Router{ds: ds, artwork: artwork, streamer: streamer, share: share, archiver: archiver} +func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, decider stream.TranscodeDecider, share core.Share, archiver core.Archiver) *Router { + p := &Router{ds: ds, artwork: artwork, streamer: streamer, decider: decider, share: share, archiver: archiver} shareRoot := path.Join(conf.Server.BasePath, consts.URLPathPublic) p.assetsHandler = http.StripPrefix(shareRoot, http.FileServer(http.FS(ui.BuildAssets()))) p.Handler = p.routes() diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 9394c8830..530588759 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -137,7 +137,7 @@ var _ = Describe("Artwork Serving", Ordered, func() { artRouter = buildArtworkRouter(artSvc) router = artRouter // so the shared doReq/doRawReq helpers hit the artwork-wired router - pubRouter = public.New(ds, artSvc, streamerSpy, core.NewShare(ds), noopArchiver{}) + pubRouter = public.New(ds, artSvc, streamerSpy, stream.NewTranscodeDecider(ds, ffm), core.NewShare(ds), noopArchiver{}) }) It("emits a bare optimistic coverArt id before the queue is drained", func() { From caa2f8a0c012328efb9d5685d67b8dc45d24a635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 6 Oct 2026 06:50:11 -0700 Subject: [PATCH 3/4] fix(ui): make playlist toggle switches visible in all themes (#6277) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ui): make playlist toggle switches visible in all themes The Public and Auto-import switches in the playlist list did not set a color, so Material-UI used the theme's secondary color. Many themes use secondary as a surface color close to the table background, which made checked switches nearly invisible (Catppuccin, Rosé Pine, Monokai, Moonbase and others). Set color="primary" on the playlist switch, like every other switch in the app, and make primary the default MuiSwitch color in useCurrentTheme so future switches cannot regress. Fixes #6272. * refactor(ui): drop secondary switch overrides from themes Dracula, Gruvbox Dark, Tokyo Night and Tokyo Night Light styled checked MuiSwitch colorSecondary to work around the same invisible-switch problem (Gruvbox in #5064). With primary as the default switch color and every switch in the app using it, no switch renders with colorSecondary anymore, so these overrides are dead code. --- ui/src/playlist/PlaylistList.jsx | 1 + ui/src/playlist/PlaylistList.test.jsx | 27 ++++++++++++++++++++++++++ ui/src/themes/dracula.js | 10 ---------- ui/src/themes/gruvboxDark.js | 10 ---------- ui/src/themes/tokyoNight.js | 10 ---------- ui/src/themes/tokyoNightLight.js | 10 ---------- ui/src/themes/useCurrentTheme.js | 2 ++ ui/src/themes/useCurrentTheme.test.jsx | 27 ++++++++++++++++++++++++++ 8 files changed, 57 insertions(+), 40 deletions(-) diff --git a/ui/src/playlist/PlaylistList.jsx b/ui/src/playlist/PlaylistList.jsx index 14d819a4e..e1695e980 100644 --- a/ui/src/playlist/PlaylistList.jsx +++ b/ui/src/playlist/PlaylistList.jsx @@ -95,6 +95,7 @@ export const ToggleField = ({ resource, source }) => { return ( diff --git a/ui/src/playlist/PlaylistList.test.jsx b/ui/src/playlist/PlaylistList.test.jsx index 6c714b827..c05833166 100644 --- a/ui/src/playlist/PlaylistList.test.jsx +++ b/ui/src/playlist/PlaylistList.test.jsx @@ -2,6 +2,7 @@ import React from 'react' import { render, screen } from '@testing-library/react' import { describe, it, expect, vi } from 'vitest' import { TestContext } from 'ra-test' +import { RecordContextProvider } from 'react-admin' import { PlaylistLove, ToggleField, ToggleAutoImport } from './PlaylistList' vi.mock('../config', () => ({ @@ -14,6 +15,7 @@ vi.mock('../common', () => ({ {record?.starred ? 'starred' : 'not-starred'} ), + isWritable: (ownerId) => ownerId === 'me', })) describe('', () => { @@ -55,3 +57,28 @@ describe('playlist toggles without a record', () => { expect(container.innerHTML).toBe('') }) }) + +// Secondary is a surface color in many themes, so these toggles must use primary +describe('', () => { + const renderToggle = (record) => + render( + + + + + , + ) + + it.each([ + ['owner', 'me', false], + ['non-owner', 'someone-else', true], + ])('renders a primary-colored switch for the %s', (_, ownerId, disabled) => { + renderToggle({ id: 'pl-1', public: true, ownerId }) + const input = screen.getByRole('checkbox') + const switchBase = input.closest('.MuiSwitch-switchBase') + expect(input.checked).toBe(true) + expect(input.disabled).toBe(disabled) + expect(switchBase.classList).toContain('MuiSwitch-colorPrimary') + expect(switchBase.classList).not.toContain('MuiSwitch-colorSecondary') + }) +}) diff --git a/ui/src/themes/dracula.js b/ui/src/themes/dracula.js index 2e4ae38e5..45559c3af 100644 --- a/ui/src/themes/dracula.js +++ b/ui/src/themes/dracula.js @@ -185,16 +185,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: green, - }, - '&$checked + $track': { - backgroundColor: green, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/gruvboxDark.js b/ui/src/themes/gruvboxDark.js index 0f4cbd7c4..3e2955dcd 100644 --- a/ui/src/themes/gruvboxDark.js +++ b/ui/src/themes/gruvboxDark.js @@ -121,16 +121,6 @@ export default { boxShadow: '3px 3px 5px #3c3836', }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: '#458588', - }, - '&$checked + $track': { - backgroundColor: '#458588', - }, - }, - }, NDMobileArtistDetails: { bgContainer: { background: diff --git a/ui/src/themes/tokyoNight.js b/ui/src/themes/tokyoNight.js index 07d372a6b..9f6424b77 100644 --- a/ui/src/themes/tokyoNight.js +++ b/ui/src/themes/tokyoNight.js @@ -184,16 +184,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: blue, - }, - '&$checked + $track': { - backgroundColor: blue, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/tokyoNightLight.js b/ui/src/themes/tokyoNightLight.js index f84cd0be9..a61c0fe87 100644 --- a/ui/src/themes/tokyoNightLight.js +++ b/ui/src/themes/tokyoNightLight.js @@ -184,16 +184,6 @@ export default { color: `${foreground} !important`, }, }, - MuiSwitch: { - colorSecondary: { - '&$checked': { - color: blue, - }, - '&$checked + $track': { - backgroundColor: blue, - }, - }, - }, NDAlbumGridView: { albumName: { marginTop: '0.5rem', diff --git a/ui/src/themes/useCurrentTheme.js b/ui/src/themes/useCurrentTheme.js index 4ccefe820..fbb5e9bc8 100644 --- a/ui/src/themes/useCurrentTheme.js +++ b/ui/src/themes/useCurrentTheme.js @@ -63,6 +63,8 @@ const useCurrentTheme = () => { ...theme.props, MuiUseMediaQuery: { noSsr: true }, MuiPopover: { disableScrollLock: true }, + // MUI defaults to secondary, which many themes use as a surface color + MuiSwitch: { color: 'primary' }, }, }), [theme], diff --git a/ui/src/themes/useCurrentTheme.test.jsx b/ui/src/themes/useCurrentTheme.test.jsx index 65c3be8c6..6553d9866 100644 --- a/ui/src/themes/useCurrentTheme.test.jsx +++ b/ui/src/themes/useCurrentTheme.test.jsx @@ -3,6 +3,10 @@ import { Provider } from 'react-redux' import { createStore } from 'redux' import mediaQuery from 'css-mediaquery' import { renderHook } from '@testing-library/react-hooks' +import { render, screen } from '@testing-library/react' +import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles' +import Switch from '@material-ui/core/Switch' +import themes from './index' import useCurrentTheme from './useCurrentTheme' import { themeReducer } from '../reducers/themeReducer' import { AUTO_THEME_ID } from '../consts' @@ -161,4 +165,27 @@ describe('useCurrentTheme', () => { expect(document.body.style.backgroundColor).toBe('rgb(18, 18, 18)') }) }) + describe('switch color', () => { + it.each(Object.keys(themes))( + 'renders switches with the primary color in %s', + (theme) => { + const { result } = renderHook(() => useCurrentTheme(), { + wrapper: ({ children }) => ( + + {children} + + ), + }) + render( + + {}} /> + , + ) + const switchBase = screen + .getByRole('checkbox') + .closest('.MuiSwitch-switchBase') + expect(switchBase.classList).toContain('MuiSwitch-colorPrimary') + }, + ) + }) }) From 52135913d4747c8f8ddcfd8e7004202b8f53b594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 6 Oct 2026 07:57:07 -0700 Subject: [PATCH 4/4] fix(artwork): don't crash the server when a playlist's tracks can't be loaded (#6267) Playlist().Tracks returns nil when its internal Get fails (for example when the context is canceled at shutdown), and resolvePlaylist called GetAlbumIDs on it, panicking with a nil pointer dereference. The artwork drain runs on a bare goroutine, so the panic killed the whole server. resolvePlaylist now returns an error when Tracks is nil, and the worker recovers panics per item: it logs the panic with the item details and stack, and marks the item as a failed attempt so the rest of the batch still runs. Fixes #6266 --- core/artwork/resolve.go | 7 ++++-- core/artwork/resolve_test.go | 10 +++++++++ core/artwork/worker.go | 18 +++++++++++++++- core/artwork/worker_test.go | 42 ++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 40baa2495..fb07332fe 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -374,8 +374,11 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso } } - albumIDs, err := r.ds.Playlist().Tracks(ctx, pl.ID, false). - GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"}) + tracks := r.ds.Playlist().Tracks(ctx, pl.ID, false) + if tracks == nil { + return resolution{}, fmt.Errorf("resolvePlaylist: could not load tracks for playlist %s", pl.ID) + } + albumIDs, err := tracks.GetAlbumIDs(ctx, model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"}) if err != nil { return resolution{}, err } diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index da144d8e2..2a36531bb 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -707,6 +707,16 @@ var _ = Describe("resolveItem", func() { Expect(err).To(HaveOccurred()) Expect(res).To(Equal(resolution{})) }) + + It("returns an error when the playlist tracks cannot be loaded", func() { + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "pl4", Name: "Playlist"}}) + ds.MockedPlaylist = plRepo + + res, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl4"}) + Expect(err).To(HaveOccurred()) + Expect(res).To(Equal(resolution{})) + }) }) }) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 28e51958c..4be99f92e 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -4,9 +4,11 @@ import ( "bytes" "cmp" "context" + "fmt" "io" "math" "math/rand/v2" + "runtime/debug" "sync" "time" @@ -244,7 +246,7 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc item.ImageType = cmp.Or(item.ImageType, model.ImageTypePrimary) trace := &ChainTrace{} ctx = withTrace(ctx, trace) - out, got, retryIn := w.proc.acquire(ctx, item) + out, got, retryIn := w.safeAcquire(ctx, item) queue := w.proc.ds.ArtworkQueue() switch out { @@ -286,6 +288,20 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc return out, got } +// safeAcquire turns a panic into a failed attempt: the drain runs on a bare goroutine, so an +// unrecovered panic would crash the server, and the still-queued row would crash it again on restart. +func (w *Worker) safeAcquire(ctx context.Context, item model.ArtworkQueueItem) (out outcome, got *acquired, retryIn time.Duration) { + defer func() { + if r := recover(); r != nil { + log.Error(ctx, "Artwork: Panic while processing item", "kind", item.ItemKind, "id", item.ItemID, + "imageType", item.ImageType, "attempts", item.Attempts, "panic", r, "stack", string(debug.Stack())) + traceStage(ctx, "panic", fmt.Errorf("%v", r)) + out, got, retryIn = outcomeFailed, nil, 0 + } + }() + return w.proc.acquire(ctx, item) +} + // recordGiveUp keeps the last failure on the state row after the queue row is deleted. An item // that never resolved has no row to update, and creating one would settle it absent. func (w *Worker) recordGiveUp(ctx context.Context, item model.ArtworkQueueItem, trace string) { diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index a6c07b763..80ca68bc3 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -142,6 +142,18 @@ func (v *visibilityPlaylistRepo) Get(ctx context.Context, id string) (*model.Pla return v.MockPlaylistRepo.Get(ctx, id) } +type panickingAlbumRepo struct { + *tests.MockAlbumRepo + panicID string +} + +func (r *panickingAlbumRepo) Get(ctx context.Context, id string) (*model.Album, error) { + if id == r.panicID { + panic("boom") + } + return r.MockAlbumRepo.Get(ctx, id) +} + func adminUserRepo() *tests.MockedUserRepo { repo := tests.CreateMockUserRepo() Expect(repo.Put(GinkgoT().Context(), &model.User{ID: "admin", UserName: "admin", IsAdmin: true})).To(Succeed()) @@ -278,6 +290,36 @@ var _ = Describe("Worker", func() { Expect(err).To(MatchError(model.ErrNotFound), "a timeout must never settle on absent") }) + It("fails an item that panics, without stopping the rest of the batch", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + albums := tests.CreateMockAlbumRepo() + albums.SetData(model.Albums{ + {ID: "alboom", Name: "Album", FolderIDs: []string{"f1"}}, + {ID: "alok", Name: "Album", FolderIDs: []string{"f1"}}, + }) + ds.MockedAlbum = &panickingAlbumRepo{MockAlbumRepo: albums, panicID: "alboom"} + Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alboom"})).To(Succeed()) + Expect(queueRepo.Enqueue(ctx, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alok"})).To(Succeed()) + + n, err := w.drain(ctx, 1) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(2)) + + it := findQueued(queueRepo, "al", "alboom") + Expect(it).ToNot(BeNil(), "a panicking item must be rescheduled, not dropped") + Expect(it.Attempts).To(Equal(1)) + Expect(it.RetryAt).To(BeTemporally(">", time.Now())) + Expect(it.Trace).To(ContainSubstring("boom")) + + Expect(findQueued(queueRepo, "al", "alok")).To(BeNil()) + ia, err := artRepo.GetItemArtwork(ctx, model.KindAlbumArtwork, "alok", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Source).To(Equal("folder")) + }) + It("reschedules past the provider's requested delay when it exceeds the backoff", func() { conf.Server.CoverArtPriority = "external" ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "al9", Name: "Album"}})