From 44e7502aef70f7e073039fc5be3d448413a09d19 Mon Sep 17 00:00:00 2001
From: Deluan <deluan@navidrome.org>
Date: Sun, 18 Jul 2021 18:28:51 -0400
Subject: [PATCH] Log warning when artist has a MBID of Various Artists

---
 consts/consts.go                 |  7 +++---
 persistence/album_repository.go  |  2 +-
 persistence/artist_repository.go |  2 +-
 persistence/helpers.go           | 40 +++++++++++++++++---------------
 persistence/helpers_test.go      | 10 ++++----
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/consts/consts.go b/consts/consts.go
index 8a8b16b4..04ce5e89 100644
--- a/consts/consts.go
+++ b/consts/consts.go
@@ -85,9 +85,10 @@ var (
 )
 
 var (
-	VariousArtists   = "Various Artists"
-	VariousArtistsID = fmt.Sprintf("%x", md5.Sum([]byte(strings.ToLower(VariousArtists))))
-	UnknownArtist    = "[Unknown Artist]"
+	VariousArtists      = "Various Artists"
+	VariousArtistsID    = fmt.Sprintf("%x", md5.Sum([]byte(strings.ToLower(VariousArtists))))
+	UnknownArtist       = "[Unknown Artist]"
+	VariousArtistsMbzId = "89ad4ac3-39f7-470e-963a-56509c546377"
 
 	ServerStart = time.Now()
 )
diff --git a/persistence/album_repository.go b/persistence/album_repository.go
index d651d27a..f9cc827b 100644
--- a/persistence/album_repository.go
+++ b/persistence/album_repository.go
@@ -236,7 +236,7 @@ func (r *albumRepository) refresh(ids ...string) error {
 
 		al.AlbumArtistID, al.AlbumArtist = getAlbumArtist(al)
 		al.MinYear = getMinYear(al.Years)
-		al.MbzAlbumID = getMbzId(r.ctx, al.MbzAlbumID, r.tableName, al.Name)
+		al.MbzAlbumID = getMostFrequentMbzID(r.ctx, al.MbzAlbumID, r.tableName, al.Name)
 		al.Comment = getComment(al.Comments, zwsp)
 		if al.CurrentId != "" {
 			toUpdate++
diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go
index 252fdc4b..76119c3c 100644
--- a/persistence/artist_repository.go
+++ b/persistence/artist_repository.go
@@ -198,7 +198,7 @@ func (r *artistRepository) refresh(ids ...string) error {
 		} else {
 			toInsert++
 		}
-		ar.MbzArtistID = getMbzId(r.ctx, ar.MbzArtistID, r.tableName, ar.Name)
+		ar.MbzArtistID = getMostFrequentMbzID(r.ctx, ar.MbzArtistID, r.tableName, ar.Name)
 		err := r.Put(&ar.Artist)
 		if err != nil {
 			return err
diff --git a/persistence/helpers.go b/persistence/helpers.go
index 5b6dc806..0dd03b07 100644
--- a/persistence/helpers.go
+++ b/persistence/helpers.go
@@ -7,8 +7,9 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/Masterminds/squirrel"
 	"github.com/navidrome/navidrome/consts"
+
+	"github.com/Masterminds/squirrel"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/utils"
@@ -59,31 +60,32 @@ func (e existsCond) ToSql() (string, []interface{}, error) {
 	return sql, args, err
 }
 
-func getMbzId(ctx context.Context, mbzIDS, entityName, name string) string {
-	ids := strings.Fields(mbzIDS)
+func getMostFrequentMbzID(ctx context.Context, mbzIDs, entityName, name string) string {
+	ids := strings.Fields(mbzIDs)
 	if len(ids) == 0 {
 		return ""
 	}
+	if len(ids) == 1 {
+		return ids[0]
+	}
 	idCounts := map[string]int{}
-	for _, id := range ids {
-		if c, ok := idCounts[id]; ok {
-			idCounts[id] = c + 1
-		} else {
-			idCounts[id] = 1
-		}
-	}
-
-	var topKey string
+	var topId string
 	var topCount int
-	for k, v := range idCounts {
-		if v > topCount {
-			topKey = k
-			topCount = v
+	for _, id := range ids {
+		c := idCounts[id] + 1
+		idCounts[id] = c
+		if c > topCount {
+			topId = id
+			topCount = c
 		}
 	}
 
-	if len(idCounts) > 1 && name != consts.VariousArtists {
-		log.Warn(ctx, "Multiple MBIDs found for "+entityName, "name", name, "mbids", idCounts)
+	if name != consts.VariousArtists {
+		if topId == consts.VariousArtistsMbzId {
+			log.Warn(ctx, "Artist with mbid of Various Artists", "name", name, "mbid", topId)
+		} else {
+			log.Warn(ctx, "Multiple MBIDs found for "+entityName, "name", name, "mbids", idCounts)
+		}
 	}
-	return topKey
+	return topId
 }
diff --git a/persistence/helpers_test.go b/persistence/helpers_test.go
index 4a9f05a4..511849e1 100644
--- a/persistence/helpers_test.go
+++ b/persistence/helpers_test.go
@@ -53,7 +53,7 @@ var _ = Describe("Helpers", func() {
 		})
 	})
 
-	Describe("Exists", func() {
+	Describe("exists", func() {
 		It("constructs the correct EXISTS query", func() {
 			e := exists("album", squirrel.Eq{"id": 1})
 			sql, args, err := e.ToSql()
@@ -63,15 +63,15 @@ var _ = Describe("Helpers", func() {
 		})
 	})
 
-	Describe("getMbzId", func() {
+	Describe("getMostFrequentMbzID", func() {
 		It(`returns "" when no ids are passed`, func() {
-			Expect(getMbzId(context.TODO(), " ", "", "")).To(Equal(""))
+			Expect(getMostFrequentMbzID(context.TODO(), " ", "", "")).To(Equal(""))
 		})
 		It(`returns the only id passed`, func() {
-			Expect(getMbzId(context.TODO(), "1234 ", "", "")).To(Equal("1234"))
+			Expect(getMostFrequentMbzID(context.TODO(), "111 ", "", "")).To(Equal("111"))
 		})
 		It(`returns the id with higher frequency`, func() {
-			Expect(getMbzId(context.TODO(), "1 2 3 4 1", "", "")).To(Equal("1"))
+			Expect(getMostFrequentMbzID(context.TODO(), "1 2 3 4 2", "", "")).To(Equal("2"))
 		})
 	})
 })