From 9101dfaa17dc362a8aaf1b7da5006e0c1761b858 Mon Sep 17 00:00:00 2001 From: Alexander Mankuta Date: Fri, 21 Aug 2026 17:53:36 +0300 Subject: [PATCH] fix(smartplaylist): add matching on artist fields This adds machinery for matching on fields in the track's artist/album artist. And as the most useful example implements mapping for MBIDs. Artist/Album Artist MBID fields on the track were deprecated some time ago and are not populated now. This new mapping fixes that. Related #4421 --- model/criteria/fields.go | 19 +++++- model/criteria/fields_test.go | 18 +++++- persistence/criteria_sql.go | 71 +++++++++++++++++------ persistence/criteria_sql_test.go | 77 +++++++++++++++++++++++++ persistence/playlist_repository_test.go | 28 +++++++++ 5 files changed, 193 insertions(+), 20 deletions(-) diff --git a/model/criteria/fields.go b/model/criteria/fields.go index a9a634c01..695ea2a37 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -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}, diff --git a/model/criteria/fields_test.go b/model/criteria/fields_test.go index 2367101a8..d03b5442d 100644 --- a/model/criteria/fields_test.go +++ b/model/criteria/fields_test.go @@ -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()) + }) + }) }) diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 890f757d7..be6823b43 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -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 { diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 8d0069b0d..9fb6ab81e 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -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"}, diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 9697e6fff..a1fffa532 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -231,6 +231,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{