diff --git a/model/mediafile.go b/model/mediafile.go index ec83b76fd..8a0220d8e 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -429,7 +429,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 39bc83fa9..718f9921c 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -168,15 +168,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 fc767b0ff..51e2262a1 100644 --- a/server/subsonic/media_annotation_test.go +++ b/server/subsonic/media_annotation_test.go @@ -31,6 +31,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() { submissionTime := time.Now() r := newGetRequest("id=12", "id=34") @@ -71,10 +77,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) @@ -94,6 +117,16 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.Playing).To(HaveLen(1)) Expect(playTracker.Playing).To(HaveKey("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) {