From 94aff0871a10081a395c787778d5aac42a2db965 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 23 Sep 2026 10:44:33 -0400 Subject: [PATCH] fix(server): address review feedback - writeDeleteManyResponse uses rest.RespondWithJSON, so the response now has Content-Type: application/json. This also removes a marshal error branch that could never run. - createInitialAdminUser returns the CountAll error instead of panicking, and wraps its errors. initialSetup now stops the server with log.Fatal when setup fails. Before, the error was dropped and the server started with a half-done setup. - Trim comments that described PR history. --- server/auth.go | 1 - server/auth_test.go | 2 -- server/initial_setup.go | 15 ++++++--------- server/initial_setup_test.go | 10 ++++++---- server/nativeapi/delete_many_response_test.go | 5 ++--- server/nativeapi/native_api.go | 12 +----------- server/public/handle_shares.go | 1 - server/public/handle_shares_test.go | 5 +---- 8 files changed, 16 insertions(+), 35 deletions(-) diff --git a/server/auth.go b/server/auth.go index cc82845c8..2aaa93e63 100644 --- a/server/auth.go +++ b/server/auth.go @@ -159,7 +159,6 @@ func createAdminUser(ctx context.Context, ds model.DataStore, username, password } err := ds.User(ctx).Put(&initialUser) if err != nil { - // Log the username only: initialUser carries the password in clear text log.Error(ctx, "Could not create initial user", "user", initialUser.UserName, err) return fmt.Errorf("creating initial user: %w", err) } diff --git a/server/auth_test.go b/server/auth_test.go index 9fdd84d06..abe144a12 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -76,8 +76,6 @@ var _ = Describe("Auth", func() { }) }) - // createAdminUser used to swallow the Put error and return nil, so a failure - // fell through to doLogin and surfaced as a misleading 401. Describe("createAdmin when the user cannot be stored", func() { It("responds 500 rather than falling through to login", func() { failing := dsWithFailingPut(errors.New("db is down")) diff --git a/server/initial_setup.go b/server/initial_setup.go index f033d4a51..e75220abe 100644 --- a/server/initial_setup.go +++ b/server/initial_setup.go @@ -16,7 +16,7 @@ import ( func initialSetup(ds model.DataStore) { ctx := context.TODO() - _ = ds.WithTx(func(tx model.DataStore) error { + err := ds.WithTx(func(tx model.DataStore) error { if err := tx.Library(ctx).StoreMusicFolder(); err != nil { return err } @@ -36,6 +36,9 @@ func initialSetup(ds model.DataStore) { err = properties.Put(consts.InitialSetupFlagKey, time.Now().String()) return err }, "initial setup") + if err != nil { + log.Fatal("Error running initial setup", err) + } } // If the Dev Admin user is not present, create it @@ -43,7 +46,7 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { users := ds.User(context.TODO()) c, err := users.CountAll(model.QueryOptions{Filters: squirrel.Eq{"user_name": consts.DevInitialUserName}}) if err != nil { - panic(fmt.Sprintf("Could not access User table: %s", err)) + return fmt.Errorf("could not access User table: %w", err) } if c == 0 { newID := id.NewRandom() @@ -57,14 +60,8 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { NewPassword: initialPassword, IsAdmin: true, } - // The Put error used to be assigned to a shadowed err and dropped, so a - // failure was reported as success and initialSetup went on to commit the - // "setup complete" flag — leaving no admin user, and skipping setup on - // every later boot. if err := users.Put(&initialUser); err != nil { - // Log the username rather than the whole struct, as everywhere else - log.Error("Could not create initial admin user", "user", consts.DevInitialUserName, err) - return err + return fmt.Errorf("could not create initial admin user: %w", err) } } return nil diff --git a/server/initial_setup_test.go b/server/initial_setup_test.go index b6d97d178..0ce8a39fa 100644 --- a/server/initial_setup_test.go +++ b/server/initial_setup_test.go @@ -10,8 +10,6 @@ import ( . "github.com/onsi/gomega" ) -// failingPutUserRepo counts users normally but fails to store them, to exercise the -// error path of the initial user creation. type failingPutUserRepo struct { model.UserRepository err error @@ -47,11 +45,15 @@ var _ = Describe("initial_setup", func() { Expect(ur.CountAll()).To(Equal(int64(1))) }) - // The error was assigned to a shadowed err and dropped, so the caller saw - // success and went on to commit the "setup complete" flag. It("returns the error when the user cannot be stored", func() { boom := errors.New("db is down") Expect(createInitialAdminUser(dsWithFailingPut(boom), "pass123")).To(MatchError(boom)) }) + + It("returns the error when the user table cannot be read", func() { + boom := errors.New("db is down") + ds = &tests.MockDataStore{MockedUser: &tests.MockedUserRepo{Error: boom}} + Expect(createInitialAdminUser(ds, "pass123")).To(MatchError(boom)) + }) }) }) diff --git a/server/nativeapi/delete_many_response_test.go b/server/nativeapi/delete_many_response_test.go index 9517d3b47..7d911d6cd 100644 --- a/server/nativeapi/delete_many_response_test.go +++ b/server/nativeapi/delete_many_response_test.go @@ -29,8 +29,6 @@ var _ = Describe("writeDeleteManyResponse", func() { Expect(write("a", "b")).To(HaveKeyWithValue("ids", ConsistOf("a", "b"))) }) - // ids come straight from the query string. html.EscapeString, used previously to - // build the single-id body, does not escape backslashes, so `{"id":"a\"}` went out. It("stays valid JSON when the id contains a backslash", func() { Expect(write(`a\`)).To(HaveKeyWithValue("id", `a\`)) }) @@ -43,8 +41,9 @@ var _ = Describe("writeDeleteManyResponse", func() { Expect(write("a&b")).To(HaveKeyWithValue("id", "a&b")) }) - It("responds 200 on success", func() { + It("responds 200 with a JSON content type", func() { write("abc123") Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) }) }) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 3476ec5a0..a7c53df09 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -2,7 +2,6 @@ package nativeapi import ( "context" - "encoding/json" "net/http" "strconv" "time" @@ -205,9 +204,6 @@ func (api *Router) addMissingFilesRoute(r chi.Router) { } func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []string) { - // Marshal both shapes instead of building the single-id case by hand: ids come - // from the query string, and html.EscapeString leaves backslashes untouched, so - // an id containing one produced a malformed body. var payload any if len(ids) == 1 { payload = struct { @@ -218,13 +214,7 @@ func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []strin Ids []string `json:"ids"` }{Ids: ids} } - resp, err := json.Marshal(payload) - if err != nil { - log.Error(r.Context(), "Error marshaling response", "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - if _, err := w.Write(resp); err != nil { + if err := rest.RespondWithJSON(w, http.StatusOK, payload); err != nil { log.Error(r.Context(), "Error writing response", "ids", ids, err) } } diff --git a/server/public/handle_shares.go b/server/public/handle_shares.go index b69549e7c..367bff501 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -58,7 +58,6 @@ func (pub *Router) handleM3U(w http.ResponseWriter, r *http.Request) { } s = pub.mapShareToM3U(r, *s) - // Content-Type must be set before WriteHeader, otherwise it is dropped w.Header().Set("Content-Type", "audio/x-mpegurl") w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(s.ToM3U8())) //nolint:gosec diff --git a/server/public/handle_shares_test.go b/server/public/handle_shares_test.go index b299f4cfd..1bf631fd4 100644 --- a/server/public/handle_shares_test.go +++ b/server/public/handle_shares_test.go @@ -30,7 +30,6 @@ var _ = Describe("handleM3U", func() { return w } - // Content-Type used to be set after WriteHeader, which silently drops it. It("sets the M3U content type", func() { share := &model.Share{ID: "abc123", Tracks: model.MediaFiles{{ID: "t1", Title: "Track 1"}}} shareRepo.ID = share.ID @@ -39,9 +38,7 @@ var _ = Describe("handleM3U", func() { w := makeRequest("abc123") Expect(w.Code).To(Equal(http.StatusOK)) - // Result() reports the headers as they were when WriteHeader ran, which is - // what the client actually receives. w.Header() would still show a - // Content-Type set too late to be sent. + // Result() has the headers sent at WriteHeader time, unlike w.Header() Expect(w.Result().Header.Get("Content-Type")).To(Equal("audio/x-mpegurl")) Expect(w.Body.String()).To(HavePrefix("#EXTM3U")) })