mirror of
https://github.com/navidrome/navidrome.git
synced 2026-04-28 03:19:38 +00:00
fix(server): return 404 instead of 500 for non-existent playlists
Some checks failed
Pipeline: Test, Lint, Build / Get version info (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test JS code (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint i18n files (push) Has been cancelled
Pipeline: Test, Lint, Build / Check Docker configuration (push) Has been cancelled
Pipeline: Test, Lint, Build / Build (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-1 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-2 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-3 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-4 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-5 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-6 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-7 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-8 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-9 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-10 (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to GHCR (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to Docker Hub (push) Has been cancelled
Pipeline: Test, Lint, Build / Cleanup digest artifacts (push) Has been cancelled
Pipeline: Test, Lint, Build / Build Windows installers (push) Has been cancelled
Pipeline: Test, Lint, Build / Package/Release (push) Has been cancelled
Pipeline: Test, Lint, Build / Upload Linux PKG (push) Has been cancelled
Some checks failed
Pipeline: Test, Lint, Build / Get version info (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test JS code (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint i18n files (push) Has been cancelled
Pipeline: Test, Lint, Build / Check Docker configuration (push) Has been cancelled
Pipeline: Test, Lint, Build / Build (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-1 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-2 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-3 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-4 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-5 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-6 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-7 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-8 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-9 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-10 (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to GHCR (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to Docker Hub (push) Has been cancelled
Pipeline: Test, Lint, Build / Cleanup digest artifacts (push) Has been cancelled
Pipeline: Test, Lint, Build / Build Windows installers (push) Has been cancelled
Pipeline: Test, Lint, Build / Package/Release (push) Has been cancelled
Pipeline: Test, Lint, Build / Upload Linux PKG (push) Has been cancelled
The native API endpoints GET /playlist/{id}/tracks and
GET /playlist/{id}/tracks/{id} were panicking with a nil pointer
dereference (resulting in a 500) when the playlist did not exist.
This happened because Tracks() returns nil for missing playlists,
and the nil repository was passed directly to the rest handler.
Extracted a shared playlistTracksHandler that checks for nil and
returns 404 early. Added tests covering both the error and happy paths.
This commit is contained in:
parent
f00af7f983
commit
b64d8ad334
3 changed files with 193 additions and 35 deletions
|
|
@ -19,47 +19,33 @@ import (
|
|||
|
||||
type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc
|
||||
|
||||
func getPlaylist(ds model.DataStore) http.HandlerFunc {
|
||||
// Add a middleware to capture the playlistId
|
||||
wrapper := func(handler restHandler) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
constructor := func(ctx context.Context) rest.Repository {
|
||||
plsRepo := ds.Playlist(ctx)
|
||||
plsId := chi.URLParam(r, "playlistId")
|
||||
p := req.Params(r)
|
||||
start := p.Int64Or("_start", 0)
|
||||
return plsRepo.Tracks(plsId, start == 0)
|
||||
}
|
||||
|
||||
handler(constructor).ServeHTTP(w, r)
|
||||
}
|
||||
}
|
||||
|
||||
func playlistTracksHandler(ds model.DataStore, handler restHandler, refreshSmartPlaylist func(*http.Request) bool) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
accept := r.Header.Get("accept")
|
||||
if strings.ToLower(accept) == "audio/x-mpegurl" {
|
||||
plsId := chi.URLParam(r, "playlistId")
|
||||
tracks := ds.Playlist(r.Context()).Tracks(plsId, refreshSmartPlaylist(r))
|
||||
if tracks == nil {
|
||||
http.Error(w, "not found", http.StatusNotFound)
|
||||
return
|
||||
}
|
||||
handler(func(ctx context.Context) rest.Repository { return tracks }).ServeHTTP(w, r)
|
||||
}
|
||||
}
|
||||
|
||||
func getPlaylist(ds model.DataStore) http.HandlerFunc {
|
||||
handler := playlistTracksHandler(ds, rest.GetAll, func(r *http.Request) bool {
|
||||
return req.Params(r).Int64Or("_start", 0) == 0
|
||||
})
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
if strings.ToLower(r.Header.Get("accept")) == "audio/x-mpegurl" {
|
||||
handleExportPlaylist(ds)(w, r)
|
||||
return
|
||||
}
|
||||
wrapper(rest.GetAll)(w, r)
|
||||
handler(w, r)
|
||||
}
|
||||
}
|
||||
|
||||
func getPlaylistTrack(ds model.DataStore) http.HandlerFunc {
|
||||
// Add a middleware to capture the playlistId
|
||||
wrapper := func(handler restHandler) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
constructor := func(ctx context.Context) rest.Repository {
|
||||
plsRepo := ds.Playlist(ctx)
|
||||
plsId := chi.URLParam(r, "playlistId")
|
||||
return plsRepo.Tracks(plsId, true)
|
||||
}
|
||||
|
||||
handler(constructor).ServeHTTP(w, r)
|
||||
}
|
||||
}
|
||||
|
||||
return wrapper(rest.Get)
|
||||
return playlistTracksHandler(ds, rest.Get, func(*http.Request) bool { return true })
|
||||
}
|
||||
|
||||
func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc {
|
||||
|
|
|
|||
167
server/nativeapi/playlists_test.go
Normal file
167
server/nativeapi/playlists_test.go
Normal file
|
|
@ -0,0 +1,167 @@
|
|||
package nativeapi
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"time"
|
||||
|
||||
"github.com/deluan/rest"
|
||||
"github.com/navidrome/navidrome/conf"
|
||||
"github.com/navidrome/navidrome/conf/configtest"
|
||||
"github.com/navidrome/navidrome/consts"
|
||||
"github.com/navidrome/navidrome/core/auth"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/server"
|
||||
"github.com/navidrome/navidrome/tests"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
type mockPlaylistTrackRepo struct {
|
||||
model.PlaylistTrackRepository
|
||||
tracks model.PlaylistTracks
|
||||
}
|
||||
|
||||
func (m *mockPlaylistTrackRepo) Count(...rest.QueryOptions) (int64, error) {
|
||||
return int64(len(m.tracks)), nil
|
||||
}
|
||||
|
||||
func (m *mockPlaylistTrackRepo) ReadAll(...rest.QueryOptions) (any, error) {
|
||||
return m.tracks, nil
|
||||
}
|
||||
|
||||
func (m *mockPlaylistTrackRepo) EntityName() string {
|
||||
return "playlist_track"
|
||||
}
|
||||
|
||||
func (m *mockPlaylistTrackRepo) NewInstance() any {
|
||||
return &model.PlaylistTrack{}
|
||||
}
|
||||
|
||||
func (m *mockPlaylistTrackRepo) Read(id string) (any, error) {
|
||||
for _, t := range m.tracks {
|
||||
if t.ID == id {
|
||||
return &t, nil
|
||||
}
|
||||
}
|
||||
return nil, rest.ErrNotFound
|
||||
}
|
||||
|
||||
var _ = Describe("Playlist Tracks Endpoint", func() {
|
||||
var (
|
||||
router http.Handler
|
||||
ds *tests.MockDataStore
|
||||
plsRepo *tests.MockPlaylistRepo
|
||||
userRepo *tests.MockedUserRepo
|
||||
w *httptest.ResponseRecorder
|
||||
)
|
||||
|
||||
BeforeEach(func() {
|
||||
DeferCleanup(configtest.SetupConfig())
|
||||
conf.Server.SessionTimeout = time.Minute
|
||||
|
||||
plsRepo = &tests.MockPlaylistRepo{}
|
||||
userRepo = tests.CreateMockUserRepo()
|
||||
|
||||
ds = &tests.MockDataStore{
|
||||
MockedPlaylist: plsRepo,
|
||||
MockedUser: userRepo,
|
||||
MockedProperty: &tests.MockedPropertyRepo{},
|
||||
}
|
||||
|
||||
auth.Init(ds)
|
||||
|
||||
testUser := model.User{
|
||||
ID: "user-1",
|
||||
UserName: "testuser",
|
||||
Name: "Test User",
|
||||
IsAdmin: false,
|
||||
NewPassword: "testpass",
|
||||
}
|
||||
err := userRepo.Put(&testUser)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil)
|
||||
router = server.JWTVerifier(nativeRouter)
|
||||
w = httptest.NewRecorder()
|
||||
})
|
||||
|
||||
createAuthenticatedRequest := func(method, path string) *http.Request {
|
||||
req := httptest.NewRequest(method, path, nil)
|
||||
testUser := model.User{ID: "user-1", UserName: "testuser"}
|
||||
token, err := auth.CreateToken(&testUser)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token)
|
||||
return req
|
||||
}
|
||||
|
||||
Describe("GET /playlist/{playlistId}/tracks", func() {
|
||||
It("returns 404 when playlist does not exist", func() {
|
||||
req := createAuthenticatedRequest("GET", "/playlist/non-existent/tracks")
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
Expect(w.Code).To(Equal(http.StatusNotFound))
|
||||
})
|
||||
|
||||
It("returns tracks when playlist exists", func() {
|
||||
plsRepo.TracksReturn = &mockPlaylistTrackRepo{
|
||||
tracks: model.PlaylistTracks{
|
||||
{ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"},
|
||||
{ID: "2", MediaFileID: "mf-2", PlaylistID: "pls-1"},
|
||||
},
|
||||
}
|
||||
|
||||
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks")
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
Expect(w.Code).To(Equal(http.StatusOK))
|
||||
|
||||
var response []model.PlaylistTrack
|
||||
err := json.Unmarshal(w.Body.Bytes(), &response)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(response).To(HaveLen(2))
|
||||
Expect(response[0].ID).To(Equal("1"))
|
||||
Expect(response[1].ID).To(Equal("2"))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("GET /playlist/{playlistId}/tracks/{id}", func() {
|
||||
It("returns 404 when playlist does not exist", func() {
|
||||
req := createAuthenticatedRequest("GET", "/playlist/non-existent/tracks/1")
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
Expect(w.Code).To(Equal(http.StatusNotFound))
|
||||
})
|
||||
|
||||
It("returns the track when playlist exists", func() {
|
||||
plsRepo.TracksReturn = &mockPlaylistTrackRepo{
|
||||
tracks: model.PlaylistTracks{
|
||||
{ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"},
|
||||
},
|
||||
}
|
||||
|
||||
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks/1")
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
Expect(w.Code).To(Equal(http.StatusOK))
|
||||
|
||||
var response model.PlaylistTrack
|
||||
err := json.Unmarshal(w.Body.Bytes(), &response)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(response.ID).To(Equal("1"))
|
||||
Expect(response.MediaFileID).To(Equal("mf-1"))
|
||||
})
|
||||
|
||||
It("returns 404 when track does not exist in playlist", func() {
|
||||
plsRepo.TracksReturn = &mockPlaylistTrackRepo{
|
||||
tracks: model.PlaylistTracks{},
|
||||
}
|
||||
|
||||
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks/999")
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
Expect(w.Code).To(Equal(http.StatusNotFound))
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue