From 1e5ad0d3f8ca81c69d84f29444b41bb10877f27a Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 19 Sep 2026 17:43:46 -0400 Subject: [PATCH] refactor(smartplaylists): simplify child playlist reference handling Share one extractor for child ids and paths, return only the normalized rules instead of a playlist copy, and resolve each path reference in a single switch. --- model/criteria/criteria.go | 32 ++++++++---------------- model/criteria/criteria_test.go | 4 +-- model/criteria/operators.go | 27 +++++--------------- model/playlist.go | 26 +++++++++---------- model/playlist_test.go | 8 +++--- persistence/smart_playlist_repository.go | 16 ++++++------ 6 files changed, 42 insertions(+), 71 deletions(-) diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index f36f76d25..e6db3b481 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -64,33 +64,21 @@ func (c Criteria) IsPercentageLimit() bool { } func (c Criteria) ChildPlaylistIds() []string { - if c.Expression == nil { - return nil - } + return c.childPlaylistRefs(conjunction.ChildPlaylistIds) +} +func (c Criteria) ChildPlaylistPaths() []string { + return c.childPlaylistRefs(conjunction.ChildPlaylistPaths) +} + +func (c Criteria) childPlaylistRefs(extract func(conjunction) []string) []string { parent, ok := c.Expression.(conjunction) if !ok { return nil } - - ids := parent.ChildPlaylistIds() - slices.Sort(ids) - return slices.Compact(ids) -} - -func (c Criteria) ChildPlaylistPaths() []string { - if c.Expression == nil { - return nil - } - - parent, ok := c.Expression.(interface{ ChildPlaylistPaths() []string }) - if !ok { - return nil - } - - paths := parent.ChildPlaylistPaths() - slices.Sort(paths) - return slices.Compact(paths) + refs := extract(parent) + slices.Sort(refs) + return slices.Compact(refs) } func (c Criteria) MarshalJSON() ([]byte, error) { diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 26f14393e..de5124568 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -366,8 +366,8 @@ var _ = Describe("Criteria", func() { 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)) + paths := goObj.ChildPlaylistPaths() + gomega.Expect(paths).To(gomega.ConsistOf(topLevelInPlaylistPath, nestedAnyNotInPlaylistPath)) }) It("ignores empty child playlist paths", func() { c := Criteria{Expression: All{InPlaylist{"path": ""}, NotInPlaylist{"path": ""}}} diff --git a/model/criteria/operators.go b/model/criteria/operators.go index d49eb1f1e..ec32f2d16 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -1,8 +1,9 @@ package criteria -// Conjunctions need to implement this interface, to allow Criteria to extract child playlist IDs recursively +// Conjunctions need to implement this interface, to allow Criteria to extract child playlist references recursively type conjunction interface { ChildPlaylistIds() []string + ChildPlaylistPaths() []string } type ( @@ -16,13 +17,9 @@ func (all All) MarshalJSON() ([]byte, error) { return marshalConjunction("all", all) } -func (all All) ChildPlaylistIds() (ids []string) { - return extractPlaylistIds(all) -} +func (all All) ChildPlaylistIds() []string { return extractPlaylistField(all, "id") } -func (all All) ChildPlaylistPaths() (paths []string) { - return extractPlaylistPaths(all) -} +func (all All) ChildPlaylistPaths() []string { return extractPlaylistField(all, "path") } type ( Any []Expression @@ -35,13 +32,9 @@ func (any Any) MarshalJSON() ([]byte, error) { return marshalConjunction("any", any) } -func (any Any) ChildPlaylistIds() (ids []string) { - return extractPlaylistIds(any) -} +func (any Any) ChildPlaylistIds() []string { return extractPlaylistField(any, "id") } -func (any Any) ChildPlaylistPaths() (paths []string) { - return extractPlaylistPaths(any) -} +func (any Any) ChildPlaylistPaths() []string { return extractPlaylistField(any, "path") } type Is map[string]any type Eq = Is @@ -197,11 +190,3 @@ func extractPlaylistField(inputRule any, field string) (values []string) { } return } - -func extractPlaylistIds(inputRule any) (ids []string) { - return extractPlaylistField(inputRule, "id") -} - -func extractPlaylistPaths(inputRule any) (paths []string) { - return extractPlaylistField(inputRule, "path") -} diff --git a/model/playlist.go b/model/playlist.go index 467680d2e..19fedc9fa 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -141,16 +141,15 @@ func (pls Playlist) UploadedImagePath() string { return UploadedImagePath(consts.EntityPlaylist, pls.UploadedImage) } -func (pls Playlist) WithNormalizeChildPaths() Playlist { +// NormalizedRules returns the rules with child playlist paths resolved to absolute, OS-native paths. +func (pls Playlist) NormalizedRules() *criteria.Criteria { if pls.Rules == nil || pls.Rules.Expression == nil { - return pls + return pls.Rules } rules := *pls.Rules rules.Expression = normalizePlaylistPaths(pls.Rules.Expression, pls.Path) - plsClone := pls - plsClone.Rules = &rules - return plsClone + return &rules } func normalizePlaylistPaths(inputRule criteria.Expression, referencingPlaylistPath string) criteria.Expression { @@ -177,22 +176,23 @@ func normalizePlaylistPaths(inputRule criteria.Expression, referencingPlaylistPa } func normalizeChildPathRule(rule map[string]any, referencingPlaylistPath string) map[string]any { - normalized := maps.Clone(rule) path, ok := rule["path"].(string) if !ok || path == "" { - return normalized + return rule } // References use forward slashes to stay portable, while Playlist.Path is OS-native. path = filepath.FromSlash(path) - normalized["path"] = path - if isAbsPlaylistRef(path) { - normalized["path"] = filepath.Clean(path) - } else if referencingPlaylistPath != "" { - normalized["path"] = filepath.Clean(filepath.Join(filepath.Dir(referencingPlaylistPath), path)) - } else { + switch { + case isAbsPlaylistRef(path): + path = filepath.Clean(path) + case referencingPlaylistPath != "": + path = filepath.Join(filepath.Dir(referencingPlaylistPath), path) + default: log.Warn("Cannot resolve relative playlist reference: playlist has no file path", "reference", path) } + normalized := maps.Clone(rule) + normalized["path"] = path return normalized } diff --git a/model/playlist_test.go b/model/playlist_test.go index 8fab8ccaa..7ffb69b63 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -90,7 +90,7 @@ var _ = Describe("Playlist", func() { }) }) - Describe("WithNormalizeChildPaths()", func() { + Describe("NormalizedRules()", func() { // absPath builds an OS-native absolute path so these specs also run on Windows. absPath := func(parts ...string) string { abs, err := filepath.Abs(filepath.Join(parts...)) @@ -98,7 +98,7 @@ var _ = Describe("Playlist", func() { return abs } normalize := func(pls model.Playlist) criteria.Expression { - return pls.WithNormalizeChildPaths().Rules.Expression + return pls.NormalizedRules().Expression } It("resolves relative references against the playlist folder", func() { @@ -175,7 +175,7 @@ var _ = Describe("Playlist", func() { } pls := model.Playlist{Path: absPath("test", "my-playlist.nsp"), Rules: &rules} - normalized := *pls.WithNormalizeChildPaths().Rules + normalized := *pls.NormalizedRules() normalized.Expression = rules.Expression Expect(normalized).To(Equal(rules)) }) @@ -183,7 +183,7 @@ var _ = Describe("Playlist", func() { It("does not mutate the original playlist rules", func() { original := criteria.All{criteria.InPlaylist{"path": "child.nsp"}} pls := model.Playlist{Path: absPath("test", "my-playlist.nsp"), Rules: &criteria.Criteria{Expression: original}} - _ = pls.WithNormalizeChildPaths() + _ = pls.NormalizedRules() Expect(original[0]).To(BeEquivalentTo(criteria.InPlaylist{"path": "child.nsp"})) }) }) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index eec2aecbd..24c6f5fc5 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -1,11 +1,13 @@ package persistence import ( + "slices" "time" . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "golang.org/x/text/unicode/norm" ) @@ -42,18 +44,17 @@ func (r *playlistRepository) refreshSmartPlaylistTree(pls *model.Playlist, visit return false } - normalisedPls := pls.WithNormalizeChildPaths() - rulesSQL := newSmartPlaylistCriteria(*normalisedPls.Rules, withSmartPlaylistOwner(*usr)) + rulesSQL := newSmartPlaylistCriteria(*pls.NormalizedRules(), withSmartPlaylistOwner(*usr)) - if !r.refreshChildPlaylists(&normalisedPls, rulesSQL, visited) { + if !r.refreshChildPlaylists(pls, rulesSQL, visited) { return false } - if err := r.resolvePercentageLimit(&normalisedPls, &rulesSQL, usr.ID); err != nil { + if err := r.resolvePercentageLimit(pls, &rulesSQL, usr.ID); err != nil { return false } - sq := r.buildSmartPlaylistQuery(&normalisedPls, rulesSQL, usr.ID) + sq := r.buildSmartPlaylistQuery(pls, 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) @@ -114,10 +115,7 @@ func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL conditions = append(conditions, Eq{"playlist.id": childPlaylistIds}) } if len(childPlaylistPaths) > 0 { - lookupPaths := make([]string, 0, len(childPlaylistPaths)) - for _, path := range childPlaylistPaths { - lookupPaths = append(lookupPaths, pathVariants(path)...) - } + lookupPaths := slices.Concat(slice.Map(childPlaylistPaths, pathVariants)...) conditions = append(conditions, Eq{"playlist.path": lookupPaths}) }