diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 3fdd19af2..8bdcea8ed 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -285,13 +285,16 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { } // Update when the playlist was last refreshed (for cache purposes) - updSql := Update(r.tableName).Set("evaluated_at", time.Now()).Where(Eq{"id": pls.ID}) + now := time.Now() + updSql := Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID}) _, err = r.executeSQL(updSql) if err != nil { log.Error(r.ctx, "Error updating smart playlist", "playlist", pls.Name, "id", pls.ID, err) return false } + pls.EvaluatedAt = &now + log.Debug(r.ctx, "Refreshed playlist", "playlist", pls.Name, "id", pls.ID, "numTracks", pls.SongCount, "elapsed", time.Since(start)) return true diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 05a36352f..232eb14b4 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -4,6 +4,7 @@ import ( "time" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" @@ -160,14 +161,23 @@ var _ = Describe("PlaylistRepository", func() { }) }) - // TODO Validate these tests - XContext("child smart playlists", func() { - When("refresh day has expired", func() { + Context("child smart playlists", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + When("refresh delay has expired", func() { It("should refresh tracks for smart playlist referenced in parent smart playlist criteria", func() { conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second - nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Rules: rules} + childRules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Contains{"title": "Day"}, + }, + } + nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Public: true, Rules: childRules} Expect(repo.Put(&nestedPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(nestedPls.ID) }) parentPls := model.Playlist{Name: "Parent", OwnerID: "userid", Rules: &criteria.Criteria{ Expression: criteria.All{ @@ -175,45 +185,69 @@ var _ = Describe("PlaylistRepository", func() { }, }} Expect(repo.Put(&parentPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(parentPls.ID) }) + // Nested playlist has not been evaluated yet nestedPlsRead, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) + Expect(nestedPlsRead.EvaluatedAt).To(BeNil()) - _, err = repo.GetWithTracks(parentPls.ID, true, false) + // Getting parent with refresh should recursively refresh the nested playlist + pls, err := repo.GetWithTracks(parentPls.ID, true, false) Expect(err).ToNot(HaveOccurred()) + Expect(pls.EvaluatedAt).ToNot(BeNil()) + Expect(*pls.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) - // Check that the nested playlist was refreshed by parent get by verifying evaluatedAt is updated since first nestedPls get + // Parent should have tracks from the nested playlist + Expect(pls.Tracks).To(HaveLen(1)) + Expect(pls.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID)) + + // Nested playlist should now have been refreshed (EvaluatedAt set) nestedPlsAfterParentGet, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) - - Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally(">", *nestedPlsRead.EvaluatedAt)) + Expect(nestedPlsAfterParentGet.EvaluatedAt).ToNot(BeNil()) + Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) }) }) - When("refresh day has not expired", func() { + When("refresh delay has not expired", func() { It("should NOT refresh tracks for smart playlist referenced in parent smart playlist criteria", func() { conf.Server.SmartPlaylistRefreshDelay = 1 * time.Hour + childEvaluatedAt := time.Now().Add(-30 * time.Minute) - nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Rules: rules} + childRules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Contains{"title": "Day"}, + }, + } + nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Public: true, Rules: childRules, EvaluatedAt: &childEvaluatedAt} Expect(repo.Put(&nestedPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(nestedPls.ID) }) + // Parent has no EvaluatedAt, so it WILL refresh, but the child should not parentPls := model.Playlist{Name: "Parent", OwnerID: "userid", Rules: &criteria.Criteria{ Expression: criteria.All{ criteria.InPlaylist{"id": nestedPls.ID}, }, }} Expect(repo.Put(&parentPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(parentPls.ID) }) nestedPlsRead, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) - _, err = repo.GetWithTracks(parentPls.ID, true, false) + // Getting parent with refresh should NOT recursively refresh the nested playlist + parent, err := repo.GetWithTracks(parentPls.ID, true, false) Expect(err).ToNot(HaveOccurred()) - // Check that the nested playlist was not refreshed by parent get by verifying evaluatedAt is not updated since first nestedPls get + // Parent should have been refreshed (its EvaluatedAt was nil) + Expect(parent.EvaluatedAt).ToNot(BeNil()) + Expect(*parent.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + + // Nested playlist should NOT have been refreshed (still within delay window) nestedPlsAfterParentGet, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) - + Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", childEvaluatedAt, time.Second)) Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(Equal(*nestedPlsRead.EvaluatedAt)) }) }) diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index fbf9deb99..b8807563e 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -12,6 +12,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" + . "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/req" "github.com/navidrome/navidrome/utils/slice" ) @@ -162,7 +163,11 @@ func (api *Router) buildPlaylist(ctx context.Context, p model.Playlist) response pls.Duration = int32(p.Duration) pls.Created = p.CreatedAt if p.IsSmartPlaylist() { - pls.Changed = time.Now() + if p.EvaluatedAt != nil { + pls.Changed = *p.EvaluatedAt + } else { + pls.Changed = time.Now() + } } else { pls.Changed = p.UpdatedAt } @@ -176,6 +181,24 @@ func (api *Router) buildPlaylist(ctx context.Context, p model.Playlist) response pls.Owner = p.OwnerName pls.Public = p.Public pls.CoverArt = p.CoverArtID().String() + pls.OpenSubsonicPlaylist = buildOSPlaylist(ctx, p) return pls } + +func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubsonicPlaylist { + pls := responses.OpenSubsonicPlaylist{} + + if p.IsSmartPlaylist() { + pls.Readonly = true + + if p.EvaluatedAt != nil { + pls.ValidUntil = P(p.EvaluatedAt.Add(conf.Server.SmartPlaylistRefreshDelay)) + } + } else { + user, ok := request.UserFrom(ctx) + pls.Readonly = !ok || p.OwnerID != user.ID + } + + return &pls +} diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index 20da12dd7..05701fc1f 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -7,6 +7,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -25,96 +26,194 @@ var _ = Describe("buildPlaylist", func() { ds = &tests.MockDataStore{} router = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) ctx = context.Background() - - createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC) - updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) - - playlist = model.Playlist{ - ID: "pls-1", - Name: "My Playlist", - Comment: "Test comment", - OwnerName: "admin", - Public: true, - SongCount: 10, - Duration: 600, - CreatedAt: createdAt, - UpdatedAt: updatedAt, - } }) - Context("with minimal client", func() { + Describe("normal playlist", func() { BeforeEach(func() { - conf.Server.Subsonic.MinimalClients = "minimal-client" - player := model.Player{Client: "minimal-client"} - ctx = request.WithPlayer(ctx, player) + createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC) + updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) + + playlist = model.Playlist{ + ID: "pls-1", + Name: "My Playlist", + Comment: "Test comment", + OwnerName: "admin", + OwnerID: "1234", + Public: true, + SongCount: 10, + Duration: 600, + CreatedAt: createdAt, + UpdatedAt: updatedAt, + } }) - It("returns only basic fields", func() { - result := router.buildPlaylist(ctx, playlist) + Context("with minimal client", func() { + BeforeEach(func() { + conf.Server.Subsonic.MinimalClients = "minimal-client" + player := model.Player{Client: "minimal-client"} + ctx = request.WithPlayer(ctx, player) + }) - Expect(result.Id).To(Equal("pls-1")) - Expect(result.Name).To(Equal("My Playlist")) - Expect(result.SongCount).To(Equal(int32(10))) - Expect(result.Duration).To(Equal(int32(600))) - Expect(result.Created).To(Equal(playlist.CreatedAt)) - Expect(result.Changed).To(Equal(playlist.UpdatedAt)) + It("returns only basic fields", func() { + result := router.buildPlaylist(ctx, playlist) - // These should not be set - Expect(result.Comment).To(BeEmpty()) - Expect(result.Owner).To(BeEmpty()) - Expect(result.Public).To(BeFalse()) - Expect(result.CoverArt).To(BeEmpty()) + Expect(result.Id).To(Equal("pls-1")) + Expect(result.Name).To(Equal("My Playlist")) + Expect(result.SongCount).To(Equal(int32(10))) + Expect(result.Duration).To(Equal(int32(600))) + Expect(result.Created).To(Equal(playlist.CreatedAt)) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) + + // These should not be set + Expect(result.Comment).To(BeEmpty()) + Expect(result.Owner).To(BeEmpty()) + Expect(result.Public).To(BeFalse()) + Expect(result.CoverArt).To(BeEmpty()) + }) + }) + + Context("with non-minimal client", func() { + BeforeEach(func() { + conf.Server.Subsonic.MinimalClients = "minimal-client" + player := model.Player{Client: "regular-client"} + ctx = request.WithPlayer(ctx, player) + }) + + It("returns all fields", func() { + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Id).To(Equal("pls-1")) + Expect(result.Name).To(Equal("My Playlist")) + Expect(result.SongCount).To(Equal(int32(10))) + Expect(result.Duration).To(Equal(int32(600))) + Expect(result.Created).To(Equal(playlist.CreatedAt)) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) + Expect(result.Comment).To(Equal("Test comment")) + Expect(result.Owner).To(Equal("admin")) + Expect(result.Public).To(BeTrue()) + Expect(result.Readonly).To(BeTrue()) + }) + + It("returns all fields when as owner", func() { + ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) + + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Id).To(Equal("pls-1")) + Expect(result.Name).To(Equal("My Playlist")) + Expect(result.SongCount).To(Equal(int32(10))) + Expect(result.Duration).To(Equal(int32(600))) + Expect(result.Created).To(Equal(playlist.CreatedAt)) + Expect(result.Changed).To(Equal(playlist.UpdatedAt)) + Expect(result.Comment).To(Equal("Test comment")) + Expect(result.Owner).To(Equal("admin")) + Expect(result.Public).To(BeTrue()) + Expect(result.Readonly).To(BeFalse()) + }) + }) + + Context("when minimal clients list is empty", func() { + BeforeEach(func() { + conf.Server.Subsonic.MinimalClients = "" + player := model.Player{Client: "any-client"} + ctx = request.WithPlayer(ctx, player) + }) + + It("returns all fields", func() { + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Comment).To(Equal("Test comment")) + Expect(result.Owner).To(Equal("admin")) + Expect(result.Public).To(BeTrue()) + }) + }) + + Context("when no player in context", func() { + It("returns all fields", func() { + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Comment).To(Equal("Test comment")) + Expect(result.Owner).To(Equal("admin")) + Expect(result.Public).To(BeTrue()) + }) }) }) - Context("with non-minimal client", func() { + Describe("smart playlist", func() { + evaluatedAt := time.Date(2023, 2, 20, 15, 45, 0, 0, time.UTC) + validUntil := evaluatedAt.Add(5 * time.Second) + BeforeEach(func() { - conf.Server.Subsonic.MinimalClients = "minimal-client" - player := model.Player{Client: "regular-client"} - ctx = request.WithPlayer(ctx, player) + createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC) + updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) + + playlist = model.Playlist{ + ID: "pls-1", + Name: "My Playlist", + Comment: "Test comment", + OwnerName: "admin", + OwnerID: "1234", + Public: true, + SongCount: 10, + Duration: 600, + CreatedAt: createdAt, + UpdatedAt: updatedAt, + EvaluatedAt: &evaluatedAt, + Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "title"}}, + }, + } }) - It("returns all fields", func() { - result := router.buildPlaylist(ctx, playlist) + Context("with minimal client", func() { + BeforeEach(func() { + conf.Server.Subsonic.MinimalClients = "minimal-client" + player := model.Player{Client: "minimal-client"} + ctx = request.WithPlayer(ctx, player) + }) - Expect(result.Id).To(Equal("pls-1")) - Expect(result.Name).To(Equal("My Playlist")) - Expect(result.SongCount).To(Equal(int32(10))) - Expect(result.Duration).To(Equal(int32(600))) - Expect(result.Created).To(Equal(playlist.CreatedAt)) - Expect(result.Changed).To(Equal(playlist.UpdatedAt)) - Expect(result.Comment).To(Equal("Test comment")) - Expect(result.Owner).To(Equal("admin")) - Expect(result.Public).To(BeTrue()) + It("returns only basic fields", func() { + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Id).To(Equal("pls-1")) + Expect(result.Name).To(Equal("My Playlist")) + Expect(result.SongCount).To(Equal(int32(10))) + Expect(result.Duration).To(Equal(int32(600))) + Expect(result.Created).To(Equal(playlist.CreatedAt)) + Expect(result.Changed).To(Equal(evaluatedAt)) + + // These should not be set + Expect(result.Comment).To(BeEmpty()) + Expect(result.Owner).To(BeEmpty()) + Expect(result.Public).To(BeFalse()) + Expect(result.CoverArt).To(BeEmpty()) + Expect(result.OpenSubsonicPlaylist).To(BeNil()) + }) + }) + + Context("with non-minimal client", func() { + BeforeEach(func() { + conf.Server.Subsonic.MinimalClients = "minimal-client" + player := model.Player{Client: "regular-client"} + ctx = request.WithPlayer(ctx, player) + }) + + It("returns all fields", func() { + result := router.buildPlaylist(ctx, playlist) + Expect(result.Id).To(Equal("pls-1")) + Expect(result.Name).To(Equal("My Playlist")) + Expect(result.SongCount).To(Equal(int32(10))) + Expect(result.Duration).To(Equal(int32(600))) + Expect(result.Created).To(Equal(playlist.CreatedAt)) + Expect(result.Changed).To(Equal(*playlist.EvaluatedAt)) + Expect(result.Comment).To(Equal("Test comment")) + Expect(result.Owner).To(Equal("admin")) + Expect(result.Public).To(BeTrue()) + Expect(result.Readonly).To(BeTrue()) + Expect(result.ValidUntil).To(Equal(&validUntil)) + }) }) }) - - Context("when minimal clients list is empty", func() { - BeforeEach(func() { - conf.Server.Subsonic.MinimalClients = "" - player := model.Player{Client: "any-client"} - ctx = request.WithPlayer(ctx, player) - }) - - It("returns all fields", func() { - result := router.buildPlaylist(ctx, playlist) - - Expect(result.Comment).To(Equal("Test comment")) - Expect(result.Owner).To(Equal("admin")) - Expect(result.Public).To(BeTrue()) - }) - }) - - Context("when no player in context", func() { - It("returns all fields", func() { - result := router.buildPlaylist(ctx, playlist) - - Expect(result.Comment).To(Equal("Test comment")) - Expect(result.Owner).To(Equal("admin")) - Expect(result.Public).To(BeTrue()) - }) - }) - }) var _ = Describe("UpdatePlaylist", func() { diff --git a/server/subsonic/responses/.snapshots/Responses Playlists with data should match .JSON b/server/subsonic/responses/.snapshots/Responses Playlists with data should match .JSON index 5263fb07c..963a207d5 100644 --- a/server/subsonic/responses/.snapshots/Responses Playlists with data should match .JSON +++ b/server/subsonic/responses/.snapshots/Responses Playlists with data should match .JSON @@ -14,9 +14,20 @@ "duration": 120, "public": true, "owner": "admin", + "created": "2023-02-20T14:45:00Z", + "changed": "2023-02-20T14:45:00Z", + "coverArt": "pl-123123123123", + "readonly": true, + "validUntil": "2023-02-20T14:45:00Z" + }, + { + "id": "333", + "name": "ccc", + "songCount": 0, + "duration": 0, "created": "0001-01-01T00:00:00Z", "changed": "0001-01-01T00:00:00Z", - "coverArt": "pl-123123123123" + "readonly": false }, { "id": "222", diff --git a/server/subsonic/responses/.snapshots/Responses Playlists with data should match .XML b/server/subsonic/responses/.snapshots/Responses Playlists with data should match .XML index 6bc26c593..759f7b52d 100644 --- a/server/subsonic/responses/.snapshots/Responses Playlists with data should match .XML +++ b/server/subsonic/responses/.snapshots/Responses Playlists with data should match .XML @@ -1,6 +1,7 @@ - + + diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index c5e07395e..4ddcbe00f 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -298,16 +298,17 @@ type AlbumList2 struct { } type Playlist struct { - Id string `xml:"id,attr" json:"id"` - Name string `xml:"name,attr" json:"name"` - Comment string `xml:"comment,attr,omitempty" json:"comment,omitempty"` - SongCount int32 `xml:"songCount,attr" json:"songCount"` - Duration int32 `xml:"duration,attr" json:"duration"` - Public bool `xml:"public,attr,omitempty" json:"public,omitempty"` - Owner string `xml:"owner,attr,omitempty" json:"owner,omitempty"` - Created time.Time `xml:"created,attr" json:"created"` - Changed time.Time `xml:"changed,attr" json:"changed"` - CoverArt string `xml:"coverArt,attr,omitempty" json:"coverArt,omitempty"` + Id string `xml:"id,attr" json:"id"` + Name string `xml:"name,attr" json:"name"` + Comment string `xml:"comment,attr,omitempty" json:"comment,omitempty"` + SongCount int32 `xml:"songCount,attr" json:"songCount"` + Duration int32 `xml:"duration,attr" json:"duration"` + Public bool `xml:"public,attr,omitempty" json:"public,omitempty"` + Owner string `xml:"owner,attr,omitempty" json:"owner,omitempty"` + Created time.Time `xml:"created,attr" json:"created"` + Changed time.Time `xml:"changed,attr" json:"changed"` + CoverArt string `xml:"coverArt,attr,omitempty" json:"coverArt,omitempty"` + *OpenSubsonicPlaylist `xml:",omitempty" json:",omitempty"` /* @@ -315,6 +316,11 @@ type Playlist struct { */ } +type OpenSubsonicPlaylist struct { + Readonly bool `xml:"readonly,attr,omitempty" json:"readonly"` + ValidUntil *time.Time `xml:"validUntil,attr,omitempty" json:"validUntil,omitempty"` +} + type Playlists struct { Playlist []Playlist `xml:"playlist" json:"playlist,omitempty"` } diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index 2ee8e080d..ccf15afe3 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/server/subsonic/responses" . "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/gg" . "github.com/onsi/ginkgo/v2" @@ -531,9 +532,9 @@ var _ = Describe("Responses", func() { }) Context("with data", func() { - timestamp, _ := time.Parse(time.RFC3339, "2020-04-11T16:43:00Z04:00") + timestamp := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) BeforeEach(func() { - pls := make([]Playlist, 2) + pls := make([]Playlist, 3) pls[0] = Playlist{ Id: "111", Name: "aaa", @@ -545,8 +546,13 @@ var _ = Describe("Responses", func() { CoverArt: "pl-123123123123", Created: timestamp, Changed: timestamp, + OpenSubsonicPlaylist: &responses.OpenSubsonicPlaylist{ + Readonly: true, + ValidUntil: ×tamp, + }, } - pls[1] = Playlist{Id: "222", Name: "bbb"} + pls[1] = Playlist{Id: "333", Name: "ccc", OpenSubsonicPlaylist: &responses.OpenSubsonicPlaylist{}} + pls[2] = Playlist{Id: "222", Name: "bbb"} response.Playlists.Playlist = pls })