remote/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-08 02:17:25 +02:00
Code Issues Projects Releases Packages Wiki Activity

refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)

Browse source 
* test: add tests for recordingdate alias resolution in smart playlists

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: update FieldInfo structure and simplify fieldMap initialization

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: move sort parsing logic from persistence to criteria package

Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.

* refactor: standardize field access in smartPlaylistCriteria structure

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: add ResolveLimit method to Criteria

Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.

* refactor: improve child playlist loading and error handling in refresh logic

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: extract smart playlist logic to dedicated files

Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.

* refactor: break refreshSmartPlaylist into smaller focused methods

Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.

* refactor: deduplicate child playlist IDs in Criteria

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor: simplify withSmartPlaylistOwner to accept model.User

Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.

* fix: handle empty sort fields and propagate child playlist load errors

OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.

* refactor: log warning when no valid sort fields are found

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão 2026-04-26 14:49:59 -04:00 • committed by GitHub
commit 1bd736dae9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 1069 additions and 827 deletions
Download patch file Download diff file Expand all files Collapse all files

20
model/criteria/criteria.go
View file

@ -4,6 +4,7 @@ package criteria
import (
"encoding/json"
"errors"
"slices"
"github.com/navidrome/navidrome/log"
)
@ -42,6 +43,16 @@ func (c Criteria) EffectiveLimit(totalCount int64) int {
return 0
}
// ResolveLimit converts a percentage-based limit into an absolute Limit using
// the given totalCount. It is a no-op when a fixed Limit is already set or when
// no percentage limit is configured.
func (c *Criteria) ResolveLimit(totalCount int64) {
if !c.IsPercentageLimit() {
return
}
c.Limit = c.EffectiveLimit(totalCount)
}
// IsPercentageLimit returns true when the criteria uses a valid percentage-based
// limit (i.e. LimitPercent is in [1, 100] and no fixed Limit overrides it).
func (c Criteria) IsPercentageLimit() bool {
@ -53,11 +64,14 @@ func (c Criteria) ChildPlaylistIds() []string {
return nil
}
if parent, ok := c.Expression.(conjunction); ok {
return parent.ChildPlaylistIds()
parent, ok := c.Expression.(conjunction)
if !ok {
return nil
}
return nil
ids := parent.ChildPlaylistIds()
slices.Sort(ids)
return slices.Compact(ids)
}
func (c Criteria) MarshalJSON() ([]byte, error) {

47
model/criteria/criteria_test.go
View file

@ -177,6 +177,39 @@ var _ = Describe("Criteria", func() {
})
})
Describe("ResolveLimit", func() {
It("resolves percentage to absolute limit preserving LimitPercent", func() {
c := Criteria{LimitPercent: 10}
c.ResolveLimit(450)
gomega.Expect(c.Limit).To(gomega.Equal(45))
})
It("does nothing when Limit is already set", func() {
c := Criteria{Limit: 50, LimitPercent: 10}
c.ResolveLimit(1000)
gomega.Expect(c.Limit).To(gomega.Equal(50))
})
It("does nothing when no limit is configured", func() {
c := Criteria{}
c.ResolveLimit(1000)
gomega.Expect(c.Limit).To(gomega.Equal(0))
})
It("sets minimum 1 when percentage rounds to 0 and totalCount > 0", func() {
c := Criteria{LimitPercent: 1}
c.ResolveLimit(5)
gomega.Expect(c.Limit).To(gomega.Equal(1))
})
It("is idempotent when called twice", func() {
c := Criteria{LimitPercent: 10}
c.ResolveLimit(450)
c.ResolveLimit(450)
gomega.Expect(c.Limit).To(gomega.Equal(45))
})
})
Describe("IsPercentageLimit", func() {
It("returns true when LimitPercent is set and Limit is 0", func() {
c := Criteria{LimitPercent: 10}
@ -269,5 +302,19 @@ var _ = Describe("Criteria", func() {
ids := Criteria{Expression: Is{"title": "Low Rider"}}.ChildPlaylistIds()
gomega.Expect(ids).To(gomega.BeEmpty())
})
It("deduplicates repeated playlist IDs", func() {
sharedID := uuid.NewString()
goObj = Criteria{
Expression: All{
InPlaylist{"id": sharedID},
Any{
InPlaylist{"id": sharedID},
NotInPlaylist{"id": sharedID},
},
},
}
ids := goObj.ChildPlaylistIds()
gomega.Expect(ids).To(gomega.Equal([]string{sharedID}))
})
})
})

