diff --git a/CHANGELOG.md b/CHANGELOG.md index b711fb75..dc7ba113 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## v0.22.24 + +- Delete new uploaded record files in case of record DB persist error ([#5845](https://github.com/pocketbase/pocketbase/issues/5845)). + + ## v0.22.23 - Updated the hooks watcher to account for the case when hooksDir is a symlink ([#5789](https://github.com/pocketbase/pocketbase/issues/5789)). diff --git a/forms/record_upsert.go b/forms/record_upsert.go index cdfe972b..55358de1 100644 --- a/forms/record_upsert.go +++ b/forms/record_upsert.go @@ -756,6 +756,8 @@ func (form *RecordUpsert) Submit(interceptors ...InterceptorFunc[*models.Record] dao := form.dao.Clone() + var uploaded []string + // upload new files (if any) // // note: executed after the default BeforeCreateFunc and BeforeUpdateFunc hook actions @@ -765,7 +767,9 @@ func (form *RecordUpsert) Submit(interceptors ...InterceptorFunc[*models.Record] dao.BeforeCreateFunc = func(eventDao *daos.Dao, m models.Model, action func() error) error { newAction := func() error { if m.TableName() == form.record.TableName() && m.GetId() == form.record.GetId() { - if err := form.processFilesToUpload(); err != nil { + var err error + uploaded, err = form.processFilesToUpload() + if err != nil { return err } } @@ -783,7 +787,9 @@ func (form *RecordUpsert) Submit(interceptors ...InterceptorFunc[*models.Record] dao.BeforeUpdateFunc = func(eventDao *daos.Dao, m models.Model, action func() error) error { newAction := func() error { if m.TableName() == form.record.TableName() && m.GetId() == form.record.GetId() { - if err := form.processFilesToUpload(); err != nil { + var err error + uploaded, err = form.processFilesToUpload() + if err != nil { return err } } @@ -801,6 +807,9 @@ func (form *RecordUpsert) Submit(interceptors ...InterceptorFunc[*models.Record] // persist the record model if err := dao.SaveRecord(form.record); err != nil { + // cleanup - try to delete only the successfully uploaded files + form.deleteFilesByNamesList(uploaded) + return form.prepareError(err) } @@ -819,30 +828,30 @@ func (form *RecordUpsert) Submit(interceptors ...InterceptorFunc[*models.Record] }, interceptors...) } -func (form *RecordUpsert) processFilesToUpload() error { +func (form *RecordUpsert) processFilesToUpload() ([]string, error) { if len(form.filesToUpload) == 0 { - return nil // no parsed file fields + return nil, nil // no parsed file fields } if !form.record.HasId() { - return errors.New("the record doesn't have an id") + return nil, errors.New("the record doesn't have an id") } - fs, err := form.app.NewFilesystem() + fsys, err := form.app.NewFilesystem() if err != nil { - return err + return nil, err } - defer fs.Close() + defer fsys.Close() var uploadErrors []error // list of upload errors - var uploaded []string // list of uploaded file paths + var uploaded []string // list of uploaded file names for fieldKey := range form.filesToUpload { for i, file := range form.filesToUpload[fieldKey] { path := form.record.BaseFilesPath() + "/" + file.Name - if err := fs.UploadFile(file, path); err == nil { + if err := fsys.UploadFile(file, path); err == nil { // keep track of the already uploaded file - uploaded = append(uploaded, path) + uploaded = append(uploaded, file.Name) } else { // store the upload error uploadErrors = append(uploadErrors, fmt.Errorf("file %d: %v", i, err)) @@ -854,10 +863,10 @@ func (form *RecordUpsert) processFilesToUpload() error { // cleanup - try to delete the successfully uploaded files (if any) form.deleteFilesByNamesList(uploaded) - return fmt.Errorf("failed to upload all files: %v", uploadErrors) + return nil, fmt.Errorf("failed to upload all files: %v", uploadErrors) } - return nil + return uploaded, nil } func (form *RecordUpsert) processFilesToDelete() (err error) { @@ -876,24 +885,28 @@ func (form *RecordUpsert) deleteFilesByNamesList(filenames []string) ([]string, return filenames, errors.New("the record doesn't have an id") } - fs, err := form.app.NewFilesystem() + isNew := form.record.IsNew() + + baseDir := form.record.BaseFilesPath() + + fsys, err := form.app.NewFilesystem() if err != nil { return filenames, err } - defer fs.Close() + defer fsys.Close() var deleteErrors []error for i := len(filenames) - 1; i >= 0; i-- { filename := filenames[i] - path := form.record.BaseFilesPath() + "/" + filename + path := baseDir + "/" + filename - if err := fs.Delete(path); err == nil { + if err := fsys.Delete(path); err == nil { // remove the deleted file from the list filenames = append(filenames[:i], filenames[i+1:]...) // try to delete the related file thumbs (if any) - fs.DeletePrefix(form.record.BaseFilesPath() + "/thumbs_" + filename + "/") + fsys.DeletePrefix(baseDir + "/thumbs_" + filename + "/") } else { // store the delete error deleteErrors = append(deleteErrors, fmt.Errorf("file %d: %v", i, err)) @@ -904,6 +917,11 @@ func (form *RecordUpsert) deleteFilesByNamesList(filenames []string) ([]string, return filenames, fmt.Errorf("failed to delete all files: %v", deleteErrors) } + // cleanup empty dir for new records if there are no other files + if isNew && fsys.IsEmptyDir(baseDir) { + _ = fsys.Delete(baseDir) + } + return filenames, nil } diff --git a/forms/record_upsert_test.go b/forms/record_upsert_test.go index 6a730f81..b037f06b 100644 --- a/forms/record_upsert_test.go +++ b/forms/record_upsert_test.go @@ -619,6 +619,94 @@ func TestRecordUpsertSubmitInterceptors(t *testing.T) { } } +func TestRecordUpsertSubmitFailureFilesCleanup(t *testing.T) { + app, _ := tests.NewTestApp() + defer app.Cleanup() + + collection, err := app.Dao().FindCollectionByNameOrId("demo3") + if err != nil { + t.Fatal(err) + } + + // ensure that there is a unique index constraint + collection.Indexes = append(collection.Indexes, `CREATE UNIQUE INDEX "idx_unique_demo3_title" on "demo3" ("title")`) + if err := app.Dao().SaveCollection(collection); err != nil { + t.Fatal(err) + } + + f1, err := filesystem.NewFileFromBytes([]byte("test1"), "new_test1") + if err != nil { + t.Fatal(err) + } + + f2, err := filesystem.NewFileFromBytes([]byte("test1"), "new_test2") + if err != nil { + t.Fatal(err) + } + + uploadedFiles := []string{f1.Name, f2.Name} + + existing, err := app.Dao().FindRecordById(collection.Id, "mk5fmymtx4wsprk") + if err != nil { + t.Fatal(err) + } + + scenarios := []struct { + recordId string + }{ + {""}, + {"7nwo8tuiatetxdm"}, + {"1tmknxy2868d869"}, + } + + for _, s := range scenarios { + sname := s.recordId + if sname == "" { + sname = "new" + } + + t.Run(sname, func(t *testing.T) { + var originalFiles []string + + var record *models.Record + if s.recordId == "" { + record = models.NewRecord(collection) + } else { + record, err = app.Dao().FindRecordById(collection.Id, s.recordId) + if err != nil { + t.Fatal(err) + } + originalFiles = record.GetStringSlice("files") + } + + record.Set("title", existing.GetString("title")) + + form := forms.NewRecordUpsert(app, record) + + form.AddFiles("files", f1, f2) + + err := form.Submit() + if err == nil { + t.Fatal("Expected form submit to fail") + } + + // ensure that the original files still exists + for _, name := range originalFiles { + if !hasRecordFile(app, record, name) { + t.Fatalf("Missing original file %q", name) + } + } + + // ensure that the new uploaded files were deleted + for _, name := range uploadedFiles { + if hasRecordFile(app, record, name) { + t.Fatalf("Expected file %q to be deleted", name) + } + } + }) + } +} + func TestRecordUpsertWithCustomId(t *testing.T) { app, _ := tests.NewTestApp() defer app.Cleanup() diff --git a/tools/filesystem/filesystem.go b/tools/filesystem/filesystem.go index bb0e6ec3..55fbdc0d 100644 --- a/tools/filesystem/filesystem.go +++ b/tools/filesystem/filesystem.go @@ -344,6 +344,26 @@ func (s *System) DeletePrefix(prefix string) []error { return failed } +// Checks if the provided dir prefix doesn't have any files. +// +// A trailing slash will be appended to a non-empty dir string argument +// to ensure that the checked prefix is a "directory". +// +// Returns "false" in case the has at least one file, otherwise - "true". +func (s *System) IsEmptyDir(dir string) bool { + if dir != "" && !strings.HasSuffix(dir, "/") { + dir += "/" + } + + iter := s.bucket.List(&blob.ListOptions{ + Prefix: dir, + }) + + _, err := iter.Next(s.ctx) + + return err == io.EOF +} + var inlineServeContentTypes = []string{ // image "image/png", "image/jpg", "image/jpeg", "image/gif", "image/webp", "image/x-icon", "image/bmp", diff --git a/tools/filesystem/filesystem_test.go b/tools/filesystem/filesystem_test.go index 57e880ca..60d5b2e2 100644 --- a/tools/filesystem/filesystem_test.go +++ b/tools/filesystem/filesystem_test.go @@ -169,6 +169,41 @@ func TestFileSystemDeletePrefixWithTrailingSlash(t *testing.T) { } } +func TestFileSystemIsEmptyDir(t *testing.T) { + dir := createTestDir(t) + defer os.RemoveAll(dir) + + fsys, err := filesystem.NewLocal(dir) + if err != nil { + t.Fatal(err) + } + defer fsys.Close() + + scenarios := []struct { + dir string + expected bool + }{ + {"", false}, // special case that shouldn't be suffixed with delimiter to search for any files within the bucket + {"/", true}, + {"missing", true}, + {"missing/", true}, + {"test", false}, + {"test/", false}, + {"empty", true}, + {"empty/", true}, + } + + for _, s := range scenarios { + t.Run(s.dir, func(t *testing.T) { + result := fsys.IsEmptyDir(s.dir) + + if result != s.expected { + t.Fatalf("Expected %v, got %v", s.expected, result) + } + }) + } +} + func TestFileSystemUploadMultipart(t *testing.T) { dir := createTestDir(t) defer os.RemoveAll(dir)