fix(server): preserve m3u file order on import (#3314)

* fix(playlist): preserve m3u file order on import - 3307

Signed-off-by: Kendall Garner <17521368+kgarner7@users.noreply.github.com>

* test(server): cover playlist order

* refactor(server): micro-optimizations

* refactor(server): micro-optimizations

* fix(server): playlists imported from reader (POST /playlist) are not synced

* refactor(server): only allocate the capacity required to hold a playlist chunk

---------

Signed-off-by: Kendall Garner <17521368+kgarner7@users.noreply.github.com>
Co-authored-by: Deluan <deluan@navidrome.org>

core/playlists.go

@@ -54,9 +54,9 @@ func (s *playlists) ImportM3U(ctx context.Context, reader io.Reader) (*model.Pla
 	pls := &model.Playlist{
 		OwnerID: owner.ID,
 		Public:  false,
-		Sync:    true,
+		Sync:    false,
 	}
-	pls, err := s.parseM3U(ctx, pls, "", reader)
+	err := s.parseM3U(ctx, pls, "", reader)
 	if err != nil {
 		log.Error(ctx, "Error parsing playlist", err)
 		return nil, err
@@ -84,10 +84,11 @@ func (s *playlists) parsePlaylist(ctx context.Context, playlistFile string, base
 	extension := strings.ToLower(filepath.Ext(playlistFile))
 	switch extension {
 	case ".nsp":
-		return s.parseNSP(ctx, pls, file)
+		err = s.parseNSP(ctx, pls, file)
 	default:
-		return s.parseM3U(ctx, pls, baseDir, file)
+		err = s.parseM3U(ctx, pls, baseDir, file)
 	}
+	return pls, err
 }
 
 func (s *playlists) newSyncedPlaylist(baseDir string, playlistFile string) (*model.Playlist, error) {
@@ -111,14 +112,14 @@ func (s *playlists) newSyncedPlaylist(baseDir string, playlistFile string) (*mod
 	return pls, nil
 }
 
-func (s *playlists) parseNSP(ctx context.Context, pls *model.Playlist, file io.Reader) (*model.Playlist, error) {
+func (s *playlists) parseNSP(ctx context.Context, pls *model.Playlist, file io.Reader) error {
 	nsp := &nspFile{}
 	reader := jsoncommentstrip.NewReader(file)
 	dec := json.NewDecoder(reader)
 	err := dec.Decode(nsp)
 	if err != nil {
 		log.Error(ctx, "Error parsing SmartPlaylist", "playlist", pls.Name, err)
-		return nil, err
+		return err
 	}
 	pls.Rules = &nsp.Criteria
 	if nsp.Name != "" {
@@ -127,20 +128,18 @@ func (s *playlists) parseNSP(ctx context.Context, pls *model.Playlist, file io.R
 	if nsp.Comment != "" {
 		pls.Comment = nsp.Comment
 	}
-	return pls, nil
+	return nil
 }
 
-func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, baseDir string, reader io.Reader) (*model.Playlist, error) {
+func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, baseDir string, reader io.Reader) error {
 	mediaFileRepository := s.ds.MediaFile(ctx)
 	var mfs model.MediaFiles
 	for lines := range slice.CollectChunks(slice.LinesFrom(reader), 400) {
-		var filteredLines []string
+		filteredLines := make([]string, 0, len(lines))
 		for _, line := range lines {
 			line := strings.TrimSpace(line)
 			if strings.HasPrefix(line, "#PLAYLIST:") {
-				if split := strings.Split(line, ":"); len(split) >= 2 {
+				pls.Name = line[len("#PLAYLIST:"):]
-					pls.Name = split[1]
-				}
 				continue
 			}
 			// Skip empty lines and extended info
@@ -161,10 +160,18 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, baseDir s
 			log.Warn(ctx, "Error reading files from DB", "playlist", pls.Name, err)
 			continue
 		}
-		if len(found) != len(filteredLines) {
+		existing := make(map[string]int, len(found))
-			logMissingFiles(ctx, pls, filteredLines, found)
+		for idx := range found {
+			existing[found[idx].Path] = idx
+		}
+		for _, path := range filteredLines {
+			idx, ok := existing[path]
+			if ok {
+				mfs = append(mfs, found[idx])
+			} else {
+				log.Warn(ctx, "Path in playlist not found", "playlist", pls.Name, "path", path)
+			}
 		}
-		mfs = append(mfs, found...)
 	}
 	if pls.Name == "" {
 		pls.Name = time.Now().Format(time.RFC3339)
@@ -172,20 +179,7 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, baseDir s
 	pls.Tracks = nil
 	pls.AddMediaFiles(mfs)
 
-	return pls, nil
+	return nil
-}
-
-func logMissingFiles(ctx context.Context, pls *model.Playlist, lines []string, found model.MediaFiles) {
-	missing := make(map[string]bool)
-	for _, line := range lines {
-		missing[line] = true
-	}
-	for _, mf := range found {
-		delete(missing, mf.Path)
-	}
-	for path := range missing {
-		log.Warn(ctx, "Path in playlist not found", "playlist", pls.Name, "path", path)
-	}
 }
 
 func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist) error {

core/playlists_test.go

@@ -3,19 +3,20 @@ package core
 import (
 	"context"
 	"os"
+	"strconv"
+	"strings"
 	"time"
 
+	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/criteria"
 	"github.com/navidrome/navidrome/model/request"
-
-	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/tests"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
 var _ = Describe("Playlists", func() {
-	var ds model.DataStore
+	var ds *tests.MockDataStore
 	var ps Playlists
 	var mp mockedPlaylist
 	ctx := context.Background()
@@ -23,8 +24,7 @@ var _ = Describe("Playlists", func() {
 	BeforeEach(func() {
 		mp = mockedPlaylist{}
 		ds = &tests.MockDataStore{
-			MockedMediaFile: &mockedMediaFile{},
+			MockedPlaylist: &mp,
-			MockedPlaylist:  &mp,
 		}
 		ctx = request.WithUser(ctx, model.User{ID: "123"})
 	})
@@ -32,12 +32,13 @@ var _ = Describe("Playlists", func() {
 	Describe("ImportFile", func() {
 		BeforeEach(func() {
 			ps = NewPlaylists(ds)
+			ds.MockedMediaFile = &mockedMediaFileRepo{}
 		})
 
 		Describe("M3U", func() {
 			It("parses well-formed playlists", func() {
 				pls, err := ps.ImportFile(ctx, "tests/fixtures", "playlists/pls1.m3u")
-				Expect(err).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
 				Expect(pls.OwnerID).To(Equal("123"))
 				Expect(pls.Tracks).To(HaveLen(3))
 				Expect(pls.Tracks[0].Path).To(Equal("tests/fixtures/test.mp3"))
@@ -48,13 +49,13 @@ var _ = Describe("Playlists", func() {
 
 			It("parses playlists using LF ending", func() {
 				pls, err := ps.ImportFile(ctx, "tests/fixtures/playlists", "lf-ended.m3u")
-				Expect(err).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
 				Expect(pls.Tracks).To(HaveLen(2))
 			})
 
 			It("parses playlists using CR ending (old Mac format)", func() {
 				pls, err := ps.ImportFile(ctx, "tests/fixtures/playlists", "cr-ended.m3u")
-				Expect(err).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
 				Expect(pls.Tracks).To(HaveLen(2))
 			})
 		})
@@ -62,7 +63,7 @@ var _ = Describe("Playlists", func() {
 		Describe("NSP", func() {
 			It("parses well-formed playlists", func() {
 				pls, err := ps.ImportFile(ctx, "tests/fixtures", "playlists/recently_played.nsp")
-				Expect(err).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
 				Expect(mp.last).To(Equal(pls))
 				Expect(pls.OwnerID).To(Equal("123"))
 				Expect(pls.Name).To(Equal("Recently Played"))
@@ -76,18 +77,28 @@ var _ = Describe("Playlists", func() {
 	})
 
 	Describe("ImportM3U", func() {
+		var repo *mockedMediaFileFromListRepo
 		BeforeEach(func() {
+			repo = &mockedMediaFileFromListRepo{}
+			ds.MockedMediaFile = repo
 			ps = NewPlaylists(ds)
 			ctx = request.WithUser(ctx, model.User{ID: "123"})
 		})
 
 		It("parses well-formed playlists", func() {
-			f, _ := os.Open("tests/fixtures/playlists/pls-post-with-name.m3u")
+			repo.data = []string{
+				"tests/fixtures/test.mp3",
+				"tests/fixtures/test.ogg",
+				"/tests/fixtures/01 Invisible (RED) Edit Version.mp3",
+			}
+			f, _ := os.Open("tests/fixtures/playlists/pls-with-name.m3u")
 			defer f.Close()
 			pls, err := ps.ImportM3U(ctx, f)
+			Expect(err).ToNot(HaveOccurred())
 			Expect(pls.OwnerID).To(Equal("123"))
 			Expect(pls.Name).To(Equal("playlist 1"))
-			Expect(err).To(BeNil())
+			Expect(pls.Sync).To(BeFalse())
+			Expect(pls.Tracks).To(HaveLen(3))
 			Expect(pls.Tracks[0].Path).To(Equal("tests/fixtures/test.mp3"))
 			Expect(pls.Tracks[1].Path).To(Equal("tests/fixtures/test.ogg"))
 			Expect(pls.Tracks[2].Path).To(Equal("/tests/fixtures/01 Invisible (RED) Edit Version.mp3"))
@@ -97,32 +108,72 @@ var _ = Describe("Playlists", func() {
 		})
 
 		It("sets the playlist name as a timestamp if the #PLAYLIST directive is not present", func() {
-			f, _ := os.Open("tests/fixtures/playlists/pls-post.m3u")
+			repo.data = []string{
+				"tests/fixtures/test.mp3",
+				"tests/fixtures/test.ogg",
+				"/tests/fixtures/01 Invisible (RED) Edit Version.mp3",
+			}
+			f, _ := os.Open("tests/fixtures/playlists/pls-without-name.m3u")
 			defer f.Close()
 			pls, err := ps.ImportM3U(ctx, f)
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
 			_, err = time.Parse(time.RFC3339, pls.Name)
-			Expect(err).To(BeNil())
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pls.Tracks).To(HaveLen(3))
+		})
+
+		It("returns only tracks that exist in the database and in the same other as the m3u", func() {
+			repo.data = []string{
+				"test1.mp3",
+				"test2.mp3",
+				"test3.mp3",
+			}
+			m3u := strings.Join([]string{
+				"test3.mp3",
+				"test1.mp3",
+				"test4.mp3",
+				"test2.mp3",
+			}, "\n")
+			f := strings.NewReader(m3u)
+			pls, err := ps.ImportM3U(ctx, f)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pls.Tracks).To(HaveLen(3))
+			Expect(pls.Tracks[0].Path).To(Equal("test3.mp3"))
+			Expect(pls.Tracks[1].Path).To(Equal("test1.mp3"))
+			Expect(pls.Tracks[2].Path).To(Equal("test2.mp3"))
 		})
 	})
 })
 
-type mockedMediaFile struct {
+// mockedMediaFileRepo's FindByPaths method returns a list of MediaFiles with the same paths as the input
+type mockedMediaFileRepo struct {
 	model.MediaFileRepository
 }
 
-func (r *mockedMediaFile) FindByPath(s string) (*model.MediaFile, error) {
+func (r *mockedMediaFileRepo) FindByPaths(paths []string) (model.MediaFiles, error) {
-	return &model.MediaFile{
+	var mfs model.MediaFiles
-		ID:   "123",
+	for idx, path := range paths {
-		Path: s,
+		mfs = append(mfs, model.MediaFile{
-	}, nil
+			ID:   strconv.Itoa(idx),
+			Path: path,
+		})
+	}
+	return mfs, nil
 }
 
-func (r *mockedMediaFile) FindByPaths(paths []string) (model.MediaFiles, error) {
+// mockedMediaFileFromListRepo's FindByPaths method returns a list of MediaFiles based on the data field
+type mockedMediaFileFromListRepo struct {
+	model.MediaFileRepository
+	data []string
+}
+
+func (r *mockedMediaFileFromListRepo) FindByPaths([]string) (model.MediaFiles, error) {
 	var mfs model.MediaFiles
-	for _, path := range paths {
+	for idx, path := range r.data {
-		mf, _ := r.FindByPath(path)
+		mfs = append(mfs, model.MediaFile{
-		mfs = append(mfs, *mf)
+			ID:   strconv.Itoa(idx),
+			Path: path,
+		})
 	}
 	return mfs, nil
 }