mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 02:17:25 +02:00
fix(nativeapi): stop partial PUTs from clearing untouched columns (#6058)
* fix(nativeapi): stop partial PUTs from clearing untouched columns The REST layer parses the request body's top-level JSON keys and passes them to Repository.Update as colsToUpdate. The radio and library repositories discarded that list and issued a full-row UPDATE, so any field absent from the body was written as its zero value. For radio this wiped uploaded_image, deleting the station's cover on every partial update (the Web UI is unaffected because its form submits the whole record). For library it silently cleared remote_path and default_new_users. Thread the column list through to Put in both repositories, and extract the column-selection half of filterUpdateValues into selectUpdateColumns so library, which hand-builds its update map, shares the same rule instead of copying it. Fixes #6057 * refactor(persistence): drop pluginRepository's dead rest.Persistable methods Save and Update had no callers: PUT /api/plugin/{id} is served by the hand-written updatePlugin handler over a typed request struct, and the route only wires rest.GetAll and rest.Get. Both methods delegated to Put, which upserts all twelve columns, so wiring rest.Put to this repository would have reintroduced the partial-update clobbering fixed in the previous commit. Removing them, along with the rest.Persistable assertion, makes that a compile error instead of a silent data loss. Put itself is unchanged and still backs plugin discovery.
This commit is contained in:
parent
dbd26ba2e7
commit
96b051ffa7
10 changed files with 71 additions and 45 deletions
|
|
@ -191,7 +191,7 @@ func (r *libraryRepositoryWrapper) Save(entity any) (string, error) {
|
|||
return strconv.Itoa(lib.ID), nil
|
||||
}
|
||||
|
||||
func (r *libraryRepositoryWrapper) Update(id string, entity any, _ ...string) error {
|
||||
func (r *libraryRepositoryWrapper) Update(id string, entity any, cols ...string) error {
|
||||
lib := entity.(*model.Library)
|
||||
libID, err := strconv.Atoi(id)
|
||||
if err != nil {
|
||||
|
|
@ -211,7 +211,7 @@ func (r *libraryRepositoryWrapper) Update(id string, entity any, _ ...string) er
|
|||
|
||||
pathChanged := originalLib.Path != lib.Path
|
||||
|
||||
err = r.LibraryRepository.Put(lib)
|
||||
err = r.LibraryRepository.Put(lib, cols...)
|
||||
if err != nil {
|
||||
return r.mapError(err)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -188,6 +188,15 @@ var _ = Describe("Library Service", func() {
|
|||
Expect(libraryRepo.Data[1].Path).To(Equal(newTempDir))
|
||||
})
|
||||
|
||||
It("forwards the columns sent by the client to the repository", func() {
|
||||
library := &model.Library{ID: 1, Name: "Updated Library", Path: tempDir}
|
||||
|
||||
err := repo.Update("1", library, "name", "path")
|
||||
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
Expect(libraryRepo.PutCols).To(Equal([]string{"name", "path"}))
|
||||
})
|
||||
|
||||
It("fails when library doesn't exist", func() {
|
||||
// Create a unique temporary directory to avoid path conflicts
|
||||
uniqueTempDir, err := os.MkdirTemp("", "navidrome-nonexistent-")
|
||||
|
|
|
|||
|
|
@ -45,7 +45,7 @@ type LibraryRepository interface {
|
|||
GetPath(id int) (string, error)
|
||||
GetAll(...QueryOptions) (Libraries, error)
|
||||
CountAll(...QueryOptions) (int64, error)
|
||||
Put(*Library) error
|
||||
Put(l *Library, colsToUpdate ...string) error
|
||||
Delete(id int) error
|
||||
StoreMusicFolder() error
|
||||
AddArtist(id int, artistID string) error
|
||||
|
|
|
|||
|
|
@ -70,7 +70,7 @@ func (r *libraryRepository) GetPath(id int) (string, error) {
|
|||
}
|
||||
}
|
||||
|
||||
func (r *libraryRepository) Put(l *model.Library) error {
|
||||
func (r *libraryRepository) Put(l *model.Library, colsToUpdate ...string) error {
|
||||
if l.ID == model.DefaultLibraryID {
|
||||
currentLib, err := r.Get(1)
|
||||
// if we are creating it, it's ok.
|
||||
|
|
@ -89,13 +89,13 @@ func (r *libraryRepository) Put(l *model.Library) error {
|
|||
err = r.db.Model(l).Insert()
|
||||
} else {
|
||||
// Try to update first
|
||||
cols := map[string]any{
|
||||
cols := selectUpdateColumns(map[string]any{
|
||||
"name": l.Name,
|
||||
"path": l.Path,
|
||||
"remote_path": l.RemotePath,
|
||||
"default_new_users": l.DefaultNewUsers,
|
||||
"updated_at": l.UpdatedAt,
|
||||
}
|
||||
}, colsToUpdate...)
|
||||
cols["updated_at"] = l.UpdatedAt
|
||||
sq := Update(r.tableName).SetMap(cols).Where(Eq{"id": l.ID})
|
||||
rowsAffected, updateErr := r.executeSQL(sq)
|
||||
if updateErr != nil {
|
||||
|
|
@ -340,7 +340,7 @@ func (r *libraryRepository) Update(id string, entity any, cols ...string) error
|
|||
}
|
||||
|
||||
lib.ID = idInt
|
||||
return r.Put(lib)
|
||||
return r.Put(lib, cols...)
|
||||
}
|
||||
|
||||
var _ model.LibraryRepository = (*libraryRepository)(nil)
|
||||
|
|
|
|||
|
|
@ -52,6 +52,26 @@ var _ = Describe("LibraryRepository", func() {
|
|||
})
|
||||
})
|
||||
|
||||
Context("when colsToUpdate is specified", func() {
|
||||
It("only writes the requested columns", func() {
|
||||
lib := &model.Library{
|
||||
Name: "Original Library",
|
||||
Path: "/music/original",
|
||||
RemotePath: "/remote/original",
|
||||
DefaultNewUsers: true,
|
||||
}
|
||||
Expect(repo.Put(lib)).To(Succeed())
|
||||
|
||||
Expect(repo.Put(&model.Library{ID: lib.ID, Name: "Renamed", Path: lib.Path}, "name", "path")).To(Succeed())
|
||||
|
||||
saved, err := repo.Get(lib.ID)
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(saved.Name).To(Equal("Renamed"))
|
||||
Expect(saved.RemotePath).To(Equal("/remote/original"))
|
||||
Expect(saved.DefaultNewUsers).To(BeTrue())
|
||||
})
|
||||
})
|
||||
|
||||
Context("when ID is non-zero and record exists", func() {
|
||||
It("updates the existing record", func() {
|
||||
// First create a library
|
||||
|
|
|
|||
|
|
@ -141,31 +141,5 @@ func (r *pluginRepository) ReadAll(options ...rest.QueryOptions) (any, error) {
|
|||
return r.GetAll(r.parseRestOptions(r.ctx, options...))
|
||||
}
|
||||
|
||||
func (r *pluginRepository) Save(entity any) (string, error) {
|
||||
p := entity.(*model.Plugin)
|
||||
if !r.isPermitted() {
|
||||
return "", rest.ErrPermissionDenied
|
||||
}
|
||||
err := r.Put(p)
|
||||
if errors.Is(err, model.ErrNotFound) {
|
||||
return "", rest.ErrNotFound
|
||||
}
|
||||
return p.ID, err
|
||||
}
|
||||
|
||||
func (r *pluginRepository) Update(id string, entity any, cols ...string) error {
|
||||
p := entity.(*model.Plugin)
|
||||
p.ID = id
|
||||
if !r.isPermitted() {
|
||||
return rest.ErrPermissionDenied
|
||||
}
|
||||
err := r.Put(p)
|
||||
if errors.Is(err, model.ErrNotFound) {
|
||||
return rest.ErrNotFound
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
var _ model.PluginRepository = (*pluginRepository)(nil)
|
||||
var _ rest.Repository = (*pluginRepository)(nil)
|
||||
var _ rest.Persistable = (*pluginRepository)(nil)
|
||||
|
|
|
|||
|
|
@ -152,7 +152,7 @@ func (r *radioRepository) Update(id string, entity any, cols ...string) error {
|
|||
if !r.isPermitted() {
|
||||
return rest.ErrPermissionDenied
|
||||
}
|
||||
err := r.Put(t)
|
||||
err := r.Put(t, cols...)
|
||||
if errors.Is(err, model.ErrNotFound) {
|
||||
return rest.ErrNotFound
|
||||
}
|
||||
|
|
|
|||
|
|
@ -141,6 +141,24 @@ var _ = Describe("RadioRepository", func() {
|
|||
)))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("Update", func() {
|
||||
It("only writes the columns sent by the client", func() {
|
||||
radio := radioWithHomePage
|
||||
radio.UploadedImage = "cover.png"
|
||||
Expect(repo.Put(&radio)).To(Succeed())
|
||||
|
||||
persistable := repo.(rest.Persistable)
|
||||
Expect(persistable.Update(radio.ID, &model.Radio{Name: "Renamed"}, "name")).To(Succeed())
|
||||
|
||||
item, err := repo.Get(radio.ID)
|
||||
Expect(err).To(BeNil())
|
||||
Expect(item.Name).To(Equal("Renamed"))
|
||||
Expect(item.UploadedImage).To(Equal("cover.png"))
|
||||
Expect(item.StreamUrl).To(Equal(radio.StreamUrl))
|
||||
Expect(item.HomePageUrl).To(Equal(radio.HomePageUrl))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Describe("Regular User", func() {
|
||||
|
|
|
|||
|
|
@ -560,13 +560,11 @@ func (r sqlRepository) putByMatch(filter Sqlizer, id string, m any, colsToUpdate
|
|||
return r.put(res.ID, m, colsToUpdate...)
|
||||
}
|
||||
|
||||
// filterUpdateValues selects, from a marshaled column map, the values to write in an UPDATE on the
|
||||
// row identified by id: only the requested colsToUpdate (or all columns when none are specified),
|
||||
// dropping columns that must never be overwritten on update (created_at, birth_time).
|
||||
func filterUpdateValues(values map[string]any, id string, colsToUpdate ...string) map[string]any {
|
||||
// selectUpdateColumns keeps only the requested colsToUpdate (or all columns when none are
|
||||
// specified), dropping columns that must never be overwritten on update (created_at, birth_time).
|
||||
func selectUpdateColumns(values map[string]any, colsToUpdate ...string) map[string]any {
|
||||
updateValues := map[string]any{}
|
||||
|
||||
// This is a map of the columns that need to be updated, if specified
|
||||
c2upd := slice.ToMap(colsToUpdate, func(s string) (string, struct{}) {
|
||||
return toSnakeCase(s), struct{}{}
|
||||
})
|
||||
|
|
@ -576,7 +574,6 @@ func filterUpdateValues(values map[string]any, id string, colsToUpdate ...string
|
|||
}
|
||||
}
|
||||
|
||||
updateValues["id"] = id
|
||||
delete(updateValues, "created_at")
|
||||
// To avoid updating the media_file birth_time on each scan. Not the best solution, but it works for now
|
||||
// TODO move to mediafile_repository when each repo has its own upsert method
|
||||
|
|
@ -584,6 +581,12 @@ func filterUpdateValues(values map[string]any, id string, colsToUpdate ...string
|
|||
return updateValues
|
||||
}
|
||||
|
||||
func filterUpdateValues(values map[string]any, id string, colsToUpdate ...string) map[string]any {
|
||||
updateValues := selectUpdateColumns(values, colsToUpdate...)
|
||||
updateValues["id"] = id
|
||||
return updateValues
|
||||
}
|
||||
|
||||
func (r sqlRepository) put(id string, m any, colsToUpdate ...string) (newId string, err error) {
|
||||
values, err := toSQLArgs(m)
|
||||
if err != nil {
|
||||
|
|
|
|||
|
|
@ -14,9 +14,10 @@ import (
|
|||
|
||||
type MockLibraryRepo struct {
|
||||
model.LibraryRepository
|
||||
Data map[int]model.Library
|
||||
Err error
|
||||
PutFn func(*model.Library) error // Allow custom Put behavior for testing
|
||||
Data map[int]model.Library
|
||||
Err error
|
||||
PutFn func(*model.Library) error // Allow custom Put behavior for testing
|
||||
PutCols []string
|
||||
}
|
||||
|
||||
func (m *MockLibraryRepo) SetData(data model.Libraries) {
|
||||
|
|
@ -90,7 +91,8 @@ func (m *MockLibraryRepo) GetPath(id int) (string, error) {
|
|||
return "", model.ErrNotFound
|
||||
}
|
||||
|
||||
func (m *MockLibraryRepo) Put(library *model.Library) error {
|
||||
func (m *MockLibraryRepo) Put(library *model.Library, colsToUpdate ...string) error {
|
||||
m.PutCols = colsToUpdate
|
||||
if m.PutFn != nil {
|
||||
return m.PutFn(library)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue