feat: make Unicode handling in external API calls configurable

- Add DevPreserveUnicodeInExternalCalls config option (default: false) - Refactor external provider to use NameForExternal() method on auxArtist - Remove redundant Name field from auxArtist struct - Update all external API calls (image, URL, biography, similar, top songs, MBID) to use configurable Unicode handling - Add comprehensive tests for both Unicode-preserving and normalized behaviors - Refactor tests to use constants and improved structure with BeforeEach blocks Fixes issue where Spotify integration failed to find artist images for artists with Unicode characters (e.g., en dash) in their names. Signed-off-by: Deluan <deluan@navidrome.org>
2025-08-10 00:52:20 +00:00 · 2025-06-29 13:31:12 -04:00
parent 91e7f7b5c9
commit 8da042dc28
3 changed files with 115 additions and 47 deletions
--- a/conf/configuration.go
+++ b/conf/configuration.go
@@ -106,29 +106,30 @@ type configOptions struct {
 	Agents                          string

 	// DevFlags. These are used to enable/disable debugging and incomplete features
-	DevLogLevels                     map[string]string `json:",omitempty"`
-	DevLogSourceLine                 bool
-	DevEnableProfiler                bool
-	DevAutoCreateAdminPassword       string
-	DevAutoLoginUsername             string
-	DevActivityPanel                 bool
-	DevActivityPanelUpdateRate       time.Duration
-	DevSidebarPlaylists              bool
-	DevShowArtistPage                bool
-	DevUIShowConfig                  bool
-	DevNewEventStream                bool
-	DevOffsetOptimize                int
-	DevArtworkMaxRequests            int
-	DevArtworkThrottleBacklogLimit   int
-	DevArtworkThrottleBacklogTimeout time.Duration
-	DevArtistInfoTimeToLive          time.Duration
-	DevAlbumInfoTimeToLive           time.Duration
-	DevExternalScanner               bool
-	DevScannerThreads                uint
-	DevInsightsInitialDelay          time.Duration
-	DevEnablePlayerInsights          bool
-	DevPluginCompilationTimeout      time.Duration
-	DevExternalArtistFetchMultiplier float64
+	DevLogLevels                      map[string]string `json:",omitempty"`
+	DevLogSourceLine                  bool
+	DevEnableProfiler                 bool
+	DevAutoCreateAdminPassword        string
+	DevAutoLoginUsername              string
+	DevActivityPanel                  bool
+	DevActivityPanelUpdateRate        time.Duration
+	DevSidebarPlaylists               bool
+	DevShowArtistPage                 bool
+	DevUIShowConfig                   bool
+	DevNewEventStream                 bool
+	DevOffsetOptimize                 int
+	DevArtworkMaxRequests             int
+	DevArtworkThrottleBacklogLimit    int
+	DevArtworkThrottleBacklogTimeout  time.Duration
+	DevArtistInfoTimeToLive           time.Duration
+	DevAlbumInfoTimeToLive            time.Duration
+	DevExternalScanner                bool
+	DevScannerThreads                 uint
+	DevInsightsInitialDelay           time.Duration
+	DevEnablePlayerInsights           bool
+	DevPluginCompilationTimeout       time.Duration
+	DevExternalArtistFetchMultiplier  float64
+	DevPreserveUnicodeInExternalCalls bool
 }

 type scannerOptions struct {
@@ -601,6 +602,7 @@ func setViperDefaults() {
 	viper.SetDefault("devenableplayerinsights", true)
 	viper.SetDefault("devplugincompilationtimeout", time.Minute)
 	viper.SetDefault("devexternalartistfetchmultiplier", 1.5)
+	viper.SetDefault("devpreserveunicodeinexternalcalls", false)
 }

 func init() {
--- a/core/external/provider.go
+++ b/core/external/provider.go
@@ -56,7 +56,15 @@ type auxAlbum struct {

 type auxArtist struct {
 	model.Artist
-	Name string
+}
+
+// Name returns the appropriate artist name for external API calls
+// based on the DevPreserveUnicodeInExternalCalls configuration option
+func (a *auxArtist) Name() string {
+	if conf.Server.DevPreserveUnicodeInExternalCalls {
+		return a.Artist.Name
+	}
+	return str.Clear(a.Artist.Name)
 }

 type Agents interface {
@@ -181,7 +189,6 @@ func (e *provider) getArtist(ctx context.Context, id string) (auxArtist, error)
 	switch v := entity.(type) {
 	case *model.Artist:
 		artist.Artist = *v
-		artist.Name = str.Clear(v.Name)
 	case *model.MediaFile:
 		return e.getArtist(ctx, v.ArtistID)
 	case *model.Album:
@@ -211,7 +218,7 @@ func (e *provider) refreshArtistInfo(ctx context.Context, id string) (auxArtist,
 	// If we don't have any info, retrieves it now
 	updatedAt := V(artist.ExternalInfoUpdatedAt)
 	if updatedAt.IsZero() {
-		log.Debug(ctx, "ArtistInfo not cached. Retrieving it now", "updatedAt", updatedAt, "id", id, "name", artist.Name)
+		log.Debug(ctx, "ArtistInfo not cached. Retrieving it now", "updatedAt", updatedAt, "id", id, "name", artist.Name())
 		artist, err = e.populateArtistInfo(ctx, artist)
 		if err != nil {
 			return auxArtist{}, err
@@ -220,7 +227,7 @@ func (e *provider) refreshArtistInfo(ctx context.Context, id string) (auxArtist,

 	// If info is expired, trigger a populateArtistInfo in the background
 	if time.Since(updatedAt) > conf.Server.DevArtistInfoTimeToLive {
-		log.Debug("Found expired cached ArtistInfo, refreshing in the background", "updatedAt", updatedAt, "name", artist.Name)
+		log.Debug("Found expired cached ArtistInfo, refreshing in the background", "updatedAt", updatedAt, "name", artist.Name())
 		e.artistQueue.enqueue(&artist)
 	}
 	return artist, nil
@@ -230,7 +237,7 @@ func (e *provider) populateArtistInfo(ctx context.Context, artist auxArtist) (au
 	start := time.Now()
 	// Get MBID first, if it is not yet available
 	if artist.MbzArtistID == "" {
-		mbid, err := e.ag.GetArtistMBID(ctx, artist.ID, artist.Name)
+		mbid, err := e.ag.GetArtistMBID(ctx, artist.ID, artist.Name())
 		if mbid != "" && err == nil {
 			artist.MbzArtistID = mbid
 		}
@@ -246,14 +253,14 @@ func (e *provider) populateArtistInfo(ctx context.Context, artist auxArtist) (au
 	_ = g.Wait()

 	if utils.IsCtxDone(ctx) {
-		log.Warn(ctx, "ArtistInfo update canceled", "elapsed", "id", artist.ID, "name", artist.Name, time.Since(start), ctx.Err())
+		log.Warn(ctx, "ArtistInfo update canceled", "elapsed", "id", artist.ID, "name", artist.Name(), time.Since(start), ctx.Err())
 		return artist, ctx.Err()
 	}

 	artist.ExternalInfoUpdatedAt = P(time.Now())
 	err := e.ds.Artist(ctx).UpdateExternalInfo(&artist.Artist)
 	if err != nil {
-		log.Error(ctx, "Error trying to update artist external information", "id", artist.ID, "name", artist.Name,
+		log.Error(ctx, "Error trying to update artist external information", "id", artist.ID, "name", artist.Name(),
 			"elapsed", time.Since(start), err)
 	} else {
 		log.Trace(ctx, "ArtistInfo collected", "artist", artist, "elapsed", time.Since(start))
@@ -281,7 +288,7 @@ func (e *provider) ArtistRadio(ctx context.Context, id string, count int) (model
 		}

 		topCount := max(count, 20)
-		topSongs, err := e.getMatchingTopSongs(ctx, e.ag, &auxArtist{Name: a.Name, Artist: a}, topCount)
+		topSongs, err := e.getMatchingTopSongs(ctx, e.ag, &auxArtist{Artist: a}, topCount)
 		if err != nil {
 			log.Warn(ctx, "Error getting artist's top songs", "artist", a.Name, err)
 			return nil
@@ -401,9 +408,9 @@ func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (
 }

 func (e *provider) getMatchingTopSongs(ctx context.Context, agent agents.ArtistTopSongsRetriever, artist *auxArtist, count int) (model.MediaFiles, error) {
-	songs, err := agent.GetArtistTopSongs(ctx, artist.ID, artist.Name, artist.MbzArtistID, count)
+	songs, err := agent.GetArtistTopSongs(ctx, artist.ID, artist.Name(), artist.MbzArtistID, count)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get top songs for artist %s: %w", artist.Name, err)
+		return nil, fmt.Errorf("failed to get top songs for artist %s: %w", artist.Name(), err)
 	}

 	mbidMatches, err := e.loadTracksByMBID(ctx, songs)
@@ -415,13 +422,13 @@ func (e *provider) getMatchingTopSongs(ctx context.Context, agent agents.ArtistT
 		return nil, fmt.Errorf("failed to load tracks by title: %w", err)
 	}

-	log.Trace(ctx, "Top Songs loaded", "name", artist.Name, "numSongs", len(songs), "numMBIDMatches", len(mbidMatches), "numTitleMatches", len(titleMatches))
+	log.Trace(ctx, "Top Songs loaded", "name", artist.Name(), "numSongs", len(songs), "numMBIDMatches", len(mbidMatches), "numTitleMatches", len(titleMatches))
 	mfs := e.selectTopSongs(songs, mbidMatches, titleMatches, count)

 	if len(mfs) == 0 {
-		log.Debug(ctx, "No matching top songs found", "name", artist.Name)
+		log.Debug(ctx, "No matching top songs found", "name", artist.Name())
 	} else {
-		log.Debug(ctx, "Found matching top songs", "name", artist.Name, "numSongs", len(mfs))
+		log.Debug(ctx, "Found matching top songs", "name", artist.Name(), "numSongs", len(mfs))
 	}

 	return mfs, nil
@@ -518,7 +525,7 @@ func (e *provider) selectTopSongs(songs []agents.Song, byMBID, byTitle map[strin
 }

 func (e *provider) callGetURL(ctx context.Context, agent agents.ArtistURLRetriever, artist *auxArtist) {
-	artisURL, err := agent.GetArtistURL(ctx, artist.ID, artist.Name, artist.MbzArtistID)
+	artisURL, err := agent.GetArtistURL(ctx, artist.ID, artist.Name(), artist.MbzArtistID)
 	if err != nil {
 		return
 	}
@@ -526,7 +533,7 @@ func (e *provider) callGetURL(ctx context.Context, agent agents.ArtistURLRetriev
 }

 func (e *provider) callGetBiography(ctx context.Context, agent agents.ArtistBiographyRetriever, artist *auxArtist) {
-	bio, err := agent.GetArtistBiography(ctx, artist.ID, str.Clear(artist.Name), artist.MbzArtistID)
+	bio, err := agent.GetArtistBiography(ctx, artist.ID, artist.Name(), artist.MbzArtistID)
 	if err != nil {
 		return
 	}
@@ -536,7 +543,7 @@ func (e *provider) callGetBiography(ctx context.Context, agent agents.ArtistBiog
 }

 func (e *provider) callGetImage(ctx context.Context, agent agents.ArtistImageRetriever, artist *auxArtist) {
-	images, err := agent.GetArtistImages(ctx, artist.ID, artist.Name, artist.MbzArtistID)
+	images, err := agent.GetArtistImages(ctx, artist.ID, artist.Name(), artist.MbzArtistID)
 	if err != nil {
 		return
 	}
@@ -555,13 +562,13 @@ func (e *provider) callGetImage(ctx context.Context, agent agents.ArtistImageRet

 func (e *provider) callGetSimilar(ctx context.Context, agent agents.ArtistSimilarRetriever, artist *auxArtist,
 	limit int, includeNotPresent bool) {
-	similar, err := agent.GetSimilarArtists(ctx, artist.ID, artist.Name, artist.MbzArtistID, limit)
+	similar, err := agent.GetSimilarArtists(ctx, artist.ID, artist.Name(), artist.MbzArtistID, limit)
 	if len(similar) == 0 || err != nil {
 		return
 	}
 	start := time.Now()
 	sa, err := e.mapSimilarArtists(ctx, similar, limit, includeNotPresent)
-	log.Debug(ctx, "Mapped Similar Artists", "artist", artist.Name, "numSimilar", len(sa), "elapsed", time.Since(start))
+	log.Debug(ctx, "Mapped Similar Artists", "artist", artist.Name(), "numSimilar", len(sa), "elapsed", time.Since(start))
 	if err != nil {
 		return
 	}
@@ -635,11 +642,7 @@ func (e *provider) findArtistByName(ctx context.Context, artistName string) (*au
 	if len(artists) == 0 {
 		return nil, model.ErrNotFound
 	}
-	artist := &auxArtist{
-		Artist: artists[0],
-		Name:   str.Clear(artists[0].Name),
-	}
-	return artist, nil
+	return &auxArtist{Artist: artists[0]}, nil
 }

 func (e *provider) loadSimilar(ctx context.Context, artist *auxArtist, count int, includeNotPresent bool) error {
@@ -655,7 +658,7 @@ func (e *provider) loadSimilar(ctx context.Context, artist *auxArtist, count int
 		Filters: squirrel.Eq{"artist.id": ids},
 	})
 	if err != nil {
-		log.Error("Error loading similar artists", "id", artist.ID, "name", artist.Name, err)
+		log.Error("Error loading similar artists", "id", artist.ID, "name", artist.Name(), err)
 		return err
 	}

--- a/core/external/provider_artistimage_test.go
+++ b/core/external/provider_artistimage_test.go
@@ -265,6 +265,69 @@ var _ = Describe("Provider - ArtistImage", func() {
 		mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1")
 		mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "")
 	})
+
+	Context("Unicode handling in artist names", func() {
+		var artistWithEnDash *model.Artist
+		var expectedURL *url.URL
+
+		const (
+			originalArtistName   = "Run–D.M.C." // Artist name with en dash
+			normalizedArtistName = "Run-D.M.C." // Normalized version with hyphen
+		)
+
+		BeforeEach(func() {
+			// Test with en dash (–) in artist name like "Run–D.M.C."
+			artistWithEnDash = &model.Artist{ID: "artist-endash", Name: originalArtistName}
+			mockArtistRepo.Mock = mock.Mock{} // Reset default expectations
+			mockArtistRepo.On("Get", "artist-endash").Return(artistWithEnDash, nil).Once()
+
+			expectedURL, _ = url.Parse("http://example.com/rundmc.jpg")
+
+			// Mock the image agent to return an image for the artist
+			mockImageAgent.On("GetArtistImages", ctx, "artist-endash", mock.AnythingOfType("string"), "").
+				Return([]agents.ExternalImage{
+					{URL: "http://example.com/rundmc.jpg", Size: 1000},
+				}, nil).Once()
+
+		})
+
+		When("DevPreserveUnicodeInExternalCalls is true", func() {
+			BeforeEach(func() {
+				conf.Server.DevPreserveUnicodeInExternalCalls = true
+			})
+			It("preserves Unicode characters in artist names", func() {
+				// Act
+				imgURL, err := provider.ArtistImage(ctx, "artist-endash")
+
+				// Assert
+				Expect(err).ToNot(HaveOccurred())
+				Expect(imgURL).To(Equal(expectedURL))
+				mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-endash")
+				// This is the key assertion: ensure the original Unicode name is used
+				mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-endash", originalArtistName, "")
+			})
+		})
+
+		When("DevPreserveUnicodeInExternalCalls is false", func() {
+			BeforeEach(func() {
+				conf.Server.DevPreserveUnicodeInExternalCalls = false
+			})
+
+			It("normalizes Unicode characters)", func() {
+				conf.Server.DevPreserveUnicodeInExternalCalls = false
+
+				// Act
+				imgURL, err := provider.ArtistImage(ctx, "artist-endash")
+
+				// Assert
+				Expect(err).ToNot(HaveOccurred())
+				Expect(imgURL).To(Equal(expectedURL))
+				mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-endash")
+				// This assertion ensures the normalized name is used (en dash → hyphen)
+				mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-endash", normalizedArtistName, "")
+			})
+		})
+	})
 })

 // mockArtistImageAgent implementation using testify/mock