[#5845] delete new uploaded record files in case of DB persist error

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)).

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
 }
 

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()

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",

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)