diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index 3bc539132..c1ba8714c 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -519,18 +519,13 @@ jobs: docker buildx imagetools inspect ${{ env.REGISTRY_IMAGE }}:${{ steps.docker.outputs.version }} push-manifest-dockerhub: - name: Push to Docker Hub (${{ matrix.repository }}) + name: Push to Docker Hub runs-on: ubuntu-latest permissions: contents: read needs: [build, check-push-enabled] if: needs.check-push-enabled.outputs.is_enabled == 'true' && vars.DOCKER_HUB_REPO != '' continue-on-error: true - strategy: - fail-fast: false - matrix: - # Always push to DOCKER_HUB_REPO. The upstream repository also pushes to the navidrome/navidrome org repository. - repository: ${{ fromJSON(github.repository == 'navidrome/navidrome' && format('["{0}","navidrome/navidrome"]', vars.DOCKER_HUB_REPO) || format('["{0}"]', vars.DOCKER_HUB_REPO)) }} steps: - uses: actions/checkout@v7 @@ -546,7 +541,7 @@ jobs: id: docker with: github_token: ${{ secrets.GITHUB_TOKEN }} - hub_repository: ${{ matrix.repository }} + hub_repository: ${{ vars.DOCKER_HUB_REPO }} hub_username: ${{ secrets.DOCKER_HUB_USERNAME }} hub_password: ${{ secrets.DOCKER_HUB_PASSWORD }} @@ -563,7 +558,7 @@ jobs: - name: Inspect image in Docker Hub run: | - docker buildx imagetools inspect ${{ matrix.repository }}:${{ steps.docker.outputs.version }} + docker buildx imagetools inspect ${{ vars.DOCKER_HUB_REPO }}:${{ steps.docker.outputs.version }} cleanup-digests: name: Cleanup digest artifacts diff --git a/README.md b/README.md index e1a2955a1..4bc85e6a6 100644 --- a/README.md +++ b/README.md @@ -35,18 +35,11 @@ See instructions on the [project's website](https://www.navidrome.org/docs/insta ## Cloud Hosting -Several cloud hosting providers partner with us to offer [officially supported, cloud-hosted solutions](https://www.navidrome.org/docs/installation/managed). If you sign up with any of these providers, a share of the revenue funds the development of Navidrome at no additional cost for you. - -Run on PikaPods -
-Deploy with Zenith -
-Deploy on ElfHosted - - - - +[PikaPods](https://www.pikapods.com) has partnered with us to offer you an +[officially supported, cloud-hosted solution](https://www.navidrome.org/docs/installation/managed/#pikapods). +A share of the revenue helps fund the development of Navidrome at no additional cost for you. +[![PikaPods](https://www.pikapods.com/static/run-button.svg)](https://www.pikapods.com/pods?run=navidrome) ## Features diff --git a/adapters/gotaglib/end_to_end_test.go b/adapters/gotaglib/end_to_end_test.go index 0f9a90d94..e7dd18ac1 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(model.Library{ID: 1}, "folderID")) + return new(metadata.ToMediaFile(1, "folderID")) } BeforeEach(func() { diff --git a/cmd/inspect.go b/cmd/inspect.go index 05f569f3e..5e88793cc 100644 --- a/cmd/inspect.go +++ b/cmd/inspect.go @@ -1,17 +1,13 @@ 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" @@ -32,7 +28,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(cmd.Context(), args) + runInspector(args) }, } @@ -59,24 +55,18 @@ func prettyMarshal(v any) ([]byte, error) { return []byte(res.String()), nil } -func runInspector(ctx context.Context, args []string) { +func runInspector(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 } - 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, "") + output, err := core.Inspect(filePath, 1, "") if err != nil { log.Warn("Unable to process file", "file", filePath, "error", err) continue @@ -87,33 +77,3 @@ func runInspector(ctx context.Context, 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 deleted file mode 100644 index 728dc8770..000000000 --- a/cmd/inspect_test.go +++ /dev/null @@ -1,61 +0,0 @@ -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 e39c55365..089f09472 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -190,20 +190,16 @@ func schedulePeriodicScan(ctx context.Context) func() error { } } -// 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) +func pidHashChanged(ds model.DataStore) (bool, error) { + pidAlbum, err := ds.Property().DefaultGet(context.Background(), consts.PIDAlbumKey, "") if err != nil { - return nil, err + return false, err } - var names []string - for _, lib := range libs { - if lib.PIDChanged() { - names = append(names, lib.Name) - } + pidTrack, err := ds.Property().DefaultGet(context.Background(), consts.PIDTrackKey, "") + if err != nil { + return false, err } - return names, nil + return !strings.EqualFold(pidAlbum, conf.Server.PID.Album) || !strings.EqualFold(pidTrack, conf.Server.PID.Track), nil } // runInitialScan runs an initial scan of the music library if needed. @@ -218,12 +214,12 @@ func runInitialScan(ctx context.Context) func() error { if err != nil { return err } - pidChangedLibs, err := librariesWithChangedPID(ctx, ds) + pidHasChanged, err := pidHashChanged(ds) if err != nil { return err } scanOnStartup := conf.Server.Scanner.Enabled && conf.Server.Scanner.ScanOnStartup - scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || len(pidChangedLibs) > 0 + scanNeeded := scanOnStartup || inProgress || fullScanRequired == "1" || pidHasChanged time.Sleep(2 * time.Second) // Wait 2 seconds before the initial scan if scanNeeded { s := CreateScanner(ctx) @@ -231,9 +227,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 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 pidHasChanged: + log.Warn(ctx, "PID config changed, performing full scan") + fullScanRequired = "1" case inProgress: log.Warn(ctx, "Resuming interrupted scan") default: diff --git a/cmd/root_test.go b/cmd/root_test.go index 423cd2a8f..af8d44e7e 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -1,7 +1,6 @@ package cmd import ( - "errors" "net/http" "net/http/httptest" "path" @@ -10,8 +9,6 @@ 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" ) @@ -47,30 +44,3 @@ 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 f35a31fb1..72ec67f90 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -18,16 +18,11 @@ import ( "github.com/navidrome/navidrome/persistence" ) -// existingDBFile returns the database file (DbPath minus DSN params), and whether it exists. -func existingDBFile() (string, bool) { - path, _, _ := strings.Cut(conf.Server.DbPath, "?") - _, err := os.Stat(path) - return path, err == nil -} - -// requireExistingDB aborts the command when the database file does not exist. +// requireExistingDB aborts the command when the database file (DbPath minus DSN +// params) does not exist. func requireExistingDB() { - if path, ok := existingDBFile(); !ok { + path, _, _ := strings.Cut(conf.Server.DbPath, "?") + if _, err := os.Stat(path); os.IsNotExist(err) { log.Fatal("No existing database", "path", path) } } diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 2a396689d..19f92d9d5 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -95,9 +95,8 @@ 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, transcodeDecider, dataStore, share, artworkArtwork) + archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) players := core.NewPlayers(dataStore) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) @@ -111,6 +110,7 @@ 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,10 +159,9 @@ 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, transcodeDecider, dataStore, share, artworkArtwork) - router := public.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, share, archiver) + archiver := core.NewArchiver(mediaStreamer, dataStore, share, artworkArtwork) + router := public.New(dataStore, artworkArtwork, mediaStreamer, share, archiver) return router } diff --git a/consts/consts.go b/consts/consts.go index 42e9ec42f..9bdac9125 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -156,6 +156,8 @@ 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/archiver.go b/core/archiver.go index 60eb44858..6f362322a 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -35,14 +35,13 @@ type Archiver interface { ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error } -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} +func NewArchiver(ms stream.MediaStreamer, ds model.DataStore, shares Share, artwork artwork.Artwork) Archiver { + return &archiver{ds: ds, ms: ms, shares: shares, artwork: artwork} } type archiver struct { ds model.DataStore ms stream.MediaStreamer - decider stream.TranscodeDecider shares Share artwork artwork.Artwork } @@ -79,9 +78,8 @@ 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 { - 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) { + file := a.albumFilename(mf, format, isMultiDisc, folder) + if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, 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 @@ -206,9 +204,8 @@ func (a *archiver) zipMediaFiles(ctx context.Context, id, name string, format st zippedMfs := make(model.MediaFiles, len(mfs)) for idx, mf := range mfs { - 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) { + file := a.playlistFilename(mf, format, idx) + if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, 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() @@ -254,14 +251,7 @@ 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) 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 { +func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.MediaFile, format string, bitrate int, filename string) error { path := mf.AbsolutePath() // Open the source before writing the zip entry header so a rejection @@ -269,13 +259,13 @@ func (a *archiver) addFileToZip(ctx context.Context, z *zip.Writer, mf model.Med // archive. var r io.ReadCloser var err error - if req.Format != "raw" { - r, err = a.ms.NewStream(ctx, &mf, req) + if format != "raw" && format != "" { + r, err = a.ms.NewStream(ctx, &mf, stream.Request{Format: format, BitRate: bitrate}) } else { r, err = os.Open(path) } if err != nil { - log.Error(ctx, "Error opening file for zipping", "file", path, "format", req.Format, err) + log.Error(ctx, "Error opening file for zipping", "file", path, "format", format, err) return err } defer func() { diff --git a/core/archiver_test.go b/core/archiver_test.go index 178d1b6b9..4e00ce78c 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -26,7 +26,6 @@ var _ = Describe("Archiver", func() { var ( arch core.Archiver ms *mockMediaStreamer - dc *fakeDecider ds *mockDataStore sh *mockShare ca *mockCoverArt @@ -34,11 +33,10 @@ var _ = Describe("Archiver", func() { BeforeEach(func() { ms = &mockMediaStreamer{} - dc = &fakeDecider{} sh = &mockShare{} ds = &mockDataStore{} ca = &mockCoverArt{images: map[string][]byte{}} - arch = core.NewArchiver(ms, dc, ds, sh, ca) + arch = core.NewArchiver(ms, ds, sh, ca) }) Context("ZipAlbum", func() { @@ -68,23 +66,6 @@ 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() { @@ -315,30 +296,6 @@ 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"}}, @@ -614,19 +571,6 @@ 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/core/artwork/folders_artist.go b/core/artwork/folders_artist.go index 3403935db..40972f71d 100644 --- a/core/artwork/folders_artist.go +++ b/core/artwork/folders_artist.go @@ -30,7 +30,7 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p if libFS == nil { return nil, "", fmt.Errorf("artist folder lookup unavailable") } - rel, err := filepath.Rel(libPath, artistFolder) + rel, err := utils.RelPath(libPath, artistFolder) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return nil, "", fmt.Errorf(`artist folder '%s' is outside library '%s'`, artistFolder, libPath) } diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index fb07332fe..40baa2495 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -374,11 +374,8 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso } } - 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()"}) + albumIDs, err := r.ds.Playlist().Tracks(ctx, pl.ID, false). + 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 2a36531bb..da144d8e2 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -707,16 +707,6 @@ 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 4be99f92e..6122f88f2 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -4,11 +4,11 @@ import ( "bytes" "cmp" "context" + "errors" "fmt" "io" "math" "math/rand/v2" - "runtime/debug" "sync" "time" @@ -28,6 +28,10 @@ const ( // giveUpAfter bounds the retry budget from enqueue; past it the item settles and only an // explicit reprocess retries it. giveUpAfter = 12 * time.Hour + // itemTimeout must outlast a slow but healthy item: each image agent's call plus its download, in turn. + itemTimeout = 3 * time.Minute + // shutdownGrace lets cancelled work unwind before Run returns and the DB closes, without waiting on a stuck item. + shutdownGrace = 5 * time.Second ) // drainPool drains one class of work with its own slot budget, so a blocking kind cannot @@ -52,8 +56,13 @@ type Worker struct { gatesMu sync.Mutex gates map[string]*extGate + + // busy holds the itemKeys still being acquired, including ones a drain stopped waiting for. + busy sync.Map } +type itemKey struct{ kind, id, imageType string } + func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg ffmpeg.FFmpeg, broker events.Broker, imgCache cache.FileCache) *Worker { w := &Worker{ proc: &processor{ds: ds, store: store}, @@ -244,10 +253,56 @@ func (w *Worker) broadcastRefresh(ctx context.Context, found []model.ArtworkQueu func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outcome, *acquired) { item.ImageType = cmp.Or(item.ImageType, model.ImageTypePrimary) - trace := &ChainTrace{} - ctx = withTrace(ctx, trace) - out, got, retryIn := w.safeAcquire(ctx, item) + ctx = withTrace(ctx, &ChainTrace{}) + out, got, retryIn := w.acquireWithTimeout(ctx, item) + if ctx.Err() != nil { + return outcomeFailed, nil // shutting down: leave the row for the next run + } + w.settle(ctx, item, out, retryIn) + return out, got +} +// acquireWithTimeout stops waiting on a stuck item, which Go can't stop, and never runs one item twice at once. +func (w *Worker) acquireWithTimeout(ctx context.Context, item model.ArtworkQueueItem) (outcome, *acquired, time.Duration) { + key := itemKey{item.ItemKind, item.ItemID, item.ImageType} + if _, running := w.busy.LoadOrStore(key, struct{}{}); running { + log.Debug(ctx, "Artwork: Item still running from an earlier drain", "kind", item.ItemKind, "id", item.ItemID) + traceStage(ctx, "worker", errors.New("previous attempt still running")) + return outcomeFailed, nil, 0 + } + type result struct { + out outcome + got *acquired + retryIn time.Duration + } + done := make(chan result, 1) + go func() { + // The deadline stops work that honors ctx; the select below covers work that doesn't. + actx, cancel := context.WithTimeout(ctx, itemTimeout) + defer cancel() + out, got, retryIn := w.proc.acquire(actx, item) + // Cleared before the send, so the next drain never sees a finished item as busy. + w.busy.Delete(key) + done <- result{out, got, retryIn} + }() + select { + case r := <-done: + return r.out, r.got, r.retryIn + case <-ctx.Done(): + select { + case <-done: + case <-time.After(shutdownGrace): + } + return outcomeFailed, nil, 0 + case <-time.After(itemTimeout): + log.Error(ctx, "Artwork: Item timed out, moving on", "kind", item.ItemKind, "id", item.ItemID, "timeout", itemTimeout) + traceStage(ctx, "worker", fmt.Errorf("timed out after %s", itemTimeout)) + return outcomeFailed, nil, 0 + } +} + +// settle deletes or reschedules the queue row for an acquire outcome. +func (w *Worker) settle(ctx context.Context, item model.ArtworkQueueItem, out outcome, retryIn time.Duration) { queue := w.proc.ds.ArtworkQueue() switch out { case outcomeFound, outcomeAbsent: @@ -258,7 +313,7 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc } case outcomeFoundStale, outcomeFailed: retryAt := time.Now().Add(retryDelay(item.Attempts, retryIn)) - encoded := trace.encode("") + encoded := traceFrom(ctx).encode("") if retryAt.Before(item.EnqueuedAt.Add(giveUpAfter)) { // A mid-flight re-enqueue reset retry_at; stale backoff must not stomp its // fresh, immediate eligibility. @@ -285,21 +340,6 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc log.Warn(ctx, "Artwork: Could not remove exhausted queue item", "kind", item.ItemKind, "id", item.ItemID, err) } } - 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 diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 80ca68bc3..a6c07b763 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -142,18 +142,6 @@ 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()) @@ -290,36 +278,6 @@ 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"}}) diff --git a/core/artwork/worker_timing_test.go b/core/artwork/worker_timing_test.go index 63f12d03c..7896f6f09 100644 --- a/core/artwork/worker_timing_test.go +++ b/core/artwork/worker_timing_test.go @@ -1,13 +1,17 @@ package artwork import ( + "context" "errors" "io" "testing" "testing/synctest" "time" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/gomega" ) @@ -113,3 +117,109 @@ func TestArtworkGatePerAgentBreakerIsolation(t *testing.T) { g.Expect(aCalls).To(Equal(1)) }) } + +// newStuckItemWorker queues one artist whose agent blocks until the returned channel is closed. +func newStuckItemWorker(t *testing.T) (*Worker, *tests.MockArtworkQueueRepo, *fakeImageAgent, chan struct{}) { + t.Cleanup(configtest.SetupConfig()) + conf.Server.ArtistArtPriority = "external" + conf.Server.DevArtworkExternalMaxRPS = 1000 + + block := make(chan struct{}) + agent := &fakeImageAgent{name: "stuckAgent", block: block} + ag := imageAgents(agent) + artists := tests.CreateMockArtistRepo() + artists.SetData(model.Artists{{ID: "ar1", Name: "Artist"}}) + queue := tests.CreateMockArtworkQueueRepo() + ds := &tests.MockDataStore{MockedArtist: artists, MockedFolder: &fakeFolderRepo{}, MockedArtwork: tests.CreateMockArtworkRepo(), MockedArtworkQueue: queue} + w := NewWorker(ds, NewImageStore(t.TempDir()), ag, tests.NewMockFFmpeg(""), &fakeEventBroker{}, nil) + if err := queue.Enqueue(t.Context(), model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar1"}); err != nil { + t.Fatal(err) + } + return w, queue, agent, block +} + +// Go can't stop a stuck item, so the drain must stop waiting for it, or the whole pool stalls. +func TestArtworkDrainMovesOnFromAStuckItem(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + g := NewWithT(t) + w, queue, agent, block := newStuckItemWorker(t) + ctx := t.Context() + + start := time.Now() + n, err := w.drain(ctx, 1) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(n).To(Equal(1)) + g.Expect(time.Since(start)).To(Equal(itemTimeout), "the drain stops waiting once the item times out") + row := findQueued(queue, "ar", "ar1") + g.Expect(row).ToNot(BeNil(), "a timed-out item is rescheduled, not dropped") + g.Expect(row.Attempts).To(Equal(1)) + g.Expect(row.RetryAt).To(BeTemporally(">", time.Now())) + g.Expect(row.Trace).To(ContainSubstring("timed out")) + + // Due again while the first run is still stuck: it must not start a second one. + time.Sleep(time.Until(row.RetryAt)) + start = time.Now() + _, err = w.drain(ctx, 1) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(time.Since(start)).To(BeZero(), "a still-running item is rescheduled without waiting") + row = findQueued(queue, "ar", "ar1") + g.Expect(row.Attempts).To(Equal(2)) + g.Expect(row.Trace).To(ContainSubstring("still running")) + + // Once the stuck run returns, the next attempt runs normally and settles the row. + close(block) + synctest.Wait() + time.Sleep(time.Until(row.RetryAt)) + _, err = w.drain(ctx, 1) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(findQueued(queue, "ar", "ar1")).To(BeNil()) + g.Expect(agent.artistCalls).To(Equal(2), "one stuck call and one retry, never two at once") + }) +} + +// Shutdown joins cancelled work that unwinds, so nothing touches the DB after Run returns. +func TestArtworkDrainShutdownWaitsForWorkToUnwind(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + g := NewWithT(t) + w, queue, _, block := newStuckItemWorker(t) + before := *findQueued(queue, "ar", "ar1") + ctx, cancel := context.WithCancel(t.Context()) + go func() { + time.Sleep(time.Second) + cancel() + time.Sleep(100 * time.Millisecond) + close(block) + }() + + start := time.Now() + _, err := w.drain(ctx, 1) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(time.Since(start)).To(Equal(time.Second+100*time.Millisecond), "the drain waits for the cancelled item to return") + _, running := w.busy.Load(itemKey{"ar", "ar1", model.ImageTypePrimary}) + g.Expect(running).To(BeFalse()) + g.Expect(*findQueued(queue, "ar", "ar1")).To(Equal(before), "the row is left for the next run") + }) +} + +// Shutdown must not wait on a stuck item for longer than the grace period. +func TestArtworkDrainShutdownDoesNotWaitOnAStuckItem(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + g := NewWithT(t) + w, queue, _, block := newStuckItemWorker(t) + before := *findQueued(queue, "ar", "ar1") + ctx, cancel := context.WithCancel(t.Context()) + go func() { + time.Sleep(time.Second) + cancel() + }() + + start := time.Now() + _, err := w.drain(ctx, 1) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(time.Since(start)).To(Equal(time.Second+shutdownGrace), "the drain gives up on the stuck item after the grace period") + g.Expect(*findQueued(queue, "ar", "ar1")).To(Equal(before), "the row is left for the next run") + + close(block) + synctest.Wait() + }) +} diff --git a/core/inspect.go b/core/inspect.go index c60459b88..01ec33760 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, lib model.Library, folderId string) (*InspectOutput, error) { +func Inspect(filePath string, libraryId int, folderId string) (*InspectOutput, error) { path, file := filepath.Split(filePath) s, err := storage.For(path) @@ -39,22 +39,12 @@ func Inspect(filePath string, lib model.Library, folderId string) (*InspectOutpu return nil, model.ErrNotFound } - md := metadata.New(scannerPath(lib, filePath), tag) + md := metadata.New(path, tag) result := &InspectOutput{ File: filePath, RawTags: tags[file].Tags, - MappedTags: new(md.ToMediaFile(lib, folderId)), + MappedTags: new(md.ToMediaFile(libraryId, 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 deleted file mode 100644 index 0ac90990c..000000000 --- a/core/inspect_test.go +++ /dev/null @@ -1,45 +0,0 @@ -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 f1153da26..628ee4b7b 100644 --- a/core/library.go +++ b/core/library.go @@ -2,12 +2,10 @@ package core import ( "context" - "errors" "fmt" "io/fs" "os" "path/filepath" - "slices" "strconv" "strings" "time" @@ -17,7 +15,6 @@ 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" @@ -203,22 +200,23 @@ 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) } - 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) + // 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 || pidChanged) && r.scanner != nil { - go r.triggerScan(ctx, lib, "updated") + if r.scanner != nil { + go r.triggerScan(ctx, lib, "updated") + } } // Send library refresh event to all clients @@ -327,15 +325,6 @@ 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} } @@ -343,11 +332,6 @@ 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) { @@ -415,27 +399,11 @@ 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: 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 { + warnings, err := r.scanner.ScanAll(ctx, false) // Quick scan for new library + 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 e6ebb1974..5402eac22 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -322,37 +322,6 @@ 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() { @@ -710,48 +679,6 @@ 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 1d2ff5df4..4a78a7f3f 100644 --- a/core/metrics/insights.go +++ b/core/metrics/insights.go @@ -10,7 +10,6 @@ import ( "path/filepath" "runtime" "runtime/debug" - "slices" "strings" "sync" "sync/atomic" @@ -270,13 +269,9 @@ func (c *insightsCollector) collect(ctx context.Context) []byte { if err != nil { log.Trace(ctx, "Error reading radios count", err) } - libs, err := c.ds.Library().GetAll(ctx) + data.Library.Libraries, err = c.ds.Library().CountAll(ctx) if err != nil { - 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 + log.Trace(ctx, "Error reading libraries count", err) } 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 b5991b095..658bd92dc 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 := model.NewLibraryMatcher(libs) - lib, ok := matcher.FindLibrary(dir) + matcher := 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 ab95b8850..9ca1c9b6c 100644 --- a/core/playlists/parse_m3u.go +++ b/core/playlists/parse_m3u.go @@ -1,11 +1,13 @@ package playlists import ( + "cmp" "context" "fmt" "io" "net/url" "path/filepath" + "slices" "strings" "time" @@ -13,6 +15,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/slice" "golang.org/x/text/unicode/norm" ) @@ -146,7 +149,7 @@ func (r pathResolution) ToQualifiedString() (string, error) { if !r.valid { return "", fmt.Errorf("invalid path resolution") } - relativePath, err := filepath.Rel(r.libraryPath, r.absolutePath) + relativePath, err := utils.RelPath(r.libraryPath, r.absolutePath) if err != nil { return "", err } @@ -154,9 +157,61 @@ 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 *model.LibraryMatcher + matcher *libraryMatcher } // newPathResolver creates a pathResolver with libraries loaded from the datastore. @@ -165,7 +220,7 @@ func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, er if err != nil { return nil, err } - matcher := model.NewLibraryMatcher(libs) + matcher := newLibraryMatcher(libs) return &pathResolver{matcher: matcher}, nil } @@ -192,14 +247,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 { - lib, ok := r.matcher.FindLibrary(absolutePath) - if !ok { + libID, libPath := r.matcher.findLibraryForPath(absolutePath) + if libID == 0 { return pathResolution{valid: false} } return pathResolution{ absolutePath: absolutePath, - libraryPath: filepath.Clean(lib.Path), - libraryID: lib.ID, + libraryPath: libPath, + libraryID: libID, valid: true, } } @@ -234,7 +289,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 *model.LibraryMatcher, owner model.User) string { +func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher, owner model.User) string { value = strings.TrimSpace(value) if value == "" { return "" @@ -254,7 +309,7 @@ func resolveImageURL(value string, folder *model.Folder, matcher *model.LibraryM 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 ced6c2b16..b6a3a96f9 100644 --- a/core/playlists/parse_m3u_test.go +++ b/core/playlists/parse_m3u_test.go @@ -9,6 +9,187 @@ 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/core/playlists/playlists.go b/core/playlists/playlists.go index c9bc03b97..8e7ccf93c 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -15,6 +15,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils" ) type Playlists interface { @@ -77,7 +78,7 @@ func InPath(folder model.Folder) bool { if conf.Server.PlaylistsPath == "" { return true } - rel, _ := filepath.Rel(folder.LibraryPath, folder.AbsolutePath()) + rel, _ := utils.RelPath(folder.LibraryPath, folder.AbsolutePath()) for path := range strings.SplitSeq(conf.Server.PlaylistsPath, string(filepath.ListSeparator)) { if match, _ := doublestar.Match(path, rel); match { return true diff --git a/core/storage/local/local.go b/core/storage/local/local.go index e2ce00a1b..686838565 100644 --- a/core/storage/local/local.go +++ b/core/storage/local/local.go @@ -76,17 +76,6 @@ func (lfs *localFS) ResolveSymlink(name string) (string, error) { return filepath.EvalSymlinks(filepath.Join(lfs.root, filepath.FromSlash(name))) } -// ReadLink and Lstat implement fs.ReadLinkFS, so callers can detect symlinks without following them. -var _ fs.ReadLinkFS = (*localFS)(nil) - -func (lfs *localFS) ReadLink(name string) (string, error) { - return fs.ReadLink(lfs.FS, name) -} - -func (lfs *localFS) Lstat(name string) (fs.FileInfo, error) { - return fs.Lstat(lfs.FS, name) -} - func (lfs *localFS) ReadTags(path ...string) (map[string]metadata.Info, error) { res, err := lfs.extractor.Parse(path...) if err != nil { diff --git a/db/migrations/20260929221042_add_library_pid_columns.sql b/db/migrations/20260929221042_add_library_pid_columns.sql deleted file mode 100644 index 487512287..000000000 --- a/db/migrations/20260929221042_add_library_pid_columns.sql +++ /dev/null @@ -1,18 +0,0 @@ --- +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 e80d22c89..1e33222ac 100644 --- a/model/library.go +++ b/model/library.go @@ -1,13 +1,10 @@ package model import ( - "cmp" "context" - "strings" "time" "github.com/deluan/rest" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/utils/slice" ) @@ -30,38 +27,6 @@ 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 ( @@ -94,8 +59,6 @@ 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 deleted file mode 100644 index 83af96f9f..000000000 --- a/model/library_matcher.go +++ /dev/null @@ -1,57 +0,0 @@ -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 deleted file mode 100644 index 09e6f7e33..000000000 --- a/model/library_matcher_test.go +++ /dev/null @@ -1,91 +0,0 @@ -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 deleted file mode 100644 index 4e799e13f..000000000 --- a/model/library_test.go +++ /dev/null @@ -1,73 +0,0 @@ -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 2135824d2..6d12feba9 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -8,14 +8,15 @@ 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(lib model.Library, folderID string) model.MediaFile { +func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf := model.MediaFile{ - LibraryID: lib.ID, + LibraryID: libID, FolderID: folderID, Tags: maps.Clone(md.tags), } @@ -83,9 +84,8 @@ func (md Metadata) ToMediaFile(lib model.Library, folderID string) model.MediaFi mf.AlbumArtist = md.mapDisplayAlbumArtist(mf) // Persistent IDs - pid := lib.EffectivePID() - mf.PID = md.trackPID(mf, pid) - mf.AlbumID = md.albumID(mf, pid.Album) + mf.PID = md.trackPID(mf) + mf.AlbumID = md.albumID(mf, conf.Server.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 c19398841..baaf8fab5 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -30,23 +30,9 @@ var _ = Describe("ToMediaFile", func() { var toMediaFile = func(tags model.RawTags) model.MediaFile { props.Tags = tags md = metadata.New("filepath", props) - return md.ToMediaFile(model.Library{ID: 1}, "folderID") + return md.ToMediaFile(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 db652fb8b..ec66e12b9 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(model.Library{ID: 1}, "folderID") + return md.ToMediaFile(1, "folderID") } Describe("ARTIST(S) tags", func() { diff --git a/model/metadata/metadata_test.go b/model/metadata/metadata_test.go index a1a675006..c84d93981 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(model.Library{}, "0") + return md.ToMediaFile(0, "0") } DescribeTable("Gain", diff --git a/model/metadata/persistent_ids.go b/model/metadata/persistent_ids.go index b66ce824a..db315dc6b 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,13 +22,12 @@ 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. albumSpec is the album PID -// spec used to resolve the `albumid` attribute. +// skipped and the function looks for the next field. // // 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, albumSpec string, prependLibId bool, hash hashFunc) string { +func computePID(mf model.MediaFile, md Metadata, spec string, prependLibId bool, hash hashFunc) string { switch spec { case "track_legacy": return legacyTrackID(mf, prependLibId) @@ -42,7 +41,7 @@ func computePID(mf model.MediaFile, md Metadata, spec, albumSpec string, prepend values := make([]string, len(attributes)) hasValue := false for i, attr := range attributes { - v := getPIDAttr(mf, md, attr, prependLibId, spec, albumSpec, hash) + v := getPIDAttr(mf, md, attr, prependLibId, spec, hash) if v != "" { hasValue = true } @@ -59,15 +58,15 @@ func computePID(mf model.MediaFile, md Metadata, spec, albumSpec string, prepend return hash(pid) } -func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec, albumSpec string, hash hashFunc) string { +func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, spec string, hash hashFunc) string { attr = strings.TrimSpace(strings.ToLower(attr)) switch attr { case "albumid": - if spec == albumSpec { + if spec == conf.Server.PID.Album { log.Error("Recursive PID definition detected, ignoring `albumid`", "spec", spec) return "" } - return computePID(mf, md, albumSpec, albumSpec, prependLibId, hash) + return computePID(mf, md, conf.Server.PID.Album, prependLibId, hash) case "folder": return filepath.Dir(mf.Path) case "albumartistid": @@ -80,50 +79,18 @@ func getPIDAttr(mf model.MediaFile, md Metadata, attr string, prependLibId bool, return md.String(model.TagName(attr)) } -// 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) trackPID(mf model.MediaFile) string { + return computePID(mf, md, conf.Server.PID.Track, true, id.NewHash) } func (md Metadata) albumID(mf model.MediaFile, pidConf string) string { - return computePID(mf, md, pidConf, pidConf, true, id.NewHash) + return computePID(mf, md, 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 9f6eaf1f4..8e38bbd42 100644 --- a/model/metadata/persistent_ids_test.go +++ b/model/metadata/persistent_ids_test.go @@ -3,7 +3,8 @@ package metadata import ( "strings" - "github.com/navidrome/navidrome/consts" + "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" @@ -12,18 +13,16 @@ import ( var _ = Describe("getPID", func() { var ( - md Metadata - mf model.MediaFile - sum hashFunc - albumSpec string + md Metadata + mf model.MediaFile + sum hashFunc ) getPID := func(mf model.MediaFile, md Metadata, spec string, prependLibId bool) string { - return computePID(mf, md, spec, albumSpec, prependLibId, sum) + return computePID(mf, md, spec, prependLibId, sum) } BeforeEach(func() { sum = func(s ...string) string { return "(" + strings.Join(s, ",") + ")" } - albumSpec = consts.DefaultAlbumPID }) Context("attributes are tags", func() { @@ -67,7 +66,8 @@ var _ = Describe("getPID", func() { Context("calculated attributes", func() { BeforeEach(func() { - albumSpec = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" + DeferCleanup(configtest.SetupConfig()) + conf.Server.PID.Album = "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 - albumSpec = "albumid,album,albumversion,releasedate" - spec := albumSpec + conf.Server.PID.Album = "albumid,album,albumversion,releasedate" + spec := conf.Server.PID.Album md.tags = map[model.TagName][]string{ "album": {"Album Name"}, "albumversion": {"Version"}, @@ -205,7 +205,8 @@ var _ = Describe("getPID", func() { }) When("prependLibId is true with nested albumid", func() { It("should handle nested albumid calls correctly", func() { - albumSpec = "album" + DeferCleanup(configtest.SetupConfig()) + conf.Server.PID.Album = "album" spec := "albumid" md.tags = map[model.TagName][]string{"album": {"Test Album"}} mf.AlbumArtist = "Test Artist" @@ -305,34 +306,3 @@ 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 d22c3d0d6..36c9007fb 100644 --- a/model/scanner.go +++ b/model/scanner.go @@ -2,15 +2,12 @@ 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 5a8168754..ce7d2f37b 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -195,28 +195,6 @@ 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 91e54e5d4..e582c3f2f 100644 --- a/model/tag_mappings_test.go +++ b/model/tag_mappings_test.go @@ -192,22 +192,3 @@ 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/album_repository.go b/persistence/album_repository.go index 6a940a320..8e7662b8f 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -131,7 +131,6 @@ var albumFilters = sync.OnceValue(func() map[string]filterFunc { "recently_played": recentlyPlayedFilter, "starred": annotationBoolFilter("starred"), "has_rating": annotationBoolFilter("rating"), - "played": annotationBoolFilter("play_count"), "missing": booleanFilter, "genre_id": genreFilter(AlbumGenres), "role_total_id": allRolesFilter, diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 0235805c0..7652e703d 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -417,80 +417,6 @@ var _ = Describe("AlbumRepository", func() { } }) }) - - Describe("played", func() { - var playedAlbum model.Album - - BeforeEach(func() { - playedAlbum = model.Album{ID: "played-album", Name: "Played Album", LibraryID: 1, SongCount: 1} - Expect(albumRepo.Put(ctx, &playedAlbum)).To(Succeed()) - Expect(albumRepo.IncPlayCount(ctx, playedAlbum.ID, time.Now())).To(Succeed()) - }) - - AfterEach(func() { - _, _ = albumRepo.executeSQL(ctx, squirrel.Delete("annotation").Where(squirrel.Eq{"item_id": playedAlbum.ID})) - _, _ = albumRepo.executeSQL(ctx, squirrel.Delete("album").Where(squirrel.Eq{"id": playedAlbum.ID})) - }) - - It("false includes items without annotations", func() { - res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ - Filters: map[string]any{"played": "false"}, - }) - Expect(err).ToNot(HaveOccurred()) - albums := res - - var found bool - for _, a := range albums { - if a.ID == albumWithoutAnnotation.ID { - found = true - break - } - } - Expect(found).To(BeTrue(), "Album without annotation should be included in played=false filter") - }) - - It("true excludes items without annotations", func() { - res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ - Filters: map[string]any{"played": "true"}, - }) - Expect(err).ToNot(HaveOccurred()) - albums := res - - for _, a := range albums { - Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID)) - } - }) - - It("true includes items with play count", func() { - res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ - Filters: map[string]any{"played": "true"}, - }) - Expect(err).ToNot(HaveOccurred()) - albums := res - - var found bool - for _, a := range albums { - if a.ID == playedAlbum.ID { - found = true - Expect(a.PlayCount).To(BeNumerically(">", 0)) - break - } - } - Expect(found).To(BeTrue(), "Album with play count should be included in played=true filter") - }) - - It("false excludes items with play count", func() { - res, err := albumRepo.ReadAll(ctx, rest.QueryOptions{ - Filters: map[string]any{"played": "false"}, - }) - Expect(err).ToNot(HaveOccurred()) - albums := res - - for _, a := range albums { - Expect(a.ID).ToNot(Equal(playedAlbum.ID)) - } - }) - }) }) Describe("Album.PlayCount", func() { diff --git a/persistence/library_repository.go b/persistence/library_repository.go index 85da65cbc..bf6b8995e 100644 --- a/persistence/library_repository.go +++ b/persistence/library_repository.go @@ -93,8 +93,6 @@ 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}) @@ -178,15 +176,6 @@ 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 bf485a06f..0ff470861 100644 --- a/persistence/library_repository_test.go +++ b/persistence/library_repository_test.go @@ -270,38 +270,6 @@ 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/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index 84fdffa4d..ee2794454 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -352,15 +352,6 @@ func GetDBXBuilder() *dbx.DB { return dbx.NewFromDB(db.Db(), db.Dialect) } -// storedRows reads playlist_tracks without joining media_file, so it also shows rows pointing to no track. -func storedRows(playlistID string) []string { - GinkgoHelper() - var rows []string - Expect(GetDBXBuilder().NewQuery("SELECT id || ':' || media_file_id FROM playlist_tracks WHERE playlist_id = {:id} ORDER BY id"). - Bind(dbx.Params{"id": playlistID}).Column(&rows)).To(Succeed()) - return rows -} - // collectCursor takes the cursor's underlying func type so the named cursor types // (model.AlbumCursor, ...) infer T. func collectCursor[T any](cursor func(func(T, error) bool), err error) []T { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 1a1b05a5c..41b75266c 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -281,7 +281,7 @@ func (r *playlistRepository) updatePlaylist(ctx context.Context, playlistId stri } // addTracks is the only path that writes playlist_tracks rows (smart playlists aside), so it owns -// the library and existence check: every caller, including a full replace through Put, goes through it. +// the library check: every caller, including a full replace through Put, goes through it. func (r *playlistRepository) addTracks(ctx context.Context, playlistId string, startingPos int, mediaFileIds []string) (int, error) { mediaFileIds, err := r.keepAccessible(ctx, mediaFileIds) if err != nil { @@ -305,9 +305,12 @@ func (r *playlistRepository) addTracks(ctx context.Context, playlistId string, s return len(mediaFileIds), r.refreshCounters(ctx, &model.Playlist{ID: playlistId}) } -// keepAccessible drops ids the caller cannot read or that match no track, preserving order and duplicates. -// Chunked because callers pass unbounded id lists (M3U import), well past SQLITE_MAX_VARIABLE_NUMBER. +// keepAccessible drops ids the caller cannot read, preserving order and duplicates. Chunked +// because callers pass unbounded id lists (M3U import), well past SQLITE_MAX_VARIABLE_NUMBER. func (r *playlistRepository) keepAccessible(ctx context.Context, mediaFileIds []string) ([]string, error) { + if visible, err := r.visibleLibraryIDs(ctx); err == nil && r.userSeesAllLibraries(ctx, visible) { + return mediaFileIds, nil + } accessible := make(map[string]struct{}, len(mediaFileIds)) for chunk := range slices.Chunk(slice.Unique(mediaFileIds), 200) { sq := r.applyLibraryFilter(ctx, Select("id").From("media_file").Where(Eq{"id": chunk}), "media_file") diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index b7be0588b..93d37c928 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -469,26 +469,6 @@ var _ = Describe("PlaylistRepository", func() { }) }) - Describe("removeOrphans", func() { - It("deletes rows pointing to no track and renumbers the rest", func() { - pls := model.Playlist{Name: "Orphans", OwnerID: "userid"} - Expect(repo.Put(ctx, &pls)).To(Succeed()) - DeferCleanup(func() { Expect(repo.Delete(ctx, pls.ID)).To(Succeed()) }) - // Rows as an older version stored them, with an unknown id between real tracks - _, err := GetDBXBuilder().NewQuery(`INSERT INTO playlist_tracks (playlist_id, media_file_id, id) - VALUES ({:id}, '1001', 1), ({:id}, 'no-such-track', 2), ({:id}, '1002', 3)`). - Bind(dbx.Params{"id": pls.ID}).Execute() - Expect(err).ToNot(HaveOccurred()) - - Expect(repo.(*playlistRepository).removeOrphans(ctx)).To(Succeed()) - - Expect(storedRows(pls.ID)).To(Equal([]string{"1:1001", "2:1002"})) - got, err := repo.Get(ctx, pls.ID) - Expect(err).ToNot(HaveOccurred()) - Expect(got.SongCount).To(Equal(2)) - }) - }) - // Exists is ctx-sensitive through userFilter, so callers that only want "does it still exist" // -- the public image route serving a share -- must elevate, or a private playlist looks gone. Describe("Exists visibility", func() { diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 587410305..392446cef 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -213,7 +213,7 @@ func (r *playlistTrackRepository) Insert(ctx context.Context, mediaFileIds []str if err != nil || inserted == n { return inserted, err } - // The shift above reserved a slot per requested id, so ids dropped by keepAccessible + // The shift above reserved a slot per requested id, so ids dropped by the library filter // leave a hole. Close it. return inserted, r.playlistRepo.renumber(ctx, r.playlistId) } diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index 29911ebd4..119c57116 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -83,14 +83,12 @@ var _ = Describe("PlaylistTrackRepository", func() { Describe("Insert", func() { var tracks model.PlaylistTrackRepository - var plsID string BeforeEach(func() { plsRepo := NewPlaylistRepository(GetDBXBuilder()) pls := model.Playlist{Name: "Insert", OwnerID: "userid", OwnerName: "userid"} Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) DeferCleanup(func() { Expect(plsRepo.Delete(ctx, pls.ID)).To(Succeed()) }) - plsID = pls.ID tracks = plsRepo.Tracks(ctx, pls.ID, false) Expect(tracks.Add(ctx, []string{songDayInALife.ID, songRadioactivity.ID})).To(Equal(2)) @@ -125,40 +123,6 @@ var _ = Describe("PlaylistTrackRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect([]string{all[0].ID, all[1].ID, all[2].ID}).To(Equal([]string{"1", "2", "3"})) }) - - It("drops ids that match no track, keeping positions contiguous", func() { - Expect(tracks.Insert(ctx, []string{"no-such-track", songComeTogether.ID}, 1)).To(Equal(1)) - - Expect(storedRows(plsID)).To(Equal([]string{ - "1:" + songComeTogether.ID, "2:" + songDayInALife.ID, "3:" + songRadioactivity.ID, - })) - }) - }) - - Describe("Add", func() { - var tracks model.PlaylistTrackRepository - var plsID string - - BeforeEach(func() { - plsRepo := NewPlaylistRepository(GetDBXBuilder()) - pls := model.Playlist{Name: "Add", OwnerID: "userid", OwnerName: "userid"} - Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) - DeferCleanup(func() { Expect(plsRepo.Delete(ctx, pls.ID)).To(Succeed()) }) - plsID = pls.ID - tracks = plsRepo.Tracks(ctx, pls.ID, false) - }) - - It("drops ids that match no track and counts only the tracks added", func() { - Expect(tracks.Add(ctx, []string{songDayInALife.ID, "no-such-track", songComeTogether.ID, "!!not-an-id!!"})).To(Equal(2)) - - Expect(storedRows(plsID)).To(Equal([]string{"1:" + songDayInALife.ID, "2:" + songComeTogether.ID})) - }) - - It("adds nothing when no id matches a track", func() { - Expect(tracks.Add(ctx, []string{"no-such-track", "!!not-an-id!!"})).To(BeZero()) - - Expect(storedRows(plsID)).To(BeEmpty()) - }) }) Describe("Reorder", func() { diff --git a/persistence/sql_annotations_test.go b/persistence/sql_annotations_test.go index 6761331d9..ec28c610a 100644 --- a/persistence/sql_annotations_test.go +++ b/persistence/sql_annotations_test.go @@ -91,8 +91,6 @@ var _ = Describe("Annotation Filters", func() { Entry("starred=false", "starred", "false", "COALESCE(starred, 0) = 0", []any(nil)), Entry("starred=True (case insensitive)", "starred", "True", "COALESCE(starred, 0) > 0", []any(nil)), Entry("rating=true", "rating", "true", "COALESCE(rating, 0) > 0", []any(nil)), - Entry("play_count=true", "play_count", "true", "COALESCE(play_count, 0) > 0", []any(nil)), - Entry("play_count=false", "play_count", "false", "COALESCE(play_count, 0) = 0", []any(nil)), ) It("returns nil if value is not a string", func() { diff --git a/resources/hosting/elfhosted.svg b/resources/hosting/elfhosted.svg deleted file mode 100644 index 86bc86df9..000000000 --- a/resources/hosting/elfhosted.svg +++ /dev/null @@ -1,77 +0,0 @@ -Deploy on ElfHosted - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -Deploy on ElfHosted diff --git a/resources/hosting/pikapods.svg b/resources/hosting/pikapods.svg deleted file mode 100644 index a3865feea..000000000 --- a/resources/hosting/pikapods.svg +++ /dev/null @@ -1,25 +0,0 @@ -Run on PikaPods - -Run on PikaPods Button - - - - - - - - - - - - - - - - - - diff --git a/resources/hosting/zenith.svg b/resources/hosting/zenith.svg deleted file mode 100644 index 0b5b11539..000000000 --- a/resources/hosting/zenith.svg +++ /dev/null @@ -1,16 +0,0 @@ -Deploy with Zenith - - - - - - - - - - - - - - - diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index 238919259..f0d8a0e06 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -328,22 +328,11 @@ "totalDuration": "Duração", "defaultNewUsers": "Padrão para Novos Usuários", "createdAt": "Data de Criação", - "updatedAt": "Últ. Atualização", - "pidAlbum": "Agrupamento de álbuns", - "pidTrack": "Identificação das faixas" + "updatedAt": "Últ. Atualização" }, "sections": { "basic": "Informações Básicas", - "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" + "statistics": "Estatísticas" }, "actions": { "scan": "Scanear Biblioteca", @@ -373,9 +362,7 @@ "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", - "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." + "noLibrariesAssigned": "Nenhuma biblioteca atribuída a este usuário" } }, "plugin": { diff --git a/scanner/controller.go b/scanner/controller.go index 1b13c1846..5eed6c58d 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -25,7 +25,7 @@ import ( ) var ( - ErrAlreadyScanning = model.ErrAlreadyScanning + ErrAlreadyScanning = errors.New("already scanning") ) 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, will resume an interrupted full scan, -// or will rescan a library in full because its PID config changed, in one of the included libraries. +// EffectiveFullScan reports whether a scan was requested as full or will resume an interrupted +// full scan 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 || library.NeedsPIDRescan() + return library.FullScanInProgress }) } diff --git a/scanner/controller_test.go b/scanner/controller_test.go index 974540e32..bdcb99eda 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -2,7 +2,6 @@ package scanner_test import ( "context" - "time" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -71,21 +70,14 @@ 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, 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}, + {ID: 1, FullScanInProgress: true}, + {ID: 2}, }) 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 4edaecacd..7b6a6b097 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -40,7 +40,6 @@ 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) @@ -52,13 +51,12 @@ 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) - prevAlbumPIDConf string // Album PID spec of the last finished scan, only when it differs from the current one - 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) + lock sync.Mutex + numFolders atomic.Int64 } func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, fullScan bool, targetFolders []string) (*scanJob, error) { @@ -79,32 +77,16 @@ 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, - prevAlbumPIDConf: prevAlbumPIDConf, + lib: lib, + fs: fsys, + lastUpdates: lastUpdates, + targetFolders: targetFolders, }, nil } @@ -140,13 +122,14 @@ 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 - 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 + prevAlbumPIDConf string + imageChanges *imageChangeCollector } func (p *phaseFolders) description() string { @@ -155,6 +138,12 @@ 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 @@ -184,7 +173,7 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { // Check if folder is outdated if folder.isOutdated() { - if !folder.job.lib.FullScanInProgress { + if !p.state.fullScan { // 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() { @@ -250,7 +239,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 || entry.job.lib.FullScanInProgress { + if !foundInDB || p.state.fullScan { filesToImport[fullPath] = dbTrack } else { info, err := af.Info() @@ -300,18 +289,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, entry.id) + track := md.ToMediaFile(entry.job.lib.ID, 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 := track.AlbumID + prevAlbumID := "" if prev := toImport[filePath]; prev != nil { prevAlbumID = prev.AlbumID - } else if entry.job.prevAlbumPIDConf != "" { - prevAlbumID = md.AlbumID(track, entry.job.prevAlbumPIDConf) + } else { + prevAlbumID = md.AlbumID(track, p.prevAlbumPIDConf) } _, ok := entry.albumIDMap[track.AlbumID] if prevAlbumID != track.AlbumID && !ok { @@ -464,7 +453,7 @@ func (p *phaseFolders) persistFolder(ctx context.Context, tx model.DataStore, en if len(queueItems) > 0 { queue := tx.ArtworkQueue() enqueue := queue.Enqueue - if entry.job.lib.FullScanInProgress { + if p.state.fullScan { enqueue = queue.EnqueueIfMissing } if err := enqueue(ctx, queueItems...); err != nil { diff --git a/scanner/scanner.go b/scanner/scanner.go index 305c443f4..f156d15fd 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -4,15 +4,18 @@ 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" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/run" "github.com/navidrome/navidrome/utils/slice" ) @@ -30,7 +33,6 @@ 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) { @@ -47,17 +49,31 @@ func (s *scanState) sendWarning(msg string) { s.sendProgress(&ProgressInfo{Warning: msg}) } -func (s *scanState) markFailed(libID int) { - if s.failedLibs == nil { - s.failedLibs = map[int]bool{} - } - s.failedLibs[libID] = true -} - func (s *scanState) sendError(err error) { s.sendProgress(&ProgressInfo{Error: err.Error()}) } +// 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 := utils.RelPath(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 *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets []model.ScanTarget, progress chan<- *ProgressInfo) { startTime := time.Now() @@ -89,7 +105,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] }) for _, target := range targets { - folderPath := model.LibraryRelativePath(libPaths[target.LibraryID], target.FolderPath) + folderPath := libraryRelativePath(libPaths[target.LibraryID], target.FolderPath) if folderPath == "" { folderPath = "." } @@ -122,10 +138,6 @@ 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 @@ -204,13 +216,10 @@ func (s *scannerImpl) prepareLibrariesForScan(ctx context.Context, state *scanSt var successfulLibs []model.Library for _, lib := range state.libraries { - // 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 { + if lib.LastScanStartedAt.IsZero() { // 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 || pidRescan) + return tx.Library().ScanBegin(ctx, lib.ID, state.fullScan) }, "scanner: begin library scan") if err != nil { log.Error(ctx, "Scanner: Error marking scan start", "lib", lib.Name, err) @@ -332,12 +341,11 @@ 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) } - // 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 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) } 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 e8abb7c7d..0778bd6ec 100644 --- a/scanner/scanner_internal_test.go +++ b/scanner/scanner_internal_test.go @@ -4,6 +4,8 @@ package scanner import ( "context" "errors" + "os" + "path/filepath" "sync/atomic" ppl "github.com/google/go-pipeline/pkg/pipeline" @@ -11,6 +13,43 @@ 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 f6634c875..546baf756 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -835,170 +835,4 @@ 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/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index d39cad5f6..1864a6a44 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -43,13 +43,6 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- continue } - // A full walk never descends into symlinked folders when following is disabled, so a - // target reached through one (e.g. a watcher event for a new link) is skipped too. - if !conf.Server.Scanner.FollowSymlinks && isSymlinkedPath(job.fs, folderPath) { - log.Debug(ctx, "Scanner: Skipping symlinked target folder, following is disabled", "path", folderPath) - continue - } - // Create checker and push patterns from root to this folder checker := newIgnoreChecker(job.fs) err = checker.PushAllParents(ctx, folderPath) @@ -232,18 +225,6 @@ func isDirOrSymlinkToDir(fsys fs.FS, baseDir string, dirEnt fs.DirEntry) (bool, return fileInfo.IsDir(), nil } -// isSymlinkedPath returns true if folderPath, or any of its parent folders, is a symbolic link. -// It needs fsys to implement fs.ReadLinkFS, otherwise links are followed and never detected. -func isSymlinkedPath(fsys fs.FS, folderPath string) bool { - for p := path.Clean(folderPath); p != "." && p != "/"; p = path.Dir(p) { - info, err := fs.Lstat(fsys, p) - if err == nil && info.Mode()&fs.ModeSymlink != 0 { - return true - } - } - return false -} - const maxSymlinkHops = 40 // resolveEntryName returns the name to classify the entry by, and whether to diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 0c2aba3e0..43939e5c2 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -4,10 +4,8 @@ import ( "context" "fmt" "io/fs" - "maps" "os" "path/filepath" - "slices" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -262,44 +260,6 @@ var _ = Describe("walk_dir_tree", func() { // Folders not in targets should remain in lastUpdates Expect(job.lastUpdates).To(HaveKey(model.FolderID(job.lib, "OtherArtist/Album3"))) }) - - // #6292: a watcher event for a new folder symlink makes the link itself a scan target - Context("symlinked target folders (production local storage FS)", func() { - BeforeEach(func() { - libRoot := GinkgoT().TempDir() - Expect(os.MkdirAll(filepath.Join(libRoot, "Mozart", "Album1"), 0755)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(libRoot, "Mozart", "Album1", "track.mp3"), []byte("AUDIO"), 0600)).To(Succeed()) - Expect(os.Symlink("Mozart", filepath.Join(libRoot, "Wolfgang Amadeus Mozart"))).To(Succeed()) - job = &scanJob{fs: newLocalMusicFS(libRoot), lib: model.Library{Path: libRoot}} - }) - - walkTargets := func(targets ...string) map[string]*folderEntry { - results, err := walkDirTree(ctx, job, targets...) - Expect(err).ToNot(HaveOccurred()) - folders := map[string]*folderEntry{} - for folder := range results { - folders[folder.path] = folder - } - return folders - } - - DescribeTable("with FollowSymlinks disabled", - func(target string, expected ...string) { - conf.Server.Scanner.FollowSymlinks = false - Expect(slices.Collect(maps.Keys(walkTargets(target)))).To(ConsistOf(expected)) - }, - Entry("skips a target that is a symlink", "Wolfgang Amadeus Mozart"), - Entry("skips a target under a symlinked folder", "Wolfgang Amadeus Mozart/Album1"), - Entry("walks a regular target", "Mozart", "Mozart", "Mozart/Album1"), - ) - - It("walks a symlinked target when FollowSymlinks is enabled", func() { - conf.Server.Scanner.FollowSymlinks = true - folders := walkTargets("Wolfgang Amadeus Mozart") - Expect(folders).To(HaveKey("Wolfgang Amadeus Mozart/Album1")) - Expect(folders["Wolfgang Amadeus Mozart/Album1"].audioFiles).To(HaveKey("track.mp3")) - }) - }) }) }) @@ -473,8 +433,8 @@ var _ = Describe("walk_dir_tree", func() { }) // Regression for #5752: the production localFS must resolve file symlinks. - // fs.ReadLink-based resolution can't follow targets outside the library - // root, so full OS-level resolution is required. + // It wraps os.DirFS behind the fs.FS interface, so fs.ReadLink-based + // resolution is not available and full OS-level resolution is required. Context("production local storage FS", func() { var libRoot string var musicFS storage.MusicFS @@ -500,7 +460,12 @@ var _ = Describe("walk_dir_tree", func() { Expect(os.Symlink(filepath.Join(pool, "mid.wav"), filepath.Join(libRoot, "evil.wav"))).To(Succeed()) Expect(os.Symlink(filepath.Join(pool, "missing.mp3"), filepath.Join(libRoot, "broken.mp3"))).To(Succeed()) - musicFS = newLocalMusicFS(libRoot) + u, err := storage.LocalPathToURL(libRoot) + Expect(err).ToNot(HaveOccurred()) + s, err := storage.For(u.String()) + Expect(err).ToNot(HaveOccurred()) + musicFS, err = s.FS() + Expect(err).ToNot(HaveOccurred()) }) walkRoot := func() *folderEntry { @@ -735,17 +700,6 @@ func getDirEntry(baseDir, name string) os.DirEntry { panic(fmt.Sprintf("Could not find %s in %s", name, baseDir)) } -// newLocalMusicFS returns the production local storage MusicFS rooted at libRoot -func newLocalMusicFS(libRoot string) storage.MusicFS { - u, err := storage.LocalPathToURL(libRoot) - Expect(err).ToNot(HaveOccurred()) - s, err := storage.For(u.String()) - Expect(err).ToNot(HaveOccurred()) - musicFS, err := s.FS() - Expect(err).ToNot(HaveOccurred()) - return musicFS -} - // mockMusicFS is a mock implementation of the MusicFS interface that supports symlinks type mockMusicFS struct { storage.MusicFS diff --git a/scanner/watcher.go b/scanner/watcher.go index 1ac5468f0..3c0fbdbe7 100644 --- a/scanner/watcher.go +++ b/scanner/watcher.go @@ -13,6 +13,7 @@ import ( "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/singleton" ) @@ -245,7 +246,7 @@ func (w *watcher) processLibraryEvents(ctx context.Context, lib *model.Library, log.Debug(ctx, "Watcher stopped due to context cancellation", "libraryID", lib.ID, "name", lib.Name) return nil case path := <-events: - path, err := filepath.Rel(absLibPath, path) + path, err := utils.RelPath(absLibPath, path) if err != nil { log.Error(ctx, "Error getting relative path", "libraryID", lib.ID, "absolutePath", absLibPath, "path", path, err) continue diff --git a/server/jellyfin/README.md b/server/jellyfin/README.md index 48e69cb05..9315ac4f9 100644 --- a/server/jellyfin/README.md +++ b/server/jellyfin/README.md @@ -158,8 +158,8 @@ and `MusicArtist` queries accept it too, matching artists credited on an album o `Years`; `StudioIds` (record labels, as listed by `GET Studios`); `SearchTerm`; `Filters` (`IsFavorite`, `IsFavoriteOrLikes`, `IsPlayed`, `IsUnplayed`) and the standalone `isFavorite`/`isPlayed` booleans it can also be expressed as — `Filters` wins when both are sent, as -in Jellyfin; `Likes` and `Dislikes` are not supported, and `IsFolder`, `IsNotFolder` and -`IsResumable` have no Navidrome equivalent, so all five are ignored; `SortBy`/`SortOrder` (every recognized key is applied in order, so secondary keys break ties; +in Jellyfin; `Likes`, `Dislikes`, `IsFolder`, `IsNotFolder` and `IsResumable` have no Navidrome +equivalent and are ignored; `SortBy`/`SortOrder` (every recognized key is applied in order, so secondary keys break ties; unrecognized keys are skipped, and `Random` always sorts alone); `StartIndex`/`Limit`; and `Ids` (batch fetch by id). `Recursive=false` with a library `ParentId` returns direct children only (no tracks — no track is a library's direct child). @@ -176,7 +176,7 @@ returns direct children only (no tracks — no track is a library's direct child | Artists / genres / labels | `GET Artists`, `GET Artists/AlbumArtists`, `GET Genres`, `GET MusicGenres`, `GET Studios`, `GET Items/Filters` | | Similar / mixes | `GET Artists/{itemId}/Similar`, `GET Items/{itemId}/Similar`, `GET Albums/{itemId}/Similar`, `GET {Items,Songs,Albums,Artists,Playlists}/{itemId}/InstantMix`, `GET Artists/InstantMix?id=`, `GET MusicGenres/InstantMix?id=` | | Images | `GET Items/{itemId}/Images/{type}[/{index}]` (public), `POST`/`DELETE Items/{itemId}/Images/{type}` (playlist cover, authenticated) | -| Favorites / ratings for songs, albums, artists, and playlists | `POST`/`DELETE UserFavoriteItems/{itemId}`, `POST`/`DELETE Users/{userId}/FavoriteItems/{itemId}`, `POST`/`DELETE UserItems/{itemId}/Rating`, `POST`/`DELETE Users/{userId}/Items/{itemId}/Rating`, `GET`/`POST UserItems/{itemId}/UserData`, `GET`/`POST Users/{userId}/Items/{itemId}/UserData` | +| Favorites / ratings for songs, albums, artists, and playlists | `POST`/`DELETE UserFavoriteItems/{itemId}`, `POST`/`DELETE Users/{userId}/FavoriteItems/{itemId}`, `POST`/`DELETE Users/{userId}/Items/{itemId}/Rating`, `GET UserItems/{itemId}/UserData`, `GET Users/{userId}/Items/{itemId}/UserData` | | Streaming | `GET Audio/{itemId}/stream[.{container}]`, `GET Audio/{itemId}/universal`, `GET Audio/{itemId}/main.m3u8`, `GET Items/{itemId}/File`, `GET Items/{itemId}/Download`, `GET`/`POST Items/{itemId}/PlaybackInfo` (`HEAD` too on stream, universal, File, Download and images; a transcode HEAD answers without starting it) | | Lyrics | `GET Audio/{itemId}/Lyrics` | | Playback reporting | `POST Sessions/Playing`, `POST Sessions/Playing/Progress`, `POST Sessions/Playing/Stopped`, `POST Sessions/Playing/Ping` (no-op), `POST Sessions/Capabilities[/Full]` | @@ -402,20 +402,12 @@ make test PKG=./server/jellyfin/... in the playlist, so ids change when entries are inserted or removed; re-read the list after an edit. Real Jellyfin uses the song id. Clients that echo `PlaylistItemId` back (Finamp) work; clients that send the song id as `EntryIds` or to `Move` (Jellify) get `404`. -- **Ratings are stored as 0-5 stars.** Jellyfin's 0-10 `Rating` is rounded to the nearest star - (any nonzero value keeps at least one), so it reads back as an even number. `likes=true` stores - 5 stars and `likes=false` 1 star, like Jellyfin's 10 and 1. A rating of 0 clears the rating. -- **`POST .../Rating` needs a value.** Jellyfin clears the rating when `likes` is missing; Navidrome - answers `400` and keeps it, as it does for a malformed or out-of-range value. Use - `DELETE .../Rating` to clear. A numeric `rating` param (0-10) is also accepted when `likes` is - absent. -- **`POST .../UserData` writes only `Rating`, `Likes` and `IsFavorite`.** `Played`, `PlayCount`, - `LastPlayedDate` and `PlaybackPositionTicks` are ignored. A body that is not a single JSON - object, or a `Rating` outside 0-10, is a `400` and nothing is written. -- **Annotation writes can't target another user.** Rating and UserData writes whose `userId` names - a different user get `403`, even for admins; Jellyfin lets admins edit other users' data. -- **Missing endpoints.** `UserPlayedItems` (mark played/unplayed) and `Search/Hints` are not - implemented. Play counts only change through playback reporting. +- **Rating uses the `rating` param.** `POST Users/{userId}/Items/{itemId}/Rating` reads a `rating` + (0-10) and stores it as 0-5 stars. Jellyfin's `likes` boolean is not read, so a client that sends + only `likes` clears the rating. +- **Missing endpoints.** `UserPlayedItems` (mark played/unplayed), `Search/Hints` and the non-legacy + `UserItems/{itemId}/Rating` are not implemented. Play counts only change through playback + reporting. - **Some `Fields` are never emitted.** `ProviderIds`, `People`, `Etag` and `DateLastMediaAdded` are accepted but never returned. - **Blurhashes can be missing.** `ImageBlurHashes` carries the real blurhash that the artwork diff --git a/server/jellyfin/annotations.go b/server/jellyfin/annotations.go index 1bb0b7846..a589798be 100644 --- a/server/jellyfin/annotations.go +++ b/server/jellyfin/annotations.go @@ -1,52 +1,47 @@ package jellyfin import ( - "encoding/json" "errors" - "io" + "math" "net/http" - "strconv" - "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/server/jellyfin/dto" + "github.com/navidrome/navidrome/utils/req" ) -type annotatedItem struct { - repo model.AnnotatedRepository - resource string // name used in refreshResource events - ann model.Annotations -} - -// resolveAnnotated finds id's repo and the caller's annotations. Only albums and songs are library-gated -// (artists span libraries; playlists gate in Get). When !ok, the response has already been written. -func (api *Router) resolveAnnotated(w http.ResponseWriter, r *http.Request, id string) (item annotatedItem, ok bool) { +// resolveAnnotated finds which annotated repo owns id, returning the resource name used in +// refreshResource events. Albums and songs 404 when the user can't access their library; artists +// span libraries (library_artist), so have no single LibraryID to gate on and rely on list-time +// scoping. PlaylistRepository.Get enforces playlist visibility. When repo is nil the response has +// already been written, so callers must return without writing the annotation. +func (api *Router) resolveAnnotated(w http.ResponseWriter, r *http.Request, id string) (repo model.AnnotatedRepository, resource string) { ctx := r.Context() entity, err := model.GetEntityByID(ctx, api.ds, id) if err != nil && !errors.Is(err, model.ErrNotFound) { api.internalError(w, r, err) - return annotatedItem{}, false + return nil, "" } u, _ := request.UserFrom(ctx) switch e := entity.(type) { case *model.Album: if u.HasLibraryAccess(e.LibraryID) { - return annotatedItem{api.ds.Album(), "album", e.Annotations}, true + return api.ds.Album(), "album" } case *model.Artist: - return annotatedItem{api.ds.Artist(), "artist", e.Annotations}, true + return api.ds.Artist(), "artist" case *model.MediaFile: if u.HasLibraryAccess(e.LibraryID) { - return annotatedItem{api.ds.MediaFile(), "song", e.Annotations}, true + return api.ds.MediaFile(), "song" } case *model.Playlist: - return annotatedItem{api.ds.Playlist(), "playlist", e.Annotations}, true + return api.ds.Playlist(), "playlist" } // Unknown ids, inaccessible-library items and non-annotatable entities (radios) all read as absent. http.Error(w, "Not Found", http.StatusNotFound) - return annotatedItem{}, false + return nil, "" } // getUserItemData returns the caller's play/favorite/rating state for a single item. Jellify @@ -70,135 +65,65 @@ func (api *Router) getUserItemData(w http.ResponseWriter, r *http.Request) { api.ok(w, r, data) } -// userDataChange holds already-validated writes; a nil field leaves that annotation untouched. -type userDataChange struct { - rating *int // Navidrome stars; 0 clears - favorite *bool -} - -// applyUserData writes change for the itemId path param and answers with the item's resulting -// UserData, as Jellyfin does. -func (api *Router) applyUserData(w http.ResponseWriter, r *http.Request, change userDataChange) { +func (api *Router) setFavorite(w http.ResponseWriter, r *http.Request, starred bool) { id, ok := itemIDParam(w, r, "itemId") if !ok { return } - item, ok := api.resolveAnnotated(w, r, id) + repo, resource := api.resolveAnnotated(w, r, id) + if repo == nil { + return + } + if err := repo.SetStar(r.Context(), starred, id); err != nil { + api.internalError(w, r, err) + return + } + api.broker.SendMessage(r.Context(), (&events.RefreshResource{}).With(resource, id)) + encodedID := dto.EncodeID(id) + api.ok(w, r, &dto.UserItemDataDto{IsFavorite: starred, Key: encodedID, ItemId: encodedID}) +} + +func (api *Router) markFavorite(w http.ResponseWriter, r *http.Request) { api.setFavorite(w, r, true) } +func (api *Router) unmarkFavorite(w http.ResponseWriter, r *http.Request) { + api.setFavorite(w, r, false) +} + +func (api *Router) setItemRating(w http.ResponseWriter, r *http.Request, rating int) { + id, ok := itemIDParam(w, r, "itemId") if !ok { return } - ctx := r.Context() - if change.rating != nil { - if err := item.repo.SetRating(ctx, *change.rating, id); err != nil { - api.internalError(w, r, err) - return - } - item.ann.Rating = *change.rating - } - if change.favorite != nil { - if err := item.repo.SetStar(ctx, *change.favorite, id); err != nil { - api.internalError(w, r, err) - return - } - item.ann.Starred = *change.favorite - } - if change.rating != nil || change.favorite != nil { - api.broker.SendMessage(ctx, (&events.RefreshResource{}).With(item.resource, id)) - } - api.ok(w, r, dto.UserData(item.ann, id)) -} - -// applyCallerUserData is applyUserData for routes that may name a userId: only the caller's own -// annotations can be written, so naming another user is refused. Undecodable ids are ignored. -func (api *Router) applyCallerUserData(w http.ResponseWriter, r *http.Request, change userDataChange) { - raw := chi.URLParam(r, "userId") - if raw == "" { - raw = r.URL.Query().Get("userid") - } - if target, ok := dto.DecodeID(raw); ok { - if u, _ := request.UserFrom(r.Context()); target != u.ID { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } - } - api.applyUserData(w, r, change) -} - -func (api *Router) markFavorite(w http.ResponseWriter, r *http.Request) { - api.applyUserData(w, r, userDataChange{favorite: new(true)}) -} - -func (api *Router) unmarkFavorite(w http.ResponseWriter, r *http.Request) { - api.applyUserData(w, r, userDataChange{favorite: new(false)}) -} - -// setRating reads Jellyfin's likes flag, or the numeric rating extension older clients send. Unlike -// Jellyfin, a missing or malformed value is a 400 rather than a clear: clearing is DELETE's job. -func (api *Router) setRating(w http.ResponseWriter, r *http.Request) { - q := r.URL.Query() - var stars int - switch { - case q.Has("likes"): - likes, err := strconv.ParseBool(q.Get("likes")) - if err != nil { - http.Error(w, "likes must be true or false", http.StatusBadRequest) - return - } - stars = dto.StarsFromLikes(likes) - case q.Has("rating"): - rating, err := strconv.ParseFloat(q.Get("rating"), 64) - s, ok := dto.StarsFromRating(rating) - if err != nil || !ok { - http.Error(w, "rating must be a number from 0 to 10", http.StatusBadRequest) - return - } - stars = s - default: - http.Error(w, "likes is required", http.StatusBadRequest) + repo, resource := api.resolveAnnotated(w, r, id) + if repo == nil { return } - api.applyCallerUserData(w, r, userDataChange{rating: &stars}) + if err := repo.SetRating(r.Context(), rating, id); err != nil { + api.internalError(w, r, err) + return + } + api.broker.SendMessage(r.Context(), (&events.RefreshResource{}).With(resource, id)) + encodedID := dto.EncodeID(id) + d := &dto.UserItemDataDto{Key: encodedID, ItemId: encodedID} + if rating > 0 { + jfRating := float64(rating) * 2 // Navidrome 0-5 -> Jellyfin 0-10, mirrors dto.UserData + d.Rating = &jfRating + } + api.ok(w, r, d) +} + +// setRating maps Jellyfin's 0-10 rating (a nullable double, so fractional values are valid) to +// Navidrome's 0-5 stars. A nonzero rating floors at one star: rounding to 0 would clear it, since +// SetRating(0) is the delete path. +func (api *Router) setRating(w http.ResponseWriter, r *http.Request) { + jfRating := req.Params(r).Float64Or("rating", 0) + jfRating = min(max(jfRating, 0), 10) // clamp: a client sending e.g. Rating=100 must not write an out-of-domain rating + rating := int(math.Round(jfRating / 2)) + if jfRating > 0 { + rating = max(rating, 1) + } + api.setItemRating(w, r, rating) } func (api *Router) removeRating(w http.ResponseWriter, r *http.Request) { - api.applyCallerUserData(w, r, userDataChange{rating: new(0)}) -} - -// updateUserItemData writes Rating, Likes and IsFavorite; omitted or null fields are left untouched. -// Play state is ignored because it only changes through playback reporting. -func (api *Router) updateUserItemData(w http.ResponseWriter, r *http.Request) { - body, ok := decodeUserDataBody(w, r) - if !ok { - return - } - change := userDataChange{favorite: body.IsFavorite} - // Jellyfin applies Likes before Rating, so Rating wins when a body carries both. - switch { - case body.Rating != nil: - stars, ok := dto.StarsFromRating(*body.Rating) - if !ok { - http.Error(w, "Rating must be from 0 to 10", http.StatusBadRequest) - return - } - change.rating = &stars - case body.Likes != nil: - change.rating = new(dto.StarsFromLikes(*body.Likes)) - } - api.applyCallerUserData(w, r, change) -} - -const maxUserDataBody = 64 << 10 - -// decodeUserDataBody accepts exactly one JSON object; Decode alone would ignore anything after it. -func decodeUserDataBody(w http.ResponseWriter, r *http.Request) (*dto.UpdateUserItemDataDto, bool) { - dec := json.NewDecoder(http.MaxBytesReader(w, r.Body, maxUserDataBody)) - var body *dto.UpdateUserItemDataDto - err := dec.Decode(&body) - if err == nil && body != nil { - if _, err = dec.Token(); errors.Is(err, io.EOF) { - return body, true - } - } - http.Error(w, "Bad Request", http.StatusBadRequest) - return nil, false + api.setItemRating(w, r, 0) } diff --git a/server/jellyfin/annotations_test.go b/server/jellyfin/annotations_test.go index 69d73ed05..a436503ba 100644 --- a/server/jellyfin/annotations_test.go +++ b/server/jellyfin/annotations_test.go @@ -5,8 +5,6 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "strings" - "time" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -23,7 +21,7 @@ var _ = Describe("Annotations", func() { var broker *fakeEventBroker // alice has access to library 1 only. ctxUser := func() context.Context { - return request.WithUser(GinkgoT().Context(), model.User{ID: testID("u1"), UserName: "alice", Libraries: model.Libraries{{ID: 1, Name: "Music"}}}) + return request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice", Libraries: model.Libraries{{ID: 1, Name: "Music"}}}) } BeforeEach(func() { @@ -32,16 +30,6 @@ var _ = Describe("Annotations", func() { api = &Router{ds: ds, broker: broker} }) - songWith := func(ann model.Annotations) *tests.MockMediaFileRepo { - mfRepo := ds.MediaFile().(*tests.MockMediaFileRepo) - mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song", LibraryID: 1, Annotations: ann}}) - return mfRepo - } - songReq := func(method, target, body string, params ...string) *http.Request { - r := httptest.NewRequest(method, target, strings.NewReader(body)).WithContext(ctxUser()) - return withChiURLParam(r, append([]string{"itemId", dto.EncodeID(testID("s1"))}, params...)...) - } - Describe("markFavorite / unmarkFavorite", func() { It("stars a song and returns IsFavorite=true", func() { mfRepo := ds.MediaFile().(*tests.MockMediaFileRepo) @@ -280,275 +268,41 @@ var _ = Describe("Annotations", func() { Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) }) + It("clamps a Rating above 10 to Navidrome's max (5)", func() { + mfRepo := ds.MediaFile().(*tests.MockMediaFileRepo) + mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song", LibraryID: 1}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/Users/u1/Items/s1/Rating?Rating=100", nil).WithContext(ctxUser()) + r = withChiURLParam(r, "itemId", dto.EncodeID(testID("s1"))) + invoke(api.setRating, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(5)) + }) + + It("clamps a negative Rating to Navidrome's min (0)", func() { + mfRepo := ds.MediaFile().(*tests.MockMediaFileRepo) + mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song", LibraryID: 1}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/Users/u1/Items/s1/Rating?Rating=-5", nil).WithContext(ctxUser()) + r = withChiURLParam(r, "itemId", dto.EncodeID(testID("s1"))) + invoke(api.setRating, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(0)) + }) + It("emits a refreshResource event when rating a song", func() { - songWith(model.Annotations{}) - invoke(api.setRating, httptest.NewRecorder(), songReq("POST", "/Users/u1/Items/s1/Rating?Rating=8", "")) + mfRepo := ds.MediaFile().(*tests.MockMediaFileRepo) + mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song", LibraryID: 1}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/Users/u1/Items/s1/Rating?Rating=8", nil).WithContext(ctxUser()) + r = withChiURLParam(r, "itemId", dto.EncodeID(testID("s1"))) + invoke(api.setRating, w, r) Expect(broker.Events).To(HaveLen(1)) Expect(broker.Events[0].Data(broker.Events[0])).To(Equal(`{"song":["` + testID("s1") + `"]}`)) }) - - Context("Jellyfin likes semantics", func() { - It("stores likes=true as 5 stars and reports Rating 10 / Likes true", func() { - mfRepo := songWith(model.Annotations{}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating?likes=true", "")) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(5)) - d := parseUserData(w) - Expect(*d.Rating).To(Equal(10.0)) - Expect(*d.Likes).To(BeTrue()) - }) - - It("stores likes=false as 1 star (Jellyfin's dislike is Rating 1), not a clear", func() { - mfRepo := songWith(model.Annotations{Rating: 4}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating?Likes=False", "")) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(1)) - d := parseUserData(w) - Expect(*d.Rating).To(Equal(2.0)) - Expect(*d.Likes).To(BeFalse()) - }) - - It("prefers likes over the numeric rating extension when both are sent", func() { - mfRepo := songWith(model.Annotations{}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating?likes=false&rating=10", "")) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(1)) - }) - - It("still clears on an explicit rating=0 from numeric clients", func() { - mfRepo := songWith(model.Annotations{Rating: 4}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating?rating=0", "")) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(0)) - Expect(parseUserData(w).Rating).To(BeNil()) - }) - - It("keeps the favorite and play state and reports them in the response", func() { - playDate := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) - mfRepo := songWith(model.Annotations{Starred: true, PlayCount: 3, PlayDate: &playDate}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating?likes=true", "")) - Expect(w.Code).To(Equal(http.StatusOK)) - song := mfRepo.Data[testID("s1")] - Expect(song.Starred).To(BeTrue()) - Expect(song.PlayCount).To(Equal(int64(3))) - Expect(song.PlayDate).To(Equal(&playDate)) - d := parseUserData(w) - Expect(d.IsFavorite).To(BeTrue()) - Expect(d.PlayCount).To(Equal(3)) - Expect(d.Played).To(BeTrue()) - }) - - DescribeTable("rejects a missing or malformed value without touching the stored rating", - func(query string) { - mfRepo := songWith(model.Annotations{Rating: 4}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/UserItems/s1/Rating"+query, "")) - Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - Expect(broker.Events).To(BeEmpty()) - }, - Entry("no params (Jellyfin would clear)", ""), - Entry("unrelated params only", "?userId=x&foo=1"), - Entry("empty likes", "?likes="), - Entry("non-boolean likes", "?likes=maybe"), - Entry("non-boolean likes even with a valid rating", "?likes=maybe&rating=8"), - Entry("empty rating", "?rating="), - Entry("non-numeric rating", "?rating=abc"), - Entry("NaN rating", "?rating=NaN"), - Entry("infinite rating", "?rating=Inf"), - Entry("rating above 10", "?Rating=100"), - Entry("negative rating", "?Rating=-5"), - ) - }) - - Context("authorization", func() { - DescribeTable("refuses a userId naming another user and leaves the rating", - func(method, target string, params ...string) { - handler := api.setRating - if method == "DELETE" { - handler = api.removeRating - } - mfRepo := songWith(model.Annotations{Rating: 4}) - w := httptest.NewRecorder() - invoke(handler, w, songReq(method, target, "", params...)) - Expect(w.Code).To(Equal(http.StatusForbidden)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - Expect(broker.Events).To(BeEmpty()) - }, - Entry("path userId", "POST", "/Users/u2/Items/s1/Rating?likes=true", "userId", dto.EncodeID(testID("u2"))), - Entry("query userId on the /UserItems form", "POST", "/UserItems/s1/Rating?likes=true&userId="+dto.EncodeID(testID("u2"))), - Entry("clearing", "DELETE", "/Users/u2/Items/s1/Rating", "userId", dto.EncodeID(testID("u2"))), - ) - - It("accepts the caller's own userId", func() { - mfRepo := songWith(model.Annotations{}) - w := httptest.NewRecorder() - invoke(api.setRating, w, songReq("POST", "/Users/u1/Items/s1/Rating?likes=true", "", "userId", dto.EncodeID(testID("u1")))) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(5)) - }) - }) - }) - - Describe("updateUserItemData", func() { - post := func(body string, params ...string) *httptest.ResponseRecorder { - w := httptest.NewRecorder() - invoke(api.updateUserItemData, w, songReq("POST", "/UserItems/s1/UserData", body, params...)) - return w - } - - It("maps a 0-10 Rating to stars and answers with the full UserData", func() { - mfRepo := songWith(model.Annotations{Starred: true, PlayCount: 2}) - w := post(`{"Rating":8}`) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - d := parseUserData(w) - Expect(*d.Rating).To(Equal(8.0)) - Expect(*d.Likes).To(BeTrue()) - Expect(d.IsFavorite).To(BeTrue()) - Expect(d.PlayCount).To(Equal(2)) - Expect(d.ItemId).To(Equal(dto.EncodeID(testID("s1")))) - Expect(broker.Events).To(HaveLen(1)) - }) - - DescribeTable("stores the personal rating", - func(body string, before, expected int) { - mfRepo := songWith(model.Annotations{Rating: before}) - Expect(post(body).Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(expected)) - }, - Entry("Likes true", `{"Likes":true}`, 0, 5), - Entry("Likes false (Rating 1)", `{"Likes":false}`, 4, 1), - Entry("Rating wins over Likes, as Jellyfin applies Likes first", `{"Likes":true,"Rating":2}`, 0, 1), - Entry("fractional Rating rounds to the nearest star", `{"Rating":7.5}`, 0, 4), - Entry("small nonzero Rating floors at one star", `{"Rating":0.4}`, 0, 1), - Entry("Rating 0 clears", `{"Rating":0}`, 4, 0), - ) - - DescribeTable("leaves the rating untouched when Rating and Likes are omitted or null", - func(body string, wantEvents int) { - mfRepo := songWith(model.Annotations{Rating: 4}) - w := post(body) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - Expect(*parseUserData(w).Rating).To(Equal(8.0)) - Expect(broker.Events).To(HaveLen(wantEvents)) - }, - Entry("empty object writes nothing", `{}`, 0), - Entry("explicit nulls write nothing", `{"Rating":null,"Likes":null,"IsFavorite":null}`, 0), - Entry("favorite only", `{"IsFavorite":true}`, 1), - ) - - It("sets and unsets the favorite without touching the rating", func() { - mfRepo := songWith(model.Annotations{Rating: 4}) - Expect(post(`{"IsFavorite":true}`).Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Starred).To(BeTrue()) - Expect(post(`{"IsFavorite":false}`).Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Starred).To(BeFalse()) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - }) - - It("ignores play-state fields, which only change through playback reporting", func() { - mfRepo := songWith(model.Annotations{PlayCount: 2}) - w := post(`{"Played":true,"PlayCount":99,"LastPlayedDate":"2026-01-01T00:00:00Z","PlaybackPositionTicks":5}`) - Expect(w.Code).To(Equal(http.StatusOK)) - song := mfRepo.Data[testID("s1")] - Expect(song.PlayCount).To(Equal(int64(2))) - Expect(song.PlayDate).To(BeNil()) - Expect(broker.Events).To(BeEmpty()) - }) - - It("leaves state unchanged when a client echoes back the UserData it read", func() { - mfRepo := songWith(model.Annotations{Rating: 3, Starred: true, PlayCount: 1}) - echo, err := json.Marshal(dto.UserData(mfRepo.Data[testID("s1")].Annotations, testID("s1"))) - Expect(err).ToNot(HaveOccurred()) - Expect(post(string(echo)).Code).To(Equal(http.StatusOK)) - song := mfRepo.Data[testID("s1")] - Expect(song.Rating).To(Equal(3)) - Expect(song.Starred).To(BeTrue()) - Expect(song.PlayCount).To(Equal(int64(1))) - }) - - DescribeTable("rejects an invalid body without writing anything", - func(body string) { - mfRepo := songWith(model.Annotations{Rating: 4}) - Expect(post(body).Code).To(Equal(http.StatusBadRequest)) - song := mfRepo.Data[testID("s1")] - Expect(song.Rating).To(Equal(4)) - Expect(song.Starred).To(BeFalse()) - Expect(broker.Events).To(BeEmpty()) - }, - Entry("empty body", ""), - Entry("malformed JSON", `{"Rating":`), - Entry("Rating above 10, even with a valid IsFavorite", `{"IsFavorite":true,"Rating":10.5}`), - Entry("negative Rating", `{"IsFavorite":true,"Rating":-1}`), - Entry("Rating as a string", `{"Rating":"8"}`), - Entry("Likes as a string", `{"Likes":"yes"}`), - Entry("IsFavorite as a number", `{"IsFavorite":1}`), - Entry("JSON null", `null`), - Entry("JSON array", `[]`), - Entry("trailing garbage after the object", `{"Rating":8} x`), - Entry("a second JSON value after the object", `{"Rating":8}{"Rating":2}`), - Entry("a second object after whitespace", "{\"Rating\":8}\n{}"), - ) - - It("accepts trailing whitespace after the object", func() { - mfRepo := songWith(model.Annotations{}) - Expect(post("{\"Rating\":8}\r\n\t ").Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - }) - - It("rejects a body larger than the limit", func() { - mfRepo := songWith(model.Annotations{Rating: 4}) - big := `{"Rating":8,"Pad":"` + strings.Repeat("x", 70<<10) + `"}` - Expect(post(big).Code).To(Equal(http.StatusBadRequest)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - }) - - Context("authorization", func() { - It("refuses a userId naming another user without writing", func() { - mfRepo := songWith(model.Annotations{Rating: 4}) - w := post(`{"Rating":10,"IsFavorite":true}`, "userId", dto.EncodeID(testID("u2"))) - Expect(w.Code).To(Equal(http.StatusForbidden)) - Expect(mfRepo.Data[testID("s1")].Rating).To(Equal(4)) - Expect(mfRepo.Data[testID("s1")].Starred).To(BeFalse()) - }) - - It("accepts the caller's own userId", func() { - songWith(model.Annotations{}) - Expect(post(`{"Rating":10}`, "userId", dto.EncodeID(testID("u1"))).Code).To(Equal(http.StatusOK)) - }) - - It("returns 404 and writes nothing for an album in a library the user can't access", func() { - albumRepo := ds.Album().(*tests.MockAlbumRepo) - albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One", LibraryID: 2}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("POST", "/UserItems/a1/UserData", strings.NewReader(`{"Rating":10,"IsFavorite":true}`)).WithContext(ctxUser()) - invoke(api.updateUserItemData, w, withChiURLParam(r, "itemId", dto.EncodeID(testID("a1")))) - Expect(w.Code).To(Equal(http.StatusNotFound)) - Expect(albumRepo.Data[testID("a1")].Rating).To(Equal(0)) - Expect(albumRepo.Data[testID("a1")].Starred).To(BeFalse()) - }) - - It("returns 404 for an unknown item", func() { - w := post(`{"Rating":10}`) - Expect(w.Code).To(Equal(http.StatusNotFound)) - }) - }) }) }) -func parseUserData(w *httptest.ResponseRecorder) dto.UserItemDataDto { - var d dto.UserItemDataDto - Expect(json.Unmarshal(w.Body.Bytes(), &d)).To(Succeed()) - return d -} - type fakeEventBroker struct { http.Handler Events []events.Event diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index b9fd32150..4a471d1f0 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -146,8 +146,6 @@ func (api *Router) routes() http.Handler { r.Delete("/userfavoriteitems/{itemId}", api.unmarkFavorite) r.Post("/users/{userId}/favoriteitems/{itemId}", api.markFavorite) r.Delete("/users/{userId}/favoriteitems/{itemId}", api.unmarkFavorite) - r.Post("/useritems/{itemId}/rating", api.setRating) - r.Delete("/useritems/{itemId}/rating", api.removeRating) r.Post("/users/{userId}/items/{itemId}/rating", api.setRating) r.Delete("/users/{userId}/items/{itemId}/rating", api.removeRating) @@ -155,8 +153,6 @@ func (api *Router) routes() http.Handler { // /Users/{userId}/Items is the legacy spelling. r.Get("/useritems/{itemId}/userdata", api.getUserItemData) r.Get("/users/{userId}/items/{itemId}/userdata", api.getUserItemData) - r.Post("/useritems/{itemId}/userdata", api.updateUserItemData) - r.Post("/users/{userId}/items/{itemId}/userdata", api.updateUserItemData) r.Get("/artists/{itemId}/similar", api.getSimilarArtists) r.Get("/items/{itemId}/similar", api.getSimilarItems) diff --git a/server/jellyfin/dto/dto.go b/server/jellyfin/dto/dto.go index 318011d89..4a28eb313 100644 --- a/server/jellyfin/dto/dto.go +++ b/server/jellyfin/dto/dto.go @@ -40,14 +40,6 @@ type UserItemDataDto struct { Key string `json:"Key"` ItemId string `json:"ItemId,omitempty"` LastPlayedDate *string `json:"LastPlayedDate,omitempty"` - Likes *bool `json:"Likes,omitempty"` -} - -// UpdateUserItemDataDto is the POST .../UserData body; other Jellyfin fields are accepted and ignored. -type UpdateUserItemDataDto struct { - Rating *float64 `json:"Rating"` - Likes *bool `json:"Likes"` - IsFavorite *bool `json:"IsFavorite"` } type BaseItemDto struct { diff --git a/server/jellyfin/dto/mappers.go b/server/jellyfin/dto/mappers.go index 620df783f..fc7e329de 100644 --- a/server/jellyfin/dto/mappers.go +++ b/server/jellyfin/dto/mappers.go @@ -3,7 +3,6 @@ package dto import ( "cmp" "fmt" - "math" "time" "github.com/navidrome/navidrome/conf" @@ -122,30 +121,6 @@ func MediaSourceFromMediaFile(mf model.MediaFile) MediaSourceInfo { } } -// minLikeRating is Jellyfin's UserItemData.MinLikeValue: a rating at or above it reads as Likes. -const minLikeRating = 6.5 - -// StarsFromRating maps Jellyfin's 0-10 rating to 0-5 stars, rejecting out-of-range values and NaN. -// A nonzero rating floors at one star, since 0 stars means "not rated". -func StarsFromRating(rating float64) (int, bool) { - if !(rating >= 0 && rating <= 10) { - return 0, false - } - stars := int(math.Round(rating / 2)) - if rating > 0 { - stars = max(stars, 1) - } - return stars, true -} - -// StarsFromLikes mirrors Jellyfin, which stores likes as Rating 10 and dislikes as Rating 1. -func StarsFromLikes(likes bool) int { - if likes { - return 5 - } - return 1 -} - func UserData(a model.Annotations, itemID string) *UserItemDataDto { // Callers pass the raw model id; encode here so Key/ItemId match the encoded Id on the BaseItemDto. encodedID := EncodeID(itemID) @@ -159,7 +134,6 @@ func UserData(a model.Annotations, itemID string) *UserItemDataDto { if a.Rating > 0 { r := float64(a.Rating) * 2 // Navidrome 0-5 -> Jellyfin 0-10 d.Rating = &r - d.Likes = new(r >= minLikeRating) } if s := JellyfinDate(a.PlayDate); s != "" { d.LastPlayedDate = &s diff --git a/server/jellyfin/dto/mappers_test.go b/server/jellyfin/dto/mappers_test.go index 3e4ba941f..259904673 100644 --- a/server/jellyfin/dto/mappers_test.go +++ b/server/jellyfin/dto/mappers_test.go @@ -478,30 +478,6 @@ var _ = Describe("mappers", func() { Expect(item.ImageBlurHashes).To(BeNil()) }) - DescribeTable("UserData derives Likes from the rating like Jellyfin (rating >= 6.5)", - func(stars int, expected *bool) { - d := UserData(model.Annotations{Rating: stars}, testID("s1")) - Expect(d.Likes).To(Equal(expected)) - }, - Entry("unrated has no Likes", 0, nil), - Entry("1 star (Jellyfin 2) dislikes", 1, new(false)), - Entry("3 stars (Jellyfin 6) dislikes", 3, new(false)), - Entry("4 stars (Jellyfin 8) likes", 4, new(true)), - Entry("5 stars (Jellyfin 10) likes", 5, new(true)), - ) - - It("emits Rating as a whole number and omits Likes when unrated", func() { - rated, err := json.Marshal(UserData(model.Annotations{Rating: 4}, testID("s1"))) - Expect(err).ToNot(HaveOccurred()) - Expect(string(rated)).To(ContainSubstring(`"Rating":8,`)) - Expect(string(rated)).To(ContainSubstring(`"Likes":true`)) - - unrated, err := json.Marshal(UserData(model.Annotations{}, testID("s1"))) - Expect(err).ToNot(HaveOccurred()) - Expect(string(unrated)).ToNot(ContainSubstring(`"Rating"`)) - Expect(string(unrated)).ToNot(ContainSubstring(`"Likes"`)) - }) - It("emits DateCreated and SortName on playlists, like albums and artists", func() { p := model.Playlist{ID: testID("pl-1"), Name: "Chill", CreatedAt: time.Date(2026, 7, 1, 12, 0, 0, 0, time.UTC)} Expect(PlaylistToBaseItem(p, nil).DateCreated).To(Equal("2026-07-01T12:00:00.0000000Z")) diff --git a/server/jellyfin/e2e/annotations_test.go b/server/jellyfin/e2e/annotations_test.go index 014751f6e..182fd5436 100644 --- a/server/jellyfin/e2e/annotations_test.go +++ b/server/jellyfin/e2e/annotations_test.go @@ -130,78 +130,13 @@ var _ = Describe("Annotations", func() { Expect(*itemUserData(id).Rating).To(Equal(float64(8))) }) - It("rejects an out-of-range rating and keeps the stored one", func() { + It("clamps an out-of-range rating to the valid domain", func() { id := albumID("Help!") - Expect(post("/Users/admin-1/Items/"+enc(id)+"/Rating?Rating=8", "").Code).To(Equal(http.StatusOK)) - Expect(post("/Users/admin-1/Items/"+enc(id)+"/Rating?Rating=100", "").Code).To(Equal(http.StatusBadRequest)) - Expect(*itemUserData(id).Rating).To(Equal(float64(8))) - }) - - It("stores Jellyfin likes on the current /UserItems route instead of clearing", func() { - id := songID("So What") - var liked dto.UserItemDataDto - parseInto(post("/UserItems/"+enc(id)+"/Rating?likes=true", ""), &liked) - Expect(*liked.Rating).To(Equal(float64(10))) - Expect(*liked.Likes).To(BeTrue()) - - var disliked dto.UserItemDataDto - parseInto(post("/Users/admin-1/Items/"+enc(id)+"/Rating?likes=false", ""), &disliked) - Expect(*disliked.Rating).To(Equal(float64(2))) - Expect(*disliked.Likes).To(BeFalse()) - Expect(*itemUserData(id).Likes).To(BeFalse()) - - Expect(del("/UserItems/" + enc(id) + "/Rating").Code).To(Equal(http.StatusOK)) - Expect(itemUserData(id).Rating).To(BeNil()) - Expect(itemUserData(id).Likes).To(BeNil()) - }) - - It("does not clear a rating when the request has no value", func() { - id := songID("So What") - Expect(post("/UserItems/"+enc(id)+"/Rating?likes=true", "").Code).To(Equal(http.StatusOK)) - Expect(post("/UserItems/"+enc(id)+"/Rating", "").Code).To(Equal(http.StatusBadRequest)) - Expect(post("/UserItems/"+enc(id)+"/Rating?likes=sure", "").Code).To(Equal(http.StatusBadRequest)) - Expect(*itemUserData(id).Rating).To(Equal(float64(10))) - }) - }) - - Describe("POST /UserItems/{id}/UserData", func() { - It("sets Rating, Likes and IsFavorite on both route spellings", func() { - id := songID("Come Together") var data dto.UserItemDataDto - parseInto(post("/UserItems/"+enc(id)+"/UserData", `{"Rating":6}`), &data) - Expect(*data.Rating).To(Equal(float64(6))) - Expect(*data.Likes).To(BeFalse()) - - parseInto(post("/Users/"+enc(adminUser.ID)+"/Items/"+enc(id)+"/UserData", `{"Likes":true,"IsFavorite":true}`), &data) - stored := itemUserData(id) - Expect(*stored.Rating).To(Equal(float64(10))) - Expect(*stored.Likes).To(BeTrue()) - Expect(stored.IsFavorite).To(BeTrue()) - }) - - It("leaves omitted fields and play state untouched", func() { - id := songID("Come Together") - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"Rating":8,"IsFavorite":true}`).Code).To(Equal(http.StatusOK)) - before := itemUserData(id) - - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"Played":true,"PlayCount":42,"LastPlayedDate":"2026-01-01T00:00:00Z"}`).Code).To(Equal(http.StatusOK)) - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"IsFavorite":false}`).Code).To(Equal(http.StatusOK)) - - after := itemUserData(id) - Expect(*after.Rating).To(Equal(float64(8))) - Expect(after.IsFavorite).To(BeFalse()) - Expect(after.PlayCount).To(Equal(before.PlayCount)) - Expect(after.Played).To(Equal(before.Played)) - Expect(after.LastPlayedDate).To(Equal(before.LastPlayedDate)) - }) - - It("rejects invalid bodies without writing", func() { - id := songID("Come Together") - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"Rating":11,"IsFavorite":true}`).Code).To(Equal(http.StatusBadRequest)) - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"Rating":8} {"Rating":2}`).Code).To(Equal(http.StatusBadRequest)) - stored := itemUserData(id) - Expect(stored.Rating).To(BeNil()) - Expect(stored.IsFavorite).To(BeFalse()) + parseInto(post("/Users/admin-1/Items/"+enc(id)+"/Rating?Rating=100", ""), &data) + // 100 clamps to 10 (Jellyfin) -> 5 (Navidrome) -> 10 back out. + Expect(data.Rating).ToNot(BeNil()) + Expect(*data.Rating).To(Equal(float64(10))) }) }) }) diff --git a/server/jellyfin/e2e/multiuser_test.go b/server/jellyfin/e2e/multiuser_test.go index f4f2c43d2..905d4e16f 100644 --- a/server/jellyfin/e2e/multiuser_test.go +++ b/server/jellyfin/e2e/multiuser_test.go @@ -4,7 +4,6 @@ import ( "net/http" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/server/jellyfin/dto" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -29,36 +28,6 @@ var _ = Describe("Multi-user access control", func() { }) }) - Describe("personal ratings", func() { - userData := func(u model.User, id string) *dto.UserItemDataDto { - var data dto.UserItemDataDto - parseInto(getAs(u, "/UserItems/"+enc(id)+"/UserData"), &data) - return &data - } - - It("keeps each user's rating separate", func() { - id := songID("So What") - Expect(post("/UserItems/"+enc(id)+"/UserData", `{"Rating":10}`).Code).To(Equal(http.StatusOK)) - Expect(postAs(regularUser, "/UserItems/"+enc(id)+"/Rating?likes=false", "").Code).To(Equal(http.StatusOK)) - - Expect(*userData(adminUser, id).Rating).To(Equal(float64(10))) - Expect(*userData(regularUser, id).Rating).To(Equal(float64(2))) - }) - - It("refuses writes that name another user, even for an admin, and changes nothing", func() { - id := songID("So What") - Expect(postAs(regularUser, "/UserItems/"+enc(id)+"/UserData", `{"Rating":4}`).Code).To(Equal(http.StatusOK)) - - Expect(postAs(regularUser, "/Users/"+enc(adminUser.ID)+"/Items/"+enc(id)+"/UserData", `{"Rating":10}`).Code).To(Equal(http.StatusForbidden)) - Expect(post("/Users/"+enc(regularUser.ID)+"/Items/"+enc(id)+"/Rating?likes=true", "").Code).To(Equal(http.StatusForbidden)) - Expect(del("/Users/" + enc(regularUser.ID) + "/Items/" + enc(id) + "/Rating").Code).To(Equal(http.StatusForbidden)) - Expect(post("/UserItems/"+enc(id)+"/UserData?userId="+enc(regularUser.ID), `{"Rating":10}`).Code).To(Equal(http.StatusForbidden)) - - Expect(userData(adminUser, id).Rating).To(BeNil()) - Expect(*userData(regularUser, id).Rating).To(Equal(float64(4))) - }) - }) - Describe("private playlists", func() { It("does not expose another user's private playlist", func() { adminPl := createPlaylist("Admin Private", nil) @@ -84,7 +53,6 @@ var _ = Describe("Multi-user access control", func() { adminPl := createPlaylist("Admin Private", nil) Expect(postAs(regularUser, "/Users/user-1/FavoriteItems/"+enc(adminPl), "").Code).To(Equal(http.StatusNotFound)) Expect(postAs(regularUser, "/Users/user-1/Items/"+enc(adminPl)+"/Rating?Rating=10", "").Code).To(Equal(http.StatusNotFound)) - Expect(postAs(regularUser, "/UserItems/"+enc(adminPl)+"/UserData", `{"Rating":10}`).Code).To(Equal(http.StatusNotFound)) }) It("lets each user manage their own playlist", func() { diff --git a/server/jellyfin/items_test.go b/server/jellyfin/items_test.go index bdf0be253..18789a34b 100644 --- a/server/jellyfin/items_test.go +++ b/server/jellyfin/items_test.go @@ -18,13 +18,11 @@ import ( . "github.com/onsi/gomega" ) -// withChiURLParam simulates chi's routing having captured path parameters (key, value pairs), since -// these tests call handlers directly instead of going through the full router. -func withChiURLParam(r *http.Request, kv ...string) *http.Request { +// withChiURLParam simulates chi's routing having captured a path parameter, since these +// tests call handlers directly instead of going through the full router. +func withChiURLParam(r *http.Request, key, value string) *http.Request { rctx := chi.NewRouteContext() - for i := 0; i+1 < len(kv); i += 2 { - rctx.URLParams.Add(kv[i], kv[i+1]) - } + rctx.URLParams.Add(key, value) return r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) } diff --git a/server/nativeapi/inspect.go b/server/nativeapi/inspect.go index 61013dd5a..f1e6c4539 100644 --- a/server/nativeapi/inspect.go +++ b/server/nativeapi/inspect.go @@ -22,12 +22,7 @@ func doInspect(ctx context.Context, ds model.DataStore, id string) (*core.Inspec return nil, model.ErrNotFound } - lib, err := ds.Library().Get(ctx, file.LibraryID) - if err != nil { - return nil, err - } - - return core.Inspect(file.AbsolutePath(), *lib, file.FolderID) + return core.Inspect(file.AbsolutePath(), file.LibraryID, file.FolderID) } func inspect(ds model.DataStore) http.HandlerFunc { diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index 46c7ca210..37ae56c2b 100644 --- a/server/public/handle_streams.go +++ b/server/public/handle_streams.go @@ -60,8 +60,9 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { return } - streamReq := pub.decider.ResolveRequest(ctx, mf, info.format, info.bitrate, 0) - stream, err := pub.streamer.NewStream(ctx, mf, streamReq) + stream, err := pub.streamer.NewStream(ctx, mf, streampkg.Request{ + Format: info.format, BitRate: info.bitrate, + }) 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 965bc7e05..4b4a3545b 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{MockedTranscoding: &tests.MockTranscodingRepo{}} + ds = &tests.MockDataStore{} shareRepo = &tests.MockShareRepo{} ds.MockedShare = shareRepo streamer = &mockStreamer{} - pub = &Router{ds: ds, streamer: streamer, decider: stream.NewTranscodeDecider(ds, tests.NewMockFFmpeg(""))} + pub = &Router{ds: ds, streamer: streamer} }) makeRequest := func(token string) *httptest.ResponseRecorder { @@ -152,17 +152,8 @@ var _ = Describe("handleStream", func() { makeRequest(token) Expect(streamer.called).To(BeTrue()) - }) - - 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})) + Expect(streamer.req.Format).To(Equal("mp3")) + Expect(streamer.req.BitRate).To(Equal(192)) }) 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 8239ef927..142c474bd 100644 --- a/server/public/public.go +++ b/server/public/public.go @@ -21,15 +21,14 @@ 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, decider stream.TranscodeDecider, share core.Share, archiver core.Archiver) *Router { - p := &Router{ds: ds, artwork: artwork, streamer: streamer, decider: decider, share: share, archiver: archiver} +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} 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/serve_index.go b/server/serve_index.go index 167197403..4b093b953 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -58,8 +58,6 @@ 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 277513768..e2df55c4b 100644 --- a/server/serve_index_test.go +++ b/server/serve_index_test.go @@ -89,8 +89,6 @@ 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/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 530588759..9394c8830 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, stream.NewTranscodeDecider(ds, ffm), core.NewShare(ds), noopArchiver{}) + pubRouter = public.New(ds, artSvc, streamerSpy, core.NewShare(ds), noopArchiver{}) }) It("emits a bare optimistic coverArt id before the queue is drained", func() { diff --git a/tests/mock_library_repo.go b/tests/mock_library_repo.go index 6de2b9265..e21dcccce 100644 --- a/tests/mock_library_repo.go +++ b/tests/mock_library_repo.go @@ -145,17 +145,6 @@ 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/album/AlbumDetails.jsx b/ui/src/album/AlbumDetails.jsx index 8d636a047..7fbc479db 100644 --- a/ui/src/album/AlbumDetails.jsx +++ b/ui/src/album/AlbumDetails.jsx @@ -16,7 +16,6 @@ import { useRecordContext, useTranslate, } from 'react-admin' -import clsx from 'clsx' import Lightbox from 'react-image-lightbox' import config from '../config' import 'react-image-lightbox/style.css' @@ -80,9 +79,6 @@ const useStyles = makeStyles( alignItems: 'center', justifyContent: 'center', }, - noCoverAnimation: { - '&, &::before, &::after': { animation: 'none' }, - }, cover: { objectFit: 'contain', cursor: 'pointer', @@ -254,12 +250,7 @@ const AlbumDetails = (props) => { return (
-
+
({ - default: { - getAlbumInfo: () => - Promise.resolve({ - json: { 'subsonic-response': { status: 'ok', albumInfo: {} } }, - }), - getCoverArtUrl: () => '', - }, -})) - -vi.mock('react-admin', async () => { - const actual = await vi.importActual('react-admin') - return { - ...actual, - useDataProvider: () => ({ getOne: vi.fn() }), - useNotify: () => vi.fn(), - useRefresh: () => vi.fn(), - } -}) +import { Details } from './AlbumDetails' // Mock useMediaQuery vi.mock('@material-ui/core', async () => { @@ -365,49 +343,3 @@ describe('Details component', () => { }) }) }) - -describe('AlbumDetails cover animation', () => { - const albumRecord = { - id: '123', - name: 'Test Album', - songCount: 12, - duration: 3600, - size: 102400, - } - const originalEnableCoverAnimation = config.enableCoverAnimation - - beforeEach(() => { - vi.mocked(useMediaQuery).mockReturnValue(false) - }) - - afterEach(() => { - config.enableCoverAnimation = originalEnableCoverAnimation - }) - - const renderAlbum = () => - render( - - - - - , - ) - - test('applies noCoverAnimation when enableCoverAnimation is false', () => { - config.enableCoverAnimation = false - const { container } = renderAlbum() - const cover = container.querySelector('[class*="coverParent"]') - - expect(cover).not.toBeNull() - expect(cover.className).toMatch(/noCoverAnimation/) - }) - - test('omits noCoverAnimation when enableCoverAnimation is true', () => { - config.enableCoverAnimation = true - const { container } = renderAlbum() - const cover = container.querySelector('[class*="coverParent"]') - - expect(cover).not.toBeNull() - expect(cover.className).not.toMatch(/noCoverAnimation/) - }) -}) diff --git a/ui/src/album/AlbumList.jsx b/ui/src/album/AlbumList.jsx index 81e4dcd14..3cb8a648e 100644 --- a/ui/src/album/AlbumList.jsx +++ b/ui/src/album/AlbumList.jsx @@ -163,7 +163,6 @@ const AlbumFilter = (props) => { /> - {config.enableFavourites && ( ( + ) -const Notification = (props) => { - const classes = useStyles() - return ( - - ) -} - export default Notification diff --git a/ui/src/library/LibraryCreate.jsx b/ui/src/library/LibraryCreate.jsx index 8166bb2f3..0e69964b6 100644 --- a/ui/src/library/LibraryCreate.jsx +++ b/ui/src/library/LibraryCreate.jsx @@ -1,5 +1,4 @@ import React, { useCallback } from 'react' -import PropTypes from 'prop-types' import { Create, SimpleForm, @@ -11,34 +10,7 @@ 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() @@ -101,12 +73,9 @@ const LibraryCreate = (props) => { return ( } {...props}> - - - ) diff --git a/ui/src/library/LibraryEdit.jsx b/ui/src/library/LibraryEdit.jsx index c42c7ac4b..53d17ac7f 100644 --- a/ui/src/library/LibraryEdit.jsx +++ b/ui/src/library/LibraryEdit.jsx @@ -1,11 +1,9 @@ -import React, { useCallback, useState } from 'react' -import PropTypes from 'prop-types' +import React, { useCallback } from 'react' import { Edit, FormWithRedirect, TextInput, BooleanInput, - Confirm, required, SaveButton, useTranslate, @@ -24,9 +22,6 @@ import { ReadOnlySizeField, Title, } from '../common' -import config from '../config' -import { PIDInputs } from './PIDInput' -import { pidConfigChanged } from './pidPresets' const useStyles = makeStyles({ toolbar: { @@ -58,131 +53,8 @@ const CustomToolbar = ({ showDelete, ...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 translate = useTranslate() const [mutate] = useMutation() const notify = useNotify() const redirect = useRedirect() @@ -221,11 +93,91 @@ 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 deleted file mode 100644 index 926adc839..000000000 --- a/ui/src/library/LibraryEdit.test.jsx +++ /dev/null @@ -1,125 +0,0 @@ -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 deleted file mode 100644 index 6481dc392..000000000 --- a/ui/src/library/PIDInput.jsx +++ /dev/null @@ -1,114 +0,0 @@ -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 deleted file mode 100644 index 0483fc691..000000000 --- a/ui/src/library/pidPresets.js +++ /dev/null @@ -1,33 +0,0 @@ -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 deleted file mode 100644 index dab1c82e2..000000000 --- a/ui/src/library/pidPresets.test.js +++ /dev/null @@ -1,65 +0,0 @@ -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) - }) -}) diff --git a/ui/src/playlist/PlaylistList.jsx b/ui/src/playlist/PlaylistList.jsx index e1695e980..d2b17b108 100644 --- a/ui/src/playlist/PlaylistList.jsx +++ b/ui/src/playlist/PlaylistList.jsx @@ -67,15 +67,15 @@ const PlaylistFilter = (props) => { ) } -export const ToggleField = ({ resource, source }) => { +const TogglePublicInput = ({ resource, source }) => { const record = useRecordContext() const notify = useNotify() - const [toggle] = useUpdate( + const [togglePublic] = useUpdate( resource, - record?.id, + record.id, { ...record, - [source]: !record?.[source], + public: !record.public, }, { undoable: false, @@ -86,25 +86,48 @@ export const ToggleField = ({ resource, source }) => { ) const handleClick = (e) => { - toggle() + togglePublic() e.stopPropagation() } - if (!record) return null - return ( ) } -export const ToggleAutoImport = (props) => { +const ToggleAutoImport = ({ resource, source }) => { const record = useRecordContext() - return record?.path ? : null + const notify = useNotify() + const [ToggleAutoImport] = useUpdate( + resource, + record.id, + { + ...record, + sync: !record.sync, + }, + { + undoable: false, + onFailure: (error) => { + notify('ra.page.error', 'warning') + }, + }, + ) + const handleClick = (e) => { + ToggleAutoImport() + e.stopPropagation() + } + + return record.path ? ( + + ) : null } const PlaylistListBulkActions = (props) => { @@ -146,7 +169,9 @@ const PlaylistList = (props) => { updatedAt: isDesktop && ( ), - public: !isXsmall && , + public: !isXsmall && ( + + ), comment: , sync: !isXsmall && ( diff --git a/ui/src/playlist/PlaylistList.test.jsx b/ui/src/playlist/PlaylistList.test.jsx index c05833166..4fbc6d516 100644 --- a/ui/src/playlist/PlaylistList.test.jsx +++ b/ui/src/playlist/PlaylistList.test.jsx @@ -1,9 +1,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' +import { PlaylistLove } from './PlaylistList' vi.mock('../config', () => ({ default: { enableFavourites: true }, @@ -15,7 +13,6 @@ vi.mock('../common', () => ({ {record?.starred ? 'starred' : 'not-starred'} ), - isWritable: (ownerId) => ownerId === 'me', })) describe('', () => { @@ -35,50 +32,3 @@ describe('', () => { }) }) }) - -// react-admin evicts records older than 10 minutes while the list still holds -// their ids, so rows can render with no record. -describe('playlist toggles without a record', () => { - it(' renders nothing', () => { - const { container } = render( - - - , - ) - expect(container.innerHTML).toBe('') - }) - - it(' renders nothing', () => { - const { container } = render( - - - , - ) - 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/amusic.js b/ui/src/themes/amusic.js index eef7f90d6..55205baf3 100644 --- a/ui/src/themes/amusic.js +++ b/ui/src/themes/amusic.js @@ -79,16 +79,6 @@ export default { color: '#eee', backgroundColor: '#ff4e6b', }, - containedPrimary: { - color: '#fff', - backgroundColor: '#D60017', - '&:hover': { - backgroundColor: '#a30011', - '@media (hover: none)': { - backgroundColor: '#D60017', - }, - }, - }, textSizeSmall: { fontSize: '0.8rem', paddingRight: '0.5rem', @@ -202,11 +192,6 @@ export default { paddingBottom: '1rem', }, }, - NDNotification: { - undo: { - color: '#fff', - }, - }, RaConfirm: { confirmPrimary: { color: '#fff', diff --git a/ui/src/themes/dracula.js b/ui/src/themes/dracula.js index 45559c3af..2e4ae38e5 100644 --- a/ui/src/themes/dracula.js +++ b/ui/src/themes/dracula.js @@ -185,6 +185,16 @@ 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 3e2955dcd..0f4cbd7c4 100644 --- a/ui/src/themes/gruvboxDark.js +++ b/ui/src/themes/gruvboxDark.js @@ -121,6 +121,16 @@ 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/index.js b/ui/src/themes/index.js index 285e579ae..391a97f2c 100644 --- a/ui/src/themes/index.js +++ b/ui/src/themes/index.js @@ -25,8 +25,6 @@ import MoonbaseAlphaTheme from './moonbaseAlpha' import MoonbaseBravoTheme from './moonbaseBravo' import TokyoNightLightTheme from './tokyoNightLight' import TokyoNightTheme from './tokyoNight' -import SolarizedDarkTheme from './solarizedDark' -import SolarizedLightTheme from './solarizedLight' export default { // Classic default themes @@ -55,8 +53,6 @@ export default { RosePineDawnTheme, RosePineMoonTheme, RosePineTheme, - SolarizedDarkTheme, - SolarizedLightTheme, SpotifyTheme, SquiddiesGlassTheme, TokyoNightLightTheme, diff --git a/ui/src/themes/nuclear.js b/ui/src/themes/nuclear.js index bf804139e..b34896c97 100644 --- a/ui/src/themes/nuclear.js +++ b/ui/src/themes/nuclear.js @@ -86,13 +86,6 @@ export default { }, }, }, - NDNotification: { - undo: { - '& .MuiButton-label': { - color: 'inherit', - }, - }, - }, MuiChip: { root: { backgroundColor: nukeCol['accent'], diff --git a/ui/src/themes/solarizedDark.css.js b/ui/src/themes/solarizedDark.css.js deleted file mode 100644 index b357b8a0b..000000000 --- a/ui/src/themes/solarizedDark.css.js +++ /dev/null @@ -1,148 +0,0 @@ -const stylesheet = ` - -.react-jinke-music-player-main svg:active, .react-jinke-music-player-main svg:hover { - color: #268bd2 -} - -.react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-handle, .react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-track { - background-color: #268bd2 -} - -.react-jinke-music-player-main ::-webkit-scrollbar-thumb { - background-color: #268bd2; -} - -.react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-handle:active { - box-shadow: 0 0 2px #268bd2 -} - -.react-jinke-music-player-main .audio-item.playing svg { - color: #268bd2 -} - -.react-jinke-music-player-main .audio-item.playing .player-singer { - color: #268bd2 !important -} - -.react-jinke-music-player-main .loading svg { - color: #268bd2 !important -} - -.react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-handle { - border: hidden; - box-shadow: rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px; -} - -.rc-slider-rail, .rc-slider-track { - height: 6px; -} - -.react-jinke-music-player-main .rc-slider-rail, -.react-jinke-music-player-mobile-progress .rc-slider-rail { - background-color: rgba(147, 161, 161, .2) !important -} - -.rc-slider { - padding: 3px 0; -} - -.sound-operation > div:nth-child(4) { - transform: translateX(-50%) translateY(5%) !important; -} - -.sound-operation { - padding: 4px 0; -} - -.react-jinke-music-player-main .music-player-panel { - background-color: #073642; - color: #93a1a1; - box-shadow: 0 0 8px rgba(0, 0, 0, 0.25); -} - -.audio-lists-panel { - background-color: #073642; - bottom: 6.25rem; - box-shadow: rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px; -} - -.audio-lists-panel-content .audio-item.playing { - background-color: rgba(0, 0, 0, 0); -} - -.audio-lists-panel-content .audio-item:nth-child(2n+1) { - background-color: rgba(0, 0, 0, 0); -} - -.audio-lists-panel-content .audio-item:active, -.audio-lists-panel-content .audio-item:hover { - background-color: #0d4251; -} - -.audio-lists-panel-header { - border-bottom: 1px solid rgba(0, 0, 0, 0.25); - box-shadow: none; -} - -.react-jinke-music-player-main .music-player-panel .panel-content .player-content .audio-lists-btn { - background-color: rgba(0, 0, 0, 0); - box-shadow: 0 0 0 0; -} - -.audio-lists-panel-content .audio-item { - line-height: 32px; -} - -.react-jinke-music-player-main .music-player-panel .panel-content .img-content { - box-shadow: rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px; -} - -.react-jinke-music-player-main .music-player-lyric { - color: #93a1a1; - -webkit-text-stroke: 0.5px #002b36; - font-weight: bolder; -} - -.react-jinke-music-player-main .lyric-btn-active, .react-jinke-music-player-main .lyric-btn-active svg { - color: #268bd2 !important; -} - -.audio-lists-panel-content .audio-item.playing, .audio-lists-panel-content .audio-item.playing svg { - color: #268bd2 -} - -.audio-lists-panel-content .audio-item:active .group:not(.player-delete) svg, .audio-lists-panel-content .audio-item:hover .group:not(.player-delete) svg { - color: #268bd2 -} - -.audio-lists-panel-content .audio-item .player-icons { - scale: 75%; -} - -/* Mobile */ - -.react-jinke-music-player-mobile-cover { - border: none; - box-shadow: rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px; -} - -.react-jinke-music-player .music-player-controller { - border: none; - box-shadow: rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px; - color: #268bd2; -} - -.react-jinke-music-player .music-player-controller .music-player-controller-setting { - color: rgba(38, 139, 210, .3); -} - -.react-jinke-music-player-mobile-progress .rc-slider-handle, .react-jinke-music-player-mobile-progress .rc-slider-track { - background-color: #268bd2; -} - -.react-jinke-music-player-mobile-progress .rc-slider-handle { - border: none; -} -` - -export default stylesheet diff --git a/ui/src/themes/solarizedDark.js b/ui/src/themes/solarizedDark.js deleted file mode 100644 index 1e1fcdff4..000000000 --- a/ui/src/themes/solarizedDark.js +++ /dev/null @@ -1,380 +0,0 @@ -import stylesheet from './solarizedDark.css.js' - -// Solarized palette by Ethan Schoonover: https://ethanschoonover.com/solarized/ -const background = '#002b36' // base03 -const surface = '#073642' // base02 -const currentLine = '#0d4251' // base02, slightly lighter for hover and headers -const foreground = '#93a1a1' // base1 -const comment = '#839496' // base0 -const blue = '#268bd2' -const link = '#57b5ae' // cyan, lightened to keep text links above 4.5:1 -const accent = '#2aa198' // cyan, since violet is too dark on base03 -const red = '#dc322f' - -// For Album, Playlist play button -const musicListActions = { - alignItems: 'center', - '@global': { - 'button:first-child:not(:only-child)': { - '@media screen and (max-width: 720px)': { - transform: 'scale(1.5)', - margin: '1rem', - '&:hover': { - transform: 'scale(1.6) !important', - }, - }, - transform: 'scale(2)', - margin: '1.5rem', - minWidth: 0, - padding: 5, - transition: 'transform .3s ease', - backgroundColor: `${blue} !important`, - color: background, - borderRadius: 500, - border: 0, - '&:hover': { - transform: 'scale(2.1)', - backgroundColor: `${blue} !important`, - border: 0, - }, - }, - 'button:only-child': { - margin: '1.5rem', - }, - 'button:first-child>span:first-child': { - padding: 0, - }, - 'button:first-child>span:first-child>span': { - display: 'none', - }, - 'button>span:first-child>span, button:not(:first-child)>span:first-child>svg': - { - color: foreground, - }, - }, -} - -export default { - themeName: 'Solarized Dark', - palette: { - primary: { - main: blue, - }, - secondary: { - main: accent, - contrastText: background, - }, - error: { - main: red, - }, - type: 'dark', - background: { - default: background, - paper: surface, - }, - }, - overrides: { - MuiPaper: { - root: { - color: foreground, - backgroundColor: surface, - }, - }, - MuiAppBar: { - positionFixed: { - backgroundColor: `${surface} !important`, - boxShadow: - 'rgba(15, 17, 21, 0.25) 0px 4px 6px, rgba(15, 17, 21, 0.1) 0px 5px 7px', - }, - colorSecondary: { - color: foreground, - }, - }, - MuiDrawer: { - root: { - background: background, - }, - }, - MuiButton: { - textPrimary: { - color: blue, - }, - textSecondary: { - color: foreground, - }, - }, - MuiIconButton: { - root: { - color: foreground, - }, - }, - MuiChip: { - root: { - backgroundColor: currentLine, - }, - }, - MuiFormGroup: { - root: { - color: foreground, - }, - }, - MuiFormLabel: { - root: { - color: comment, - '&$focused': { - color: blue, - }, - }, - }, - MuiFormHelperText: { - error: { - color: red, - }, - }, - MuiToolbar: { - root: { - backgroundColor: `${surface} !important`, - }, - }, - MuiOutlinedInput: { - root: { - '& $notchedOutline': { - borderColor: currentLine, - }, - '&:hover $notchedOutline': { - borderColor: comment, - }, - '&$focused $notchedOutline': { - borderColor: blue, - }, - }, - }, - MuiFilledInput: { - root: { - backgroundColor: currentLine, - '&:hover': { - backgroundColor: comment, - }, - '&$focused': { - backgroundColor: currentLine, - }, - }, - }, - MuiTableRow: { - root: { - transition: 'background-color .3s ease', - '&:hover': { - backgroundColor: `${currentLine} !important`, - }, - // Cells have an opaque background, so the hover must be applied to them too - '&:hover > .MuiTableCell-root': { - background: `${currentLine} !important`, - }, - }, - }, - MuiTableHead: { - root: { - color: foreground, - background: surface, - }, - }, - MuiTableCell: { - root: { - color: foreground, - background: `${surface} !important`, - borderBottom: `1px solid ${currentLine}`, - }, - head: { - color: `${foreground} !important`, - background: `${currentLine} !important`, - }, - body: { - color: `${foreground} !important`, - }, - }, - NDAlbumGridView: { - albumName: { - marginTop: '0.5rem', - fontWeight: 700, - color: foreground, - }, - albumSubtitle: { - color: comment, - }, - albumContainer: { - backgroundColor: surface, - borderRadius: '8px', - padding: '.75rem', - transition: 'background-color .3s ease', - '&:hover': { - backgroundColor: currentLine, - }, - }, - albumPlayButton: { - backgroundColor: blue, - borderRadius: '50%', - boxShadow: '0 8px 8px rgb(0 0 0 / 30%)', - padding: '0.35rem', - transition: 'padding .3s ease', - '&:hover': { - background: `${blue} !important`, - padding: '0.45rem', - }, - }, - }, - NDPlaylistDetails: { - container: { - background: `linear-gradient(${currentLine}, transparent)`, - borderRadius: 0, - paddingTop: '2.5rem !important', - boxShadow: 'none', - }, - title: { - fontWeight: 700, - color: foreground, - }, - details: { - fontSize: '.875rem', - color: comment, - }, - }, - NDAlbumDetails: { - root: { - background: `linear-gradient(${currentLine}, transparent)`, - borderRadius: 0, - boxShadow: 'none', - }, - cardContents: { - alignItems: 'center', - paddingTop: '1.5rem', - }, - recordName: { - fontWeight: 700, - color: foreground, - }, - recordArtist: { - fontSize: '.875rem', - fontWeight: 700, - color: accent, - }, - recordMeta: { - fontSize: '.875rem', - color: comment, - }, - }, - NDCollapsibleComment: { - commentBlock: { - fontSize: '.875rem', - color: comment, - }, - }, - NDAlbumShow: { - albumActions: musicListActions, - }, - NDPlaylistShow: { - playlistActions: musicListActions, - }, - NDAudioPlayer: { - audioTitle: { - color: foreground, - fontSize: '0.875rem', - }, - songTitle: { - fontWeight: 400, - }, - songInfo: { - fontSize: '0.675rem', - color: comment, - }, - }, - NDLogin: { - systemNameLink: { - color: link, - }, - welcome: { - color: foreground, - }, - card: { - minWidth: 300, - background: surface, - }, - button: { - boxShadow: '3px 3px 5px #00212b', - }, - }, - NDMobileArtistDetails: { - bgContainer: { - background: `linear-gradient(to bottom, rgba(0 43 54 / 72%), ${background})!important`, - }, - }, - RaLayout: { - content: { - padding: '0 !important', - background: background, - }, - root: { - backgroundColor: background, - }, - }, - RaList: { - content: { - backgroundColor: background, - }, - }, - RaListToolbar: { - toolbar: { - backgroundColor: background, - padding: '0 .55rem !important', - }, - }, - RaSidebar: { - fixed: { - backgroundColor: background, - }, - drawerPaper: { - backgroundColor: `${background} !important`, - }, - }, - RaMenuItemLink: { - root: { - color: foreground, - '&[aria-current="page"]': { - color: `${blue} !important`, - }, - '&[aria-current="page"] .MuiListItemIcon-root': { - color: `${blue} !important`, - }, - }, - active: { - color: `${blue} !important`, - '& .MuiListItemIcon-root': { - color: `${blue} !important`, - }, - }, - }, - RaLink: { - link: { - color: link, - }, - }, - RaButton: { - button: { - margin: '0 5px 0 5px', - }, - }, - RaPaginationActions: { - currentPageButton: { - border: `2px solid ${blue}`, - }, - button: { - backgroundColor: currentLine, - minWidth: 48, - margin: '0 4px', - }, - }, - }, - player: { - theme: 'dark', - stylesheet, - }, -} diff --git a/ui/src/themes/solarizedLight.css.js b/ui/src/themes/solarizedLight.css.js deleted file mode 100644 index c6a599562..000000000 --- a/ui/src/themes/solarizedLight.css.js +++ /dev/null @@ -1,123 +0,0 @@ -const stylesheet = ` -.react-jinke-music-player-main.light-theme .loading svg { - color: #268bd2; - font-size: 24px -} - -.react-jinke-music-player-mobile-play-model-tip { - background-color: #268bd2; -} - -.react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-handle, .react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-track { - background-color: #268bd2 -} - -.react-jinke-music-player-main .music-player-panel .panel-content .rc-slider-handle:active { - box-shadow: 0 0 2px #268bd2 -} - -.react-jinke-music-player-main.light-theme .audio-item.playing svg { - color: #268bd2 -} - -.react-jinke-music-player-main.light-theme .audio-item.playing .player-singer { - color: #268bd2 !important -} - -.audio-lists-panel-content .audio-item.playing, .audio-lists-panel-content .audio-item.playing svg { - color: #268bd2 -} - -.audio-lists-panel-content .audio-item:active .group:not(.player-delete) svg, .audio-lists-panel-content .audio-item:hover .group:not(.player-delete) svg { - color: #268bd2 -} - -.react-jinke-music-player-main.light-theme ::-webkit-scrollbar-thumb { - background-color: #268bd2; -} - -.react-jinke-music-player-main.light-theme svg { - color: #586e75 -} - -.react-jinke-music-player-main.light-theme svg:active, .react-jinke-music-player-main.light-theme svg:hover { - color: #268bd2 -} - -.react-jinke-music-player-main.light-theme .rc-slider-rail { - background-color: rgba(88, 110, 117, .12) !important -} - -.react-jinke-music-player-main.light-theme .music-player-controller { - background-color: #fdf6e3; - border-color: #fdf6e3 -} - -.react-jinke-music-player-main.light-theme .music-player-panel { - background-color: #fdf6e3; - box-shadow: 0 1px 2px 0 rgba(0, 43, 54, .05); - color: #586e75 -} - -.react-jinke-music-player-main.light-theme .music-player-panel .img-content { - box-shadow: 0 0 10px #e4ddc6 -} - -.react-jinke-music-player-main.light-theme .music-player-panel .progress-load-bar { - background-color: rgba(88, 110, 117, .08) !important -} - -.react-jinke-music-player-main.light-theme .rc-switch { - color: #fdf6e3 -} - -.react-jinke-music-player-main.light-theme .rc-switch:after { - background-color: #fdf6e3 -} - -.react-jinke-music-player-main.light-theme .rc-switch-checked { - background-color: #268bd2 !important; - border: 1px solid #268bd2 -} - -.react-jinke-music-player-main.light-theme .rc-switch-inner { - color: #fdf6e3 -} - -.react-jinke-music-player-main.light-theme .audio-lists-btn { - background-color: #eee8d5 !important -} - -.react-jinke-music-player-main.light-theme .audio-lists-btn:active, .react-jinke-music-player-main.light-theme .audio-lists-btn:hover { - background-color: #e4ddc6; - color: #586e75 -} - -.react-jinke-music-player-main.light-theme .audio-lists-btn > .group:hover, .react-jinke-music-player-main.light-theme .audio-lists-btn > .group:hover > svg { - color: #268bd2 -} - -.react-jinke-music-player-main.light-theme .audio-lists-panel { - background-color: #fdf6e3; - box-shadow: 0 0 2px #e4ddc6; - color: #586e75 -} - -.react-jinke-music-player-main.light-theme .audio-lists-panel .audio-item { - background-color: #fdf6e3 -} - -.react-jinke-music-player-main.light-theme .audio-lists-panel .audio-item:nth-child(odd) { - background-color: #f5efdc !important -} - -.react-jinke-music-player-main.light-theme .audio-lists-panel .audio-item.playing { - background-color: #e4ddc6 !important -} - -.react-jinke-music-player-main.light-theme .audio-lists-panel .audio-item.playing, .react-jinke-music-player-main.light-theme .audio-lists-panel .audio-item.playing svg { - color: #268bd2 !important -} -` - -export default stylesheet diff --git a/ui/src/themes/solarizedLight.js b/ui/src/themes/solarizedLight.js deleted file mode 100644 index 65e0241e9..000000000 --- a/ui/src/themes/solarizedLight.js +++ /dev/null @@ -1,380 +0,0 @@ -import stylesheet from './solarizedLight.css.js' - -// Solarized palette by Ethan Schoonover: https://ethanschoonover.com/solarized/ -const background = '#eee8d5' // base2 -const surface = '#fdf6e3' // base3 -const currentLine = '#e4ddc6' // base2, slightly darker for hover and headers -const foreground = '#586e75' // base01 -const comment = '#657b83' // base00 -const blue = '#268bd2' -const link = '#1c6599' // blue, darkened to keep text links above 4.5:1 -const accent = '#5d61a9' // violet, darkened to keep text above 4.5:1 -const red = '#dc322f' - -// For Album, Playlist play button -const musicListActions = { - alignItems: 'center', - '@global': { - 'button:first-child:not(:only-child)': { - '@media screen and (max-width: 720px)': { - transform: 'scale(1.5)', - margin: '1rem', - '&:hover': { - transform: 'scale(1.6) !important', - }, - }, - transform: 'scale(2)', - margin: '1.5rem', - minWidth: 0, - padding: 5, - transition: 'transform .3s ease', - backgroundColor: `${blue} !important`, - color: background, - borderRadius: 500, - border: 0, - '&:hover': { - transform: 'scale(2.1)', - backgroundColor: `${blue} !important`, - border: 0, - }, - }, - 'button:only-child': { - margin: '1.5rem', - }, - 'button:first-child>span:first-child': { - padding: 0, - }, - 'button:first-child>span:first-child>span': { - display: 'none', - }, - 'button>span:first-child>span, button:not(:first-child)>span:first-child>svg': - { - color: foreground, - }, - }, -} - -export default { - themeName: 'Solarized Light', - palette: { - primary: { - main: blue, - }, - secondary: { - main: accent, - contrastText: surface, - }, - error: { - main: red, - }, - type: 'light', - background: { - default: background, - paper: surface, - }, - }, - overrides: { - MuiPaper: { - root: { - color: foreground, - backgroundColor: surface, - }, - }, - MuiAppBar: { - positionFixed: { - backgroundColor: `${surface} !important`, - boxShadow: - 'rgba(0, 43, 54, 0.12) 0px 4px 6px, rgba(0, 43, 54, 0.06) 0px 5px 7px', - }, - colorSecondary: { - color: foreground, - }, - }, - MuiDrawer: { - root: { - background: background, - }, - }, - MuiButton: { - textPrimary: { - color: blue, - }, - textSecondary: { - color: foreground, - }, - }, - MuiIconButton: { - root: { - color: foreground, - }, - }, - MuiChip: { - root: { - backgroundColor: currentLine, - }, - }, - MuiFormGroup: { - root: { - color: foreground, - }, - }, - MuiFormLabel: { - root: { - color: comment, - '&$focused': { - color: blue, - }, - }, - }, - MuiFormHelperText: { - error: { - color: red, - }, - }, - MuiToolbar: { - root: { - backgroundColor: `${surface} !important`, - }, - }, - MuiOutlinedInput: { - root: { - '& $notchedOutline': { - borderColor: currentLine, - }, - '&:hover $notchedOutline': { - borderColor: comment, - }, - '&$focused $notchedOutline': { - borderColor: blue, - }, - }, - }, - MuiFilledInput: { - root: { - backgroundColor: currentLine, - '&:hover': { - backgroundColor: comment, - }, - '&$focused': { - backgroundColor: currentLine, - }, - }, - }, - MuiTableRow: { - root: { - transition: 'background-color .3s ease', - '&:hover': { - backgroundColor: `${currentLine} !important`, - }, - // Cells have an opaque background, so the hover must be applied to them too - '&:hover > .MuiTableCell-root': { - background: `${currentLine} !important`, - }, - }, - }, - MuiTableHead: { - root: { - color: foreground, - background: surface, - }, - }, - MuiTableCell: { - root: { - color: foreground, - background: `${surface} !important`, - borderBottom: `1px solid ${currentLine}`, - }, - head: { - color: `${foreground} !important`, - background: `${currentLine} !important`, - }, - body: { - color: `${foreground} !important`, - }, - }, - NDAlbumGridView: { - albumName: { - marginTop: '0.5rem', - fontWeight: 700, - color: foreground, - }, - albumSubtitle: { - color: comment, - }, - albumContainer: { - backgroundColor: surface, - borderRadius: '8px', - padding: '.75rem', - transition: 'background-color .3s ease', - '&:hover': { - backgroundColor: currentLine, - }, - }, - albumPlayButton: { - backgroundColor: blue, - borderRadius: '50%', - boxShadow: '0 8px 8px rgb(0 0 0 / 20%)', - padding: '0.35rem', - transition: 'padding .3s ease', - '&:hover': { - background: `${blue} !important`, - padding: '0.45rem', - }, - }, - }, - NDPlaylistDetails: { - container: { - background: `linear-gradient(${currentLine}, transparent)`, - borderRadius: 0, - paddingTop: '2.5rem !important', - boxShadow: 'none', - }, - title: { - fontWeight: 700, - color: foreground, - }, - details: { - fontSize: '.875rem', - color: comment, - }, - }, - NDAlbumDetails: { - root: { - background: `linear-gradient(${currentLine}, transparent)`, - borderRadius: 0, - boxShadow: 'none', - }, - cardContents: { - alignItems: 'center', - paddingTop: '1.5rem', - }, - recordName: { - fontWeight: 700, - color: foreground, - }, - recordArtist: { - fontSize: '.875rem', - fontWeight: 700, - color: accent, - }, - recordMeta: { - fontSize: '.875rem', - color: comment, - }, - }, - NDCollapsibleComment: { - commentBlock: { - fontSize: '.875rem', - color: comment, - }, - }, - NDAlbumShow: { - albumActions: musicListActions, - }, - NDPlaylistShow: { - playlistActions: musicListActions, - }, - NDAudioPlayer: { - audioTitle: { - color: foreground, - fontSize: '0.875rem', - }, - songTitle: { - fontWeight: 400, - }, - songInfo: { - fontSize: '0.675rem', - color: comment, - }, - }, - NDLogin: { - systemNameLink: { - color: link, - }, - welcome: { - color: foreground, - }, - card: { - minWidth: 300, - background: surface, - }, - button: { - boxShadow: '3px 3px 5px #93a1a1', - }, - }, - NDMobileArtistDetails: { - bgContainer: { - background: `linear-gradient(to bottom, rgba(238 232 213 / 72%), ${background})!important`, - }, - }, - RaLayout: { - content: { - padding: '0 !important', - background: background, - }, - root: { - backgroundColor: background, - }, - }, - RaList: { - content: { - backgroundColor: background, - }, - }, - RaListToolbar: { - toolbar: { - backgroundColor: background, - padding: '0 .55rem !important', - }, - }, - RaSidebar: { - fixed: { - backgroundColor: background, - }, - drawerPaper: { - backgroundColor: `${background} !important`, - }, - }, - RaMenuItemLink: { - root: { - color: foreground, - '&[aria-current="page"]': { - color: `${blue} !important`, - }, - '&[aria-current="page"] .MuiListItemIcon-root': { - color: `${blue} !important`, - }, - }, - active: { - color: `${blue} !important`, - '& .MuiListItemIcon-root': { - color: `${blue} !important`, - }, - }, - }, - RaLink: { - link: { - color: link, - }, - }, - RaButton: { - button: { - margin: '0 5px 0 5px', - }, - }, - RaPaginationActions: { - currentPageButton: { - border: `2px solid ${blue}`, - }, - button: { - backgroundColor: currentLine, - minWidth: 48, - margin: '0 4px', - }, - }, - }, - player: { - theme: 'light', - stylesheet, - }, -} diff --git a/ui/src/themes/tokyoNight.js b/ui/src/themes/tokyoNight.js index 9f6424b77..07d372a6b 100644 --- a/ui/src/themes/tokyoNight.js +++ b/ui/src/themes/tokyoNight.js @@ -184,6 +184,16 @@ 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 a61c0fe87..f84cd0be9 100644 --- a/ui/src/themes/tokyoNightLight.js +++ b/ui/src/themes/tokyoNightLight.js @@ -184,6 +184,16 @@ 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 fbb5e9bc8..4ccefe820 100644 --- a/ui/src/themes/useCurrentTheme.js +++ b/ui/src/themes/useCurrentTheme.js @@ -63,8 +63,6 @@ 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 6553d9866..65c3be8c6 100644 --- a/ui/src/themes/useCurrentTheme.test.jsx +++ b/ui/src/themes/useCurrentTheme.test.jsx @@ -3,10 +3,6 @@ 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' @@ -165,27 +161,4 @@ 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') - }, - ) - }) }) diff --git a/utils/cache/spread_fs.go b/utils/cache/spread_fs.go index 11801b324..3ebd00e1a 100644 --- a/utils/cache/spread_fs.go +++ b/utils/cache/spread_fs.go @@ -12,6 +12,7 @@ import ( "github.com/djherbis/fscache" "github.com/djherbis/stream" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/utils" ) const completeMarkerSuffix = ".complete" @@ -97,7 +98,7 @@ func (sfs *spreadFS) walkDataFiles(visit func(absoluteFilePath string)) error { log.Error("Error loading cache", "dir", sfs.root, err) return nil } - path, err := filepath.Rel(sfs.root, absoluteFilePath) + path, err := utils.RelPath(sfs.root, absoluteFilePath) if err != nil { return nil //nolint:nilerr } diff --git a/utils/files.go b/utils/files.go index 2fce307ea..8ef522e5e 100644 --- a/utils/files.go +++ b/utils/files.go @@ -40,3 +40,16 @@ func FileExists(path string) bool { _, err := os.Stat(path) return err == nil || !os.IsNotExist(err) } + +// RelPath is filepath.Rel without the UNC share root hang (golang/go#79784). Remove it after moving to Go 1.28. +func RelPath(basePath, targPath string) (string, error) { + return filepath.Rel(normalizeUNCRoot(basePath), normalizeUNCRoot(targPath)) +} + +func normalizeUNCRoot(p string) string { + vol := filepath.VolumeName(p) + if len(vol) > 2 && strings.Trim(p[len(vol):], `\/`) == "" { + return vol + string(filepath.Separator) + } + return p +} diff --git a/utils/files_test.go b/utils/files_test.go index c6e578f05..04b4d4e0b 100644 --- a/utils/files_test.go +++ b/utils/files_test.go @@ -223,3 +223,26 @@ var _ = Describe("FileExists", func() { Expect(result).To(Or(BeTrue(), BeFalse())) // Should not panic }) }) + +var _ = Describe("RelPath", func() { + DescribeTable("returns the same result as filepath.Rel", + func(base, target string) { + base, target = filepath.FromSlash(base), filepath.FromSlash(target) + expected, expectedErr := filepath.Rel(base, target) + rel, err := utils.RelPath(base, target) + Expect(rel).To(Equal(expected)) + if expectedErr == nil { + Expect(err).ToNot(HaveOccurred()) + } else { + Expect(err).To(MatchError(expectedErr.Error())) + } + }, + Entry("same path", "/music", "/music"), + Entry("same path with trailing separator", "/music", "/music/"), + Entry("child path", "/music", "/music/artist/album"), + Entry("sibling path", "/music/a", "/music/b"), + Entry("parent path", "/music/artist", "/music"), + Entry("relative paths", "music", "music/artist"), + Entry("absolute and relative paths", "/music", "music"), + ) +}) diff --git a/utils/files_windows_test.go b/utils/files_windows_test.go new file mode 100644 index 000000000..419291b98 --- /dev/null +++ b/utils/files_windows_test.go @@ -0,0 +1,40 @@ +package utils_test + +import ( + "time" + + "github.com/navidrome/navidrome/utils" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("RelPath on Windows", func() { + type result struct { + rel string + err error + } + + DescribeTable("handles UNC share roots without hanging", + func(base, target, expected string) { + done := make(chan result, 1) + go func() { + rel, err := utils.RelPath(base, target) + done <- result{rel, err} + }() + var res result + Eventually(done).WithTimeout(5 * time.Second).Should(Receive(&res)) + Expect(res.err).ToNot(HaveOccurred()) + Expect(res.rel).To(Equal(expected)) + }, + Entry("root and root with trailing separator", `\\server\Music`, `\\server\Music\`, "."), + Entry("root with trailing separator and root", `\\server\Music\`, `\\server\Music`, "."), + Entry("root and root with many separators", `\\server\Music`, `\\server\Music\\`, "."), + Entry("root with forward slashes", `//server/Music`, `//server/Music/`, "."), + Entry("root and child folder", `\\server\Music`, `\\server\Music\Artist`, "Artist"), + Entry("root with trailing separator and child folder", `\\server\Music\`, `\\server\Music\Artist\Album`, `Artist\Album`), + Entry("child folder and root", `\\server\Music\Artist`, `\\server\Music\`, ".."), + Entry("child folder and root without trailing separator", `\\server\Music\Artist`, `\\server\Music`, ".."), + Entry("root and root with different case", `\\server\Music`, `\\SERVER\music\`, "."), + Entry("drive root and child folder", `C:\`, `C:\Music`, "Music"), + ) +})