160
model/criteria/fields.go
View file

@ -2,90 +2,83 @@ package criteria
import "strings"
// FieldInfo describes a criteria field without tying it to persistence details.
// FieldInfo contains semantic metadata about a criteria field
type FieldInfo struct {
Name string
IsTag bool
IsRole bool
Numeric bool
alias string
}
var fieldMap = map[string]*fieldMetadata{
"title": {name: "title"},
"album": {name: "album"},
"hascoverart": {name: "hascoverart"},
"tracknumber": {name: "tracknumber"},
"discnumber": {name: "discnumber"},
"year": {name: "year"},
"date": {name: "date", alias: "recordingdate"},
"originalyear": {name: "originalyear"},
"originaldate": {name: "originaldate"},
"releaseyear": {name: "releaseyear"},
"releasedate": {name: "releasedate"},
"size": {name: "size"},
"compilation": {name: "compilation"},
"missing": {name: "missing"},
"explicitstatus": {name: "explicitstatus"},
"dateadded": {name: "dateadded"},
"datemodified": {name: "datemodified"},
"discsubtitle": {name: "discsubtitle"},
"comment": {name: "comment"},
"lyrics": {name: "lyrics"},
"sorttitle": {name: "sorttitle"},
"sortalbum": {name: "sortalbum"},
"sortartist": {name: "sortartist"},
"sortalbumartist": {name: "sortalbumartist"},
"albumcomment": {name: "albumcomment"},
"catalognumber": {name: "catalognumber"},
"filepath": {name: "filepath"},
"filetype": {name: "filetype"},
"codec": {name: "codec"},
"duration": {name: "duration"},
"bitrate": {name: "bitrate"},
"bitdepth": {name: "bitdepth"},
"samplerate": {name: "samplerate"},
"bpm": {name: "bpm"},
"channels": {name: "channels"},
"loved": {name: "loved"},
"dateloved": {name: "dateloved"},
"lastplayed": {name: "lastplayed"},
"daterated": {name: "daterated"},
"playcount": {name: "playcount"},
"rating": {name: "rating"},
"averagerating": {name: "averagerating", numeric: true},
"albumrating": {name: "albumrating"},
"albumloved": {name: "albumloved"},
"albumplaycount": {name: "albumplaycount"},
"albumlastplayed": {name: "albumlastplayed"},
"albumdateloved": {name: "albumdateloved"},
"albumdaterated": {name: "albumdaterated"},
"artistrating": {name: "artistrating"},
"artistloved": {name: "artistloved"},
"artistplaycount": {name: "artistplaycount"},
"artistlastplayed": {name: "artistlastplayed"},
"artistdateloved": {name: "artistdateloved"},
"artistdaterated": {name: "artistdaterated"},
"mbz_album_id": {name: "mbz_album_id"},
"mbz_album_artist_id": {name: "mbz_album_artist_id"},
"mbz_artist_id": {name: "mbz_artist_id"},
"mbz_recording_id": {name: "mbz_recording_id"},
"mbz_release_track_id": {name: "mbz_release_track_id"},
"mbz_release_group_id": {name: "mbz_release_group_id"},
"library_id": {name: "library_id", numeric: true},
var fieldMap = map[string]FieldInfo{
"title": {Name: "title"},
"album": {Name: "album"},
"hascoverart": {Name: "hascoverart"},
"tracknumber": {Name: "tracknumber"},
"discnumber": {Name: "discnumber"},
"year": {Name: "year"},
"date": {Name: "date", alias: "recordingdate"},
"originalyear": {Name: "originalyear"},
"originaldate": {Name: "originaldate"},
"releaseyear": {Name: "releaseyear"},
"releasedate": {Name: "releasedate"},
"size": {Name: "size"},
"compilation": {Name: "compilation"},
"missing": {Name: "missing"},
"explicitstatus": {Name: "explicitstatus"},
"dateadded": {Name: "dateadded"},
"datemodified": {Name: "datemodified"},
"discsubtitle": {Name: "discsubtitle"},
"comment": {Name: "comment"},
"lyrics": {Name: "lyrics"},
"sorttitle": {Name: "sorttitle"},
"sortalbum": {Name: "sortalbum"},
"sortartist": {Name: "sortartist"},
"sortalbumartist": {Name: "sortalbumartist"},
"albumcomment": {Name: "albumcomment"},
"catalognumber": {Name: "catalognumber"},
"filepath": {Name: "filepath"},
"filetype": {Name: "filetype"},
"codec": {Name: "codec"},
"duration": {Name: "duration"},
"bitrate": {Name: "bitrate"},
"bitdepth": {Name: "bitdepth"},
"samplerate": {Name: "samplerate"},
"bpm": {Name: "bpm"},
"channels": {Name: "channels"},
"loved": {Name: "loved"},
"dateloved": {Name: "dateloved"},
"lastplayed": {Name: "lastplayed"},
"daterated": {Name: "daterated"},
"playcount": {Name: "playcount"},
"rating": {Name: "rating"},
"averagerating": {Name: "averagerating", Numeric: true},
"albumrating": {Name: "albumrating"},
"albumloved": {Name: "albumloved"},
"albumplaycount": {Name: "albumplaycount"},
"albumlastplayed": {Name: "albumlastplayed"},
"albumdateloved": {Name: "albumdateloved"},
"albumdaterated": {Name: "albumdaterated"},
"artistrating": {Name: "artistrating"},
"artistloved": {Name: "artistloved"},
"artistplaycount": {Name: "artistplaycount"},
"artistlastplayed": {Name: "artistlastplayed"},
"artistdateloved": {Name: "artistdateloved"},
"artistdaterated": {Name: "artistdaterated"},
"mbz_album_id": {Name: "mbz_album_id"},
"mbz_album_artist_id": {Name: "mbz_album_artist_id"},
"mbz_artist_id": {Name: "mbz_artist_id"},
"mbz_recording_id": {Name: "mbz_recording_id"},
"mbz_release_track_id": {Name: "mbz_release_track_id"},
"mbz_release_group_id": {Name: "mbz_release_group_id"},
"library_id": {Name: "library_id", Numeric: true},
// Backward compatibility: albumtype is an alias for the releasetype tag.
"albumtype": {name: "releasetype", isTag: true},
"albumtype": {Name: "releasetype", IsTag: true},
"random": {name: "random"},
"value": {name: "value"},
}
type fieldMetadata struct {
name string
isRole bool
isTag bool
alias string
numeric bool
"random": {Name: "random"},
"value": {Name: "value"},
}
// AllFieldNames returns the names of all registered criteria fields.
@ -100,15 +93,7 @@ func AllFieldNames() []string {
// LookupField returns semantic metadata for a criteria field name.
func LookupField(name string) (FieldInfo, bool) {
f, ok := fieldMap[strings.ToLower(name)]
if !ok {
return FieldInfo{}, false
}
return FieldInfo{
Name: f.name,
IsTag: f.isTag,
IsRole: f.isRole,
Numeric: f.numeric,
}, true
return f, ok
}
// AddRoles adds roles to the field map. This is used to add all artist roles to the field map, so they can be used in
@ -119,7 +104,7 @@ func AddRoles(roles []string) {
if _, ok := fieldMap[name]; ok {
continue
}
fieldMap[name] = &fieldMetadata{name: name, isRole: true}
fieldMap[name] = FieldInfo{Name: name, IsRole: true}
}
}
@ -138,7 +123,7 @@ func AddTagNames(tagNames []string) {
}
}
if _, ok := fieldMap[name]; !ok {
fieldMap[name] = &fieldMetadata{name: name, isTag: true}
fieldMap[name] = FieldInfo{Name: name, IsTag: true}
}
}
}
@ -148,9 +133,10 @@ func AddNumericTags(tagNames []string) {
for _, tagName := range tagNames {
name := strings.ToLower(tagName)
if fm, ok := fieldMap[name]; ok {
fm.numeric = true
fm.Numeric = true
fieldMap[name] = fm
} else {
fieldMap[name] = &fieldMetadata{name: name, isTag: true, numeric: true}
fieldMap[name] = FieldInfo{Name: name, IsTag: true, Numeric: true}
}
}
}

62
model/criteria/sort.go Normal file
View file

@ -0,0 +1,62 @@
package criteria
import (
"strings"
"github.com/navidrome/navidrome/log"
)
type SortField struct {
Field string
Desc bool
}
func (c Criteria) OrderByFields() []SortField {
sortValue := c.Sort
if sortValue == "" {
sortValue = "title"
}
order := strings.ToLower(strings.TrimSpace(c.Order))
if order != "" && order != "asc" && order != "desc" {
log.Error("Invalid value in 'order' field. Valid values: 'asc', 'desc'", "order", c.Order)
order = ""
}
parts := strings.Split(sortValue, ",")
fields := make([]SortField, 0, len(parts))
for _, part := range parts {
part = strings.TrimSpace(part)
if part == "" {
continue
}
desc := false
if strings.HasPrefix(part, "+") || strings.HasPrefix(part, "-") {
desc = strings.HasPrefix(part, "-")
part = strings.TrimSpace(part[1:])
}
info, ok := LookupField(part)
if !ok {
log.Error("Invalid field in 'sort' field", "sort", part)
continue
}
if order == "desc" {
desc = !desc
}
fields = append(fields, SortField{Field: info.Name, Desc: desc})
}
if len(fields) == 0 {
log.Warn("No valid sort fields found in 'sort', falling back to 'title'", "sort", sortValue)
return []SortField{{Field: "title", Desc: false}}
}
return fields
}
func (c Criteria) SortFieldNames() []string {
sortFields := c.OrderByFields()
names := make([]string, len(sortFields))
for i, sf := range sortFields {
names[i] = sf.Field
}
return names
}

