When retrieving images from external sources, avoid calling it again …

…if data is already cached locally. Relates to #2130 (comment)
navidrome · Feb 2, 2023 · f4b50c4 · f4b50c4
1 parent 4a7e86e
commit f4b50c4
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 12 deletions.
diff --git a/consts/consts.go b/consts/consts.go
@@ -117,6 +117,7 @@ var (
 	VariousArtists      = "Various Artists"
 	VariousArtistsID    = fmt.Sprintf("%x", md5.Sum([]byte(strings.ToLower(VariousArtists))))
 	UnknownArtist       = "[Unknown Artist]"
+	UnknownArtistID     = fmt.Sprintf("%x", md5.Sum([]byte(strings.ToLower(UnknownArtist))))
 	VariousArtistsMbzId = "89ad4ac3-39f7-470e-963a-56509c546377"
 
 	ServerStart = time.Now()

diff --git a/core/artwork/cache_warmer.go b/core/artwork/cache_warmer.go
@@ -48,7 +48,15 @@ type cacheWarmer struct {
 	wakeSignal chan struct{}
 }
 
+var ignoredIds = map[string]struct{}{
+	consts.VariousArtistsID: {},
+	consts.UnknownArtistID:  {},
+}
+
 func (a *cacheWarmer) PreCache(artID model.ArtworkID) {
+	if _, shouldIgnore := ignoredIds[artID.ID]; shouldIgnore {
+		return
+	}
 	a.mutex.Lock()
 	defer a.mutex.Unlock()
 	a.buffer[artID] = struct{}{}

diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go
@@ -42,7 +42,9 @@ func newArtistReader(ctx context.Context, artwork *artwork, artID model.ArtworkI
 		em:     em,
 		artist: *ar,
 	}
-	a.cacheKey.lastUpdate = ar.ExternalInfoUpdatedAt
+	// TODO Find a way to factor in the ExternalUpdateInfoAt in the cache key. Problem is that it can
+	// change _after_ retrieving from external sources, making the key invalid
+	//a.cacheKey.lastUpdate = ar.ExternalInfoUpdatedAt
 	var files []string
 	var paths []string
 	for _, al := range als {
@@ -64,10 +66,10 @@ func newArtistReader(ctx context.Context, artwork *artwork, artID model.ArtworkI
 func (a *artistReader) Key() string {
 	hash := md5.Sum([]byte(conf.Server.Agents + conf.Server.Spotify.ID))
 	return fmt.Sprintf(
-		"%s.%x.%t ",
+		"%s.%t.%x",
 		a.cacheKey.Key(),
-		hash,
 		conf.Server.EnableExternalServices,
+		hash,
 	)
 }
 

diff --git a/core/external_metadata.go b/core/external_metadata.go
@@ -180,6 +180,16 @@ func clearName(name string) string {
 }
 
 func (e *externalMetadata) UpdateArtistInfo(ctx context.Context, id string, similarCount int, includeNotPresent bool) (*model.Artist, error) {
+	artist, err := e.refreshArtistInfo(ctx, id)
+	if err != nil {
+		return nil, err
+	}
+
+	err = e.loadSimilar(ctx, artist, similarCount, includeNotPresent)
+	return &artist.Artist, err
+}
+
+func (e *externalMetadata) refreshArtistInfo(ctx context.Context, id string) (*auxArtist, error) {
 	artist, err := e.getArtist(ctx, id)
 	if err != nil {
 		return nil, err
@@ -188,30 +198,28 @@ func (e *externalMetadata) UpdateArtistInfo(ctx context.Context, id string, simi
 	// If we don't have any info, retrieves it now
 	if artist.ExternalInfoUpdatedAt.IsZero() {
 		log.Debug(ctx, "ArtistInfo not cached. Retrieving it now", "updatedAt", artist.ExternalInfoUpdatedAt, "id", id, "name", artist.Name)
-		err = e.refreshArtistInfo(ctx, artist)
+		err := e.populateArtistInfo(ctx, artist)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	// If info is expired, trigger a refresh in the background
+	// If info is expired, trigger a populateArtistInfo in the background
 	if time.Since(artist.ExternalInfoUpdatedAt) > consts.ArtistInfoTimeToLive {
 		log.Debug("Found expired cached ArtistInfo, refreshing in the background", "updatedAt", artist.ExternalInfoUpdatedAt, "name", artist.Name)
 		go func() {
 			ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
 			defer cancel()
-			err := e.refreshArtistInfo(ctx, artist)
+			err := e.populateArtistInfo(ctx, artist)
 			if err != nil {
 				log.Error("Error refreshing ArtistInfo", "id", id, "name", artist.Name, err)
 			}
 		}()
 	}
-
-	err = e.loadSimilar(ctx, artist, similarCount, includeNotPresent)
-	return &artist.Artist, err
+	return artist, nil
 }
 
-func (e *externalMetadata) refreshArtistInfo(ctx context.Context, artist *auxArtist) error {
+func (e *externalMetadata) populateArtistInfo(ctx context.Context, artist *auxArtist) error {
 	// Get MBID first, if it is not yet available
 	if artist.MbzArtistID == "" {
 		mbid, err := e.ag.GetArtistMBID(ctx, artist.ID, artist.Name)
@@ -314,12 +322,11 @@ func (e *externalMetadata) SimilarSongs(ctx context.Context, id string, count in
 }
 
 func (e *externalMetadata) ArtistImage(ctx context.Context, id string) (*url.URL, error) {
-	artist, err := e.getArtist(ctx, id)
+	artist, err := e.refreshArtistInfo(ctx, id)
 	if err != nil {
 		return nil, err
 	}
 
-	e.callGetImage(ctx, e.ag, artist)
 	if utils.IsCtxDone(ctx) {
 		log.Warn(ctx, "ArtistImage call canceled", ctx.Err())
 		return nil, ctx.Err()