From 5dce499d6dc77231a49eb8d11f3e2b595bb2066e Mon Sep 17 00:00:00 2001
From: Deluan <deluan@navidrome.org>
Date: Tue, 26 Oct 2021 14:05:28 -0400
Subject: [PATCH] Fix/Optimized Playlist tracks deletion
---
model/playlist.go | 2 +-
persistence/playlist_repository.go | 21 +++++++++-------
persistence/playlist_track_repository.go | 8 +++----
server/nativeapi/native_api.go | 11 ++++++---
server/nativeapi/playlists.go | 28 ++++++++++++++++------
server/subsonic/playlists.go | 1 -
ui/src/dataProvider/wrapperDataProvider.js | 11 +++++++++
7 files changed, 57 insertions(+), 25 deletions(-)
diff --git a/model/playlist.go b/model/playlist.go
index 280f802fa..0a941a1ac 100644
--- a/model/playlist.go
+++ b/model/playlist.go
@@ -111,6 +111,6 @@ type PlaylistTrackRepository interface {
AddAlbums(albumIds []string) (int, error)
AddArtists(artistIds []string) (int, error)
AddDiscs(discs []DiscID) (int, error)
- Delete(id string) error
+ Delete(id ...string) error
Reorder(pos int, newPos int) error
}
diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go
index add7b461f..2b41063a6 100644
--- a/persistence/playlist_repository.go
+++ b/persistence/playlist_repository.go
@@ -89,10 +89,6 @@ func (r *playlistRepository) Put(p *model.Playlist) error {
}
pls.UpdatedAt = time.Now()
- // Save tracks for later and set it to nil, to avoid trying to save it to the DB
- tracks := pls.Tracks
- pls.Tracks = nil
-
id, err := r.put(pls.ID, pls)
if err != nil {
return err
@@ -104,7 +100,7 @@ func (r *playlistRepository) Put(p *model.Playlist) error {
return nil
}
// Only update tracks if they were specified
- if tracks == nil {
+ if len(pls.Tracks) == 0 {
return nil
}
return r.updateTracks(id, p.MediaFiles())
@@ -405,15 +401,24 @@ func (r *playlistRepository) removeOrphans() error {
}
log.Debug(r.ctx, "Deleted tracks, now reordering", "id", pl.Id, "name", pl.Name, "deleted", n)
- // To reorganize the playlist, just add an empty list of new tracks
- tracks := r.Tracks(pl.Id)
- if _, err := tracks.Add(nil); err != nil {
+ // Renumber the playlist if any track was removed
+ if err := r.renumber(pl.Id); err != nil {
return err
}
}
return nil
}
+func (r *playlistRepository) renumber(id string) error {
+ var ids []string
+ sql := Select("media_file_id").From("playlist_tracks").Where(Eq{"playlist_id": id}).OrderBy("id")
+ err := r.queryAll(sql, &ids)
+ if err != nil {
+ return err
+ }
+ return r.updatePlaylist(id, ids)
+}
+
func (r *playlistRepository) isWritable(playlistId string) bool {
usr := loggedUser(r.ctx)
if usr.IsAdmin {
diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go
index bb26c4e3c..1953fb87a 100644
--- a/persistence/playlist_track_repository.go
+++ b/persistence/playlist_track_repository.go
@@ -164,18 +164,16 @@ func (r *playlistTrackRepository) getTracks() ([]string, error) {
return ids, nil
}
-func (r *playlistTrackRepository) Delete(id string) error {
+func (r *playlistTrackRepository) Delete(ids ...string) error {
if !r.isTracksEditable() {
return rest.ErrPermissionDenied
}
- err := r.delete(And{Eq{"playlist_id": r.playlistId}, Eq{"id": id}})
+ err := r.delete(And{Eq{"playlist_id": r.playlistId}, Eq{"id": ids}})
if err != nil {
return err
}
- // To renumber the playlist
- _, err = r.Add(nil)
- return err
+ return r.playlistRepo.renumber(r.playlistId)
}
func (r *playlistTrackRepository) Reorder(pos int, newPos int) error {
diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go
index 2f39dd733..d530f3849 100644
--- a/server/nativeapi/native_api.go
+++ b/server/nativeapi/native_api.go
@@ -87,6 +87,14 @@ func (n *Router) addPlaylistTrackRoute(r chi.Router) {
r.Get("/", func(w http.ResponseWriter, r *http.Request) {
getPlaylist(n.ds)(w, r)
})
+ r.With(urlParams).Route("/", func(r chi.Router) {
+ r.Delete("/", func(w http.ResponseWriter, r *http.Request) {
+ deleteFromPlaylist(n.ds)(w, r)
+ })
+ r.Post("/", func(w http.ResponseWriter, r *http.Request) {
+ addToPlaylist(n.ds)(w, r)
+ })
+ })
r.Route("/{id}", func(r chi.Router) {
r.Use(urlParams)
r.Put("/", func(w http.ResponseWriter, r *http.Request) {
@@ -96,9 +104,6 @@ func (n *Router) addPlaylistTrackRoute(r chi.Router) {
deleteFromPlaylist(n.ds)(w, r)
})
})
- r.With(urlParams).Post("/", func(w http.ResponseWriter, r *http.Request) {
- addToPlaylist(n.ds)(w, r)
- })
})
}
diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go
index 0685536a7..0f9c5ac9f 100644
--- a/server/nativeapi/playlists.go
+++ b/server/nativeapi/playlists.go
@@ -84,20 +84,34 @@ func handleExportPlaylist(ds model.DataStore) http.HandlerFunc {
func deleteFromPlaylist(ds model.DataStore) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
playlistId := utils.ParamString(r, ":playlistId")
- id := r.URL.Query().Get(":id")
- tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId)
- err := tracksRepo.Delete(id)
- if err == model.ErrNotFound {
- log.Warn("Track not found in playlist", "playlistId", playlistId, "id", id)
+ ids := r.URL.Query()["id"]
+ err := ds.WithTx(func(tx model.DataStore) error {
+ tracksRepo := tx.Playlist(r.Context()).Tracks(playlistId)
+ return tracksRepo.Delete(ids...)
+ })
+ if len(ids) == 1 && err == model.ErrNotFound {
+ log.Warn(r.Context(), "Track not found in playlist", "playlistId", playlistId, "id", ids[0])
http.Error(w, "not found", http.StatusNotFound)
return
}
if err != nil {
- log.Error("Error deleting track from playlist", "playlistId", playlistId, "id", id, err)
+ log.Error(r.Context(), "Error deleting tracks from playlist", "playlistId", playlistId, "ids", ids, err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
- _, err = w.Write([]byte("{}"))
+ var resp []byte
+ if len(ids) == 1 {
+ resp = []byte(`{"id":"` + ids[0] + `"}`)
+ } else {
+ resp, err = json.Marshal(&struct {
+ Ids []string `json:"ids"`
+ }{Ids: ids})
+ if err != nil {
+ log.Error(r.Context(), "Error marshaling delete response", "playlistId", playlistId, "ids", ids, err)
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
+ }
+ _, err = w.Write(resp)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go
index a7857b634..aa6563d39 100644
--- a/server/subsonic/playlists.go
+++ b/server/subsonic/playlists.go
@@ -69,7 +69,6 @@ func (c *PlaylistsController) create(ctx context.Context, playlistId, name strin
var pls *model.Playlist
var err error
- // If playlistID is present, override tracks
if playlistId != "" {
pls, err = tx.Playlist(ctx).Get(playlistId)
if err != nil {
diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js
index a317dfa88..a86da6536 100644
--- a/ui/src/dataProvider/wrapperDataProvider.js
+++ b/ui/src/dataProvider/wrapperDataProvider.js
@@ -19,6 +19,14 @@ const mapResource = (resource, params) => {
}
}
+const callDeleteMany = (resource, params) => {
+ const ids = params.ids.map((id) => `id=${id}`)
+ const idsParam = ids.join('&')
+ return httpClient(`${REST_URL}/${resource}?${idsParam}`, {
+ method: 'DELETE',
+ }).then((response) => ({ data: response.json.ids || [] }))
+}
+
const wrapperDataProvider = {
...dataProvider,
getList: (resource, params) => {
@@ -55,6 +63,9 @@ const wrapperDataProvider = {
},
deleteMany: (resource, params) => {
const [r, p] = mapResource(resource, params)
+ if (r.endsWith('/tracks')) {
+ return callDeleteMany(r, p)
+ }
return dataProvider.deleteMany(r, p)
},
addToPlaylist: (playlistId, data) => {