From d200933b68f2265981ca7b3791f95a9f16cf9e33 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 16 Oct 2021 13:47:10 -0400 Subject: [PATCH] Reduce number of queries for some playlists operations. Also allow admins to update/delete playlists from other users in the Subsonic API. Closes #1366 --- core/archiver.go | 2 +- model/playlist.go | 3 ++- persistence/playlist_repository.go | 8 ++++---- persistence/playlist_repository_test.go | 4 ++-- persistence/playlist_track_repository.go | 15 ++++++++++---- server/nativeapi/playlists.go | 6 +++--- server/subsonic/playlists.go | 26 +++--------------------- 7 files changed, 26 insertions(+), 38 deletions(-) diff --git a/core/archiver.go b/core/archiver.go index c8867062..47bad103 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -54,7 +54,7 @@ func (a *archiver) ZipArtist(ctx context.Context, id string, out io.Writer) erro } func (a *archiver) ZipPlaylist(ctx context.Context, id string, out io.Writer) error { - pls, err := a.ds.Playlist(ctx).Get(id) + pls, err := a.ds.Playlist(ctx).GetWithTracks(id) if err != nil { log.Error(ctx, "Error loading mediafiles from playlist", "id", id, err) return err diff --git a/model/playlist.go b/model/playlist.go index 7aefc38c..24e45791 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -31,9 +31,9 @@ type PlaylistRepository interface { Exists(id string) (bool, error) Put(pls *Playlist) error Get(id string) (*Playlist, error) + GetWithTracks(id string) (*Playlist, error) GetAll(options ...QueryOptions) (Playlists, error) FindByPath(path string) (*Playlist, error) - FindByID(id string) (*Playlist, error) Delete(id string) error Tracks(playlistId string) PlaylistTrackRepository } @@ -49,6 +49,7 @@ type PlaylistTracks []PlaylistTrack type PlaylistTrackRepository interface { ResourceRepository + GetAll(options ...QueryOptions) (PlaylistTracks, error) Add(mediaFileIds []string) (int, error) AddAlbums(albumIds []string) (int, error) AddArtists(artistIds []string) (int, error) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 9fa4237b..44ade659 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -105,6 +105,10 @@ func (r *playlistRepository) Put(p *model.Playlist) error { } func (r *playlistRepository) Get(id string) (*model.Playlist, error) { + return r.findBy(And{Eq{"id": id}, r.userFilter()}, false) +} + +func (r *playlistRepository) GetWithTracks(id string) (*model.Playlist, error) { return r.findBy(And{Eq{"id": id}, r.userFilter()}, true) } @@ -112,10 +116,6 @@ func (r *playlistRepository) FindByPath(path string) (*model.Playlist, error) { return r.findBy(Eq{"path": path}, false) } -func (r *playlistRepository) FindByID(id string) (*model.Playlist, error) { - return r.findBy(And{Eq{"id": id}, r.userFilter()}, false) -} - func (r *playlistRepository) findBy(sql Sqlizer, includeTracks bool) (*model.Playlist, error) { sel := r.newSelect().Columns("*").Where(sql) var pls []dbPlaylist diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index ebbf07ee..4588009a 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -55,7 +55,7 @@ var _ = Describe("PlaylistRepository", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) It("returns all tracks", func() { - pls, err := repo.Get(plsBest.ID) + pls, err := repo.GetWithTracks(plsBest.ID) Expect(err).To(BeNil()) Expect(pls.Name).To(Equal(plsBest.Name)) Expect(pls.Tracks).To(Equal(model.MediaFiles{ @@ -76,7 +76,7 @@ var _ = Describe("PlaylistRepository", func() { By("adds repeated songs to a playlist and keeps the order") newPls.Tracks = append(newPls.Tracks, model.MediaFile{ID: "1004"}) Expect(repo.Put(&newPls)).To(BeNil()) - saved, _ := repo.Get(newPls.ID) + saved, _ := repo.GetWithTracks(newPls.ID) Expect(saved.Tracks).To(HaveLen(3)) Expect(saved.Tracks[0].ID).To(Equal("1004")) Expect(saved.Tracks[1].ID).To(Equal("1003")) diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 2c3e54e8..03a1e767 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -48,8 +48,8 @@ func (r *playlistTrackRepository) Read(id string) (interface{}, error) { return &trk, err } -func (r *playlistTrackRepository) ReadAll(options ...rest.QueryOptions) (interface{}, error) { - sel := r.newSelect(r.parseRestOptions(options...)). +func (r *playlistTrackRepository) GetAll(options ...model.QueryOptions) (model.PlaylistTracks, error) { + sel := r.newSelect(options...). LeftJoin("annotation on ("+ "annotation.item_id = media_file_id"+ " AND annotation.item_type = 'media_file'"+ @@ -62,6 +62,10 @@ func (r *playlistTrackRepository) ReadAll(options ...rest.QueryOptions) (interfa return res, err } +func (r *playlistTrackRepository) ReadAll(options ...rest.QueryOptions) (interface{}, error) { + return r.GetAll(r.parseRestOptions(options...)) +} + func (r *playlistTrackRepository) EntityName() string { return "playlist_tracks" } @@ -211,7 +215,10 @@ func (r *playlistTrackRepository) Delete(id string) error { if err != nil { return err } - return r.updateStats() + + // To renumber the playlist + _, err = r.Add(nil) + return err } func (r *playlistTrackRepository) Reorder(pos int, newPos int) error { @@ -231,7 +238,7 @@ func (r *playlistTrackRepository) isWritable() bool { if usr.IsAdmin { return true } - pls, err := r.playlistRepo.FindByID(r.playlistId) + pls, err := r.playlistRepo.Get(r.playlistId) return err == nil && pls.Owner == usr.UserName } diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 5288bfb7..11be6ddb 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -46,7 +46,7 @@ func handleExportPlaylist(ds model.DataStore) http.HandlerFunc { ctx := r.Context() plsRepo := ds.Playlist(ctx) plsId := chi.URLParam(r, "playlistId") - pls, err := plsRepo.Get(plsId) + pls, err := plsRepo.GetWithTracks(plsId) if err == model.ErrNotFound { log.Warn("Playlist not found", "playlistId", plsId) http.Error(w, "not found", http.StatusNotFound) @@ -114,13 +114,13 @@ func addToPlaylist(ds model.DataStore) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { playlistId := utils.ParamString(r, ":playlistId") - tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId) var payload addTracksPayload err := json.NewDecoder(r.Body).Decode(&payload) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } + tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId) count, c := 0, 0 if c, err = tracksRepo.Add(payload.Ids); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) @@ -163,7 +163,6 @@ func reorderItem(ds model.DataStore) http.HandlerFunc { http.Error(w, "invalid id", http.StatusBadRequest) return } - tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId) var payload reorderPayload err := json.NewDecoder(r.Body).Decode(&payload) if err != nil { @@ -175,6 +174,7 @@ func reorderItem(ds model.DataStore) http.HandlerFunc { http.Error(w, err.Error(), http.StatusBadRequest) return } + tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId) err = tracksRepo.Reorder(id, newPos) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index 883b0bc0..aee1dbbb 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -46,7 +46,7 @@ func (c *PlaylistsController) GetPlaylist(w http.ResponseWriter, r *http.Request } func (c *PlaylistsController) getPlaylist(ctx context.Context, id string) (*responses.Subsonic, error) { - pls, err := c.ds.Playlist(ctx).Get(id) + pls, err := c.ds.Playlist(ctx).GetWithTracks(id) switch { case err == model.ErrNotFound: log.Error(ctx, err.Error(), "id", id) @@ -110,27 +110,12 @@ func (c *PlaylistsController) CreatePlaylist(w http.ResponseWriter, r *http.Requ return c.getPlaylist(ctx, id) } -func (c *PlaylistsController) delete(ctx context.Context, playlistId string) error { - return c.ds.WithTx(func(tx model.DataStore) error { - pls, err := tx.Playlist(ctx).Get(playlistId) - if err != nil { - return err - } - - owner := getUser(ctx) - if owner != pls.Owner { - return model.ErrNotAuthorized - } - return tx.Playlist(ctx).Delete(playlistId) - }) -} - func (c *PlaylistsController) DeletePlaylist(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { id, err := requiredParamString(r, "id") if err != nil { return nil, err } - err = c.delete(r.Context(), id) + err = c.ds.Playlist(r.Context()).Delete(id) if err == model.ErrNotAuthorized { return nil, newError(responses.ErrorAuthorizationFail) } @@ -143,16 +128,11 @@ func (c *PlaylistsController) DeletePlaylist(w http.ResponseWriter, r *http.Requ func (c *PlaylistsController) update(ctx context.Context, playlistId string, name *string, comment *string, public *bool, idsToAdd []string, idxToRemove []int) error { return c.ds.WithTx(func(tx model.DataStore) error { - pls, err := tx.Playlist(ctx).Get(playlistId) + pls, err := tx.Playlist(ctx).GetWithTracks(playlistId) if err != nil { return err } - owner := getUser(ctx) - if owner != pls.Owner { - return model.ErrNotAuthorized - } - if name != nil { pls.Name = *name }