fix(archiver): give same-named albums their own folder in artist zips (#6225)

* refactor: add Tags.First and slice.GroupOrdered helpers

Tags.First returns the first value of a tag or an empty string, replacing the inline len-check-then-index pattern in FullTitle, FullAlbumName, Album.FullName and the Subsonic album version mapping.

slice.GroupOrdered is slice.Group returning the groups in first-seen order, for callers that need a deterministic order the map-based Group cannot give.

* fix(archiver): give same-named albums their own folder in artist zips

Artist zips put every album in a folder named after the album, so two albums with the same name (an original and a deluxe edition, or names that only differ in characters the sanitizer replaces) were merged into one folder, with tracks mixed together and duplicate zip entries when file names collided.

The folder is now named after FullAlbumName(), so with AppendAlbumVersion on (the default) the version is part of the name, matching what clients display. Albums whose sanitized names still clash get a " [suffix]" taken from the first field that has a distinct, non-empty value for all of them: album version, year, release type, record label, catalog number, then a short album id. This follows the shape of beets' %aunique{} path function.

Albums are also grouped with slice.GroupOrdered instead of a map, so the zip is deterministic.

* fix(archiver): use the release year to tell same-named albums apart

Taggers often write an edition's date to the Date tag next to an original date, and the scanner then stores the original year in Year and the edition's year in ReleaseYear. Reissues of the same album therefore share Year, so the year disambiguator could not tell them apart and they fell through to the album id suffix. Prefer ReleaseYear and fall back to Year when it is not set.

Found by downloading an artist zip from a live server built from this branch.

* fix(archiver): let one clashing album keep the plain folder name

A disambiguator was only accepted when every clashing album had a non-empty value, so an original and its deluxe edition (with the version not appended to the name) fell through to the album id suffix. Accept a field whose values are distinct across the group even when one of them is empty, as beets' %aunique{} does: that album keeps the plain name, which the suffixed folders cannot clash with. Two or more empty values still count as a tie.

* test(archiver): refactor tests for album naming conventions and query order

core/archiver.go

@@ -2,12 +2,14 @@ package core
 
 import (
 	"archive/zip"
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
 	"io"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	"github.com/Masterminds/squirrel"
@@ -58,16 +60,16 @@ func (a *archiver) zipAlbums(ctx context.Context, id string, format string, bitr
 	}
 
 	z := createZipWriter(out, format, bitrate)
-	albums := slice.Group(mfs, func(mf model.MediaFile) string {
-		return mf.AlbumID
-	})
+	albums := slice.GroupOrdered(mfs, func(mf model.MediaFile) string { return mf.AlbumID })
+	folders := albumFolders(albums)
 	for _, album := range albums {
 		discs := slice.Group(album, func(mf model.MediaFile) int { return mf.DiscNumber })
 		isMultiDisc := len(discs) > 1
-		log.Debug(ctx, "Zipping album", "name", album[0].Album, "artist", album[0].AlbumArtist,
+		folder := folders[album[0].AlbumID]
+		log.Debug(ctx, "Zipping album", "name", album[0].Album, "artist", album[0].AlbumArtist, "folder", folder,
 			"format", format, "bitrate", bitrate, "isMultiDisc", isMultiDisc, "numTracks", len(album))
 		for _, mf := range album {
-			file := a.albumFilename(mf, format, isMultiDisc)
+			file := a.albumFilename(mf, format, isMultiDisc, folder)
 			if addErr := a.addFileToZip(ctx, z, mf, format, bitrate, file); errors.Is(addErr, stream.ErrTooManyTranscodes) {
 				// Stop iterating: continuing would just rack up more
 				// rejections from the limiter. Close finalises whatever
@@ -96,7 +98,61 @@ func createZipWriter(out io.Writer, format string, bitrate int) *zip.Writer {
 	return z
 }
 
-func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc bool) string {
+// Tried in order; the first one whose values are distinct across the clashing albums wins.
+// One album may have an empty value: it keeps the plain name, which the others can't clash with.
+var albumDisambiguators = []func(model.MediaFile) string{
+	func(mf model.MediaFile) string { return mf.Tags.First(model.TagAlbumVersion) },
+	func(mf model.MediaFile) string {
+		// Reissues share Year (often the original's) but not ReleaseYear.
+		if y := cmp.Or(mf.ReleaseYear, mf.Year); y != 0 {
+			return strconv.Itoa(y)
+		}
+		return ""
+	},
+	func(mf model.MediaFile) string { return mf.MbzAlbumType },
+	func(mf model.MediaFile) string { return mf.Tags.First(model.TagRecordLabel) },
+	func(mf model.MediaFile) string { return mf.CatalogNum },
+	func(mf model.MediaFile) string { return mf.AlbumID[:min(6, len(mf.AlbumID))] },
+	func(mf model.MediaFile) string { return mf.AlbumID },
+}
+
+// albumFolders maps each album id to its zip folder. Albums whose names sanitize to the
+// same folder get a " [suffix]" from the first disambiguator that tells them all apart.
+func albumFolders(albums [][]model.MediaFile) map[string]string {
+	byName := map[string][]model.MediaFile{}
+	for _, album := range albums {
+		name := str.SanitizeFilename(album[0].FullAlbumName())
+		byName[name] = append(byName[name], album[0])
+	}
+	folders := make(map[string]string, len(albums))
+	for name, group := range byName {
+		if len(group) == 1 {
+			folders[group[0].AlbumID] = name
+			continue
+		}
+	fields:
+		for _, field := range albumDisambiguators {
+			ids := make(map[string]string, len(group)) // suffix -> album id
+			for _, mf := range group {
+				s := str.SanitizeFilename(field(mf))
+				if _, dup := ids[s]; dup {
+					continue fields
+				}
+				ids[s] = mf.AlbumID
+			}
+			for s, id := range ids {
+				folders[id] = name
+				if s != "" {
+					folders[id] = fmt.Sprintf("%s [%s]", name, s)
+				}
+			}
+			break
+		}
+	}
+	return folders
+}
+
+func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc bool, folder string) string {
 	_, file := filepath.Split(mf.Path)
 	if format != "raw" {
 		file = strings.TrimSuffix(file, mf.Suffix) + format
@@ -104,7 +160,7 @@ func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc
 	if isMultiDisc {
 		file = fmt.Sprintf("Disc %02d/%s", mf.DiscNumber, file)
 	}
-	return fmt.Sprintf("%s/%s", str.SanitizeFilename(mf.Album), file)
+	return fmt.Sprintf("%s/%s", folder, file)
 }
 
 // ZipShare takes an already-loaded share: Share.Load records a visit, so

core/archiver_test.go

@@ -8,6 +8,8 @@ import (
 	"strings"
 
 	"github.com/Masterminds/squirrel"
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/core"
 	"github.com/navidrome/navidrome/core/stream"
 	"github.com/navidrome/navidrome/model"
@@ -91,6 +93,140 @@ var _ = Describe("Archiver", func() {
 			Expect(zr.File[0].Name).To(Equal("Album 1/01 - track1.mp3"))
 			Expect(zr.File[1].Name).To(Equal("Album 1/02 - track2.mp3"))
 		})
+
+		When("albums that share a name", func() {
+			BeforeEach(func() {
+				DeferCleanup(configtest.SetupConfig())
+			})
+
+			// zipArtistEntries zips the given tracks as artist "1" and returns the entry names in zip order.
+			zipArtistEntries := func(mfs model.MediaFiles) []string {
+				mfRepo := &mockMediaFileRepository{}
+				mfRepo.On("GetAll", mock.Anything).Return(mfs, nil)
+				ds.On("MediaFile", mock.Anything).Return(mfRepo)
+				ms.On("NewStream", mock.Anything, mock.Anything, mock.Anything).Return(io.NopCloser(strings.NewReader("test")), nil)
+
+				out := new(bytes.Buffer)
+				Expect(arch.ZipArtist(context.Background(), "1", "mp3", 128, out)).To(Succeed())
+				zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len()))
+				Expect(err).To(BeNil())
+				names := make([]string, len(zr.File))
+				for i, f := range zr.File {
+					names[i] = f.Name
+				}
+				return names
+			}
+
+			It("keeps the albums in query order", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "3", Album: "Album C"},
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Album A"},
+					{Path: "a/02.mp3", Suffix: "mp3", AlbumID: "1", Album: "Album A"},
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Album B"},
+				})
+				Expect(names).To(Equal([]string{"Album C/01.mp3", "Album A/01.mp3", "Album A/02.mp3", "Album B/01.mp3"}))
+			})
+
+			It("suffixes the year when it tells the albums apart", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01 - Intro.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001},
+					{Path: "b/01 - Intro.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 2005},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits [2001]/01 - Intro.mp3", "Greatest Hits [2005]/01 - Intro.mp3"}))
+			})
+
+			It("prefers the release year, so reissues of the same original are told apart", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 1996, ReleaseYear: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 1996, ReleaseYear: 2011},
+					{Path: "c/01.mp3", Suffix: "mp3", AlbumID: "3", Album: "Greatest Hits", Year: 1996},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits [2001]/01.mp3", "Greatest Hits [2011]/01.mp3", "Greatest Hits [1996]/01.mp3"}))
+			})
+
+			It("names the folder after the full album name", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001,
+						Tags: model.Tags{model.TagAlbumVersion: {"Original"}}},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 2005,
+						Tags: model.Tags{model.TagAlbumVersion: {"CD/Digital"}}},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits (Original)/01.mp3", "Greatest Hits (CD_Digital)/01.mp3"}))
+			})
+
+			It("prefers the album version over the year when it is not part of the name", func() {
+				conf.Server.Subsonic.AppendAlbumVersion = false
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001,
+						Tags: model.Tags{model.TagAlbumVersion: {"Original"}}},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 2005,
+						Tags: model.Tags{model.TagAlbumVersion: {"Deluxe Edition"}}},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits [Original]/01.mp3", "Greatest Hits [Deluxe Edition]/01.mp3"}))
+			})
+
+			It("leaves the one album without the field unsuffixed", func() {
+				conf.Server.Subsonic.AppendAlbumVersion = false
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 2005,
+						Tags: model.Tags{model.TagAlbumVersion: {"Deluxe Edition"}}},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits/01.mp3", "Greatest Hits [Deluxe Edition]/01.mp3"}))
+			})
+
+			It("skips a field that is empty on more than one album", func() {
+				conf.Server.Subsonic.AppendAlbumVersion = false
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Greatest Hits", Year: 2005},
+					{Path: "c/01.mp3", Suffix: "mp3", AlbumID: "3", Album: "Greatest Hits", Year: 2010,
+						Tags: model.Tags{model.TagAlbumVersion: {"Deluxe Edition"}}},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits [2001]/01.mp3", "Greatest Hits [2005]/01.mp3", "Greatest Hits [2010]/01.mp3"}))
+			})
+
+			It("skips a field that is the same on every album", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Live", Year: 2001, MbzAlbumType: "album", CatalogNum: "CAT-1"},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Live", Year: 2001, MbzAlbumType: "album", CatalogNum: "CAT-2"},
+				})
+				Expect(names).To(Equal([]string{"Live [CAT-1]/01.mp3", "Live [CAT-2]/01.mp3"}))
+			})
+
+			It("falls back to the album id when nothing differs", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "0123456789abcdef", Album: "Greatest Hits", Year: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "fedcba9876543210", Album: "Greatest Hits", Year: 2001},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits [012345]/01.mp3", "Greatest Hits [fedcba]/01.mp3"}))
+			})
+
+			It("treats names that sanitize to the same folder as a clash", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "A/B", Year: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: `A\B`, Year: 2005},
+				})
+				Expect(names).To(Equal([]string{"A_B [2001]/01.mp3", "A_B [2005]/01.mp3"}))
+			})
+
+			It("sanitizes the suffix", func() {
+				conf.Server.Subsonic.AppendAlbumVersion = false
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Hits", Tags: model.Tags{model.TagAlbumVersion: {"Vinyl"}}},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Hits", Tags: model.Tags{model.TagAlbumVersion: {"CD/Digital"}}},
+				})
+				Expect(names).To(Equal([]string{"Hits [Vinyl]/01.mp3", "Hits [CD_Digital]/01.mp3"}))
+			})
+
+			It("leaves the folder name alone when only one album has it", func() {
+				names := zipArtistEntries(model.MediaFiles{
+					{Path: "a/01.mp3", Suffix: "mp3", AlbumID: "1", Album: "Greatest Hits", Year: 2001},
+					{Path: "b/01.mp3", Suffix: "mp3", AlbumID: "2", Album: "Other", Year: 2005},
+				})
+				Expect(names).To(Equal([]string{"Greatest Hits/01.mp3", "Other/01.mp3"}))
+			})
+		})
 	})
 
 	Context("when the transcode limiter rejects a file", func() {

model/album.go

@@ -76,8 +76,8 @@ func (a Album) CoverArtID() ArtworkID {
 }
 
 func (a Album) FullName() string {
-	if conf.Server.Subsonic.AppendAlbumVersion && len(a.Tags[TagAlbumVersion]) > 0 {
-		return appendSuffix(a.Name, a.Tags[TagAlbumVersion][0])
+	if v := a.Tags.First(TagAlbumVersion); conf.Server.Subsonic.AppendAlbumVersion && v != "" {
+		return appendSuffix(a.Name, v)
 	}
 	return a.Name
 }

model/mediafile.go

@@ -105,15 +105,15 @@ type MediaFile struct {
 }
 
 func (mf MediaFile) FullTitle() string {
-	if conf.Server.Subsonic.AppendSubtitle && len(mf.Tags[TagSubtitle]) > 0 {
-		return appendSuffix(mf.Title, mf.Tags[TagSubtitle][0])
+	if s := mf.Tags.First(TagSubtitle); conf.Server.Subsonic.AppendSubtitle && s != "" {
+		return appendSuffix(mf.Title, s)
 	}
 	return mf.Title
 }
 
 func (mf MediaFile) FullAlbumName() string {
-	if conf.Server.Subsonic.AppendAlbumVersion && len(mf.Tags[TagAlbumVersion]) > 0 {
-		return appendSuffix(mf.Album, mf.Tags[TagAlbumVersion][0])
+	if v := mf.Tags.First(TagAlbumVersion); conf.Server.Subsonic.AppendAlbumVersion && v != "" {
+		return appendSuffix(mf.Album, v)
 	}
 	return mf.Album
 }

model/tag.go

@@ -80,6 +80,13 @@ func (t Tags) Values(name TagName) []string {
 	return t[name]
 }
 
+func (t Tags) First(name TagName) string {
+	if v := t[name]; len(v) > 0 {
+		return v[0]
+	}
+	return ""
+}
+
 func (t Tags) IDs() []string {
 	var ids []string
 	for name, tag := range t {

model/tag_test.go

@@ -43,6 +43,10 @@ var _ = Describe("Tag", func() {
 			Expect(tags.Values("genre")).To(ConsistOf("Rock", "Pop"))
 			Expect(tags.Values("artist")).To(ConsistOf("The Beatles"))
 		})
+		It("should get the first value by name", func() {
+			Expect(tags.First("genre")).To(Equal("Rock"))
+			Expect(tags.First("missing")).To(BeEmpty())
+		})
 
 		Describe("Hash", func() {
 			It("should always return the same value for the same tags ", func() {

server/subsonic/helpers.go

@@ -515,9 +515,7 @@ func buildOSAlbumID3(ctx context.Context, album model.Album) *responses.OpenSubs
 	dir.IsCompilation = album.Compilation
 	dir.DiscTitles = buildDiscSubtitles(album)
 	dir.ExplicitStatus = mapExplicitStatus(album.ExplicitStatus)
-	if len(album.Tags.Values(model.TagAlbumVersion)) > 0 {
-		dir.Version = album.Tags.Values(model.TagAlbumVersion)[0]
-	}
+	dir.Version = album.Tags.First(model.TagAlbumVersion)
 
 	return &dir
 }

utils/slice/slice.go

@@ -33,6 +33,23 @@ func Group[T any, K comparable](s []T, keyFunc func(T) K) map[K][]T {
 	return m
 }
 
+// GroupOrdered is Group with the groups in first-seen order.
+func GroupOrdered[T any, K comparable](s []T, keyFunc func(T) K) [][]T {
+	var groups [][]T
+	index := map[K]int{}
+	for _, item := range s {
+		k := keyFunc(item)
+		i, ok := index[k]
+		if !ok {
+			i = len(groups)
+			index[k] = i
+			groups = append(groups, nil)
+		}
+		groups[i] = append(groups[i], item)
+	}
+	return groups
+}
+
 func ToMap[T any, K comparable, V any](s []T, transformFunc func(T) (K, V)) map[K]V {
 	m := make(map[K]V, len(s))
 	for _, item := range s {

utils/slice/slice_test.go

@@ -63,6 +63,18 @@ var _ = Describe("Slice Utils", func() {
 		})
 	})
 
+	Describe("GroupOrdered", func() {
+		It("returns nil for an empty input", func() {
+			Expect(slice.GroupOrdered([]int{}, func(v int) int { return v })).To(BeNil())
+		})
+
+		It("keeps groups in first-seen order and items in input order", func() {
+			keyFunc := func(v int) int { return v % 3 }
+			result := slice.GroupOrdered([]int{2, 1, 4, 3, 5, 6, 8}, keyFunc)
+			Expect(result).To(Equal([][]int{{2, 5, 8}, {1, 4}, {3, 6}}))
+		})
+	})
+
 	Describe("ToMap", func() {
 		It("returns empty map for an empty input", func() {
 			transformFunc := func(v int) (int, string) { return v, strconv.Itoa(v) }