diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 187ab488d..bf13dc731 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -72,7 +72,8 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { scannerScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, scannerScanner) library := core.NewLibrary(dataStore, scannerScanner, watcher, broker) - router := nativeapi.New(dataStore, share, playlists, insights, library) + maintenance := core.NewMaintenance(dataStore) + router := nativeapi.New(dataStore, share, playlists, insights, library, maintenance) return router } diff --git a/core/maintenance.go b/core/maintenance.go new file mode 100644 index 000000000..c2f65d74f --- /dev/null +++ b/core/maintenance.go @@ -0,0 +1,226 @@ +package core + +import ( + "context" + "fmt" + "slices" + "sync" + "time" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/slice" +) + +type Maintenance interface { + // DeleteMissingFiles deletes specific missing files by their IDs + DeleteMissingFiles(ctx context.Context, ids []string) error + // DeleteAllMissingFiles deletes all files marked as missing + DeleteAllMissingFiles(ctx context.Context) error +} + +type maintenanceService struct { + ds model.DataStore + wg sync.WaitGroup +} + +func NewMaintenance(ds model.DataStore) Maintenance { + return &maintenanceService{ + ds: ds, + } +} + +func (s *maintenanceService) DeleteMissingFiles(ctx context.Context, ids []string) error { + return s.deleteMissing(ctx, ids) +} + +func (s *maintenanceService) DeleteAllMissingFiles(ctx context.Context) error { + return s.deleteMissing(ctx, nil) +} + +// deleteMissing handles the deletion of missing files and triggers necessary cleanup operations +func (s *maintenanceService) deleteMissing(ctx context.Context, ids []string) error { + // Track affected album IDs before deletion for refresh + affectedAlbumIDs, err := s.getAffectedAlbumIDs(ctx, ids) + if err != nil { + log.Warn(ctx, "Error tracking affected albums for refresh", err) + // Don't fail the operation, just log the warning + } + + // Delete missing files within a transaction + err = s.ds.WithTx(func(tx model.DataStore) error { + if len(ids) == 0 { + _, err := tx.MediaFile(ctx).DeleteAllMissing() + return err + } + return tx.MediaFile(ctx).DeleteMissing(ids) + }) + if err != nil { + log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) + return err + } + + // Run garbage collection to clean up orphaned records + if err := s.ds.GC(ctx); err != nil { + log.Error(ctx, "Error running GC after deleting missing tracks", err) + return err + } + + // Refresh statistics in background + s.refreshStatsAsync(ctx, affectedAlbumIDs) + + return nil +} + +// refreshAlbums recalculates album attributes (size, duration, song count, etc.) from media files. +// It uses batch queries to minimize database round-trips for efficiency. +func (s *maintenanceService) refreshAlbums(ctx context.Context, albumIDs []string) error { + if len(albumIDs) == 0 { + return nil + } + + log.Debug(ctx, "Refreshing albums", "count", len(albumIDs)) + + // Process in chunks to avoid query size limits + const chunkSize = 100 + for chunk := range slice.CollectChunks(slices.Values(albumIDs), chunkSize) { + if err := s.refreshAlbumChunk(ctx, chunk); err != nil { + return fmt.Errorf("refreshing album chunk: %w", err) + } + } + + log.Debug(ctx, "Successfully refreshed albums", "count", len(albumIDs)) + return nil +} + +// refreshAlbumChunk processes a single chunk of album IDs +func (s *maintenanceService) refreshAlbumChunk(ctx context.Context, albumIDs []string) error { + albumRepo := s.ds.Album(ctx) + mfRepo := s.ds.MediaFile(ctx) + + // Batch load existing albums + albums, err := albumRepo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"album.id": albumIDs}, + }) + if err != nil { + return fmt.Errorf("loading albums: %w", err) + } + + // Create a map for quick lookup + albumMap := make(map[string]*model.Album, len(albums)) + for i := range albums { + albumMap[albums[i].ID] = &albums[i] + } + + // Batch load all media files for these albums + mediaFiles, err := mfRepo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"album_id": albumIDs}, + Sort: "album_id, path", + }) + if err != nil { + return fmt.Errorf("loading media files: %w", err) + } + + // Group media files by album ID + filesByAlbum := make(map[string]model.MediaFiles) + for i := range mediaFiles { + albumID := mediaFiles[i].AlbumID + filesByAlbum[albumID] = append(filesByAlbum[albumID], mediaFiles[i]) + } + + // Recalculate each album from its media files + for albumID, oldAlbum := range albumMap { + mfs, hasTracks := filesByAlbum[albumID] + if !hasTracks { + // Album has no tracks anymore, skip (will be cleaned up by GC) + log.Debug(ctx, "Skipping album with no tracks", "albumID", albumID) + continue + } + + // Recalculate album from media files + newAlbum := mfs.ToAlbum() + + // Only update if something changed (avoid unnecessary writes) + if !oldAlbum.Equals(newAlbum) { + // Preserve original timestamps + newAlbum.UpdatedAt = time.Now() + newAlbum.CreatedAt = oldAlbum.CreatedAt + + if err := albumRepo.Put(&newAlbum); err != nil { + log.Error(ctx, "Error updating album during refresh", "albumID", albumID, err) + // Continue with other albums instead of failing entirely + continue + } + log.Trace(ctx, "Refreshed album", "albumID", albumID, "name", newAlbum.Name) + } + } + + return nil +} + +// getAffectedAlbumIDs returns distinct album IDs from missing media files +func (s *maintenanceService) getAffectedAlbumIDs(ctx context.Context, ids []string) ([]string, error) { + var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} + if len(ids) > 0 { + filters = squirrel.And{ + squirrel.Eq{"missing": true}, + squirrel.Eq{"id": ids}, + } + } + + mfs, err := s.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: filters, + }) + if err != nil { + return nil, err + } + + // Extract unique album IDs + albumIDMap := make(map[string]struct{}, len(mfs)) + for _, mf := range mfs { + if mf.AlbumID != "" { + albumIDMap[mf.AlbumID] = struct{}{} + } + } + + albumIDs := make([]string, 0, len(albumIDMap)) + for id := range albumIDMap { + albumIDs = append(albumIDs, id) + } + + return albumIDs, nil +} + +// refreshStatsAsync refreshes artist and album statistics in background goroutines +func (s *maintenanceService) refreshStatsAsync(ctx context.Context, affectedAlbumIDs []string) { + // Refresh artist stats in background + s.wg.Add(1) + go func() { + defer s.wg.Done() + bgCtx := request.AddValues(context.Background(), ctx) + if _, err := s.ds.Artist(bgCtx).RefreshStats(true); err != nil { + log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") + } + + // Refresh album stats in background if we have affected albums + if len(affectedAlbumIDs) > 0 { + if err := s.refreshAlbums(bgCtx, affectedAlbumIDs); err != nil { + log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) + } + } + }() +} + +// Wait waits for all background goroutines to complete. +// WARNING: This method is ONLY for testing. Never call this in production code. +// Calling Wait() in production will block until ALL background operations complete +// and may cause race conditions with new operations starting. +func (s *maintenanceService) wait() { + s.wg.Wait() +} diff --git a/core/maintenance_test.go b/core/maintenance_test.go new file mode 100644 index 000000000..8e8796ffa --- /dev/null +++ b/core/maintenance_test.go @@ -0,0 +1,382 @@ +package core + +import ( + "context" + "errors" + "sync" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/sirupsen/logrus" +) + +var _ = Describe("Maintenance", func() { + var ds *extendedDataStore + var mfRepo *extendedMediaFileRepo + var service Maintenance + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + ctx = request.WithUser(ctx, model.User{ID: "user1", IsAdmin: true}) + + ds = createTestDataStore() + mfRepo = ds.MockedMediaFile.(*extendedMediaFileRepo) + service = NewMaintenance(ds) + }) + + Describe("DeleteMissingFiles", func() { + Context("with specific IDs", func() { + It("deletes specific missing files and runs GC", func() { + // Setup: mock missing files with album IDs + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + }) + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(mfRepo.deleteMissingCalled).To(BeTrue()) + Expect(mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) + Expect(ds.gcCalled).To(BeTrue(), "GC should be called after deletion") + }) + + It("triggers artist stats refresh and album refresh after deletion", func() { + artistRepo := ds.MockedArtist.(*extendedArtistRepo) + // Setup: mock missing files with albums + albumRepo := ds.MockedAlbum.(*extendedAlbumRepo) + albumRepo.SetData(model.Albums{ + {ID: "album1", Name: "Test Album", SongCount: 5}, + }) + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: false, Size: 1000, Duration: 180}, + {ID: "mf3", AlbumID: "album1", Missing: false, Size: 2000, Duration: 200}, + }) + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).ToNot(HaveOccurred()) + + // Wait for background goroutines to complete + service.(*maintenanceService).wait() + + // RefreshStats should be called + Expect(artistRepo.IsRefreshStatsCalled()).To(BeTrue(), "Artist stats should be refreshed") + + // Album should be updated with new calculated values + Expect(albumRepo.GetPutCallCount()).To(BeNumerically(">", 0), "Album.Put() should be called to refresh album data") + }) + + It("returns error if deletion fails", func() { + mfRepo.deleteMissingError = errors.New("delete failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("delete failed")) + }) + + It("continues even if album tracking fails", func() { + mfRepo.SetError(true) + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + // Should not fail, just log warning + Expect(err).ToNot(HaveOccurred()) + Expect(mfRepo.deleteMissingCalled).To(BeTrue()) + }) + + It("returns error if GC fails", func() { + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + }) + + // Set GC to return error + ds.gcError = errors.New("gc failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("gc failed")) + }) + }) + + Context("album ID extraction", func() { + It("extracts unique album IDs from missing files", func() { + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + {ID: "mf3", AlbumID: "album2", Missing: true}, + }) + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2", "mf3"}) + + Expect(err).ToNot(HaveOccurred()) + }) + + It("skips files without album IDs", func() { + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + }) + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) + + Describe("DeleteAllMissingFiles", func() { + It("deletes all missing files and runs GC", func() { + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + {ID: "mf3", AlbumID: "album3", Missing: true}, + }) + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.gcCalled).To(BeTrue(), "GC should be called after deletion") + }) + + It("returns error if deletion fails", func() { + mfRepo.SetError(true) + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).To(HaveOccurred()) + }) + + It("handles empty result gracefully", func() { + mfRepo.SetData(model.MediaFiles{}) + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Describe("Album refresh logic", func() { + var albumRepo *extendedAlbumRepo + + BeforeEach(func() { + albumRepo = ds.MockedAlbum.(*extendedAlbumRepo) + }) + + Context("when album has no tracks after deletion", func() { + It("skips the album without updating it", func() { + // Setup album with no remaining tracks + albumRepo.SetData(model.Albums{ + {ID: "album1", Name: "Empty Album", SongCount: 1}, + }) + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + }) + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).ToNot(HaveOccurred()) + + // Wait for background goroutines to complete + service.(*maintenanceService).wait() + + // Album should NOT be updated because it has no tracks left + Expect(albumRepo.GetPutCallCount()).To(Equal(0), "Album with no tracks should not be updated") + }) + }) + + Context("when Put fails for one album", func() { + It("continues processing other albums", func() { + albumRepo.SetData(model.Albums{ + {ID: "album1", Name: "Album 1"}, + {ID: "album2", Name: "Album 2"}, + }) + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: false, Size: 1000, Duration: 180}, + {ID: "mf3", AlbumID: "album2", Missing: true}, + {ID: "mf4", AlbumID: "album2", Missing: false, Size: 2000, Duration: 200}, + }) + + // Make Put fail on first call but succeed on subsequent calls + albumRepo.putError = errors.New("put failed") + albumRepo.failOnce = true + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf3"}) + + // Should not fail even if one album's Put fails + Expect(err).ToNot(HaveOccurred()) + + // Wait for background goroutines to complete + service.(*maintenanceService).wait() + + // Put should have been called multiple times + Expect(albumRepo.GetPutCallCount()).To(BeNumerically(">", 0), "Put should be attempted") + }) + }) + + Context("when media file loading fails", func() { + It("logs warning but continues when tracking affected albums fails", func() { + // Set up log capturing + hook, cleanup := tests.LogHook() + defer cleanup() + + albumRepo.SetData(model.Albums{ + {ID: "album1", Name: "Album 1"}, + }) + mfRepo.SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + }) + // Make GetAll fail when loading media files + mfRepo.SetError(true) + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + // Deletion should succeed despite the tracking error + Expect(err).ToNot(HaveOccurred()) + Expect(mfRepo.deleteMissingCalled).To(BeTrue()) + + // Verify the warning was logged + Expect(hook.LastEntry()).ToNot(BeNil()) + Expect(hook.LastEntry().Level).To(Equal(logrus.WarnLevel)) + Expect(hook.LastEntry().Message).To(Equal("Error tracking affected albums for refresh")) + }) + }) + }) +}) + +// Test helper to create a mock DataStore with controllable behavior +func createTestDataStore() *extendedDataStore { + // Create extended datastore with GC tracking + ds := &extendedDataStore{ + MockDataStore: &tests.MockDataStore{}, + } + + // Create extended album repo with Put tracking + albumRepo := &extendedAlbumRepo{ + MockAlbumRepo: tests.CreateMockAlbumRepo(), + } + ds.MockedAlbum = albumRepo + + // Create extended artist repo with RefreshStats tracking + artistRepo := &extendedArtistRepo{ + MockArtistRepo: tests.CreateMockArtistRepo(), + } + ds.MockedArtist = artistRepo + + // Create extended media file repo with DeleteMissing support + mfRepo := &extendedMediaFileRepo{ + MockMediaFileRepo: tests.CreateMockMediaFileRepo(), + } + ds.MockedMediaFile = mfRepo + + return ds +} + +// Extension of MockMediaFileRepo to add DeleteMissing method +type extendedMediaFileRepo struct { + *tests.MockMediaFileRepo + deleteMissingCalled bool + deletedIDs []string + deleteMissingError error +} + +func (m *extendedMediaFileRepo) DeleteMissing(ids []string) error { + m.deleteMissingCalled = true + m.deletedIDs = ids + if m.deleteMissingError != nil { + return m.deleteMissingError + } + // Actually delete from the mock data + for _, id := range ids { + delete(m.Data, id) + } + return nil +} + +// Extension of MockAlbumRepo to track Put calls +type extendedAlbumRepo struct { + *tests.MockAlbumRepo + mu sync.RWMutex + putCallCount int + lastPutData *model.Album + putError error + failOnce bool +} + +func (m *extendedAlbumRepo) Put(album *model.Album) error { + m.mu.Lock() + m.putCallCount++ + m.lastPutData = album + + // Handle failOnce behavior + var err error + if m.putError != nil { + if m.failOnce { + err = m.putError + m.putError = nil // Clear error after first failure + m.mu.Unlock() + return err + } + err = m.putError + m.mu.Unlock() + return err + } + m.mu.Unlock() + + return m.MockAlbumRepo.Put(album) +} + +func (m *extendedAlbumRepo) GetPutCallCount() int { + m.mu.RLock() + defer m.mu.RUnlock() + return m.putCallCount +} + +// Extension of MockArtistRepo to track RefreshStats calls +type extendedArtistRepo struct { + *tests.MockArtistRepo + mu sync.RWMutex + refreshStatsCalled bool + refreshStatsError error +} + +func (m *extendedArtistRepo) RefreshStats(allArtists bool) (int64, error) { + m.mu.Lock() + m.refreshStatsCalled = true + err := m.refreshStatsError + m.mu.Unlock() + + if err != nil { + return 0, err + } + return m.MockArtistRepo.RefreshStats(allArtists) +} + +func (m *extendedArtistRepo) IsRefreshStatsCalled() bool { + m.mu.RLock() + defer m.mu.RUnlock() + return m.refreshStatsCalled +} + +// Extension of MockDataStore to track GC calls +type extendedDataStore struct { + *tests.MockDataStore + gcCalled bool + gcError error +} + +func (ds *extendedDataStore) GC(ctx context.Context) error { + ds.gcCalled = true + if ds.gcError != nil { + return ds.gcError + } + return ds.MockDataStore.GC(ctx) +} diff --git a/core/wire_providers.go b/core/wire_providers.go index ae365156a..16335645c 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -18,6 +18,7 @@ var Set = wire.NewSet( NewShare, NewPlaylists, NewLibrary, + NewMaintenance, agents.GetAgents, external.NewProvider, wire.Bind(new(external.Agents), new(*agents.Agents)), diff --git a/log/log.go b/log/log.go index 20119ab46..ea34e5dcb 100644 --- a/log/log.go +++ b/log/log.go @@ -11,6 +11,7 @@ import ( "runtime" "sort" "strings" + "sync" "time" "github.com/sirupsen/logrus" @@ -70,6 +71,7 @@ type levelPath struct { var ( currentLevel Level + loggerMu sync.RWMutex defaultLogger = logrus.New() logSourceLine = false rootPath string @@ -79,7 +81,9 @@ var ( // SetLevel sets the global log level used by the simple logger. func SetLevel(l Level) { currentLevel = l + loggerMu.Lock() defaultLogger.Level = logrus.TraceLevel + loggerMu.Unlock() logrus.SetLevel(logrus.Level(l)) } @@ -125,6 +129,8 @@ func SetLogSourceLine(enabled bool) { func SetRedacting(enabled bool) { if enabled { + loggerMu.Lock() + defer loggerMu.Unlock() defaultLogger.AddHook(redacted) } } @@ -133,6 +139,8 @@ func SetOutput(w io.Writer) { if runtime.GOOS == "windows" { w = CRLFWriter(w) } + loggerMu.Lock() + defer loggerMu.Unlock() defaultLogger.SetOutput(w) } @@ -158,6 +166,8 @@ func NewContext(ctx context.Context, keyValuePairs ...interface{}) context.Conte } func SetDefaultLogger(l *logrus.Logger) { + loggerMu.Lock() + defer loggerMu.Unlock() defaultLogger = l } @@ -204,6 +214,8 @@ func log(level Level, args ...interface{}) { } func Writer() io.Writer { + loggerMu.RLock() + defer loggerMu.RUnlock() return defaultLogger.Writer() } @@ -314,6 +326,8 @@ func extractLogger(ctx interface{}) (*logrus.Entry, error) { func createNewLogger() *logrus.Entry { //logrus.SetFormatter(&logrus.TextFormatter{ForceColors: true, DisableTimestamp: false, FullTimestamp: true}) //l.Formatter = &logrus.TextFormatter{ForceColors: true, DisableTimestamp: false, FullTimestamp: true} + loggerMu.RLock() + defer loggerMu.RUnlock() logger := logrus.NewEntry(defaultLogger) return logger } diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 60f7c3394..d9c722955 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -29,7 +29,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, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library.go b/server/nativeapi/library.go index f081eca78..1636e1dbb 100644 --- a/server/nativeapi/library.go +++ b/server/nativeapi/library.go @@ -13,11 +13,11 @@ import ( ) // User-library association endpoints (admin only) -func (n *Router) addUserLibraryRoute(r chi.Router) { +func (api *Router) addUserLibraryRoute(r chi.Router) { r.Route("/user/{id}/library", func(r chi.Router) { r.Use(parseUserIDMiddleware) - r.Get("/", getUserLibraries(n.libs)) - r.Put("/", setUserLibraries(n.libs)) + r.Get("/", getUserLibraries(api.libs)) + r.Put("/", setUserLibraries(api.libs)) }) } diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index 4e6d34582..950338492 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -30,7 +30,7 @@ var _ = Describe("Library API", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/missing.go b/server/nativeapi/missing.go index 0d311f492..2b455e622 100644 --- a/server/nativeapi/missing.go +++ b/server/nativeapi/missing.go @@ -8,9 +8,9 @@ import ( "github.com/Masterminds/squirrel" "github.com/deluan/rest" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/req" ) @@ -63,45 +63,32 @@ func (r *missingRepository) EntityName() string { return "missing_files" } -func deleteMissingFiles(ds model.DataStore, w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - p := req.Params(r) - ids, _ := p.Strings("id") - err := ds.WithTx(func(tx model.DataStore) error { +func deleteMissingFiles(maintenance core.Maintenance) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + p := req.Params(r) + ids, _ := p.Strings("id") + + var err error if len(ids) == 0 { - _, err := tx.MediaFile(ctx).DeleteAllMissing() - return err - } - return tx.MediaFile(ctx).DeleteMissing(ids) - }) - if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { - log.Warn(ctx, "Missing file not found", "id", ids[0]) - http.Error(w, "not found", http.StatusNotFound) - return - } - if err != nil { - log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - err = ds.GC(ctx) - if err != nil { - log.Error(ctx, "Error running GC after deleting missing tracks", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - // Refresh artist stats in background after deleting missing files - go func() { - bgCtx := request.AddValues(context.Background(), r.Context()) - if _, err := ds.Artist(bgCtx).RefreshStats(true); err != nil { - log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) + err = maintenance.DeleteAllMissingFiles(ctx) } else { - log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") + err = maintenance.DeleteMissingFiles(ctx, ids) } - }() - writeDeleteManyResponse(w, r, ids) + if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Missing file not found", "id", ids[0]) + http.Error(w, "not found", http.StatusNotFound) + return + } + if err != nil { + http.Error(w, "failed to delete missing files", http.StatusInternalServerError) + return + } + + writeDeleteManyResponse(w, r, ids) + } } var _ model.ResourceRepository = &missingRepository{} diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 370bdbd1e..969650e0a 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -22,70 +22,71 @@ import ( type Router struct { http.Handler - ds model.DataStore - share core.Share - playlists core.Playlists - insights metrics.Insights - libs core.Library + ds model.DataStore + share core.Share + playlists core.Playlists + insights metrics.Insights + libs core.Library + maintenance core.Maintenance } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService} +func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, maintenance core.Maintenance) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, maintenance: maintenance} r.Handler = r.routes() return r } -func (n *Router) routes() http.Handler { +func (api *Router) routes() http.Handler { r := chi.NewRouter() // Public - n.RX(r, "/translation", newTranslationRepository, false) + api.RX(r, "/translation", newTranslationRepository, false) // Protected r.Group(func(r chi.Router) { - r.Use(server.Authenticator(n.ds)) + r.Use(server.Authenticator(api.ds)) r.Use(server.JWTRefresher) - r.Use(server.UpdateLastAccessMiddleware(n.ds)) - n.R(r, "/user", model.User{}, true) - n.R(r, "/song", model.MediaFile{}, false) - n.R(r, "/album", model.Album{}, false) - n.R(r, "/artist", model.Artist{}, false) - n.R(r, "/genre", model.Genre{}, false) - n.R(r, "/player", model.Player{}, true) - n.R(r, "/transcoding", model.Transcoding{}, conf.Server.EnableTranscodingConfig) - n.R(r, "/radio", model.Radio{}, true) - n.R(r, "/tag", model.Tag{}, true) + r.Use(server.UpdateLastAccessMiddleware(api.ds)) + api.R(r, "/user", model.User{}, true) + api.R(r, "/song", model.MediaFile{}, false) + api.R(r, "/album", model.Album{}, false) + api.R(r, "/artist", model.Artist{}, false) + api.R(r, "/genre", model.Genre{}, false) + api.R(r, "/player", model.Player{}, true) + api.R(r, "/transcoding", model.Transcoding{}, conf.Server.EnableTranscodingConfig) + api.R(r, "/radio", model.Radio{}, true) + api.R(r, "/tag", model.Tag{}, true) if conf.Server.EnableSharing { - n.RX(r, "/share", n.share.NewRepository, true) + api.RX(r, "/share", api.share.NewRepository, true) } - n.addPlaylistRoute(r) - n.addPlaylistTrackRoute(r) - n.addSongPlaylistsRoute(r) - n.addQueueRoute(r) - n.addMissingFilesRoute(r) - n.addKeepAliveRoute(r) - n.addInsightsRoute(r) + api.addPlaylistRoute(r) + api.addPlaylistTrackRoute(r) + api.addSongPlaylistsRoute(r) + api.addQueueRoute(r) + api.addMissingFilesRoute(r) + api.addKeepAliveRoute(r) + api.addInsightsRoute(r) r.With(adminOnlyMiddleware).Group(func(r chi.Router) { - n.addInspectRoute(r) - n.addConfigRoute(r) - n.addUserLibraryRoute(r) - n.RX(r, "/library", n.libs.NewRepository, true) + api.addInspectRoute(r) + api.addConfigRoute(r) + api.addUserLibraryRoute(r) + api.RX(r, "/library", api.libs.NewRepository, true) }) }) return r } -func (n *Router) R(r chi.Router, pathPrefix string, model interface{}, persistable bool) { +func (api *Router) R(r chi.Router, pathPrefix string, model interface{}, persistable bool) { constructor := func(ctx context.Context) rest.Repository { - return n.ds.Resource(ctx, model) + return api.ds.Resource(ctx, model) } - n.RX(r, pathPrefix, constructor, persistable) + api.RX(r, pathPrefix, constructor, persistable) } -func (n *Router) RX(r chi.Router, pathPrefix string, constructor rest.RepositoryConstructor, persistable bool) { +func (api *Router) RX(r chi.Router, pathPrefix string, constructor rest.RepositoryConstructor, persistable bool) { r.Route(pathPrefix, func(r chi.Router) { r.Get("/", rest.GetAll(constructor)) if persistable { @@ -102,9 +103,9 @@ func (n *Router) RX(r chi.Router, pathPrefix string, constructor rest.Repository }) } -func (n *Router) addPlaylistRoute(r chi.Router) { +func (api *Router) addPlaylistRoute(r chi.Router) { constructor := func(ctx context.Context) rest.Repository { - return n.ds.Resource(ctx, model.Playlist{}) + return api.ds.Resource(ctx, model.Playlist{}) } r.Route("/playlist", func(r chi.Router) { @@ -114,7 +115,7 @@ func (n *Router) addPlaylistRoute(r chi.Router) { rest.Post(constructor)(w, r) return } - createPlaylistFromM3U(n.playlists)(w, r) + createPlaylistFromM3U(api.playlists)(w, r) }) r.Route("/{id}", func(r chi.Router) { @@ -126,55 +127,53 @@ func (n *Router) addPlaylistRoute(r chi.Router) { }) } -func (n *Router) addPlaylistTrackRoute(r chi.Router) { +func (api *Router) addPlaylistTrackRoute(r chi.Router) { r.Route("/playlist/{playlistId}/tracks", func(r chi.Router) { r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylist(n.ds)(w, r) + getPlaylist(api.ds)(w, r) }) r.With(server.URLParamsMiddleware).Route("/", func(r chi.Router) { r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(n.ds)(w, r) + deleteFromPlaylist(api.ds)(w, r) }) r.Post("/", func(w http.ResponseWriter, r *http.Request) { - addToPlaylist(n.ds)(w, r) + addToPlaylist(api.ds)(w, r) }) }) r.Route("/{id}", func(r chi.Router) { r.Use(server.URLParamsMiddleware) r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylistTrack(n.ds)(w, r) + getPlaylistTrack(api.ds)(w, r) }) r.Put("/", func(w http.ResponseWriter, r *http.Request) { - reorderItem(n.ds)(w, r) + reorderItem(api.ds)(w, r) }) r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(n.ds)(w, r) + deleteFromPlaylist(api.ds)(w, r) }) }) }) } -func (n *Router) addSongPlaylistsRoute(r chi.Router) { +func (api *Router) addSongPlaylistsRoute(r chi.Router) { r.With(server.URLParamsMiddleware).Get("/song/{id}/playlists", func(w http.ResponseWriter, r *http.Request) { - getSongPlaylists(n.ds)(w, r) + getSongPlaylists(api.ds)(w, r) }) } -func (n *Router) addQueueRoute(r chi.Router) { +func (api *Router) addQueueRoute(r chi.Router) { r.Route("/queue", func(r chi.Router) { - r.Get("/", getQueue(n.ds)) - r.Post("/", saveQueue(n.ds)) - r.Put("/", updateQueue(n.ds)) - r.Delete("/", clearQueue(n.ds)) + r.Get("/", getQueue(api.ds)) + r.Post("/", saveQueue(api.ds)) + r.Put("/", updateQueue(api.ds)) + r.Delete("/", clearQueue(api.ds)) }) } -func (n *Router) addMissingFilesRoute(r chi.Router) { +func (api *Router) addMissingFilesRoute(r chi.Router) { r.Route("/missing", func(r chi.Router) { - n.RX(r, "/", newMissingRepository(n.ds), false) - r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteMissingFiles(n.ds, w, r) - }) + api.RX(r, "/", newMissingRepository(api.ds), false) + r.Delete("/", deleteMissingFiles(api.maintenance)) }) } @@ -198,7 +197,7 @@ func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []strin } } -func (n *Router) addInspectRoute(r chi.Router) { +func (api *Router) addInspectRoute(r chi.Router) { if conf.Server.Inspect.Enabled { r.Group(func(r chi.Router) { if conf.Server.Inspect.MaxRequests > 0 { @@ -207,26 +206,26 @@ func (n *Router) addInspectRoute(r chi.Router) { conf.Server.Inspect.BacklogTimeout) r.Use(middleware.ThrottleBacklog(conf.Server.Inspect.MaxRequests, conf.Server.Inspect.BacklogLimit, time.Duration(conf.Server.Inspect.BacklogTimeout))) } - r.Get("/inspect", inspect(n.ds)) + r.Get("/inspect", inspect(api.ds)) }) } } -func (n *Router) addConfigRoute(r chi.Router) { +func (api *Router) addConfigRoute(r chi.Router) { if conf.Server.DevUIShowConfig { r.Get("/config/*", getConfig) } } -func (n *Router) addKeepAliveRoute(r chi.Router) { +func (api *Router) addKeepAliveRoute(r chi.Router) { r.Get("/keepalive/*", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"response":"ok", "id":"keepalive"}`)) }) } -func (n *Router) addInsightsRoute(r chi.Router) { +func (api *Router) addInsightsRoute(r chi.Router) { r.Get("/insights/*", func(w http.ResponseWriter, r *http.Request) { - last, success := n.insights.LastRun(r.Context()) + last, success := api.insights.LastRun(r.Context()) if conf.Server.EnableInsightsCollector { _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"` + last.Format("2006-01-02 15:04:05") + `", "success":` + strconv.FormatBool(success) + `}`)) } else { diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index d7209a164..b52042643 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -95,7 +95,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, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/tests/test_helpers.go b/tests/test_helpers.go index 1251c90cd..0a2cad4ad 100644 --- a/tests/test_helpers.go +++ b/tests/test_helpers.go @@ -6,7 +6,10 @@ import ( "path/filepath" "github.com/navidrome/navidrome/db" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model/id" + "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" ) type testingT interface { @@ -35,3 +38,23 @@ func ClearDB() error { `) return err } + +// LogHook sets up a logrus test hook and configures the default logger to use it. +// It returns the hook and a cleanup function to restore the default logger. +// Example usage: +// +// hook, cleanup := LogHook() +// defer cleanup() +// // ... perform logging operations ... +// Expect(hook.LastEntry()).ToNot(BeNil()) +// Expect(hook.LastEntry().Level).To(Equal(logrus.WarnLevel)) +// Expect(hook.LastEntry().Message).To(Equal("log message")) +func LogHook() (*test.Hook, func()) { + l, hook := test.NewNullLogger() + log.SetLevel(log.LevelWarn) + log.SetDefaultLogger(l) + return hook, func() { + // Restore default logger after test + log.SetDefaultLogger(logrus.New()) + } +}