103
model/criteria/sort_test.go Normal file
View file

@ -0,0 +1,103 @@
package criteria
import (
. "github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"
)
var _ = Describe("OrderByFields", func() {
It("defaults to title ascending when Sort is empty", func() {
c := Criteria{}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: false}}))
})
It("parses a single field", func() {
c := Criteria{Sort: "title"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: false}}))
})
It("parses descending prefix", func() {
c := Criteria{Sort: "-rating"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "rating", Desc: true}}))
})
It("parses ascending prefix", func() {
c := Criteria{Sort: "+title"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: false}}))
})
It("parses multiple comma-separated fields", func() {
c := Criteria{Sort: "title,-rating"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{
{Field: "title", Desc: false},
{Field: "rating", Desc: true},
}))
})
It("inverts directions when Order is desc", func() {
c := Criteria{Sort: "-date,title", Order: "desc"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{
{Field: "date", Desc: false},
{Field: "title", Desc: true},
}))
})
It("skips invalid fields", func() {
c := Criteria{Sort: "bogus,title"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: false}}))
})
It("falls back to title when all fields are invalid", func() {
c := Criteria{Sort: "bogus,invalid"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: false}}))
})
It("resolves tag aliases (albumtype -> releasetype)", func() {
c := Criteria{Sort: "albumtype"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "releasetype", Desc: false}}))
})
It("resolves field aliases (recordingdate -> date)", func() {
AddTagNames([]string{"recordingdate"})
c := Criteria{Sort: "recordingdate"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "date", Desc: false}}))
})
It("handles the random field", func() {
c := Criteria{Sort: "random"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "random", Desc: false}}))
})
It("ignores invalid Order value", func() {
c := Criteria{Sort: "-title", Order: "invalid"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{{Field: "title", Desc: true}}))
})
It("handles whitespace in fields", func() {
c := Criteria{Sort: " title , -rating "}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{
{Field: "title", Desc: false},
{Field: "rating", Desc: true},
}))
})
It("skips empty parts from trailing commas", func() {
c := Criteria{Sort: "title,,rating,"}
gomega.Expect(c.OrderByFields()).To(gomega.Equal([]SortField{
{Field: "title", Desc: false},
{Field: "rating", Desc: false},
}))
})
})
var _ = Describe("SortFieldNames", func() {
It("returns canonical field names", func() {
c := Criteria{Sort: "title,-rating,albumtype"}
gomega.Expect(c.SortFieldNames()).To(gomega.Equal([]string{"title", "rating", "releasetype"}))
})
It("defaults to title when Sort is empty", func() {
c := Criteria{}
gomega.Expect(c.SortFieldNames()).To(gomega.Equal([]string{"title"}))
})
})