feat: add artist image uploads and image-folder artwork source (#5198)

* feat: add shared ImageUploadService for entity image management

* feat: add UploadedImage field and methods to Artist model

* feat: add uploaded_image column to artist table

* feat: add ArtistImageFolder config option

* refactor: wire ImageUploadService and delegate playlist file ops to it

Wire ImageUploadService into the DI container and refactor the playlist
service to delegate image file operations (SetImage/RemoveImage) to the
shared ImageUploadService, removing duplicated file I/O logic. A local
ImageUploadService interface is defined in core/playlists to avoid an
import cycle between core and core/playlists.

* feat: artist artwork reader checks uploaded image first

* feat: add image-folder priority source for artist artwork

* feat: cache key invalidation for image-folder and uploaded images

* refactor: extract shared image upload HTTP helpers

* feat: add artist image upload/delete API endpoints

* refactor: playlist handlers use shared image upload helpers

* feat: add shared ImageUploadOverlay component

* feat: add i18n keys for artist image upload

* feat: add image upload overlay to artist detail pages

* refactor: playlist details uses shared ImageUploadOverlay component

* fix: add gosec nolint directive for ParseMultipartForm

* refactor: deduplicate image upload code and optimize dir scanning

- Remove dead ImageFilename methods from Artist and Playlist models
  (production code uses core.imageFilename exclusively)
- Extract shared uploadedImagePath helper in model/image.go
- Extract findImageInArtistFolder to deduplicate dir-scanning logic
  between fromArtistImageFolder and getArtistImageFolderModTime
- Fix fileInputRef in useCallback dependency array

* fix: include artist UpdatedAt in artwork cache key

Without this, uploading or deleting an artist image would not
invalidate the cached artwork because the cache key was only based
on album folder timestamps, not the artist's own UpdatedAt field.

* feat: add Portuguese translations for artist image upload

* refactor: use shared i18n keys for cover art upload messages

Move cover art upload/remove translations from per-entity sections
(artist, playlist) to a shared top-level "message" section, avoiding
duplication across entity types and translation files.

* refactor: move cover art i18n keys to shared message section for all languages

* refactor: simplify image upload code and eliminate redundancies

Extracted duplicate image loading/lightbox state logic from
DesktopArtistDetails and MobileArtistDetails into a shared
useArtistImageState hook. Moved entity type constants to the consts
package and replaced raw string literals throughout model, core, and
nativeapi packages. Exported model.UploadedImagePath and reused it in
core/image_upload.go to consolidate path construction. Cached the
ArtistImageFolder lookup result in artistReader to eliminate a redundant
os.ReadDir call on every artwork request.

Signed-off-by: Deluan <deluan@navidrome.org>

* style: fix prettier formatting in ImageUploadOverlay

* fix: address code review feedback on image upload error handling

- RemoveImage now returns errors instead of swallowing them
- Artist handlers distinguish not-found from other DB errors
- Defer multipart temp file cleanup after parsing

* fix: enforce hard request size limit with MaxBytesReader for image uploads

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão
2026-03-15 22:19:55 -04:00
committed by GitHub
parent be06196168
commit ab8a58157a
57 changed files with 1169 additions and 567 deletions
+3 -3
View File
@@ -442,7 +442,7 @@ var _ = BeforeSuite(func() {
buildTestFS()
s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(),
playlists.NewPlaylists(initDS), metrics.NewNoopInstance())
playlists.NewPlaylists(initDS, core.NewImageUploadService()), metrics.NewNoopInstance())
_, err = s.ScanAll(ctx, true)
Expect(err).ToNot(HaveOccurred())
@@ -479,7 +479,7 @@ func setupTestDB() {
streamerSpy = &spyStreamer{}
decider := stream.NewTranscodeDecider(ds, noopFFmpeg{})
s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(),
playlists.NewPlaylists(ds), metrics.NewNoopInstance())
playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance())
router = subsonic.New(
ds,
noopArtwork{},
@@ -489,7 +489,7 @@ func setupTestDB() {
noopProvider{},
s,
events.NoopBroker(),
playlists.NewPlaylists(ds),
playlists.NewPlaylists(ds, core.NewImageUploadService()),
noopPlayTracker{},
core.NewShare(ds),
playback.PlaybackServer(nil),
+2 -1
View File
@@ -6,6 +6,7 @@ import (
"github.com/Masterminds/squirrel"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/core"
"github.com/navidrome/navidrome/core/artwork"
"github.com/navidrome/navidrome/core/metrics"
"github.com/navidrome/navidrome/core/playlists"
@@ -53,7 +54,7 @@ var _ = Describe("Multi-Library Support", Ordered, func() {
// Run incremental scan to import lib2 content (lib1 files unchanged → skipped)
s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(),
playlists.NewPlaylists(ds), metrics.NewNoopInstance())
playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance())
_, err = s.ScanAll(ctx, false)
Expect(err).ToNot(HaveOccurred())
+72
View File
@@ -0,0 +1,72 @@
package nativeapi
import (
"context"
"errors"
"io"
"net/http"
"time"
"github.com/deluan/rest"
"github.com/go-chi/chi/v5"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/server"
)
func (api *Router) addArtistRoute(r chi.Router) {
constructor := func(ctx context.Context) rest.Repository {
return api.ds.Resource(ctx, model.Artist{})
}
r.Route("/artist", func(r chi.Router) {
r.Get("/", rest.GetAll(constructor))
r.Route("/{id}", func(r chi.Router) {
r.Use(server.URLParamsMiddleware)
r.Get("/", rest.Get(constructor))
r.Post("/image", api.uploadArtistImage())
r.Delete("/image", api.deleteArtistImage())
})
})
}
func (api *Router) uploadArtistImage() http.HandlerFunc {
return handleImageUpload(func(ctx context.Context, reader io.Reader, ext string) error {
artistID := chi.URLParamFromCtx(ctx, "id")
ar, err := api.ds.Artist(ctx).Get(artistID)
if err != nil {
if errors.Is(err, model.ErrNotFound) {
return model.ErrNotFound
}
return err
}
oldPath := ar.UploadedImagePath()
filename, err := api.imgUpload.SetImage(ctx, consts.EntityArtist, ar.ID, ar.Name, oldPath, reader, ext)
if err != nil {
return err
}
ar.UploadedImage = filename
now := time.Now()
ar.UpdatedAt = &now
return api.ds.Artist(ctx).Put(ar, "uploaded_image", "updated_at")
})
}
func (api *Router) deleteArtistImage() http.HandlerFunc {
return handleImageDelete(func(ctx context.Context) error {
artistID := chi.URLParamFromCtx(ctx, "id")
ar, err := api.ds.Artist(ctx).Get(artistID)
if err != nil {
if errors.Is(err, model.ErrNotFound) {
return model.ErrNotFound
}
return err
}
if err := api.imgUpload.RemoveImage(ctx, ar.UploadedImagePath()); err != nil {
return err
}
ar.UploadedImage = ""
now := time.Now()
ar.UpdatedAt = &now
return api.ds.Artist(ctx).Put(ar, "uploaded_image", "updated_at")
})
}
+1 -1
View File
@@ -28,7 +28,7 @@ var _ = Describe("Config API", func() {
conf.Server.DevUIShowConfig = true // Enable config endpoint for tests
ds = &tests.MockDataStore{}
auth.Init(ds)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil)
router = server.JWTVerifier(nativeRouter)
// Create test users
+120
View File
@@ -0,0 +1,120 @@
package nativeapi
import (
"context"
"errors"
"fmt"
"image"
_ "image/gif"
_ "image/jpeg"
_ "image/png"
"io"
"net/http"
"path/filepath"
"strings"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
_ "golang.org/x/image/webp"
)
const maxImageSize = 10 << 20 // 10MB
func checkImageUploadPermission(w http.ResponseWriter, r *http.Request) bool {
user, _ := request.UserFrom(r.Context())
if !conf.Server.EnableCoverArtUpload && !user.IsAdmin {
http.Error(w, "cover art upload is disabled", http.StatusForbidden)
return false
}
return true
}
func handleImageUpload(saveFn func(ctx context.Context, reader io.Reader, ext string) error) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
if !checkImageUploadPermission(w, r) {
return
}
r.Body = http.MaxBytesReader(w, r.Body, maxImageSize)
if err := r.ParseMultipartForm(maxImageSize / 2); err != nil {
log.Error(ctx, "Error parsing multipart form", err)
http.Error(w, "file too large or invalid form", http.StatusBadRequest)
return
}
defer func() {
if r.MultipartForm != nil {
if err := r.MultipartForm.RemoveAll(); err != nil {
log.Warn(ctx, "Error removing multipart temp files", err)
}
}
}()
file, header, err := r.FormFile("image")
if err != nil {
log.Error(ctx, "Error reading uploaded file", err)
http.Error(w, "missing image file", http.StatusBadRequest)
return
}
defer file.Close()
_, format, err := image.DecodeConfig(file)
if err != nil {
log.Error(ctx, "Uploaded file is not a valid image", err)
http.Error(w, "invalid image file", http.StatusBadRequest)
return
}
if seeker, ok := file.(io.Seeker); ok {
if _, err := seeker.Seek(0, io.SeekStart); err != nil {
log.Error(ctx, "Error seeking file", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
}
ext := "." + format
if ext == "." {
ext = strings.ToLower(filepath.Ext(header.Filename))
}
if ext == "" || ext == "." {
log.Error(ctx, "Could not determine image type", "filename", header.Filename)
http.Error(w, "could not determine image type", http.StatusBadRequest)
return
}
if err := saveFn(ctx, file, ext); err != nil {
if errors.Is(err, model.ErrNotAuthorized) {
http.Error(w, "not authorized", http.StatusForbidden)
return
}
if errors.Is(err, model.ErrNotFound) {
http.Error(w, "not found", http.StatusNotFound)
return
}
log.Error(ctx, "Error saving image", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
_, _ = fmt.Fprintf(w, `{"status":"ok"}`)
}
}
func handleImageDelete(deleteFn func(ctx context.Context) error) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
if !checkImageUploadPermission(w, r) {
return
}
if err := deleteFn(ctx); err != nil {
if errors.Is(err, model.ErrNotAuthorized) {
http.Error(w, "not authorized", http.StatusForbidden)
return
}
if errors.Is(err, model.ErrNotFound) {
http.Error(w, "not found", http.StatusNotFound)
return
}
log.Error(ctx, "Error removing image", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
_, _ = fmt.Fprintf(w, `{"status":"ok"}`)
}
}
+1 -1
View File
@@ -29,7 +29,7 @@ var _ = Describe("Library API", func() {
DeferCleanup(configtest.SetupConfig())
ds = &tests.MockDataStore{}
auth.Init(ds)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil)
router = server.JWTVerifier(nativeRouter)
// Create test users
+4 -3
View File
@@ -44,10 +44,11 @@ type Router struct {
users core.User
maintenance core.Maintenance
pluginManager PluginManager
imgUpload core.ImageUploadService
}
func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager) *Router {
r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager}
func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager, imgUpload core.ImageUploadService) *Router {
r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager, imgUpload: imgUpload}
r.Handler = r.routes()
return r
}
@@ -66,7 +67,7 @@ func (api *Router) routes() http.Handler {
api.RX(r, "/user", api.users.NewRepository, true)
api.R(r, "/song", model.MediaFile{}, false)
api.R(r, "/album", model.Album{}, false)
api.R(r, "/artist", model.Artist{}, false)
api.addArtistRoute(r)
api.R(r, "/genre", model.Genre{}, false)
api.R(r, "/player", model.Player{}, true)
api.R(r, "/transcoding", model.Transcoding{}, conf.Server.EnableTranscodingConfig)
+1 -1
View File
@@ -94,7 +94,7 @@ var _ = Describe("Song Endpoints", func() {
mfRepo.SetData(testSongs)
// Create the native API router and wrap it with the JWTVerifier middleware
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil)
router = server.JWTVerifier(nativeRouter)
w = httptest.NewRecorder()
})
+8 -110
View File
@@ -5,25 +5,17 @@ import (
"encoding/json"
"errors"
"fmt"
"image"
_ "image/gif"
_ "image/jpeg"
_ "image/png"
"io"
"net/http"
"path/filepath"
"strconv"
"strings"
"github.com/deluan/rest"
"github.com/go-chi/chi/v5"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/core/playlists"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
"github.com/navidrome/navidrome/utils/req"
_ "golang.org/x/image/webp"
)
type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc
@@ -234,110 +226,16 @@ func getSongPlaylists(svc playlists.Playlists) http.HandlerFunc {
}
}
const maxImageSize = 10 << 20 // 10MB
func uploadPlaylistImage(pls playlists.Playlists) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
user, _ := request.UserFrom(ctx)
if !conf.Server.EnableCoverArtUpload && !user.IsAdmin {
http.Error(w, "cover art upload is disabled", http.StatusForbidden)
return
}
p := req.Params(r)
playlistId, _ := p.String(":id")
if err := r.ParseMultipartForm(maxImageSize); err != nil { //nolint:gosec // size is limited by maxImageSize parameter
log.Error(ctx, "Error parsing multipart form", err)
http.Error(w, "file too large or invalid form", http.StatusBadRequest)
return
}
file, header, err := r.FormFile("image")
if err != nil {
log.Error(ctx, "Error reading uploaded file", err)
http.Error(w, "missing image file", http.StatusBadRequest)
return
}
defer file.Close()
// Validate the uploaded file is a valid image
_, format, err := image.DecodeConfig(file)
if err != nil {
log.Error(ctx, "Uploaded file is not a valid image", err)
http.Error(w, "invalid image file", http.StatusBadRequest)
return
}
// Reset reader after DecodeConfig consumed some bytes
if seeker, ok := file.(io.Seeker); ok {
if _, err := seeker.Seek(0, io.SeekStart); err != nil {
log.Error(ctx, "Error seeking file", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
}
// Determine file extension from decoded format or original filename
ext := "." + format
if ext == "." {
ext = strings.ToLower(filepath.Ext(header.Filename))
}
if ext == "" || ext == "." {
log.Error(ctx, "Could not determine image type", "playlistId", playlistId, "filename", header.Filename)
http.Error(w, "could not determine image type", http.StatusBadRequest)
return
}
err = pls.SetImage(ctx, playlistId, file, ext)
if errors.Is(err, model.ErrNotAuthorized) {
log.Error(ctx, "Not authorized to upload playlist image", "playlistId", playlistId, err)
http.Error(w, "not authorized", http.StatusForbidden)
return
}
if errors.Is(err, model.ErrNotFound) {
log.Error(ctx, "Playlist not found for image upload", "playlistId", playlistId, err)
http.Error(w, "not found", http.StatusNotFound)
return
}
if err != nil {
log.Error(ctx, "Error saving playlist image", "playlistId", playlistId, err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
_, _ = fmt.Fprintf(w, `{"status":"ok"}`) //nolint:gosec
}
return handleImageUpload(func(ctx context.Context, reader io.Reader, ext string) error {
playlistId := chi.URLParamFromCtx(ctx, "id")
return pls.SetImage(ctx, playlistId, reader, ext)
})
}
func deletePlaylistImage(pls playlists.Playlists) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
user, _ := request.UserFrom(ctx)
if !conf.Server.EnableCoverArtUpload && !user.IsAdmin {
http.Error(w, "cover art upload is disabled", http.StatusForbidden)
return
}
p := req.Params(r)
playlistId, _ := p.String(":id")
err := pls.RemoveImage(ctx, playlistId)
if errors.Is(err, model.ErrNotAuthorized) {
log.Error(ctx, "Not authorized to remove playlist image", "playlistId", playlistId, err)
http.Error(w, "not authorized", http.StatusForbidden)
return
}
if errors.Is(err, model.ErrNotFound) {
log.Error(ctx, "Playlist not found for image removal", "playlistId", playlistId, err)
http.Error(w, "not found", http.StatusNotFound)
return
}
if err != nil {
log.Error(ctx, "Error removing playlist image", "playlistId", playlistId, err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
_, _ = fmt.Fprintf(w, `{"status":"ok"}`) //nolint:gosec
}
return handleImageDelete(func(ctx context.Context) error {
playlistId := chi.URLParamFromCtx(ctx, "id")
return pls.RemoveImage(ctx, playlistId)
})
}
+1 -1
View File
@@ -98,7 +98,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() {
err := userRepo.Put(&testUser)
Expect(err).ToNot(HaveOccurred())
nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil)
nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil)
router = server.JWTVerifier(nativeRouter)
w = httptest.NewRecorder()
})
+1 -1
View File
@@ -33,7 +33,7 @@ var _ = Describe("Plugin API", func() {
ds = &tests.MockDataStore{}
mockManager = &tests.MockPluginManager{}
auth.Init(ds)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager)
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager, nil)
router = server.JWTVerifier(nativeRouter)
// Create test users