feat(subsonic): add OS readonly and validUntil properties in playlists (#4993)

* feat(subsonic): add OS readonly and validUntil properties

* remove duplicated test

* test: fix and enable disabled child smart playlist tests

Fixed the XContext("child smart playlists") tests that were disabled with
a TODO comment. The tests had several issues: nested playlists were missing
Public: true (required by InPlaylist criteria), the criteria matched no
test fixtures, the "not expired" test set EvaluatedAt on the parent too
(preventing it from refreshing at all), and the "expired" test dereferenced
a nil EvaluatedAt. Added proper cleanup with DeferCleanup and config
restoration via configtest.

* fix(subsonic): always include readonly field in JSON playlist responses

Removed omitempty from the JSON tag of the Readonly field in
OpenSubsonicPlaylist so that readonly: false is always serialized in
JSON responses, per the OpenSubsonic spec requirement that supported
fields must be returned with default values. Added a test case with an
empty OpenSubsonicPlaylist to verify the behavior.

---------

Co-authored-by: Deluan Quintão <deluan@navidrome.org>
This commit is contained in:
Kendall Garner 2026-02-07 00:35:54 +00:00 committed by GitHub
parent e11206f0ee
commit 6fb4cd277e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 287 additions and 104 deletions

View File

@ -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

View File

@ -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))
})
})

View File

@ -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
}

View File

@ -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() {

View File

@ -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",

View File

@ -1,6 +1,7 @@
<subsonic-response xmlns="http://subsonic.org/restapi" status="ok" version="1.16.1" type="navidrome" serverVersion="v0.55.0" openSubsonic="true">
<playlists>
<playlist id="111" name="aaa" comment="comment" songCount="2" duration="120" public="true" owner="admin" created="0001-01-01T00:00:00Z" changed="0001-01-01T00:00:00Z" coverArt="pl-123123123123"></playlist>
<playlist id="111" name="aaa" comment="comment" songCount="2" 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"></playlist>
<playlist id="333" name="ccc" songCount="0" duration="0" created="0001-01-01T00:00:00Z" changed="0001-01-01T00:00:00Z"></playlist>
<playlist id="222" name="bbb" songCount="0" duration="0" created="0001-01-01T00:00:00Z" changed="0001-01-01T00:00:00Z"></playlist>
</playlists>
</subsonic-response>

View File

@ -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"`
/*
<xs:sequence>
<xs:element name="allowedUser" type="xs:string" minOccurs="0" maxOccurs="unbounded"/> <!--Added in 1.8.0-->
@ -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"`
}

View File

@ -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: &timestamp,
},
}
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
})