mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(subsonic): return error 70 when scrobble contains invalid IDs
Implements OpenSubsonic API clarification from PR #202: the scrobble endpoint now validates all media file IDs before processing and returns error code 70 (data not found) if any ID is invalid. This ensures the entire operation fails atomically - no scrobbles are submitted when any ID in the batch is invalid, matching the OpenSubsonic specification. Changes include updating MediaFileRepository.Exists() to accept variadic IDs for efficient batch validation via a single COUNT query. See: https://github.com/opensubsonic/open-subsonic-api/pull/202 Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
parent
2731e25fd2
commit
80e9921d45
@ -354,7 +354,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
|
||||
Get(id string) (*MediaFile, error)
|
||||
GetWithParticipants(id string) (*MediaFile, error)
|
||||
|
||||
@ -143,8 +143,19 @@ 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. If no IDs are provided, it returns true.
|
||||
// If any of the IDs do not exist, it returns false. It returns an error if the database query fails.
|
||||
func (r *mediaFileRepository) Exists(ids ...string) (bool, error) {
|
||||
if len(ids) == 0 {
|
||||
return true, nil
|
||||
}
|
||||
existsQuery := Select("count(*) as exist").From("media_file").Where(Eq{"media_file.id": ids})
|
||||
var res struct{ Exist int64 }
|
||||
err := r.queryOne(existsQuery, &res)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
return res.Exist == int64(len(ids)), nil
|
||||
}
|
||||
|
||||
func (r *mediaFileRepository) Put(m *model.MediaFile) error {
|
||||
|
||||
@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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())
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@ -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) {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user