diff --git a/model/mediafile.go b/model/mediafile.go index 6be8402ae..abf63acc3 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -432,7 +432,7 @@ type MediaFileCursor iter.Seq2[MediaFile, error] type MediaFileRepository interface { CountAll(options ...QueryOptions) (int64, error) CountBySuffix(options ...QueryOptions) (map[string]int64, error) - Exists(id string) (bool, error) + Exists(ids ...string) (bool, error) Put(m *MediaFile) error UpdateProbeData(id string, data string) error Get(id string) (*MediaFile, error) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 264778ea0..995b5d189 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -147,8 +147,29 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[ return counts, nil } -func (r *mediaFileRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"media_file.id": id}) +// Exists checks if all given media file IDs exist in the database and are accessible to the current user. +// If no IDs are provided, it returns true. Duplicate IDs are handled correctly. +// If any of the IDs do not exist or are not accessible, it returns false. +func (r *mediaFileRepository) Exists(ids ...string) (bool, error) { + if len(ids) == 0 { + return true, nil + } + uniqueIds := slice.Unique(ids) + + // Process in batches to avoid hitting SQLITE_MAX_VARIABLE_NUMBER limit (default 999) + const batchSize = 300 + var totalCount int64 + for batch := range slices.Chunk(uniqueIds, batchSize) { + existsQuery := Select("count(*) as exist").From("media_file").Where(Eq{"media_file.id": batch}) + existsQuery = r.applyLibraryFilter(existsQuery) + var res struct{ Exist int64 } + err := r.queryOne(existsQuery, &res) + if err != nil { + return false, err + } + totalCount += res.Exist + } + return totalCount == int64(len(uniqueIds)), nil } func (r *mediaFileRepository) Put(m *model.MediaFile) error { diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index e8b0278c1..02d1840ef 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -169,15 +169,26 @@ func (api *Router) Scrobble(r *http.Request) (*responses.Subsonic, error) { position := p.IntOr("position", 0) ctx := r.Context() + // Validate all IDs exist before processing (OpenSubsonic compliance) + exists, err := api.ds.MediaFile(ctx).Exists(ids...) + if err != nil { + return nil, err + } + if !exists { + return nil, newError(responses.ErrorDataNotFound, "Media file not found") + } + if submission { err := api.scrobblerSubmit(ctx, ids, times) if err != nil { log.Error(ctx, "Error registering scrobbles", "ids", ids, "times", times, err) + return nil, err } } else { err := api.scrobblerNowPlaying(ctx, ids[0], position) if err != nil { log.Error(ctx, "Error setting NowPlaying", "id", ids[0], err) + return nil, err } } diff --git a/server/subsonic/media_annotation_test.go b/server/subsonic/media_annotation_test.go index f5cf434e0..68da33722 100644 --- a/server/subsonic/media_annotation_test.go +++ b/server/subsonic/media_annotation_test.go @@ -32,6 +32,12 @@ var _ = Describe("MediaAnnotationController", func() { }) Describe("Scrobble", func() { + BeforeEach(func() { + // Populate mock with valid media files + _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "12"}) + _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "34"}) + }) + It("submit all scrobbles with only the id", func() { // Back-date the baseline so the assertion still passes on platforms // with millisecond clock resolution (e.g. Windows). @@ -74,10 +80,27 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.Submissions).To(BeEmpty()) }) + It("returns error when any id is invalid", func() { + r := newGetRequest("id=invalid") + + _, err := router.Scrobble(r) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Submissions).To(BeEmpty()) + }) + + It("returns error and does not scrobble when mix of valid and invalid ids", func() { + r := newGetRequest("id=12", "id=invalid") + + _, err := router.Scrobble(r) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Submissions).To(BeEmpty()) + }) + Context("submission=false", func() { var req *http.Request BeforeEach(func() { - _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "12"}) ctx = request.WithPlayer(ctx, model.Player{ID: "player-1"}) req = newGetRequest("id=12", "submission=false") req = req.WithContext(ctx) @@ -99,6 +122,16 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.ReportedPlayback[0].State).To(Equal(scrobbler.StatePlaying)) Expect(playTracker.ReportedPlayback[0].ClientId).To(Equal("player-1")) }) + + It("returns error when id is invalid", func() { + req = newGetRequest("id=invalid", "submission=false") + req = req.WithContext(ctx) + + _, err := router.Scrobble(req) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Playing).To(BeEmpty()) + }) }) }) diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 01eacae30..21f32d02b 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -44,12 +44,16 @@ func (m *MockMediaFileRepo) SetData(mfs model.MediaFiles) { } } -func (m *MockMediaFileRepo) Exists(id string) (bool, error) { +func (m *MockMediaFileRepo) Exists(ids ...string) (bool, error) { if m.Err { return false, errors.New("error") } - _, found := m.Data[id] - return found, nil + for _, id := range ids { + if _, found := m.Data[id]; !found { + return false, nil + } + } + return true, nil } func (m *MockMediaFileRepo) Get(id string) (*model.MediaFile, error) {