From 7c1387807567519e588c398ec94d74b3041fa1af Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 12 Mar 2025 17:34:39 -0400 Subject: [PATCH 01/11] fix(subsonic): getRandomSongs with `genre` filter fix https://github.com/dweymouth/supersonic/issues/577 Signed-off-by: Deluan --- persistence/mediafile_repository_test.go | 12 ------------ server/subsonic/filter/filters.go | 16 ++++++++++------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 3b64d89fe..41b48c0c6 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -4,7 +4,6 @@ import ( "context" "time" - "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" @@ -53,17 +52,6 @@ var _ = Describe("MediaRepository", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) - XIt("filters by genre", func() { - Expect(mr.GetAll(model.QueryOptions{ - Sort: "genre.name asc, title asc", - Filters: squirrel.Eq{"genre.name": "Rock"}, - })).To(Equal(model.MediaFiles{ - songDayInALife, - songAntenna, - songComeTogether, - })) - }) - Context("Annotations", func() { It("increments play count when the tracks does not have annotations", func() { id := "incplay.firsttime" diff --git a/server/subsonic/filter/filters.go b/server/subsonic/filter/filters.go index 1cac0b674..8a333c8e2 100644 --- a/server/subsonic/filter/filters.go +++ b/server/subsonic/filter/filters.go @@ -95,7 +95,7 @@ func SongsByRandom(genre string, fromYear, toYear int) Options { } ff := And{} if genre != "" { - ff = append(ff, Eq{"genre.name": genre}) + ff = append(ff, filterByGenre(genre)) } if fromYear != 0 { ff = append(ff, GtOrEq{"year": fromYear}) @@ -118,11 +118,15 @@ func SongWithLyrics(artist, title string) Options { func ByGenre(genre string) Options { return addDefaultFilters(Options{ - Sort: "name asc", - Filters: persistence.Exists("json_tree(tags)", And{ - Like{"value": genre}, - NotEq{"atom": nil}, - }), + Sort: "name asc", + Filters: filterByGenre(genre), + }) +} + +func filterByGenre(genre string) Sqlizer { + return persistence.Exists("json_tree(tags)", And{ + Like{"value": genre}, + NotEq{"atom": nil}, }) } From 226be78bf538b2bd025d4ad5b683d6368683c695 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 12 Mar 2025 17:51:36 -0400 Subject: [PATCH 02/11] fix(scanner): full_text not being updated on scan Fixes #3813 Signed-off-by: Deluan --- scanner/phase_1_folders.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 2894878d1..ca7851f17 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -334,7 +334,8 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) // Save all new/modified artists to DB. Their information will be incomplete, but they will be refreshed later for i := range entry.artists { - err = artistRepo.Put(&entry.artists[i], "name", "mbz_artist_id", "sort_artist_name", "order_artist_name") + err = artistRepo.Put(&entry.artists[i], "name", + "mbz_artist_id", "sort_artist_name", "order_artist_name", "full_text") if err != nil { log.Error(p.ctx, "Scanner: Error persisting artist to DB", "folder", entry.path, "artist", entry.artists[i].Name, err) return err From 5fb1db60314ee800cb6ead9a9f7100da3c206661 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 12 Mar 2025 18:13:22 -0400 Subject: [PATCH 03/11] fix(scanner): watcher not working with relative MusicFolder Signed-off-by: Deluan --- scanner/watcher.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/scanner/watcher.go b/scanner/watcher.go index 3090966a7..bf4f7f9d0 100644 --- a/scanner/watcher.go +++ b/scanner/watcher.go @@ -101,22 +101,27 @@ func watchLib(ctx context.Context, lib model.Library, watchChan chan struct{}) { log.Error(ctx, "Watcher: Error watching library", "library", lib.ID, "path", lib.Path, err) return } - log.Info(ctx, "Watcher started", "library", lib.ID, "path", lib.Path) + absLibPath, err := filepath.Abs(lib.Path) + if err != nil { + log.Error(ctx, "Watcher: Error converting lib.Path to absolute", "library", lib.ID, "path", lib.Path, err) + return + } + log.Info(ctx, "Watcher started", "library", lib.ID, "libPath", lib.Path, "absoluteLibPath", absLibPath) for { select { case <-ctx.Done(): return case path := <-c: - path, err = filepath.Rel(lib.Path, path) + path, err = filepath.Rel(absLibPath, path) if err != nil { - log.Error(ctx, "Watcher: Error getting relative path", "library", lib.ID, "path", path, err) + log.Error(ctx, "Watcher: Error getting relative path", "library", lib.ID, "libPath", absLibPath, "path", path, err) continue } if isIgnoredPath(ctx, fsys, path) { log.Trace(ctx, "Watcher: Ignoring change", "library", lib.ID, "path", path) continue } - log.Trace(ctx, "Watcher: Detected change", "library", lib.ID, "path", path) + log.Trace(ctx, "Watcher: Detected change", "library", lib.ID, "path", path, "libPath", absLibPath) watchChan <- struct{}{} } } From 5c0b6fb9b7363582e351f90e594ef5e17f5e50d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 13 Mar 2025 07:10:45 -0400 Subject: [PATCH 04/11] fix(server): skip non-UTF encoding during the database migration. (#3803) Fix #3787 Signed-off-by: Deluan --- .../20241026183640_support_new_scanner.go | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/db/migrations/20241026183640_support_new_scanner.go b/db/migrations/20241026183640_support_new_scanner.go index bdf68c7cc..3e7c47f54 100644 --- a/db/migrations/20241026183640_support_new_scanner.go +++ b/db/migrations/20241026183640_support_new_scanner.go @@ -172,14 +172,20 @@ join library on media_file.library_id = library.id`, string(os.PathSeparator))) // Finally, walk the in-mem filesystem and insert all folders into the DB. err = fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { if err != nil { - return err + // Don't abort the walk, just log the error + log.Error("error walking folder to DB", "path", path, err) + return nil } - if d.IsDir() { - f := model.NewFolder(lib, path) - _, err = stmt.ExecContext(ctx, f.ID, lib.ID, f.Path, f.Name, f.ParentID) - if err != nil { - log.Error("error writing folder to DB", "path", path, err) - } + // Skip entries that are not directories + if !d.IsDir() { + return nil + } + + // Create a folder in the DB + f := model.NewFolder(lib, path) + _, err = stmt.ExecContext(ctx, f.ID, lib.ID, f.Path, f.Name, f.ParentID) + if err != nil { + log.Error("error writing folder to DB", "path", path, err) } return err }) From b952672877be6f927a0c0a44b84b3415e243fd13 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 13 Mar 2025 19:25:07 -0400 Subject: [PATCH 05/11] fix(scanner): add back the Scanner.GenreSeparators as a deprecated option This allows easy upgrade of containers in PikaPods Signed-off-by: Deluan --- conf/configuration.go | 7 +++++-- model/tag_mappings.go | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/conf/configuration.go b/conf/configuration.go index 2636fbf94..b4e38add2 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -128,7 +128,8 @@ type scannerOptions struct { WatcherWait time.Duration ScanOnStartup bool Extractor string - GroupAlbumReleases bool // Deprecated: BFR Update docs + GenreSeparators string // Deprecated: Use Tags.genre.Split instead + GroupAlbumReleases bool // Deprecated: Use PID.Album instead } type subsonicOptions struct { @@ -307,6 +308,7 @@ func Load(noConfigDump bool) { log.Warn(fmt.Sprintf("Extractor '%s' is not implemented, using 'taglib'", Server.Scanner.Extractor)) Server.Scanner.Extractor = consts.DefaultScannerExtractor } + logDeprecatedOptions("Scanner.GenreSeparators") logDeprecatedOptions("Scanner.GroupAlbumReleases") // Call init hooks @@ -489,9 +491,10 @@ func init() { viper.SetDefault("scanner.enabled", true) viper.SetDefault("scanner.schedule", "0") viper.SetDefault("scanner.extractor", consts.DefaultScannerExtractor) - viper.SetDefault("scanner.groupalbumreleases", false) viper.SetDefault("scanner.watcherwait", consts.DefaultWatcherWait) viper.SetDefault("scanner.scanonstartup", true) + viper.SetDefault("scanner.genreseparators", "") + viper.SetDefault("scanner.groupalbumreleases", false) viper.SetDefault("subsonic.appendsubtitle", true) viper.SetDefault("subsonic.artistparticipations", false) diff --git a/model/tag_mappings.go b/model/tag_mappings.go index b0cf85fae..14edd3b0e 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -175,6 +175,15 @@ func loadTagMappings() { log.Error("No tag mappings found in mappings.yaml, check the format") } + // Use Scanner.GenreSeparators if specified and Tags.genre is not defined + if conf.Server.Scanner.GenreSeparators != "" && len(conf.Server.Tags["genre"].Aliases) == 0 { + genreConf := _mappings.Main[TagName("genre")] + genreConf.Split = strings.Split(conf.Server.Scanner.GenreSeparators, "") + genreConf.SplitRx = compileSplitRegex("genre", genreConf.Split) + _mappings.Main[TagName("genre")] = genreConf + log.Debug("Loading deprecated list of genre separators", "separators", genreConf.Split) + } + // Overwrite the default mappings with the ones from the config for tag, cfg := range conf.Server.Tags { if len(cfg.Aliases) == 0 { From 2838ac36df72fefbe796e0fa88316ea6d7f106f9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 13 Mar 2025 19:47:34 -0400 Subject: [PATCH 06/11] feat(scanner): allow disabling tags with `Tags..Ignore=true` Signed-off-by: Deluan --- conf/configuration.go | 1 + model/tag_mappings.go | 29 +++++++++++++++++++++-------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/conf/configuration.go b/conf/configuration.go index b4e38add2..e1e6b2f67 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -140,6 +140,7 @@ type subsonicOptions struct { } type TagConf struct { + Ignore bool `yaml:"ignore"` Aliases []string `yaml:"aliases"` Type string `yaml:"type"` MaxLength int `yaml:"maxLength"` diff --git a/model/tag_mappings.go b/model/tag_mappings.go index 14edd3b0e..d8caa0c5d 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -1,6 +1,7 @@ package model import ( + "cmp" "maps" "regexp" "slices" @@ -186,19 +187,31 @@ func loadTagMappings() { // Overwrite the default mappings with the ones from the config for tag, cfg := range conf.Server.Tags { - if len(cfg.Aliases) == 0 { + if cfg.Ignore { delete(_mappings.Main, TagName(tag)) delete(_mappings.Additional, TagName(tag)) continue } - c := TagConf{ - Aliases: cfg.Aliases, - Type: TagType(cfg.Type), - MaxLength: cfg.MaxLength, - Split: cfg.Split, - Album: cfg.Album, - SplitRx: compileSplitRegex(TagName(tag), cfg.Split), + oldValue, ok := _mappings.Main[TagName(tag)] + if !ok { + oldValue = _mappings.Additional[TagName(tag)] } + aliases := cfg.Aliases + if len(aliases) == 0 { + aliases = oldValue.Aliases + } + split := cfg.Split + if len(split) == 0 { + split = oldValue.Split + } + c := TagConf{ + Aliases: aliases, + Split: split, + Type: cmp.Or(TagType(cfg.Type), oldValue.Type), + MaxLength: cmp.Or(cfg.MaxLength, oldValue.MaxLength), + Album: cmp.Or(cfg.Album, oldValue.Album), + } + c.SplitRx = compileSplitRegex(TagName(tag), c.Split) if _, ok := _mappings.Main[TagName(tag)]; ok { _mappings.Main[TagName(tag)] = c } else { From 938c3d44ccb96c2f0f1751d2b07f8ae29c4f061c Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Fri, 14 Mar 2025 11:01:07 +0000 Subject: [PATCH 07/11] fix(scanner): restore setsubtitle as discsubtitle for non-WMA (#3821) With old metadata, Disc Subtitle was one of `tsst`, `discsubtitle`, or `setsubtitle`. With the updated, `setsubtitle` is only available for flac. Update `mappings.yaml` to maintain prior behavior. --- resources/mappings.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/mappings.yaml b/resources/mappings.yaml index a42ceab47..45810e140 100644 --- a/resources/mappings.yaml +++ b/resources/mappings.yaml @@ -96,7 +96,7 @@ main: aliases: [ disctotal, totaldiscs ] album: true discsubtitle: - aliases: [ tsst, discsubtitle, ----:com.apple.itunes:discsubtitle, wm/setsubtitle ] + aliases: [ tsst, discsubtitle, ----:com.apple.itunes:discsubtitle, setsubtitle, wm/setsubtitle ] bpm: aliases: [ tbpm, bpm, tmpo, wm/beatsperminute ] lyrics: From 422ba2284e4baa45ed939505b12fcd378e6e339a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Mar 2025 17:44:00 -0400 Subject: [PATCH 08/11] chore(scanner): add logs to .ndignore processing Signed-off-by: Deluan --- scanner/walk_dir_tree.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 1b7bb36f1..323ba0392 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -145,7 +145,10 @@ func loadIgnoredPatterns(ctx context.Context, fsys fs.FS, currentFolder string, } // If the .ndignore file is empty, mimic the current behavior and ignore everything if len(newPatterns) == 0 { + log.Trace(ctx, "Scanner: .ndignore file is empty, ignoring everything", "path", currentFolder) newPatterns = []string{"**/*"} + } else { + log.Trace(ctx, "Scanner: .ndignore file found ", "path", ignoreFilePath, "patterns", newPatterns) } } // Combine the patterns from the .ndignore file with the ones passed as argument @@ -180,7 +183,7 @@ func loadDir(ctx context.Context, job *scanJob, dirPath string, ignorePatterns [ children = make([]string, 0, len(entries)) for _, entry := range entries { entryPath := path.Join(dirPath, entry.Name()) - if len(ignorePatterns) > 0 && isScanIgnored(ignoreMatcher, entryPath) { + if len(ignorePatterns) > 0 && isScanIgnored(ctx, ignoreMatcher, entryPath) { log.Trace(ctx, "Scanner: Ignoring entry", "path", entryPath) continue } @@ -309,6 +312,10 @@ func isEntryIgnored(name string) bool { return strings.HasPrefix(name, ".") && !strings.HasPrefix(name, "..") } -func isScanIgnored(matcher *ignore.GitIgnore, entryPath string) bool { - return matcher.MatchesPath(entryPath) +func isScanIgnored(ctx context.Context, matcher *ignore.GitIgnore, entryPath string) bool { + matches := matcher.MatchesPath(entryPath) + if matches { + log.Trace(ctx, "Scanner: Ignoring entry matching .ndignore: ", "path", entryPath) + } + return matches } From 98808e4b6dd7c2f6af82cf1dbeeb847f67c47538 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Mar 2025 19:32:26 -0400 Subject: [PATCH 09/11] docs(scanner): clarifies the purpose of the mappings.yaml file for regular users Signed-off-by: Deluan --- resources/mappings.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/resources/mappings.yaml b/resources/mappings.yaml index 45810e140..f4de96a74 100644 --- a/resources/mappings.yaml +++ b/resources/mappings.yaml @@ -1,6 +1,14 @@ #file: noinspection SpellCheckingInspection # Tag mapping adapted from https://picard-docs.musicbrainz.org/downloads/MusicBrainz_Picard_Tag_Map.html # +# NOTE FOR USERS: +# +# This file can be used as a reference to understand how Navidrome maps the tags in your music files to its fields. +# If you want to customize these mappings, please refer to https://www.navidrome.org/docs/usage/customtags/ +# +# +# NOTE FOR DEVELOPERS: +# # This file contains the mapping between the tags in your music files and the fields in Navidrome. # You can add new tags, change the aliases, or add new split characters to the existing tags. # The artists and roles keys are used to define how to split the tag values into multiple values. From ed1109ddb232184d69e655b72a2a44f9c1033e5b Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Sat, 15 Mar 2025 01:21:03 +0000 Subject: [PATCH 10/11] fix(subsonic): fix albumCount in artists (#3827) * only do subsonic instead * make sure to actually populate response first * navidrome artist filtering * address discord feedback * perPage min 36 * various artist artist_id -> albumartist_id * artist_id, role_id separate * remove all ui changes I guess * Revert role filters Signed-off-by: Deluan --------- Signed-off-by: Deluan Co-authored-by: Deluan --- server/subsonic/browsing.go | 50 +++++++++++-------- server/subsonic/helpers.go | 19 ++++++- server/subsonic/helpers_test.go | 29 +++++++++++ ...onses Indexes with data should match .JSON | 1 - ...ponses Indexes with data should match .XML | 2 +- server/subsonic/responses/responses.go | 3 +- server/subsonic/responses/responses_test.go | 1 - server/subsonic/searching.go | 1 - 8 files changed, 78 insertions(+), 28 deletions(-) diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index df4083aef..82bf50dc5 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -270,30 +270,43 @@ func (api *Router) GetGenres(r *http.Request) (*responses.Subsonic, error) { return response, nil } -func (api *Router) GetArtistInfo(r *http.Request) (*responses.Subsonic, error) { +func (api *Router) getArtistInfo(r *http.Request) (*responses.ArtistInfoBase, *model.Artists, error) { ctx := r.Context() p := req.Params(r) id, err := p.String("id") if err != nil { - return nil, err + return nil, nil, err } count := p.IntOr("count", 20) includeNotPresent := p.BoolOr("includeNotPresent", false) artist, err := api.externalMetadata.UpdateArtistInfo(ctx, id, count, includeNotPresent) + if err != nil { + return nil, nil, err + } + + base := responses.ArtistInfoBase{} + base.Biography = artist.Biography + base.SmallImageUrl = public.ImageURL(r, artist.CoverArtID(), 300) + base.MediumImageUrl = public.ImageURL(r, artist.CoverArtID(), 600) + base.LargeImageUrl = public.ImageURL(r, artist.CoverArtID(), 1200) + base.LastFmUrl = artist.ExternalUrl + base.MusicBrainzID = artist.MbzArtistID + + return &base, &artist.SimilarArtists, nil +} + +func (api *Router) GetArtistInfo(r *http.Request) (*responses.Subsonic, error) { + base, similarArtists, err := api.getArtistInfo(r) if err != nil { return nil, err } response := newResponse() response.ArtistInfo = &responses.ArtistInfo{} - response.ArtistInfo.Biography = artist.Biography - response.ArtistInfo.SmallImageUrl = public.ImageURL(r, artist.CoverArtID(), 300) - response.ArtistInfo.MediumImageUrl = public.ImageURL(r, artist.CoverArtID(), 600) - response.ArtistInfo.LargeImageUrl = public.ImageURL(r, artist.CoverArtID(), 1200) - response.ArtistInfo.LastFmUrl = artist.ExternalUrl - response.ArtistInfo.MusicBrainzID = artist.MbzArtistID - for _, s := range artist.SimilarArtists { + response.ArtistInfo.ArtistInfoBase = *base + + for _, s := range *similarArtists { similar := toArtist(r, s) if s.ID == "" { similar.Id = "-1" @@ -304,23 +317,20 @@ func (api *Router) GetArtistInfo(r *http.Request) (*responses.Subsonic, error) { } func (api *Router) GetArtistInfo2(r *http.Request) (*responses.Subsonic, error) { - info, err := api.GetArtistInfo(r) + base, similarArtists, err := api.getArtistInfo(r) if err != nil { return nil, err } response := newResponse() response.ArtistInfo2 = &responses.ArtistInfo2{} - response.ArtistInfo2.ArtistInfoBase = info.ArtistInfo.ArtistInfoBase - for _, s := range info.ArtistInfo.SimilarArtist { - similar := responses.ArtistID3{} - similar.Id = s.Id - similar.Name = s.Name - similar.AlbumCount = s.AlbumCount - similar.Starred = s.Starred - similar.UserRating = s.UserRating - similar.CoverArt = s.CoverArt - similar.ArtistImageUrl = s.ArtistImageUrl + response.ArtistInfo2.ArtistInfoBase = *base + + for _, s := range *similarArtists { + similar := toArtistID3(r, s) + if s.ID == "" { + similar.Id = "-1" + } response.ArtistInfo2.SimilarArtist = append(response.ArtistInfo2.SimilarArtist, similar) } return response, nil diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 880bb6ddf..0fb35a7b7 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -77,11 +77,26 @@ func sortName(sortName, orderName string) string { return orderName } +func getArtistAlbumCount(a model.Artist) int32 { + albumStats := a.Stats[model.RoleAlbumArtist] + + // If ArtistParticipations are set, then `getArtist` will return albums + // where the artist is an album artist OR artist. While it may be an underestimate, + // guess the count by taking a max of the album artist and artist count. This is + // guaranteed to be <= the actual count. + // Otherwise, return just the roles as album artist (precise) + if conf.Server.Subsonic.ArtistParticipations { + artistStats := a.Stats[model.RoleArtist] + return int32(max(artistStats.AlbumCount, albumStats.AlbumCount)) + } else { + return int32(albumStats.AlbumCount) + } +} + func toArtist(r *http.Request, a model.Artist) responses.Artist { artist := responses.Artist{ Id: a.ID, Name: a.Name, - AlbumCount: int32(a.AlbumCount), UserRating: int32(a.Rating), CoverArt: a.CoverArtID().String(), ArtistImageUrl: public.ImageURL(r, a.CoverArtID(), 600), @@ -96,7 +111,7 @@ func toArtistID3(r *http.Request, a model.Artist) responses.ArtistID3 { artist := responses.ArtistID3{ Id: a.ID, Name: a.Name, - AlbumCount: int32(a.AlbumCount), + AlbumCount: getArtistAlbumCount(a), CoverArt: a.CoverArtID().String(), ArtistImageUrl: public.ImageURL(r, a.CoverArtID(), 600), UserRating: int32(a.Rating), diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index dd8bd88b1..d703607ba 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -10,6 +10,10 @@ import ( ) var _ = Describe("helpers", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + Describe("fakePath", func() { var mf model.MediaFile BeforeEach(func() { @@ -134,4 +138,29 @@ var _ = Describe("helpers", func() { Entry("returns \"explicit\" when the db value is \"e\"", "e", "explicit"), Entry("returns an empty string when the db value is \"\"", "", ""), Entry("returns an empty string when there are unexpected values on the db", "abc", "")) + + Describe("getArtistAlbumCount", func() { + artist := model.Artist{ + Stats: map[model.Role]model.ArtistStats{ + model.RoleAlbumArtist: { + AlbumCount: 3, + }, + model.RoleArtist: { + AlbumCount: 4, + }, + }, + } + + It("Handles album count without artist participations", func() { + conf.Server.Subsonic.ArtistParticipations = false + result := getArtistAlbumCount(artist) + Expect(result).To(Equal(int32(3))) + }) + + It("Handles album count without with participations", func() { + conf.Server.Subsonic.ArtistParticipations = true + result := getArtistAlbumCount(artist) + Expect(result).To(Equal(int32(4))) + }) + }) }) diff --git a/server/subsonic/responses/.snapshots/Responses Indexes with data should match .JSON b/server/subsonic/responses/.snapshots/Responses Indexes with data should match .JSON index 585815fba..9f835da1a 100644 --- a/server/subsonic/responses/.snapshots/Responses Indexes with data should match .JSON +++ b/server/subsonic/responses/.snapshots/Responses Indexes with data should match .JSON @@ -12,7 +12,6 @@ { "id": "111", "name": "aaa", - "albumCount": 2, "starred": "2016-03-02T20:30:00Z", "userRating": 3, "artistImageUrl": "https://lastfm.freetls.fastly.net/i/u/300x300/2a96cbd8b46e442fc41c2b86b821562f.png" diff --git a/server/subsonic/responses/.snapshots/Responses Indexes with data should match .XML b/server/subsonic/responses/.snapshots/Responses Indexes with data should match .XML index 86495a75f..595f2ff03 100644 --- a/server/subsonic/responses/.snapshots/Responses Indexes with data should match .XML +++ b/server/subsonic/responses/.snapshots/Responses Indexes with data should match .XML @@ -1,7 +1,7 @@ - + diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index f329fae4b..c0f499ef2 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -92,7 +92,6 @@ type MusicFolders struct { type Artist struct { Id string `xml:"id,attr" json:"id"` Name string `xml:"name,attr" json:"name"` - AlbumCount int32 `xml:"albumCount,attr,omitempty" json:"albumCount,omitempty"` Starred *time.Time `xml:"starred,attr,omitempty" json:"starred,omitempty"` UserRating int32 `xml:"userRating,attr,omitempty" json:"userRating,omitempty"` CoverArt string `xml:"coverArt,attr,omitempty" json:"coverArt,omitempty"` @@ -233,7 +232,7 @@ type ArtistID3 struct { Id string `xml:"id,attr" json:"id"` Name string `xml:"name,attr" json:"name"` CoverArt string `xml:"coverArt,attr,omitempty" json:"coverArt,omitempty"` - AlbumCount int32 `xml:"albumCount,attr,omitempty" json:"albumCount,omitempty"` + AlbumCount int32 `xml:"albumCount,attr" json:"albumCount"` Starred *time.Time `xml:"starred,attr,omitempty" json:"starred,omitempty"` UserRating int32 `xml:"userRating,attr,omitempty" json:"userRating,omitempty"` ArtistImageUrl string `xml:"artistImageUrl,attr,omitempty" json:"artistImageUrl,omitempty"` diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index fed454195..f3796f10a 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -103,7 +103,6 @@ var _ = Describe("Responses", func() { Name: "aaa", Starred: &t, UserRating: 3, - AlbumCount: 2, ArtistImageUrl: "https://lastfm.freetls.fastly.net/i/u/300x300/2a96cbd8b46e442fc41c2b86b821562f.png", } index := make([]Index, 1) diff --git a/server/subsonic/searching.go b/server/subsonic/searching.go index 235ebc13f..f66846f35 100644 --- a/server/subsonic/searching.go +++ b/server/subsonic/searching.go @@ -94,7 +94,6 @@ func (api *Router) Search2(r *http.Request) (*responses.Subsonic, error) { a := responses.Artist{ Id: artist.ID, Name: artist.Name, - AlbumCount: int32(artist.AlbumCount), UserRating: int32(artist.Rating), CoverArt: artist.CoverArtID().String(), ArtistImageUrl: public.ImageURL(r, artist.CoverArtID(), 600), From beb768cd9cd00f01581fe190a345ccf8617950db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 14 Mar 2025 21:43:52 -0400 Subject: [PATCH 11/11] feat(server): add Role filters to albums (#3829) * navidrome artist filtering * address discord feedback * perPage min 36 * various artist artist_id -> albumartist_id * artist_id, role_id separate * remove all ui changes I guess * Add tests, check for possible SQL injection Signed-off-by: Deluan --------- Signed-off-by: Deluan Co-authored-by: Kendall Garner <17521368+kgarner7@users.noreply.github.com> --- persistence/album_repository.go | 28 ++++++++++++++--- persistence/album_repository_test.go | 47 ++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index f98375f21..b29a44701 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "slices" + "strings" "sync" "time" @@ -119,11 +120,17 @@ var albumFilters = sync.OnceValue(func() map[string]filterFunc { "has_rating": hasRatingFilter, "missing": booleanFilter, "genre_id": tagIDFilter, + "role_total_id": allRolesFilter, } // Add all album tags as filters for tag := range model.AlbumLevelTags() { filters[string(tag)] = tagIDFilter } + + for role := range model.AllRoles { + filters["role_"+role+"_id"] = artistRoleFilter + } + return filters }) @@ -153,14 +160,25 @@ func yearFilter(_ string, value interface{}) Sqlizer { } } -// BFR: Support other roles func artistFilter(_ string, value interface{}) Sqlizer { return Or{ - Exists("json_tree(Participants, '$.albumartist')", Eq{"value": value}), - Exists("json_tree(Participants, '$.artist')", Eq{"value": value}), + Exists("json_tree(participants, '$.albumartist')", Eq{"value": value}), + Exists("json_tree(participants, '$.artist')", Eq{"value": value}), } - // For any role: - //return Like{"Participants": fmt.Sprintf(`%%"%s"%%`, value)} +} + +func artistRoleFilter(name string, value interface{}) Sqlizer { + roleName := strings.TrimSuffix(strings.TrimPrefix(name, "role_"), "_id") + + // Check if the role name is valid. If not, return an invalid filter + if _, ok := model.AllRoles[roleName]; !ok { + return Gt{"": nil} + } + return Exists(fmt.Sprintf("json_tree(participants, '$.%s')", roleName), Eq{"value": value}) +} + +func allRolesFilter(_ string, value interface{}) Sqlizer { + return Like{"participants": fmt.Sprintf(`%%"%s"%%`, value)} } func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) { diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index dba347b30..529458c26 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "time" "github.com/navidrome/navidrome/conf" @@ -236,6 +237,52 @@ var _ = Describe("AlbumRepository", func() { } }) }) + + Describe("artistRoleFilter", func() { + DescribeTable("creates correct SQL expressions for artist roles", + func(filterName, artistID, expectedSQL string) { + sqlizer := artistRoleFilter(filterName, artistID) + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(Equal(expectedSQL)) + Expect(args).To(Equal([]interface{}{artistID})) + }, + Entry("artist role", "role_artist_id", "123", + "exists (select 1 from json_tree(participants, '$.artist') where value = ?)"), + Entry("albumartist role", "role_albumartist_id", "456", + "exists (select 1 from json_tree(participants, '$.albumartist') where value = ?)"), + Entry("composer role", "role_composer_id", "789", + "exists (select 1 from json_tree(participants, '$.composer') where value = ?)"), + ) + + It("works with the actual filter map", func() { + filters := albumFilters() + + for roleName := range model.AllRoles { + filterName := "role_" + roleName + "_id" + filterFunc, exists := filters[filterName] + Expect(exists).To(BeTrue(), fmt.Sprintf("Filter %s should exist", filterName)) + + sqlizer := filterFunc(filterName, "test-id") + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(Equal(fmt.Sprintf("exists (select 1 from json_tree(participants, '$.%s') where value = ?)", roleName))) + Expect(args).To(Equal([]interface{}{"test-id"})) + } + }) + + It("rejects invalid roles", func() { + sqlizer := artistRoleFilter("role_invalid_id", "123") + _, _, err := sqlizer.ToSql() + Expect(err).To(HaveOccurred()) + }) + + It("rejects invalid filter names", func() { + sqlizer := artistRoleFilter("invalid_name", "123") + _, _, err := sqlizer.ToSql() + Expect(err).To(HaveOccurred()) + }) + }) }) func _p(id, name string, sortName ...string) model.Participant {