nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-18 07:30:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge 9101dfaa17dc362a8aaf1b7da5006e0c1761b858 into 549dfa7f3027a325905791626820a46f2b0afb28

Browse Source
This commit is contained in:
Alexander Mankuta 2026-09-19 20:21:14 +03:00 committed by GitHub
commit 957b4fb36b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 193 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
model/criteria/fields.go
View File

@ -12,6 +12,13 @@ type FieldInfo struct {
// Nullable: isMissing/isPresent are supported on this column field. For numeric/boolean
// fields, missing means NULL; for string fields it means NULL or empty string.
Nullable bool
// ParticipantRole: this field matches an attribute of the participants with this role
// (named by ParticipantAttr), not a media_file column. Role fields (IsRole) match the
// participant names; these fields match another attribute of the same participants.
ParticipantRole string
// ParticipantAttr: which participant attribute is compared; see the ParticipantAttr*
// constants. The storage layer maps it to a column.
ParticipantAttr string
tagAlias string // If set, a tag name from mappings.yaml that resolves to this field
name string // Canonical name, populated by LookupField from the map key
@ -22,6 +29,14 @@ func (f FieldInfo) Name() string {
return f.name
}
// Participant attributes matchable by criteria fields (see FieldInfo.ParticipantAttr).
const (
// ParticipantAttrMbid is the participant's MusicBrainz artist ID. The media_file
// mbz_artist_id/mbz_album_artist_id columns are deprecated and no longer populated by
// the scanner, so these fields resolve through the participants instead.
ParticipantAttrMbid = "mbid"
)
var fieldMap = map[string]FieldInfo{
"title": {},
"album": {Nullable: true},
@ -83,8 +98,8 @@ var fieldMap = map[string]FieldInfo{
"artistdateloved": {},
"artistdaterated": {},
"mbz_album_id": {Nullable: true},
"mbz_album_artist_id": {Nullable: true},
"mbz_artist_id": {Nullable: true},
"mbz_album_artist_id": {ParticipantRole: "albumartist", ParticipantAttr: ParticipantAttrMbid},
"mbz_artist_id": {ParticipantRole: "artist", ParticipantAttr: ParticipantAttrMbid},
"mbz_recording_id": {Nullable: true},
"mbz_release_track_id": {Nullable: true},
"mbz_release_group_id": {Nullable: true},

18
model/criteria/fields_test.go
View File

@ -76,7 +76,7 @@ var _ = Describe("fields", func() {
})
It("marks mbz_* and lyrics string fields as nullable (empty means missing)", func() {
for _, name := range []string{"mbz_album_id", "mbz_album_artist_id", "mbz_artist_id",
for _, name := range []string{"mbz_album_id",
"mbz_recording_id", "mbz_release_track_id", "mbz_release_group_id", "lyrics",
"album", "comment", "catalognumber", "discsubtitle", "albumcomment",
"sorttitle", "sortalbum", "sortartist", "sortalbumartist", "explicitstatus"} {
@ -87,5 +87,21 @@ var _ = Describe("fields", func() {
}
})
It("maps artist MBID fields to participant attribute matching", func() {
// media_file.mbz_artist_id/mbz_album_artist_id are deprecated and never populated by the
// scanner; these fields must match the MBID of the corresponding participants.
field, ok := LookupField("mbz_artist_id")
gomega.Expect(ok).To(gomega.BeTrue())
gomega.Expect(field.ParticipantRole).To(gomega.Equal("artist"))
gomega.Expect(field.ParticipantAttr).To(gomega.Equal(ParticipantAttrMbid))
gomega.Expect(field.Nullable).To(gomega.BeFalse())
field, ok = LookupField("mbz_album_artist_id")
gomega.Expect(ok).To(gomega.BeTrue())
gomega.Expect(field.ParticipantRole).To(gomega.Equal("albumartist"))
gomega.Expect(field.ParticipantAttr).To(gomega.Equal(ParticipantAttrMbid))
gomega.Expect(field.Nullable).To(gomega.BeFalse())
})
})
})

71
persistence/criteria_sql.go
View File

@ -116,8 +116,8 @@ var smartPlaylistFields = map[string]smartPlaylistField{
"artistdateloved": {expr: "artist_annotation.starred_at", joinType: smartPlaylistJoinArtistAnnotation},
"artistdaterated": {expr: "artist_annotation.rated_at", joinType: smartPlaylistJoinArtistAnnotation},
"mbz_album_id": {expr: "media_file.mbz_album_id"},
"mbz_album_artist_id": {expr: "media_file.mbz_album_artist_id"},
"mbz_artist_id": {expr: "media_file.mbz_artist_id"},
"mbz_album_artist_id": {order: participantSortExpr("albumartist", "artist.mbz_artist_id")},
"mbz_artist_id": {order: participantSortExpr("artist", "artist.mbz_artist_id")},
"mbz_recording_id": {expr: "media_file.mbz_recording_id"},
"mbz_release_track_id": {expr: "media_file.mbz_release_track_id"},
"mbz_release_group_id": {expr: "media_file.mbz_release_group_id"},
@ -198,7 +198,7 @@ func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqliz
}
func isNotExpr(values map[string]any) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) {
if _, value, info, ok := singleField(values); ok && jsonField(info) {
return jsonExpr(info, squirrel.Eq{"value": value}, true), nil
}
return comparisonExpr(values, cmpNe)
@ -219,6 +219,10 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er
negate := checkAbsence == b
switch {
case info.ParticipantRole != "":
// Present means the role has a participant with a non-empty attribute; a NULL column
// never satisfies <> '', so NULLs count as missing too.
return jsonExpr(info, squirrel.NotEq{"value": ""}, negate), nil
case info.IsTag || info.IsRole:
return jsonExpr(info, nil, negate), nil
case info.Nullable:
@ -254,7 +258,7 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er
func likeExpr(values map[string]any, pattern string, negate bool) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok {
if info.IsTag || info.IsRole {
if jsonField(info) {
return jsonExpr(info, squirrel.Like{"value": fmt.Sprintf(pattern, value)}, negate), nil
}
// LIKE can't use the column index, so annotation fields keep the COALESCE form: a NULL
@ -294,8 +298,8 @@ func rangeExpr(values map[string]any) (squirrel.Sqlizer, error) {
if !ok {
return nil, fmt.Errorf("invalid field in criteria: %s", field)
}
if info.IsTag || info.IsRole {
// Tags/roles are multi-valued JSON, so splitting a range into two independent EXISTS
if jsonField(info) {
// Tags/roles are multi-valued, so splitting a range into two independent EXISTS
// subqueries would let different values satisfy each bound. Ranges are unsupported there.
return nil, fmt.Errorf("range operator not supported for tag/role field: %s", field)
}
@ -373,7 +377,31 @@ func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squir
return squirrel.Expr("media_file.id IN ("+subSQL+")", subArgs...), nil
}
// jsonField reports whether the field is matched via an EXISTS subquery (tags JSON, role
// name, or another participant attribute) rather than a direct column comparison.
func jsonField(info criteria.FieldInfo) bool {
return info.IsTag || info.IsRole || info.ParticipantRole != ""
}
// participantAttrCols maps a criteria participant attribute to the artist table column
// holding it.
var participantAttrCols = map[string]string{
criteria.ParticipantAttrMbid: "artist.mbz_artist_id",
}
// participantSortExpr resolves a participant attribute for ORDER BY. The participants JSON
// column only carries id/name/subRole, so other attributes are read through the
// media_file_artists join table; min() keeps the value deterministic for multi-artist tracks.
func participantSortExpr(role, col string) string {
return "COALESCE((select min(" + col + ") from media_file_artists mfa" +
" join artist on artist.id = mfa.artist_id" +
" where mfa.media_file_id = media_file.id and mfa.role = '" + role + "'), '')"
}
func jsonExpr(info criteria.FieldInfo, cond squirrel.Sqlizer, negate bool) squirrel.Sqlizer {
if info.ParticipantRole != "" {
return roleCond{role: info.ParticipantRole, col: participantAttrCols[info.ParticipantAttr], cond: cond, not: negate}
}
if info.IsRole {
return roleCond{role: info.Name(), cond: cond, not: negate}
}
@ -408,6 +436,7 @@ func (e tagCond) ToSql() (string, []any, error) {
type roleCond struct {
role string
col string
cond squirrel.Sqlizer
not bool
}
@ -416,7 +445,7 @@ func (e roleCond) ToSql() (string, []any, error) {
var cond string
var args []any
if e.cond != nil {
innerSQL, innerArgs, err := roleCondSQL(e.cond)
innerSQL, innerArgs, err := roleCondSQL(e.cond, e.col)
if err != nil {
return "", nil, err
}
@ -433,12 +462,15 @@ func (e roleCond) ToSql() (string, []any, error) {
}
// roleCondSQL extracts SQL from a squirrel condition and rewrites the placeholder column name.
func roleCondSQL(cond squirrel.Sqlizer) (string, []any, error) {
func roleCondSQL(cond squirrel.Sqlizer, col string) (string, []any, error) {
sql, args, err := cond.ToSql()
if err != nil {
return "", nil, err
}
return strings.ReplaceAll(sql, "value", "artist.name"), args, nil
if col == "" {
col = "artist.name"
}
return strings.ReplaceAll(sql, "value", col), args, nil
}
// roleExistsSQL wraps a condition fragment in the standard role EXISTS subquery.
@ -485,6 +517,8 @@ func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sql
isRole bool
numeric bool
tag string
role string
col string
}
groups := make(map[string]*group)
for i, s := range conds {
@ -493,10 +527,13 @@ func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sql
if c.not != negated || c.cond == nil {
continue
}
g, exists := groups["role:"+c.role]
// The column is part of the key: conditions on different artist columns for the
// same role must not be ORed inside one EXISTS.
key := "role:" + c.role + ":" + c.col
g, exists := groups[key]
if !exists {
g = &group{isRole: true}
groups["role:"+c.role] = g
g = &group{isRole: true, role: c.role, col: c.col}
groups[key] = g
}
g.entries = append(g.entries, condEntry{index: i, cond: c.cond})
case tagCond:
@ -525,9 +562,8 @@ func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sql
batchConds[i] = e.cond
}
if g.isRole {
role := key[len("role:"):]
for batch := range slices.Chunk(batchConds, jsonCondBatchSize) {
additions = append(additions, roleCondGroup{role: role, conds: batch, not: negated})
additions = append(additions, roleCondGroup{role: g.role, col: g.col, conds: batch, not: negated})
}
} else {
for batch := range slices.Chunk(batchConds, jsonCondBatchSize) {
@ -553,6 +589,7 @@ func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sql
// (optionally negated) EXISTS subquery for performance.
type roleCondGroup struct {
role string
col string
conds []squirrel.Sqlizer
not bool
}
@ -561,7 +598,7 @@ func (g roleCondGroup) ToSql() (string, []any, error) {
innerParts := make([]string, 0, len(g.conds))
allArgs := []any{g.role}
for _, c := range g.conds {
part, args, err := roleCondSQL(c)
part, args, err := roleCondSQL(c, g.col)
if err != nil {
return "", nil, err
}
@ -624,7 +661,7 @@ func sqlFields(values map[string]any) (map[string]any, error) {
if !ok {
return nil, fmt.Errorf("invalid field in criteria: %s", field)
}
if info.IsTag || info.IsRole {
if jsonField(info) {
return nil, fmt.Errorf("tag and role criteria must contain exactly one field: %s", field)
}
sqlField, ok := fieldExpr(info.Name())
@ -681,7 +718,7 @@ func (f smartPlaylistField) coalesced() string {
func comparisonExpr(values map[string]any, cmp comparator) (squirrel.Sqlizer, error) {
if _, value, info, ok := singleField(values); ok {
if info.IsTag || info.IsRole {
if jsonField(info) {
return jsonExpr(info, cmp.build(map[string]any{"value": value}), false), nil
}
if f, isAnnotation := annotationField(info); isAnnotation {

77
persistence/criteria_sql_test.go
View File

@ -107,6 +107,26 @@ var _ = Describe("Smart playlist criteria SQL", func() {
Entry("tag alias", criteria.Is{"albumtype": "album"}, "exists (select 1 from json_tree(media_file.tags, '$.releasetype') where key='value' and value = ?)", "album"),
Entry("field alias via tag registration", criteria.Is{"recordingdate": "2024-01-01"}, "media_file.date = ?", "2024-01-01"),
Entry("role is", criteria.Is{"artist": "u2"}, "exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.name = ?)", "artist", "u2"),
// mbz_artist_id/mbz_album_artist_id match the MBID of the role's participants, not the
// deprecated (and never populated) media_file columns.
Entry("role mbid is", criteria.Is{"mbz_artist_id": "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"},
"exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id = ?)",
"artist", "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"),
Entry("album artist mbid is", criteria.Is{"mbz_album_artist_id": "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"},
"exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id = ?)",
"albumartist", "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"),
Entry("role mbid is not", criteria.IsNot{"mbz_artist_id": "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"},
"not exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id = ?)",
"artist", "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"),
Entry("role mbid contains", criteria.Contains{"mbz_artist_id": "b10bbbfc"},
"exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id LIKE ?)",
"artist", "%b10bbbfc%"),
Entry("isPresent mbz_artist_id [true]", criteria.IsPresent{"mbz_artist_id": true},
"exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id <> ?)",
"artist", ""),
Entry("isMissing mbz_artist_id [true]", criteria.IsMissing{"mbz_artist_id": true},
"not exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.mbz_artist_id <> ?)",
"artist", ""),
Entry("role contains", criteria.Contains{"composer": "Lennon"}, "exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.name LIKE ?)", "composer", "%Lennon%"),
Entry("role not contains", criteria.NotContains{"artist": "u2"}, "not exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and artist.name LIKE ?)", "artist", "%u2%"),
// ReplayGain fields
@ -273,6 +293,11 @@ var _ = Describe("Smart playlist criteria SQL", func() {
Expect(err).To(MatchError(ContainSubstring("range operator not supported for tag/role field")))
})
It("returns an error for a range over a role MBID field", func() {
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"mbz_artist_id": []int{1, 5}}}).where()
Expect(err).To(MatchError(ContainSubstring("range operator not supported for tag/role field")))
})
It("returns a clear error for a malformed range value", func() {
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"playCount": []int{1, 2, 3}}}).where()
Expect(err).To(MatchError(ContainSubstring("must be a [min, max] pair")))
@ -291,6 +316,13 @@ var _ = Describe("Smart playlist criteria SQL", func() {
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "artist"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') asc"))
})
It("sorts by role MBID fields", func() {
// The participants JSON in the DB does not carry MBIDs, so sorting resolves them
// through the media_file_artists join table.
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "mbz_artist_id"}).orderBy()).
To(Equal("COALESCE((select min(artist.mbz_artist_id) from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = 'artist'), '') asc"))
})
It("casts numeric tags when sorting", func() {
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "rate"}).orderBy()).To(Equal("CAST(COALESCE(json_extract(media_file.tags, '$.rate[0].value'), '') AS REAL) asc"))
})
@ -321,6 +353,20 @@ var _ = Describe("Smart playlist criteria SQL", func() {
})
})
It("has a column mapping for every participant attribute used by a criteria field", func() {
// An unmapped attribute would resolve to an empty column, silently falling back to
// matching artist.name.
for _, name := range criteria.AllFieldNames() {
info, ok := criteria.LookupField(name)
Expect(ok).To(BeTrue(), "field %q registered but LookupField fails", name)
if info.ParticipantRole == "" {
continue
}
Expect(participantAttrCols).To(HaveKey(info.ParticipantAttr),
"criteria field %q uses participant attribute %q, which has no column in participantAttrCols", name, info.ParticipantAttr)
}
})
It("has SQL mappings for all non-tag/non-role criteria fields", func() {
for _, name := range criteria.AllFieldNames() {
info, ok := criteria.LookupField(name)
@ -371,6 +417,37 @@ var _ = Describe("Smart playlist criteria SQL", func() {
Expect(args).To(HaveExactElements("artist", "%Beatles%", "%Kraftwerk%", "%Pink Floyd%"))
})
It("merges multiple role MBID conditions in an OR group into a single EXISTS", func() {
expr := criteria.Any{
criteria.Is{"mbz_artist_id": "mbid-1"},
criteria.Is{"mbz_artist_id": "mbid-2"},
}
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("(exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id where mfa.media_file_id = media_file.id and mfa.role = ? and (artist.mbz_artist_id = ? OR artist.mbz_artist_id = ?)))"))
Expect(args).To(HaveExactElements("artist", "mbid-1", "mbid-2"))
})
It("does not merge role name and role MBID conditions for the same role", func() {
// Both resolve to role "artist" but compare different columns; merging them would OR
// a name pattern against the MBID column (or vice versa).
expr := criteria.Any{
criteria.Contains{"artist": "Beatles"},
criteria.Is{"mbz_artist_id": "mbid-1"},
}
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "exists")).To(Equal(2))
Expect(sql).To(ContainSubstring("artist.name LIKE ?"))
Expect(sql).To(ContainSubstring("artist.mbz_artist_id = ?"))
})
It("does not merge role conditions from different roles", func() {
expr := criteria.Any{
criteria.Contains{"artist": "Beatles"},

28
persistence/playlist_repository_test.go
View File

@ -266,6 +266,34 @@ var _ = Describe("PlaylistRepository", func() {
})
})
Describe("smart playlist matching by artist MBID", func() {
const beatlesMbid = "b10bbbfc-cf9e-42e0-be17-e2c3e1d2600d"
BeforeEach(func() {
_, err := GetDBXBuilder().NewQuery("update artist set mbz_artist_id = {:mbid} where id = '3'").
Bind(dbx.Params{"mbid": beatlesMbid}).Execute()
Expect(err).ToNot(HaveOccurred())
DeferCleanup(func() {
_, err := GetDBXBuilder().NewQuery("update artist set mbz_artist_id = '' where id = '3'").Execute()
Expect(err).ToNot(HaveOccurred())
})
})
It("matches all tracks whose artist has the given MBID", func() {
pls := model.Playlist{Name: "Beatles by MBID", OwnerID: "userid", Rules: &criteria.Criteria{
Expression: criteria.All{criteria.Is{"mbz_artist_id": beatlesMbid}},
Sort: "title",
}}
Expect(repo.Put(&pls)).To(Succeed())
DeferCleanup(func() { Expect(repo.Delete(pls.ID)).To(Succeed()) })
saved, err := repo.GetWithTracks(pls.ID, true, false)
Expect(err).ToNot(HaveOccurred())
trackIDs := slice.Map(saved.Tracks, func(t model.PlaylistTrack) string { return t.MediaFileID })
Expect(trackIDs).To(ConsistOf("1001", "1002", "3002")) // all songs by The Beatles
})
})
Describe("Put", func() {
It("does not overwrite counters when saving a smart playlist", func() {
pls := model.Playlist{Name: "Smart Counters", OwnerID: "userid", Rules: &criteria.Criteria{