From dd1d3907b41f6a126403f1df2cc7aa3769db61ee Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 21 May 2025 16:20:29 -0400 Subject: [PATCH 1/8] Revert "refactor(server): simplify lastfm agent initialization logic" This reverts commit 6f52c0201cdc6e92bf4e47394d79767db9c33640. Signed-off-by: Deluan --- core/agents/agents.go | 2 +- core/agents/lastfm/agent.go | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/core/agents/agents.go b/core/agents/agents.go index 9814c9f18..50a1e04ad 100644 --- a/core/agents/agents.go +++ b/core/agents/agents.go @@ -45,7 +45,7 @@ func createAgents(ds model.DataStore) *Agents { continue } enabled = append(enabled, name) - res = append(res, agent) + res = append(res, init(ds)) } log.Debug("List of agents enabled", "names", enabled) diff --git a/core/agents/lastfm/agent.go b/core/agents/lastfm/agent.go index 78ed96678..3f5f44d20 100644 --- a/core/agents/lastfm/agent.go +++ b/core/agents/lastfm/agent.go @@ -344,10 +344,22 @@ func (l *lastfmAgent) IsAuthorized(ctx context.Context, userId string) bool { func init() { conf.AddHook(func() { agents.Register(lastFMAgentName, func(ds model.DataStore) agents.Interface { - return lastFMConstructor(ds) + // This is a workaround for the fact that a (Interface)(nil) is not the same as a (*lastfmAgent)(nil) + // See https://go.dev/doc/faq#nil_error + a := lastFMConstructor(ds) + if a != nil { + return a + } + return nil }) scrobbler.Register(lastFMAgentName, func(ds model.DataStore) scrobbler.Scrobbler { - return lastFMConstructor(ds) + // Same as above - this is a workaround for the fact that a (Scrobbler)(nil) is not the same as a (*lastfmAgent)(nil) + // See https://go.dev/doc/faq#nil_error + a := lastFMConstructor(ds) + if a != nil { + return a + } + return nil }) }) } From 6731787053e6929cdf1c083ae389c71d47ac0365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 21 May 2025 21:48:49 -0400 Subject: [PATCH 2/8] fix(server): memory leak in cache warmer (#4095) * Prevent cache warmer memory leak when cache disabled * refactor(tests): replace disabledCache with mockFileCache in CacheWarmer tests Signed-off-by: Deluan * test(cache): enhance CacheWarmer tests for initialization, buffer management, and error handling Signed-off-by: Deluan --------- Signed-off-by: Deluan --- core/artwork/cache_warmer.go | 20 +++ core/artwork/cache_warmer_test.go | 216 ++++++++++++++++++++++++++++++ utils/cache/file_caches.go | 10 ++ utils/cache/file_caches_test.go | 7 + 4 files changed, 253 insertions(+) create mode 100644 core/artwork/cache_warmer_test.go diff --git a/core/artwork/cache_warmer.go b/core/artwork/cache_warmer.go index a95f968fc..2e60ca00b 100644 --- a/core/artwork/cache_warmer.go +++ b/core/artwork/cache_warmer.go @@ -31,6 +31,12 @@ func NewCacheWarmer(artwork Artwork, cache cache.FileCache) CacheWarmer { return &noopCacheWarmer{} } + // If the file cache is disabled, return a NOOP implementation + if cache.Disabled(context.Background()) { + log.Debug("Image cache disabled. Cache warmer will not run") + return &noopCacheWarmer{} + } + a := &cacheWarmer{ artwork: artwork, cache: cache, @@ -53,6 +59,9 @@ type cacheWarmer struct { } func (a *cacheWarmer) PreCache(artID model.ArtworkID) { + if a.cache.Disabled(context.Background()) { + return + } a.mutex.Lock() defer a.mutex.Unlock() a.buffer[artID] = struct{}{} @@ -74,6 +83,17 @@ func (a *cacheWarmer) run(ctx context.Context) { break } + if a.cache.Disabled(ctx) { + a.mutex.Lock() + pending := len(a.buffer) + a.buffer = make(map[model.ArtworkID]struct{}) + a.mutex.Unlock() + if pending > 0 { + log.Trace(ctx, "Cache disabled, discarding precache buffer", "bufferLen", pending) + } + return + } + // If cache not available, keep waiting if !a.cache.Available(ctx) { if len(a.buffer) > 0 { diff --git a/core/artwork/cache_warmer_test.go b/core/artwork/cache_warmer_test.go new file mode 100644 index 000000000..d35fb6e82 --- /dev/null +++ b/core/artwork/cache_warmer_test.go @@ -0,0 +1,216 @@ +package artwork + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "sync/atomic" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/cache" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("CacheWarmer", func() { + var ( + fc *mockFileCache + aw *mockArtwork + ) + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + fc = &mockFileCache{} + aw = &mockArtwork{} + }) + + Context("initialization", func() { + It("returns noop when cache is disabled", func() { + fc.SetDisabled(true) + cw := NewCacheWarmer(aw, fc) + _, ok := cw.(*noopCacheWarmer) + Expect(ok).To(BeTrue()) + }) + + It("returns noop when ImageCacheSize is 0", func() { + conf.Server.ImageCacheSize = "0" + cw := NewCacheWarmer(aw, fc) + _, ok := cw.(*noopCacheWarmer) + Expect(ok).To(BeTrue()) + }) + + It("returns noop when EnableArtworkPrecache is false", func() { + conf.Server.EnableArtworkPrecache = false + cw := NewCacheWarmer(aw, fc) + _, ok := cw.(*noopCacheWarmer) + Expect(ok).To(BeTrue()) + }) + + It("returns real implementation when properly configured", func() { + conf.Server.ImageCacheSize = "100MB" + conf.Server.EnableArtworkPrecache = true + fc.SetDisabled(false) + cw := NewCacheWarmer(aw, fc) + _, ok := cw.(*cacheWarmer) + Expect(ok).To(BeTrue()) + }) + }) + + Context("buffer management", func() { + BeforeEach(func() { + conf.Server.ImageCacheSize = "100MB" + conf.Server.EnableArtworkPrecache = true + fc.SetDisabled(false) + }) + + It("drops buffered items when cache becomes disabled", func() { + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + cw.PreCache(model.MustParseArtworkID("al-test")) + fc.SetDisabled(true) + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + }) + + It("adds multiple items to buffer", func() { + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + cw.PreCache(model.MustParseArtworkID("al-1")) + cw.PreCache(model.MustParseArtworkID("al-2")) + cw.mutex.Lock() + defer cw.mutex.Unlock() + Expect(len(cw.buffer)).To(Equal(2)) + }) + + It("deduplicates items in buffer", func() { + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + cw.PreCache(model.MustParseArtworkID("al-1")) + cw.PreCache(model.MustParseArtworkID("al-1")) + cw.mutex.Lock() + defer cw.mutex.Unlock() + Expect(len(cw.buffer)).To(Equal(1)) + }) + }) + + Context("error handling", func() { + BeforeEach(func() { + conf.Server.ImageCacheSize = "100MB" + conf.Server.EnableArtworkPrecache = true + fc.SetDisabled(false) + }) + + It("continues processing after artwork retrieval error", func() { + aw.err = errors.New("artwork error") + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + cw.PreCache(model.MustParseArtworkID("al-error")) + cw.PreCache(model.MustParseArtworkID("al-1")) + + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + }) + + It("continues processing after cache error", func() { + fc.err = errors.New("cache error") + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + cw.PreCache(model.MustParseArtworkID("al-error")) + cw.PreCache(model.MustParseArtworkID("al-1")) + + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + }) + }) + + Context("background processing", func() { + BeforeEach(func() { + conf.Server.ImageCacheSize = "100MB" + conf.Server.EnableArtworkPrecache = true + fc.SetDisabled(false) + }) + + It("processes items in batches", func() { + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + for i := 0; i < 5; i++ { + cw.PreCache(model.MustParseArtworkID(fmt.Sprintf("al-%d", i))) + } + + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + }) + + It("wakes up on new items", func() { + cw := NewCacheWarmer(aw, fc).(*cacheWarmer) + + // Add first batch + cw.PreCache(model.MustParseArtworkID("al-1")) + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + + // Add second batch + cw.PreCache(model.MustParseArtworkID("al-2")) + Eventually(func() int { + cw.mutex.Lock() + defer cw.mutex.Unlock() + return len(cw.buffer) + }).Should(Equal(0)) + }) + }) +}) + +type mockArtwork struct { + err error +} + +func (m *mockArtwork) Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (io.ReadCloser, time.Time, error) { + if m.err != nil { + return nil, time.Time{}, m.err + } + return io.NopCloser(strings.NewReader("test")), time.Now(), nil +} + +func (m *mockArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) { + return m.Get(ctx, model.ArtworkID{}, size, square) +} + +type mockFileCache struct { + disabled atomic.Bool + ready atomic.Bool + err error +} + +func (f *mockFileCache) Get(ctx context.Context, item cache.Item) (*cache.CachedStream, error) { + if f.err != nil { + return nil, f.err + } + return &cache.CachedStream{Reader: io.NopCloser(strings.NewReader("cached"))}, nil +} + +func (f *mockFileCache) Available(ctx context.Context) bool { + return f.ready.Load() && !f.disabled.Load() +} + +func (f *mockFileCache) Disabled(ctx context.Context) bool { + return f.disabled.Load() +} + +func (f *mockFileCache) SetDisabled(v bool) { + f.disabled.Store(v) + f.ready.Store(true) +} diff --git a/utils/cache/file_caches.go b/utils/cache/file_caches.go index a765fa2a4..5edc533f8 100644 --- a/utils/cache/file_caches.go +++ b/utils/cache/file_caches.go @@ -54,6 +54,9 @@ type FileCache interface { // Available checks if the cache is available Available(ctx context.Context) bool + + // Disabled reports if the cache has been permanently disabled + Disabled(ctx context.Context) bool } // NewFileCache creates a new FileCache. This function initializes the cache and starts it in the background. @@ -119,6 +122,13 @@ func (fc *fileCache) Available(_ context.Context) bool { return fc.ready.Load() && !fc.disabled } +func (fc *fileCache) Disabled(_ context.Context) bool { + fc.mutex.RLock() + defer fc.mutex.RUnlock() + + return fc.disabled +} + func (fc *fileCache) invalidate(ctx context.Context, key string) error { if !fc.Available(ctx) { log.Debug(ctx, "Cache not initialized yet. Cannot invalidate key", "cache", fc.name, "key", key) diff --git a/utils/cache/file_caches_test.go b/utils/cache/file_caches_test.go index 3679c79f3..72f4463d1 100644 --- a/utils/cache/file_caches_test.go +++ b/utils/cache/file_caches_test.go @@ -50,6 +50,13 @@ var _ = Describe("File Caches", func() { Expect(fc.cache).To(BeNil()) Expect(fc.disabled).To(BeTrue()) }) + + It("reports when cache is disabled", func() { + fc := callNewFileCache("test", "0", "test", 0, nil) + Expect(fc.Disabled(context.Background())).To(BeTrue()) + fc = callNewFileCache("test", "1KB", "test", 0, nil) + Expect(fc.Disabled(context.Background())).To(BeFalse()) + }) }) Describe("FileCache", func() { From 3953e3217dd30192d7b937989bc1bc6fc420f1f3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 21 May 2025 21:57:16 -0400 Subject: [PATCH 3/8] docs: add code guidelines for backend and frontend development Signed-off-by: Deluan --- .github/copilot-instructions.md | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..3eaa829f1 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,45 @@ +# Navidrome Code Guidelines + +This is a music streaming server written in Go with a React frontend. The application manages music libraries, provides streaming capabilities, and offers various features like artist information, artwork handling, and external service integrations. + +## Code Standards + +### Backend (Go) +- Follow standard Go conventions and idioms +- Use context propagation for cancellation signals +- Write unit tests for new functionality using Ginkgo/Gomega +- Use mutex appropriately for concurrent operations +- Implement interfaces for dependencies to facilitate testing + +### Frontend (React) +- Use functional components with hooks +- Follow React best practices for state management +- Implement PropTypes for component properties +- Prefer using React-Admin and Material-UI components +- Icons should be imported from `react-icons` only +- Follow existing patterns for API interaction + +## Repository Structure +- `core/`: Server-side business logic (artwork handling, playback, etc.) +- `ui/`: React frontend components +- `model/`: Data models and repository interfaces +- `server/`: API endpoints and server implementation +- `utils/`: Shared utility functions +- `persistence/`: Database access layer +- `scanner/`: Music library scanning functionality + +## Key Guidelines +1. Maintain cache management patterns for performance +2. Follow the existing concurrency patterns (mutex, atomic) +3. Use the testing framework appropriately (Ginkgo/Gomega for Go) +4. Keep UI components focused and reusable +5. Document configuration options in code +6. Consider performance implications when working with music libraries +7. Follow existing error handling patterns +8. Ensure compatibility with external services (LastFM, Spotify) + +## Development Workflow +- Test changes thoroughly, especially around concurrent operations +- Validate both backend and frontend interactions +- Consider how changes will affect user experience and performance +- Test with different music library sizes and configurations \ No newline at end of file From 6ac3acaaf84c2d09fe2348b511e844d3de540623 Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Thu, 22 May 2025 02:16:10 +0000 Subject: [PATCH 4/8] fix(db): allow deleting users that have shares (#4098) * fix(db): allow deleting users that have shares * remove placeholders --- ...20250522013904_share_user_id_on_delete.sql | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 db/migrations/20250522013904_share_user_id_on_delete.sql diff --git a/db/migrations/20250522013904_share_user_id_on_delete.sql b/db/migrations/20250522013904_share_user_id_on_delete.sql new file mode 100644 index 000000000..91ff653ac --- /dev/null +++ b/db/migrations/20250522013904_share_user_id_on_delete.sql @@ -0,0 +1,36 @@ +-- +goose Up +-- +goose StatementBegin +CREATE TABLE share_tmp +( + id varchar(255) not null + primary key, + expires_at datetime, + last_visited_at datetime, + resource_ids varchar not null, + created_at datetime, + updated_at datetime, + user_id varchar(255) not null + constraint share_user_id_fk + references user + on update cascade on delete cascade, + downloadable bool not null default false, + description varchar not null default '', + resource_type varchar not null default '', + contents varchar not null default '', + format varchar not null default '', + max_bit_rate integer not null default 0, + visit_count integer not null default 0 +); + + +INSERT INTO share_tmp( + id, expires_at, last_visited_at, resource_ids, created_at, updated_at, user_id, downloadable, description, resource_type, contents, format, max_bit_rate, visit_count +) SELECT id, expires_at, last_visited_at, resource_ids, created_at, updated_at, user_id, downloadable, description, resource_type, contents, format, max_bit_rate, visit_count +FROM share; + +DROP TABLE share; + +ALTER TABLE share_tmp RENAME To share; +-- +goose StatementEnd + +-- +goose Down From e5438552c63fecb6284e1b179dddae91ede869c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 21 May 2025 22:19:23 -0400 Subject: [PATCH 5/8] fix(transcoding): restrict transcoding operations to admin users (#4096) Signed-off-by: Deluan --- persistence/sql_base_repository.go | 5 ++ persistence/transcoding_repository.go | 12 +++ persistence/transcoding_repository_test.go | 96 ++++++++++++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 persistence/transcoding_repository_test.go diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index f8edff0b8..7cc24b6c4 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -65,6 +65,11 @@ func loggedUser(ctx context.Context) *model.User { } } +func isAdmin(ctx context.Context) bool { + user := loggedUser(ctx) + return user.IsAdmin +} + func (r *sqlRepository) registerModel(instance any, filters map[string]filterFunc) { if r.tableName == "" { r.tableName = strings.TrimPrefix(reflect.TypeOf(instance).String(), "*model.") diff --git a/persistence/transcoding_repository.go b/persistence/transcoding_repository.go index 9f8998b80..bdcbe7262 100644 --- a/persistence/transcoding_repository.go +++ b/persistence/transcoding_repository.go @@ -41,6 +41,9 @@ func (r *transcodingRepository) FindByFormat(format string) (*model.Transcoding, } func (r *transcodingRepository) Put(t *model.Transcoding) error { + if !isAdmin(r.ctx) { + return rest.ErrPermissionDenied + } _, err := r.put(t.ID, t) return err } @@ -69,6 +72,9 @@ func (r *transcodingRepository) NewInstance() interface{} { } func (r *transcodingRepository) Save(entity interface{}) (string, error) { + if !isAdmin(r.ctx) { + return "", rest.ErrPermissionDenied + } t := entity.(*model.Transcoding) id, err := r.put(t.ID, t) if errors.Is(err, model.ErrNotFound) { @@ -78,6 +84,9 @@ func (r *transcodingRepository) Save(entity interface{}) (string, error) { } func (r *transcodingRepository) Update(id string, entity interface{}, cols ...string) error { + if !isAdmin(r.ctx) { + return rest.ErrPermissionDenied + } t := entity.(*model.Transcoding) t.ID = id _, err := r.put(id, t) @@ -88,6 +97,9 @@ func (r *transcodingRepository) Update(id string, entity interface{}, cols ...st } func (r *transcodingRepository) Delete(id string) error { + if !isAdmin(r.ctx) { + return rest.ErrPermissionDenied + } err := r.delete(Eq{"id": id}) if errors.Is(err, model.ErrNotFound) { return rest.ErrNotFound diff --git a/persistence/transcoding_repository_test.go b/persistence/transcoding_repository_test.go new file mode 100644 index 000000000..eddc5047a --- /dev/null +++ b/persistence/transcoding_repository_test.go @@ -0,0 +1,96 @@ +package persistence + +import ( + "github.com/deluan/rest" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("TranscodingRepository", func() { + var repo model.TranscodingRepository + var adminRepo model.TranscodingRepository + + BeforeEach(func() { + ctx := log.NewContext(GinkgoT().Context()) + ctx = request.WithUser(ctx, regularUser) + repo = NewTranscodingRepository(ctx, GetDBXBuilder()) + + adminCtx := log.NewContext(GinkgoT().Context()) + adminCtx = request.WithUser(adminCtx, adminUser) + adminRepo = NewTranscodingRepository(adminCtx, GetDBXBuilder()) + }) + + AfterEach(func() { + // Clean up any transcoding created during the tests + tc, err := adminRepo.FindByFormat("test_format") + if err == nil { + err = adminRepo.(*transcodingRepository).Delete(tc.ID) + Expect(err).ToNot(HaveOccurred()) + } + }) + + Describe("Admin User", func() { + It("creates a new transcoding", func() { + base, err := adminRepo.CountAll() + Expect(err).ToNot(HaveOccurred()) + + err = adminRepo.Put(&model.Transcoding{ID: "new", Name: "new", TargetFormat: "test_format", DefaultBitRate: 320, Command: "ffmpeg"}) + Expect(err).ToNot(HaveOccurred()) + + count, err := adminRepo.CountAll() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(base + 1)) + }) + + It("updates an existing transcoding", func() { + tr := &model.Transcoding{ID: "upd", Name: "old", TargetFormat: "test_format", DefaultBitRate: 100, Command: "ffmpeg"} + Expect(adminRepo.Put(tr)).To(Succeed()) + tr.Name = "updated" + err := adminRepo.Put(tr) + Expect(err).ToNot(HaveOccurred()) + res, err := adminRepo.FindByFormat("test_format") + Expect(err).ToNot(HaveOccurred()) + Expect(res.Name).To(Equal("updated")) + }) + + It("deletes a transcoding", func() { + err := adminRepo.Put(&model.Transcoding{ID: "to-delete", Name: "temp", TargetFormat: "test_format", DefaultBitRate: 256, Command: "ffmpeg"}) + Expect(err).ToNot(HaveOccurred()) + err = adminRepo.(*transcodingRepository).Delete("to-delete") + Expect(err).ToNot(HaveOccurred()) + _, err = adminRepo.Get("to-delete") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + + Describe("Regular User", func() { + It("fails to create", func() { + err := repo.Put(&model.Transcoding{ID: "bad", Name: "bad", TargetFormat: "test_format", DefaultBitRate: 64, Command: "ffmpeg"}) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("fails to update", func() { + tr := &model.Transcoding{ID: "updreg", Name: "old", TargetFormat: "test_format", DefaultBitRate: 64, Command: "ffmpeg"} + Expect(adminRepo.Put(tr)).To(Succeed()) + + tr.Name = "bad" + err := repo.Put(tr) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + + //_ = adminRepo.(*transcodingRepository).Delete("updreg") + }) + + It("fails to delete", func() { + tr := &model.Transcoding{ID: "delreg", Name: "temp", TargetFormat: "test_format", DefaultBitRate: 64, Command: "ffmpeg"} + Expect(adminRepo.Put(tr)).To(Succeed()) + + err := repo.(*transcodingRepository).Delete("delreg") + Expect(err).To(Equal(rest.ErrPermissionDenied)) + + //_ = adminRepo.(*transcodingRepository).Delete("delreg") + }) + }) +}) From 84384006a415f4de0ac98e7824ea5d77687cb87b Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 21 May 2025 22:33:33 -0400 Subject: [PATCH 6/8] docs: update copilot instructions with important commands and linting guidelines Signed-off-by: Deluan --- .github/copilot-instructions.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 3eaa829f1..64a01c9ae 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -42,4 +42,12 @@ This is a music streaming server written in Go with a React frontend. The applic - Test changes thoroughly, especially around concurrent operations - Validate both backend and frontend interactions - Consider how changes will affect user experience and performance -- Test with different music library sizes and configurations \ No newline at end of file +- Test with different music library sizes and configurations +- Always run formatting and linting before committing changes + +## Important commands +- `make build`: Build the application +- `make test`: Run Go tests +- To run tests for a specific package, use `make test PKG=./pkgname/...` +- `make lintall`: Run linters +- `make format`: Format code \ No newline at end of file From eb944bd261a1510ee7e7126813d42cf8936c9e64 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 21 May 2025 23:13:32 -0400 Subject: [PATCH 7/8] chore: update Makefile to install golangci-lint if not present and adjust lint command Signed-off-by: Deluan --- Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 38f1ca9ed..c2a81bde9 100644 --- a/Makefile +++ b/Makefile @@ -49,8 +49,12 @@ testall: testrace ##@Development Run Go and JS tests @(cd ./ui && npm run test:ci) .PHONY: testall -lint: ##@Development Lint Go code - go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest run -v --timeout 5m +install-golangci-lint: ##@Development Install golangci-lint if not present + @which golangci-lint > /dev/null || (echo "Installing golangci-lint..." && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s v2.1.6) +.PHONY: install-golangci-lint + +lint: install-golangci-lint ##@Development Lint Go code + PATH=$$PATH:./bin golangci-lint run -v --timeout 5m .PHONY: lint lintall: lint ##@Development Lint Go and JS code From 98fdc42d097300f9c32a10437851c5516e564bed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 22 May 2025 15:48:24 -0400 Subject: [PATCH 8/8] test: fix ignored artwork tests (#4103) * Fix artwork internal tests * fix: rename artistReader functions to artistArtworkReader for clarity Signed-off-by: Deluan * fix: update artwork internal tests to handle corrupted cover scenarios Signed-off-by: Deluan --------- Signed-off-by: Deluan --- core/artwork/artwork.go | 2 +- core/artwork/artwork_internal_test.go | 119 +++++++++++++++++--------- core/artwork/reader_artist.go | 2 +- core/artwork/reader_artist_test.go | 2 +- 4 files changed, 80 insertions(+), 45 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index f1f571e0d..2e92b24c8 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -115,7 +115,7 @@ func (a *artwork) getArtworkReader(ctx context.Context, artID model.ArtworkID, s } else { switch artID.Kind { case model.KindArtistArtwork: - artReader, err = newArtistReader(ctx, a, artID, a.provider) + artReader, err = newArtistArtworkReader(ctx, a, artID, a.provider) case model.KindAlbumArtwork: artReader, err = newAlbumArtworkReader(ctx, a, artID, a.provider) case model.KindMediaFileArtwork: diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index 462027082..cfb7850bd 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -4,7 +4,11 @@ import ( "context" "errors" "image" + "image/jpeg" + "image/png" "io" + "os" + "path/filepath" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -15,11 +19,11 @@ import ( . "github.com/onsi/gomega" ) -// TODO Fix tests -var _ = XDescribe("Artwork", func() { +var _ = Describe("Artwork", func() { var aw *artwork var ds model.DataStore var ffmpeg *tests.MockFFmpeg + var folderRepo *fakeFolderRepo ctx := log.NewContext(context.TODO()) var alOnlyEmbed, alEmbedNotFound, alOnlyExternal, alExternalNotFound, alMultipleCovers model.Album var arMultipleCovers model.Artist @@ -30,20 +34,21 @@ var _ = XDescribe("Artwork", func() { conf.Server.ImageCacheSize = "0" // Disable cache conf.Server.CoverArtPriority = "folder.*, cover.*, embedded , front.*" - ds = &tests.MockDataStore{MockedTranscoding: &tests.MockTranscodingRepo{}} - alOnlyEmbed = model.Album{ID: "222", Name: "Only embed", EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3"} - alEmbedNotFound = model.Album{ID: "333", Name: "Embed not found", EmbedArtPath: "tests/fixtures/NON_EXISTENT.mp3"} - //alOnlyExternal = model.Album{ID: "444", Name: "Only external", ImageFiles: "tests/fixtures/artist/an-album/front.png"} - //alExternalNotFound = model.Album{ID: "555", Name: "External not found", ImageFiles: "tests/fixtures/NON_EXISTENT.png"} + folderRepo = &fakeFolderRepo{} + ds = &tests.MockDataStore{ + MockedTranscoding: &tests.MockTranscodingRepo{}, + MockedFolder: folderRepo, + } + alOnlyEmbed = model.Album{ID: "222", Name: "Only embed", EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", FolderIDs: []string{"f1"}} + alEmbedNotFound = model.Album{ID: "333", Name: "Embed not found", EmbedArtPath: "tests/fixtures/NON_EXISTENT.mp3", FolderIDs: []string{"f1"}} + alOnlyExternal = model.Album{ID: "444", Name: "Only external", FolderIDs: []string{"f1"}} + alExternalNotFound = model.Album{ID: "555", Name: "External not found", FolderIDs: []string{"f2"}} arMultipleCovers = model.Artist{ID: "777", Name: "All options"} alMultipleCovers = model.Album{ - ID: "666", - Name: "All options", - EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", - //Paths: []string{"tests/fixtures/artist/an-album"}, - //ImageFiles: "tests/fixtures/artist/an-album/cover.jpg" + consts.Zwsp + - // "tests/fixtures/artist/an-album/front.png" + consts.Zwsp + - // "tests/fixtures/artist/an-album/artist.png", + ID: "666", + Name: "All options", + EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", + FolderIDs: []string{"f1"}, AlbumArtistID: "777", } mfWithEmbed = model.MediaFile{ID: "22", Path: "tests/fixtures/test.mp3", HasCoverArt: true, AlbumID: "222"} @@ -65,6 +70,7 @@ var _ = XDescribe("Artwork", func() { }) Context("Embed images", func() { BeforeEach(func() { + folderRepo.result = nil ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alOnlyEmbed, alEmbedNotFound, @@ -87,12 +93,17 @@ var _ = XDescribe("Artwork", func() { }) Context("External images", func() { BeforeEach(func() { + folderRepo.result = []model.Folder{} ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alOnlyExternal, alExternalNotFound, }) }) It("returns external cover", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"front.png"}, + }} aw, err := newAlbumArtworkReader(ctx, aw, alOnlyExternal.CoverArtID(), nil) Expect(err).ToNot(HaveOccurred()) _, path, err := aw.Reader(ctx) @@ -100,6 +111,7 @@ var _ = XDescribe("Artwork", func() { Expect(path).To(Equal("tests/fixtures/artist/an-album/front.png")) }) It("returns ErrUnavailable if external file is not available", func() { + folderRepo.result = []model.Folder{} aw, err := newAlbumArtworkReader(ctx, aw, alExternalNotFound.CoverArtID(), nil) Expect(err).ToNot(HaveOccurred()) _, _, err = aw.Reader(ctx) @@ -108,6 +120,10 @@ var _ = XDescribe("Artwork", func() { }) Context("Multiple covers", func() { BeforeEach(func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg", "front.png", "artist.png"}, + }} ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alMultipleCovers, }) @@ -130,6 +146,10 @@ var _ = XDescribe("Artwork", func() { Describe("artistArtworkReader", func() { Context("Multiple covers", func() { BeforeEach(func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"artist.png"}, + }} ds.Artist(ctx).(*tests.MockArtistRepo).SetData(model.Artists{ arMultipleCovers, }) @@ -143,7 +163,7 @@ var _ = XDescribe("Artwork", func() { DescribeTable("ArtistArtPriority", func(priority string, expected string) { conf.Server.ArtistArtPriority = priority - aw, err := newArtistReader(ctx, aw, arMultipleCovers.CoverArtID(), nil) + aw, err := newArtistArtworkReader(ctx, aw, arMultipleCovers.CoverArtID(), nil) Expect(err).ToNot(HaveOccurred()) _, path, err := aw.Reader(ctx) Expect(err).ToNot(HaveOccurred()) @@ -157,12 +177,16 @@ var _ = XDescribe("Artwork", func() { Describe("mediafileArtworkReader", func() { Context("ID not found", func() { It("returns ErrNotFound if mediafile is not in the DB", func() { - _, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID(), nil) + _, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-NOT-FOUND")) Expect(err).To(MatchError(model.ErrNotFound)) }) }) Context("Embed images", func() { BeforeEach(func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"front.png"}, + }} ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alOnlyEmbed, alOnlyExternal, @@ -185,11 +209,17 @@ var _ = XDescribe("Artwork", func() { Expect(err).ToNot(HaveOccurred()) r, path, err := aw.Reader(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(io.ReadAll(r)).To(Equal([]byte("content from ffmpeg"))) + data, _ := io.ReadAll(r) + Expect(data).ToNot(BeEmpty()) Expect(path).To(Equal("tests/fixtures/test.ogg")) }) It("returns album cover if cannot read embed artwork", func() { + // Force fromTag to fail + mfCorruptedCover.Path = "tests/fixtures/DOES_NOT_EXIST.ogg" + Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfCorruptedCover)).To(Succeed()) + // Simulate ffmpeg error ffmpeg.Error = errors.New("not available") + aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID()) Expect(err).ToNot(HaveOccurred()) _, path, err := aw.Reader(ctx) @@ -207,6 +237,10 @@ var _ = XDescribe("Artwork", func() { }) Describe("resizedArtworkReader", func() { BeforeEach(func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg", "front.png"}, + }} ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alMultipleCovers, }) @@ -241,12 +275,13 @@ var _ = XDescribe("Artwork", func() { DescribeTable("resize", func(format string, landscape bool, size int) { coverFileName := "cover." + format - //dirName := createImage(format, landscape, size) + dirName := createImage(format, landscape, size) alCover = model.Album{ - ID: "444", - Name: "Only external", - //ImageFiles: filepath.Join(dirName, coverFileName), + ID: "444", + Name: "Only external", + FolderIDs: []string{"tmp"}, } + folderRepo.result = []model.Folder{{Path: dirName, ImageFiles: []string{coverFileName}}} ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alCover, }) @@ -270,24 +305,24 @@ var _ = XDescribe("Artwork", func() { }) }) -//func createImage(format string, landscape bool, size int) string { -// var img image.Image -// -// if landscape { -// img = image.NewRGBA(image.Rect(0, 0, size, size/2)) -// } else { -// img = image.NewRGBA(image.Rect(0, 0, size/2, size)) -// } -// -// tmpDir := GinkgoT().TempDir() -// f, _ := os.Create(filepath.Join(tmpDir, "cover."+format)) -// defer f.Close() -// switch format { -// case "png": -// _ = png.Encode(f, img) -// case "jpg": -// _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 75}) -// } -// -// return tmpDir -//} +func createImage(format string, landscape bool, size int) string { + var img image.Image + + if landscape { + img = image.NewRGBA(image.Rect(0, 0, size, size/2)) + } else { + img = image.NewRGBA(image.Rect(0, 0, size/2, size)) + } + + tmpDir := GinkgoT().TempDir() + f, _ := os.Create(filepath.Join(tmpDir, "cover."+format)) + defer f.Close() + switch format { + case "png": + _ = png.Encode(f, img) + case "jpg": + _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 75}) + } + + return tmpDir +} diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 217044b7a..487346b4d 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -29,7 +29,7 @@ type artistReader struct { imgFiles []string } -func newArtistReader(ctx context.Context, artwork *artwork, artID model.ArtworkID, provider external.Provider) (*artistReader, error) { +func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID, provider external.Provider) (*artistReader, error) { ar, err := artwork.ds.Artist(ctx).Get(artID.ID) if err != nil { return nil, err diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index a8dfddea8..294a5db0b 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -12,7 +12,7 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("artistReader", func() { +var _ = Describe("artistArtworkReader", func() { var _ = Describe("loadArtistFolder", func() { var ( ctx context.Context