From a5f72f267883198350f603bef31a5f42c89fa4b0 Mon Sep 17 00:00:00 2001 From: David Date: Sat, 14 Mar 2026 13:32:28 -0500 Subject: [PATCH 1/7] feat: Add support for referencing playlists using paths Signed-off-by: David --- model/criteria/criteria.go | 12 +++++++++ model/criteria/criteria_test.go | 22 ++++++++++----- model/criteria/operators.go | 34 ++++++++++++++++-------- persistence/criteria_sql.go | 12 ++++++--- persistence/criteria_sql_test.go | 3 ++- persistence/smart_playlist_repository.go | 14 +++++++--- 6 files changed, 72 insertions(+), 25 deletions(-) diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index 31d208d08..c213872ad 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -74,6 +74,18 @@ func (c Criteria) ChildPlaylistIds() []string { return slices.Compact(ids) } +func (c Criteria) ChildPlaylistPaths() []string { + if c.Expression == nil { + return nil + } + + if parent := c.Expression.(interface{ ChildPlaylistPaths() (paths []string) }); parent != nil { + return parent.ChildPlaylistPaths() + } + + return nil +} + func (c Criteria) MarshalJSON() ([]byte, error) { aux := struct { All []Expression `json:"all,omitempty"` diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 092cfd36a..c59df2708 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -235,19 +235,23 @@ var _ = Describe("Criteria", func() { Context("with child playlists", func() { var ( - topLevelInPlaylistID string - topLevelNotInPlaylistID string - nestedAnyInPlaylistID string - nestedAnyNotInPlaylistID string - nestedAllInPlaylistID string - nestedAllNotInPlaylistID string + topLevelInPlaylistID string + topLevelInPlaylistPath string + topLevelNotInPlaylistID string + nestedAnyInPlaylistID string + nestedAnyNotInPlaylistID string + nestedAllInPlaylistID string + nestedAllNotInPlaylistID string + nestedAnyNotInPlaylistPath string ) BeforeEach(func() { topLevelInPlaylistID = uuid.NewString() + topLevelInPlaylistPath = "./test.nsp" topLevelNotInPlaylistID = uuid.NewString() nestedAnyInPlaylistID = uuid.NewString() nestedAnyNotInPlaylistID = uuid.NewString() + nestedAnyNotInPlaylistPath = "../not-in-playlist.m3u" nestedAllInPlaylistID = uuid.NewString() nestedAllNotInPlaylistID = uuid.NewString() @@ -255,10 +259,12 @@ var _ = Describe("Criteria", func() { goObj = Criteria{ Expression: All{ InPlaylist{"id": topLevelInPlaylistID}, + InPlaylist{"path": topLevelInPlaylistPath}, NotInPlaylist{"id": topLevelNotInPlaylistID}, Any{ InPlaylist{"id": nestedAnyInPlaylistID}, NotInPlaylist{"id": nestedAnyNotInPlaylistID}, + NotInPlaylist{"path": nestedAnyNotInPlaylistPath}, }, All{ InPlaylist{"id": nestedAllInPlaylistID}, @@ -271,6 +277,10 @@ var _ = Describe("Criteria", func() { ids := goObj.ChildPlaylistIds() gomega.Expect(ids).To(gomega.ConsistOf(topLevelInPlaylistID, topLevelNotInPlaylistID, nestedAnyInPlaylistID, nestedAnyNotInPlaylistID, nestedAllInPlaylistID, nestedAllNotInPlaylistID)) }) + It("extracts all child smart playlist paths from expression criteria", func() { + ids := goObj.ChildPlaylistPaths() + gomega.Expect(ids).To(gomega.ConsistOf(topLevelInPlaylistPath, nestedAnyNotInPlaylistPath)) + }) It("extracts child smart playlist IDs from deeply nested expression", func() { goObj = Criteria{ Expression: Any{ diff --git a/model/criteria/operators.go b/model/criteria/operators.go index 3ddd77f8b..5fb59f266 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -22,6 +22,10 @@ func (all All) ChildPlaylistIds() (ids []string) { return extractPlaylistIds(all) } +func (all All) ChildPlaylistPaths() (paths []string) { + return extractPlaylistPaths(all) +} + type ( Any []Expression Or = Any @@ -37,6 +41,10 @@ func (any Any) ChildPlaylistIds() (ids []string) { return extractPlaylistIds(any) } +func (any Any) ChildPlaylistPaths() (paths []string) { + return extractPlaylistPaths(any) +} + type Is map[string]any type Eq = Is @@ -178,28 +186,32 @@ func (ip IsPresent) MarshalJSON() ([]byte, error) { func (ip IsPresent) fields() map[string]any { return ip } -func extractPlaylistIds(inputRule any) (ids []string) { - var id string - var ok bool - +func extractPlaylistField(inputRule any, field string) (values []string) { switch rule := inputRule.(type) { case Any: for _, rules := range rule { - ids = append(ids, extractPlaylistIds(rules)...) + values = append(values, extractPlaylistField(rules, field)...) } case All: for _, rules := range rule { - ids = append(ids, extractPlaylistIds(rules)...) + values = append(values, extractPlaylistField(rules, field)...) } case InPlaylist: - if id, ok = rule["id"].(string); ok { - ids = append(ids, id) + if value, ok := rule[field].(string); ok { + values = append(values, value) } case NotInPlaylist: - if id, ok = rule["id"].(string); ok { - ids = append(ids, id) + if value, ok := rule[field].(string); ok { + values = append(values, value) } } - return } + +func extractPlaylistIds(inputRule any) (ids []string) { + return extractPlaylistField(inputRule, "id") +} + +func extractPlaylistPaths(inputRule any) (paths []string) { + return extractPlaylistField(inputRule, "path") +} diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index a1bae3170..ef6a979ca 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -310,11 +310,15 @@ func startOfPeriod(numDays int64, from time.Time) string { } func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squirrel.Sqlizer, error) { - playlistID, ok := values["id"].(string) - if !ok { - return nil, errors.New("playlist id not given") + var condition squirrel.Sqlizer + if playlistId, ok := values["id"].(string); ok { + condition = squirrel.Eq{"pl.playlist_id": playlistId} + } else if playlistPath, ok := values["path"].(string); ok { + condition = squirrel.Eq{"playlist.path": playlistPath} + } else { + return nil, errors.New("playlist id or path not given") } - filters := squirrel.And{squirrel.Eq{"pl.playlist_id": playlistID}} + filters := squirrel.And{condition} if !c.owner.IsAdmin { if c.owner.ID == "" { filters = append(filters, squirrel.Eq{"playlist.public": 1}) diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index ae2695a4d..126910a5d 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -45,7 +45,8 @@ var _ = Describe("Smart playlist criteria SQL", func() { Entry("in range", criteria.InTheRange{"year": []int{1980, 1990}}, "(media_file.year >= ? AND media_file.year <= ?)", 1980, 1990), Entry("before", criteria.Before{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), Entry("after", criteria.After{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), - Entry("in playlist", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), + Entry("in playlist [path]", criteria.InPlaylist{"path": "lacuslacus.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path = ? AND playlist.public = ?))", "lacuslacus.nsp", 1), + Entry("in playlist [id]", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), Entry("album annotation", criteria.Gt{"albumRating": 3}, "COALESCE(album_annotation.rating, 0) > ?", 3), Entry("artist annotation", criteria.Is{"artistLoved": true}, "COALESCE(artist_annotation.starred, false) = ?", true), diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 54f316152..926bfdc7f 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -91,19 +91,21 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr // Returns false if child playlists could not be loaded (DB error), signaling the parent refresh should abort. func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool { childPlaylistIds := rulesSQL.ChildPlaylistIds() - if len(childPlaylistIds) == 0 { + childPlaylistPaths := rulesSQL.ChildPlaylistPaths() + if len(childPlaylistIds) == 0 || len(childPlaylistPaths) == 0 { return true } - childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Eq{"playlist.id": childPlaylistIds}}) + childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Or{Eq{"playlist.id": childPlaylistIds}, Eq{"playlist.path": childPlaylistPaths}}}) if err != nil { log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, err) return false } - found := make(map[string]struct{}, len(childPlaylists)) + found := make(map[string]struct{}, len(childPlaylists)*2) for i := range childPlaylists { found[childPlaylists[i].ID] = struct{}{} + found[childPlaylists[i].Path] = struct{}{} r.refreshSmartPlaylist(&childPlaylists[i]) } for _, id := range childPlaylistIds { @@ -111,6 +113,12 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL log.Warn(r.ctx, "Referenced playlist is not accessible to smart playlist owner", "playlist", pls.Name, "id", pls.ID, "childId", id, "ownerId", pls.OwnerID) } } + + for _, path := range childPlaylistPaths { + if _, ok := found[path]; !ok { + log.Warn(r.ctx, "Referenced playlist is not accessible to smart playlist owner", "playlist", pls.Name, "id", pls.ID, "path", path, "ownerId", pls.OwnerID) + } + } return true } From 622b1d02b91497160c91b937cc12648f69063169 Mon Sep 17 00:00:00 2001 From: David Date: Mon, 16 Mar 2026 21:41:47 -0500 Subject: [PATCH 2/7] feat: Support relative playlist paths in smartlists Signed-off-by: David --- model/playlist.go | 37 +++++++++++ model/playlist_test.go | 80 ++++++++++++++++++++++++ persistence/smart_playlist_repository.go | 8 ++- 3 files changed, 124 insertions(+), 1 deletion(-) diff --git a/model/playlist.go b/model/playlist.go index dc549f039..d9617cd47 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -1,6 +1,7 @@ package model import ( + "path/filepath" "slices" "strconv" "time" @@ -117,6 +118,42 @@ func (pls Playlist) UploadedImagePath() string { return UploadedImagePath(consts.EntityPlaylist, pls.UploadedImage) } +func (pls Playlist) NormalizeChildPaths() { + if pls.Rules.Expression == nil { + return + } + + normalizePlaylistPaths(pls.Rules.Expression, pls.Path) +} + +func normalizePlaylistPaths(inputRule any, referencingPlaylistPath string) { + switch rule := inputRule.(type) { + case criteria.Any: + for _, rules := range rule { + normalizePlaylistPaths(rules, referencingPlaylistPath) + } + case criteria.All: + for _, rules := range rule { + normalizePlaylistPaths(rules, referencingPlaylistPath) + } + case criteria.InPlaylist: + dir := filepath.Dir(referencingPlaylistPath) + if path, ok := rule["path"].(string); ok { + if !filepath.IsAbs(path) { + rule["path"] = filepath.Clean(filepath.Join(dir, path)) + } + } + case criteria.NotInPlaylist: + dir := filepath.Dir(referencingPlaylistPath) + if path, ok := rule["path"].(string); ok { + if !filepath.IsAbs(path) { + rule["path"] = filepath.Clean(filepath.Join(dir, path)) + } + } + } + return +} + type Playlists []Playlist type PlaylistRepository interface { diff --git a/model/playlist_test.go b/model/playlist_test.go index 9ed24f00f..2f8a9222e 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -2,6 +2,7 @@ package model_test import ( "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -43,4 +44,83 @@ var _ = Describe("Playlist", func() { Expect(pls.ToM3U8()).To(Equal(expected)) }) }) + + Describe("NormalizeChildPaths()", func() { + It("normalizes file paths", func() { + pls := model.Playlist{Rules: &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + criteria.InPlaylist{"path": "../my-test-path.m3u"}, + criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + criteria.Any{ + criteria.InPlaylist{"path": "../../in-the-test.nsp"}, + criteria.NotInPlaylist{"path": "./sibling.nsp"}, + criteria.All{ + criteria.InPlaylist{"path": "/other-root/other.m3u"}, + criteria.NotInPlaylist{"path": "../../../out-of-containment.nsp"}, + }, + }, + }, + }, + Path: "/test/nested/my-playlist.nsp"} + + pls.NormalizeChildPaths() + Expect(pls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + criteria.Any{ + criteria.InPlaylist{"path": "/in-the-test.nsp"}, + criteria.NotInPlaylist{"path": "/test/nested/sibling.nsp"}, + criteria.All{ + criteria.InPlaylist{"path": "/other-root/other.m3u"}, + criteria.NotInPlaylist{"path": "/out-of-containment.nsp"}, + }, + }, + }, + })) + }) + + It("normalizes various file paths", func() { + // Absolute path + pls := model.Playlist{ID: "123"} + pls.Rules = &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + }, + } + + pls.NormalizeChildPaths() + Expect(pls.Rules).NotTo(BeNil()) + }) + + It("handles relative paths correctly", func() { + pls := model.Playlist{ID: "123", Path: "/test/my-playlist.m3u"} + pls.Rules = &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "../my-test-path.m3u"}, + }, + } + + pls.NormalizeChildPaths() + Expect(pls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/my-test-path.m3u"}, + }, + })) + }) + + It("ignores non-path entries", func() { + pls := model.Playlist{ID: "123"} + pls.Rules = &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + }, + } + + pls.NormalizeChildPaths() + Expect(pls.Rules).NotTo(BeNil()) + }) + }) }) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 926bfdc7f..d67b7c912 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -91,8 +91,14 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr // Returns false if child playlists could not be loaded (DB error), signaling the parent refresh should abort. func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool { childPlaylistIds := rulesSQL.ChildPlaylistIds() + + if len(childPlaylistIds) == 0 { + return true + } + + pls.NormalizeChildPaths() childPlaylistPaths := rulesSQL.ChildPlaylistPaths() - if len(childPlaylistIds) == 0 || len(childPlaylistPaths) == 0 { + if len(childPlaylistPaths) == 0 { return true } From 0b4397bc2cbae091b77e141f1a09b968b38a3dd6 Mon Sep 17 00:00:00 2001 From: David Date: Tue, 26 May 2026 20:37:30 -0500 Subject: [PATCH 3/7] fix(smartplaylists): protect against nil panic Signed-off-by: David --- model/criteria/criteria.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index c213872ad..e0bee24a0 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -79,11 +79,14 @@ func (c Criteria) ChildPlaylistPaths() []string { return nil } - if parent := c.Expression.(interface{ ChildPlaylistPaths() (paths []string) }); parent != nil { - return parent.ChildPlaylistPaths() + parent, ok := c.Expression.(interface{ ChildPlaylistPaths() []string }) + if !ok { + return nil } - return nil + paths := parent.ChildPlaylistPaths() + slices.Sort(paths) + return slices.Compact(paths) } func (c Criteria) MarshalJSON() ([]byte, error) { From 62d9bc6c5a1fb50a0b316f691f34c474ab4381ad Mon Sep 17 00:00:00 2001 From: David Date: Tue, 26 May 2026 23:00:07 -0500 Subject: [PATCH 4/7] fix(smartplaylists): refreshing child playlists Signed-off-by: David --- persistence/smart_playlist_repository.go | 6 +---- persistence/smart_playlist_repository_test.go | 23 +++++++++++++++---- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index d67b7c912..29041982e 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -92,13 +92,9 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool { childPlaylistIds := rulesSQL.ChildPlaylistIds() - if len(childPlaylistIds) == 0 { - return true - } - pls.NormalizeChildPaths() childPlaylistPaths := rulesSQL.ChildPlaylistPaths() - if len(childPlaylistPaths) == 0 { + if len(childPlaylistIds) == 0 && len(childPlaylistPaths) == 0 { return true } diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 7bc705385..c76bde0ab 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -71,13 +71,23 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { criteria.Contains{"title": "Day"}, }, } - nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Public: true, Rules: childRules} + nestedPls := model.Playlist{Name: "Nested [ID]", 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{ + childRules = &criteria.Criteria{ Expression: criteria.All{ + criteria.Eq{"artist": "シートベルツ"}, + }, + } + nestedPathPls := model.Playlist{Name: "Nested [Path]", OwnerID: "userid", Path: "test.nsp", Public: true, Rules: childRules} + Expect(repo.Put(&nestedPathPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(nestedPathPls.ID) }) + + parentPls := model.Playlist{Name: "Parent", OwnerID: "userid", Rules: &criteria.Criteria{ + Expression: criteria.Any{ criteria.InPlaylist{"id": nestedPls.ID}, + criteria.InPlaylist{"path": nestedPathPls.Path}, }, }} Expect(repo.Put(&parentPls)).To(Succeed()) @@ -95,14 +105,19 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { Expect(*pls.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) // Parent should have tracks from the nested playlist - Expect(pls.Tracks).To(HaveLen(1)) + Expect(pls.Tracks).To(HaveLen(2)) Expect(pls.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID)) - // Nested playlist should now have been refreshed (EvaluatedAt set) + // Nested playlists should now have been refreshed (EvaluatedAt set) nestedPlsAfterParentGet, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) Expect(nestedPlsAfterParentGet.EvaluatedAt).ToNot(BeNil()) Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + + nestedPlsAfterParentGet, err = repo.Get(nestedPathPls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(nestedPlsAfterParentGet.EvaluatedAt).ToNot(BeNil()) + Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) }) }) From 7328bbbba040ba6b6e07ab6e703f56880901f826 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 27 May 2026 19:38:04 -0500 Subject: [PATCH 5/7] chore(smartplaylists): log field parsing error Signed-off-by: David --- model/criteria/operators.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/model/criteria/operators.go b/model/criteria/operators.go index 0ab4301dd..cb239539d 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -1,5 +1,7 @@ package criteria +import "github.com/navidrome/navidrome/log" + // Conjunctions need to implement this interface, to allow Criteria to extract child playlist IDs recursively type conjunction interface { ChildPlaylistIds() []string @@ -193,10 +195,14 @@ func extractPlaylistField(inputRule any, field string) (values []string) { case InPlaylist: if value, ok := rule[field].(string); ok { values = append(values, value) + } else { + log.Warn("Playlist field not a string", field) } case NotInPlaylist: if value, ok := rule[field].(string); ok { values = append(values, value) + } else { + log.Warn("Playlist field not a string", field) } } return From 8877d06b5c846e8ccae4683994072c827d02674c Mon Sep 17 00:00:00 2001 From: David Date: Wed, 27 May 2026 21:09:14 -0500 Subject: [PATCH 6/7] fix(smartplaylists): handle empty playlist paths Signed-off-by: David --- model/playlist.go | 16 +++++- model/playlist_test.go | 71 +++++++++--------------- persistence/criteria_sql.go | 2 +- persistence/criteria_sql_test.go | 7 +++ persistence/smart_playlist_repository.go | 7 ++- 5 files changed, 53 insertions(+), 50 deletions(-) diff --git a/model/playlist.go b/model/playlist.go index d9617cd47..587cd9dd5 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -118,8 +118,8 @@ func (pls Playlist) UploadedImagePath() string { return UploadedImagePath(consts.EntityPlaylist, pls.UploadedImage) } -func (pls Playlist) NormalizeChildPaths() { - if pls.Rules.Expression == nil { +func (pls *Playlist) NormalizeChildPaths() { + if pls.Rules == nil || pls.Rules.Expression == nil { return } @@ -127,6 +127,10 @@ func (pls Playlist) NormalizeChildPaths() { } func normalizePlaylistPaths(inputRule any, referencingPlaylistPath string) { + if referencingPlaylistPath == "" { + return + } + switch rule := inputRule.(type) { case criteria.Any: for _, rules := range rule { @@ -139,6 +143,10 @@ func normalizePlaylistPaths(inputRule any, referencingPlaylistPath string) { case criteria.InPlaylist: dir := filepath.Dir(referencingPlaylistPath) if path, ok := rule["path"].(string); ok { + if path == "" { + return + } + if !filepath.IsAbs(path) { rule["path"] = filepath.Clean(filepath.Join(dir, path)) } @@ -146,6 +154,10 @@ func normalizePlaylistPaths(inputRule any, referencingPlaylistPath string) { case criteria.NotInPlaylist: dir := filepath.Dir(referencingPlaylistPath) if path, ok := rule["path"].(string); ok { + if path == "" { + return + } + if !filepath.IsAbs(path) { rule["path"] = filepath.Clean(filepath.Join(dir, path)) } diff --git a/model/playlist_test.go b/model/playlist_test.go index 2f8a9222e..82d64ad0e 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -47,21 +47,26 @@ var _ = Describe("Playlist", func() { Describe("NormalizeChildPaths()", func() { It("normalizes file paths", func() { - pls := model.Playlist{Rules: &criteria.Criteria{ - Expression: criteria.All{ - criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, - criteria.InPlaylist{"path": "../my-test-path.m3u"}, - criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, - criteria.Any{ - criteria.InPlaylist{"path": "../../in-the-test.nsp"}, - criteria.NotInPlaylist{"path": "./sibling.nsp"}, - criteria.All{ - criteria.InPlaylist{"path": "/other-root/other.m3u"}, - criteria.NotInPlaylist{"path": "../../../out-of-containment.nsp"}, + tests.SkipOnWindows("path separator bug (#TBD-path-sep-model)") + + pls := model.Playlist{ + Rules: &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + criteria.InPlaylist{"path": "../my-test-path.m3u"}, + criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + criteria.Any{ + criteria.InPlaylist{"path": "../../in-the-test.nsp"}, + criteria.NotInPlaylist{"path": "./sibling.nsp"}, + criteria.NotInPlaylist{"path": ""}, + criteria.All{ + criteria.InPlaylist{"path": "/other-root/other.m3u"}, + criteria.NotInPlaylist{"path": "../../../out-of-containment.nsp"}, + criteria.InPlaylist{"id": "94d8ba52-7aca-40e2-af82-4cb09c43d710"}, + }, }, }, }, - }, Path: "/test/nested/my-playlist.nsp"} pls.NormalizeChildPaths() @@ -73,54 +78,32 @@ var _ = Describe("Playlist", func() { criteria.Any{ criteria.InPlaylist{"path": "/in-the-test.nsp"}, criteria.NotInPlaylist{"path": "/test/nested/sibling.nsp"}, + criteria.NotInPlaylist{"path": ""}, criteria.All{ criteria.InPlaylist{"path": "/other-root/other.m3u"}, criteria.NotInPlaylist{"path": "/out-of-containment.nsp"}, + criteria.InPlaylist{"id": "94d8ba52-7aca-40e2-af82-4cb09c43d710"}, }, }, }, })) }) - It("normalizes various file paths", func() { - // Absolute path - pls := model.Playlist{ID: "123"} - pls.Rules = &criteria.Criteria{ - Expression: criteria.All{ - criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, + It("skips normalization when playlist path is empty", func() { + pls := model.Playlist{ + Rules: &criteria.Criteria{ + Expression: criteria.All{ + criteria.InPlaylist{"path": "../my-test-path.m3u"}, + }, }, - } - - pls.NormalizeChildPaths() - Expect(pls.Rules).NotTo(BeNil()) - }) - - It("handles relative paths correctly", func() { - pls := model.Playlist{ID: "123", Path: "/test/my-playlist.m3u"} - pls.Rules = &criteria.Criteria{ - Expression: criteria.All{ - criteria.InPlaylist{"path": "../my-test-path.m3u"}, - }, - } + Path: ""} pls.NormalizeChildPaths() Expect(pls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ Expression: criteria.All{ - criteria.InPlaylist{"path": "/my-test-path.m3u"}, + criteria.InPlaylist{"path": "../my-test-path.m3u"}, }, })) }) - - It("ignores non-path entries", func() { - pls := model.Playlist{ID: "123"} - pls.Rules = &criteria.Criteria{ - Expression: criteria.All{ - criteria.InPlaylist{"path": "/not-test/not-my-test-path.m3u"}, - }, - } - - pls.NormalizeChildPaths() - Expect(pls.Rules).NotTo(BeNil()) - }) }) }) diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 6c42aea80..a538337d7 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -315,7 +315,7 @@ func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squir var condition squirrel.Sqlizer if playlistId, ok := values["id"].(string); ok { condition = squirrel.Eq{"pl.playlist_id": playlistId} - } else if playlistPath, ok := values["path"].(string); ok { + } else if playlistPath, ok := values["path"].(string); ok && playlistPath != "" { condition = squirrel.Eq{"playlist.path": playlistPath} } else { return nil, errors.New("playlist id or path not given") diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index e5f22b2aa..255f69793 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -157,6 +157,13 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression"))) }) + It("returns an error when inPlaylist has empty path", func() { + _, err := newSmartPlaylistCriteria( + criteria.Criteria{Expression: criteria.InPlaylist{"path": ""}}, + withSmartPlaylistOwner(model.User{ID: "owner-id", IsAdmin: false})).Where() + Expect(err).To(MatchError(ContainSubstring("playlist id or path not given"))) + }) + Describe("sort", func() { It("sorts by regular fields", func() { Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).OrderBy()).To(Equal("media_file.title asc")) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 29041982e..30149b610 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -91,13 +91,12 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr // Returns false if child playlists could not be loaded (DB error), signaling the parent refresh should abort. func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool { childPlaylistIds := rulesSQL.ChildPlaylistIds() - - pls.NormalizeChildPaths() childPlaylistPaths := rulesSQL.ChildPlaylistPaths() if len(childPlaylistIds) == 0 && len(childPlaylistPaths) == 0 { return true } + pls.NormalizeChildPaths() childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Or{Eq{"playlist.id": childPlaylistIds}, Eq{"playlist.path": childPlaylistPaths}}}) if err != nil { log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, err) @@ -107,7 +106,9 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL found := make(map[string]struct{}, len(childPlaylists)*2) for i := range childPlaylists { found[childPlaylists[i].ID] = struct{}{} - found[childPlaylists[i].Path] = struct{}{} + if childPlaylists[i].Path != "" { + found[childPlaylists[i].Path] = struct{}{} + } r.refreshSmartPlaylist(&childPlaylists[i]) } for _, id := range childPlaylistIds { From 4bac69466999f556ed13c42f6bbf828a068ed762 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 28 May 2026 23:43:20 -0500 Subject: [PATCH 7/7] refactor(smartplaylists): make NormalizeChildPaths non-mutating Signed-off-by: David --- model/playlist.go | 51 ++++++++++++++++-------- model/playlist_test.go | 10 +++-- persistence/smart_playlist_repository.go | 10 ++--- 3 files changed, 46 insertions(+), 25 deletions(-) diff --git a/model/playlist.go b/model/playlist.go index 587cd9dd5..0ff57669e 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -1,6 +1,7 @@ package model import ( + "maps" "path/filepath" "slices" "strconv" @@ -118,52 +119,70 @@ func (pls Playlist) UploadedImagePath() string { return UploadedImagePath(consts.EntityPlaylist, pls.UploadedImage) } -func (pls *Playlist) NormalizeChildPaths() { +func (pls Playlist) WithNormalizeChildPaths() Playlist { if pls.Rules == nil || pls.Rules.Expression == nil { - return + return pls } - normalizePlaylistPaths(pls.Rules.Expression, pls.Path) + plsClone := pls + plsClone.Rules = &criteria.Criteria{ + Sort: pls.Rules.Sort, + Limit: pls.Rules.Limit, + LimitPercent: pls.Rules.LimitPercent, + Offset: pls.Rules.Offset, + Order: pls.Rules.Order, + Expression: normalizePlaylistPaths(pls.Rules.Expression, pls.Path), + } + return plsClone } -func normalizePlaylistPaths(inputRule any, referencingPlaylistPath string) { +func normalizePlaylistPaths(inputRule criteria.Expression, referencingPlaylistPath string) criteria.Expression { if referencingPlaylistPath == "" { - return + return inputRule } switch rule := inputRule.(type) { case criteria.Any: - for _, rules := range rule { - normalizePlaylistPaths(rules, referencingPlaylistPath) + anyCriteria := make(criteria.Any, len(rule)) + for i, rules := range rule { + anyCriteria[i] = normalizePlaylistPaths(rules, referencingPlaylistPath) } + return anyCriteria case criteria.All: - for _, rules := range rule { - normalizePlaylistPaths(rules, referencingPlaylistPath) + allCriteria := make(criteria.All, len(rule)) + for i, rules := range rule { + allCriteria[i] = normalizePlaylistPaths(rules, referencingPlaylistPath) } + return allCriteria case criteria.InPlaylist: - dir := filepath.Dir(referencingPlaylistPath) + inPlaylist := maps.Clone(rule) if path, ok := rule["path"].(string); ok { if path == "" { - return + return inPlaylist } if !filepath.IsAbs(path) { - rule["path"] = filepath.Clean(filepath.Join(dir, path)) + dir := filepath.Dir(referencingPlaylistPath) + inPlaylist["path"] = filepath.Clean(filepath.Join(dir, path)) } } + return inPlaylist case criteria.NotInPlaylist: - dir := filepath.Dir(referencingPlaylistPath) + notInPlaylist := maps.Clone(rule) if path, ok := rule["path"].(string); ok { if path == "" { - return + return notInPlaylist } if !filepath.IsAbs(path) { - rule["path"] = filepath.Clean(filepath.Join(dir, path)) + dir := filepath.Dir(referencingPlaylistPath) + notInPlaylist["path"] = filepath.Clean(filepath.Join(dir, path)) } } + return notInPlaylist } - return + + return inputRule } type Playlists []Playlist diff --git a/model/playlist_test.go b/model/playlist_test.go index 82d64ad0e..2f85dd587 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -55,6 +55,7 @@ var _ = Describe("Playlist", func() { criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, criteria.InPlaylist{"path": "../my-test-path.m3u"}, criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + criteria.Eq{"artist": "Bob Dealin'"}, criteria.Any{ criteria.InPlaylist{"path": "../../in-the-test.nsp"}, criteria.NotInPlaylist{"path": "./sibling.nsp"}, @@ -69,12 +70,13 @@ var _ = Describe("Playlist", func() { }, Path: "/test/nested/my-playlist.nsp"} - pls.NormalizeChildPaths() - Expect(pls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ + newPls := pls.WithNormalizeChildPaths() + Expect(newPls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ Expression: criteria.All{ criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, criteria.InPlaylist{"path": "/test/my-test-path.m3u"}, criteria.NotInPlaylist{"path": "/not-test/not-my-test-path.m3u"}, + criteria.Eq{"artist": "Bob Dealin'"}, criteria.Any{ criteria.InPlaylist{"path": "/in-the-test.nsp"}, criteria.NotInPlaylist{"path": "/test/nested/sibling.nsp"}, @@ -98,8 +100,8 @@ var _ = Describe("Playlist", func() { }, Path: ""} - pls.NormalizeChildPaths() - Expect(pls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ + newPls := pls.WithNormalizeChildPaths() + Expect(newPls.Rules).Should(BeEquivalentTo(&criteria.Criteria{ Expression: criteria.All{ criteria.InPlaylist{"path": "../my-test-path.m3u"}, }, diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 30149b610..e2969d663 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -31,17 +31,18 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return false } - rulesSQL := newSmartPlaylistCriteria(*pls.Rules, withSmartPlaylistOwner(*usr)) + normalisedPls := pls.WithNormalizeChildPaths() + rulesSQL := newSmartPlaylistCriteria(*normalisedPls.Rules, withSmartPlaylistOwner(*usr)) - if !r.refreshChildPlaylists(pls, rulesSQL) { + if !r.refreshChildPlaylists(&normalisedPls, rulesSQL) { return false } - if err := r.resolvePercentageLimit(pls, &rulesSQL, usr.ID); err != nil { + if err := r.resolvePercentageLimit(&normalisedPls, &rulesSQL, usr.ID); err != nil { return false } - sq := r.buildSmartPlaylistQuery(pls, rulesSQL, usr.ID) + sq := r.buildSmartPlaylistQuery(&normalisedPls, rulesSQL, usr.ID) sq, err := r.addCriteria(sq, rulesSQL) if err != nil { log.Error(r.ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) @@ -96,7 +97,6 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL return true } - pls.NormalizeChildPaths() childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Or{Eq{"playlist.id": childPlaylistIds}, Eq{"playlist.path": childPlaylistPaths}}}) if err != nil { log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, err)