mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
refactor: apply Unicode handling pattern to auxAlbum
Extended the configurable Unicode handling to album names, matching the pattern already implemented for artist names. This ensures consistent behavior when DevPreserveUnicodeInExternalCalls is enabled for both artist and album external API calls. Changes: - Removed Name field from auxAlbum struct, added Name() method with Unicode logic - Updated getAlbum, UpdateAlbumInfo, populateAlbumInfo, and AlbumImage functions - Added comprehensive tests for album Unicode handling (preserve and normalize) - Fixed typo in artist image test description
This commit is contained in:
Deluan
parent
c35da57996
commit
86f74535e8
34
core/external/provider.go
vendored
34
core/external/provider.go
vendored
@ -51,7 +51,15 @@ type provider struct {
|
||||
|
||||
type auxAlbum struct {
|
||||
model.Album
|
||||
Name string
|
||||
}
|
||||
|
||||
// Name returns the appropriate album name for external API calls
|
||||
// based on the DevPreserveUnicodeInExternalCalls configuration option
|
||||
func (a *auxAlbum) Name() string {
|
||||
if conf.Server.DevPreserveUnicodeInExternalCalls {
|
||||
return a.Album.Name
|
||||
}
|
||||
return str.Clear(a.Album.Name)
|
||||
}
|
||||
|
||||
type auxArtist struct {
|
||||
@ -96,7 +104,6 @@ func (e *provider) getAlbum(ctx context.Context, id string) (auxAlbum, error) {
|
||||
switch v := entity.(type) {
|
||||
case *model.Album:
|
||||
album.Album = *v
|
||||
album.Name = str.Clear(v.Name)
|
||||
case *model.MediaFile:
|
||||
return e.getAlbum(ctx, v.AlbumID)
|
||||
default:
|
||||
@ -114,8 +121,9 @@ func (e *provider) UpdateAlbumInfo(ctx context.Context, id string) (*model.Album
|
||||
}
|
||||
|
||||
updatedAt := V(album.ExternalInfoUpdatedAt)
|
||||
albumName := album.Name()
|
||||
if updatedAt.IsZero() {
|
||||
log.Debug(ctx, "AlbumInfo not cached. Retrieving it now", "updatedAt", updatedAt, "id", id, "name", album.Name)
|
||||
log.Debug(ctx, "AlbumInfo not cached. Retrieving it now", "updatedAt", updatedAt, "id", id, "name", albumName)
|
||||
album, err = e.populateAlbumInfo(ctx, album)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@ -124,7 +132,7 @@ func (e *provider) UpdateAlbumInfo(ctx context.Context, id string) (*model.Album
|
||||
|
||||
// If info is expired, trigger a populateAlbumInfo in the background
|
||||
if time.Since(updatedAt) > conf.Server.DevAlbumInfoTimeToLive {
|
||||
log.Debug("Found expired cached AlbumInfo, refreshing in the background", "updatedAt", album.ExternalInfoUpdatedAt, "name", album.Name)
|
||||
log.Debug("Found expired cached AlbumInfo, refreshing in the background", "updatedAt", album.ExternalInfoUpdatedAt, "name", albumName)
|
||||
e.albumQueue.enqueue(&album)
|
||||
}
|
||||
|
||||
@ -133,12 +141,13 @@ func (e *provider) UpdateAlbumInfo(ctx context.Context, id string) (*model.Album
|
||||
|
||||
func (e *provider) populateAlbumInfo(ctx context.Context, album auxAlbum) (auxAlbum, error) {
|
||||
start := time.Now()
|
||||
info, err := e.ag.GetAlbumInfo(ctx, album.Name, album.AlbumArtist, album.MbzAlbumID)
|
||||
albumName := album.Name()
|
||||
info, err := e.ag.GetAlbumInfo(ctx, albumName, album.AlbumArtist, album.MbzAlbumID)
|
||||
if errors.Is(err, agents.ErrNotFound) {
|
||||
return album, nil
|
||||
}
|
||||
if err != nil {
|
||||
log.Error("Error refreshing AlbumInfo", "id", album.ID, "name", album.Name, "artist", album.AlbumArtist,
|
||||
log.Error("Error refreshing AlbumInfo", "id", album.ID, "name", albumName, "artist", album.AlbumArtist,
|
||||
"elapsed", time.Since(start), err)
|
||||
return album, err
|
||||
}
|
||||
@ -150,7 +159,7 @@ func (e *provider) populateAlbumInfo(ctx context.Context, album auxAlbum) (auxAl
|
||||
album.Description = info.Description
|
||||
}
|
||||
|
||||
images, err := e.ag.GetAlbumImages(ctx, album.Name, album.AlbumArtist, album.MbzAlbumID)
|
||||
images, err := e.ag.GetAlbumImages(ctx, albumName, album.AlbumArtist, album.MbzAlbumID)
|
||||
if err == nil && len(images) > 0 {
|
||||
sort.Slice(images, func(i, j int) bool {
|
||||
return images[i].Size > images[j].Size
|
||||
@ -169,7 +178,7 @@ func (e *provider) populateAlbumInfo(ctx context.Context, album auxAlbum) (auxAl
|
||||
|
||||
err = e.ds.Album(ctx).UpdateExternalInfo(&album.Album)
|
||||
if err != nil {
|
||||
log.Error(ctx, "Error trying to update album external information", "id", album.ID, "name", album.Name,
|
||||
log.Error(ctx, "Error trying to update album external information", "id", album.ID, "name", albumName,
|
||||
"elapsed", time.Since(start), err)
|
||||
} else {
|
||||
log.Trace(ctx, "AlbumInfo collected", "album", album, "elapsed", time.Since(start))
|
||||
@ -353,22 +362,23 @@ func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error)
|
||||
return nil, err
|
||||
}
|
||||
|
||||
images, err := e.ag.GetAlbumImages(ctx, album.Name, album.AlbumArtist, album.MbzAlbumID)
|
||||
albumName := album.Name()
|
||||
images, err := e.ag.GetAlbumImages(ctx, albumName, album.AlbumArtist, album.MbzAlbumID)
|
||||
if err != nil {
|
||||
switch {
|
||||
case errors.Is(err, agents.ErrNotFound):
|
||||
log.Trace(ctx, "Album not found in agent", "albumID", id, "name", album.Name, "artist", album.AlbumArtist)
|
||||
log.Trace(ctx, "Album not found in agent", "albumID", id, "name", albumName, "artist", album.AlbumArtist)
|
||||
return nil, model.ErrNotFound
|
||||
case errors.Is(err, context.Canceled):
|
||||
log.Debug(ctx, "GetAlbumImages call canceled", err)
|
||||
default:
|
||||
log.Warn(ctx, "Error getting album images from agent", "albumID", id, "name", album.Name, "artist", album.AlbumArtist, err)
|
||||
log.Warn(ctx, "Error getting album images from agent", "albumID", id, "name", albumName, "artist", album.AlbumArtist, err)
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
|
||||
if len(images) == 0 {
|
||||
log.Warn(ctx, "Agent returned no images without error", "albumID", id, "name", album.Name, "artist", album.AlbumArtist)
|
||||
log.Warn(ctx, "Agent returned no images without error", "albumID", id, "name", albumName, "artist", album.AlbumArtist)
|
||||
return nil, model.ErrNotFound
|
||||
}
|
||||
|
||||
|
||||
63
core/external/provider_albumimage_test.go
vendored
63
core/external/provider_albumimage_test.go
vendored
@ -260,6 +260,69 @@ var _ = Describe("Provider - AlbumImage", func() {
|
||||
mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "not-found")
|
||||
mockAlbumAgent.AssertNotCalled(GinkgoT(), "GetAlbumImages", mock.Anything, mock.Anything, mock.Anything)
|
||||
})
|
||||
|
||||
Context("Unicode handling in album names", func() {
|
||||
var albumWithEnDash *model.Album
|
||||
var expectedURL *url.URL
|
||||
|
||||
const (
|
||||
originalAlbumName = "Raising Hell–Deluxe" // Album name with en dash
|
||||
normalizedAlbumName = "Raising Hell-Deluxe" // Normalized version with hyphen
|
||||
)
|
||||
|
||||
BeforeEach(func() {
|
||||
// Test with en dash (–) in album name
|
||||
albumWithEnDash = &model.Album{ID: "album-endash", Name: originalAlbumName, AlbumArtistID: "artist-1"}
|
||||
mockArtistRepo.Mock = mock.Mock{} // Reset default expectations
|
||||
mockAlbumRepo.Mock = mock.Mock{} // Reset default expectations
|
||||
mockArtistRepo.On("Get", "album-endash").Return(nil, model.ErrNotFound).Once()
|
||||
mockAlbumRepo.On("Get", "album-endash").Return(albumWithEnDash, nil).Once()
|
||||
|
||||
expectedURL, _ = url.Parse("http://example.com/album.jpg")
|
||||
|
||||
// Mock the album agent to return an image for the album
|
||||
mockAlbumAgent.On("GetAlbumImages", ctx, mock.AnythingOfType("string"), "", "").
|
||||
Return([]agents.ExternalImage{
|
||||
{URL: "http://example.com/album.jpg", Size: 1000},
|
||||
}, nil).Once()
|
||||
})
|
||||
|
||||
When("DevPreserveUnicodeInExternalCalls is true", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.DevPreserveUnicodeInExternalCalls = true
|
||||
})
|
||||
|
||||
It("preserves Unicode characters in album names", func() {
|
||||
// Act
|
||||
imgURL, err := provider.AlbumImage(ctx, "album-endash")
|
||||
|
||||
// Assert
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(imgURL).To(Equal(expectedURL))
|
||||
mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-endash")
|
||||
// This is the key assertion: ensure the original Unicode name is used
|
||||
mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, originalAlbumName, "", "")
|
||||
})
|
||||
})
|
||||
|
||||
When("DevPreserveUnicodeInExternalCalls is false", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.DevPreserveUnicodeInExternalCalls = false
|
||||
})
|
||||
|
||||
It("normalizes Unicode characters", func() {
|
||||
// Act
|
||||
imgURL, err := provider.AlbumImage(ctx, "album-endash")
|
||||
|
||||
// Assert
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(imgURL).To(Equal(expectedURL))
|
||||
mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-endash")
|
||||
// This assertion ensures the normalized name is used (en dash → hyphen)
|
||||
mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, normalizedAlbumName, "", "")
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
// mockAlbumInfoAgent implementation
|
||||
|
||||
2
core/external/provider_artistimage_test.go
vendored
2
core/external/provider_artistimage_test.go
vendored
@ -313,7 +313,7 @@ var _ = Describe("Provider - ArtistImage", func() {
|
||||
conf.Server.DevPreserveUnicodeInExternalCalls = false
|
||||
})
|
||||
|
||||
It("normalizes Unicode characters)", func() {
|
||||
It("normalizes Unicode characters", func() {
|
||||
// Act
|
||||
imgURL, err := provider.ArtistImage(ctx, "artist-endash")
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user