[#1194] refactored forms.RecordUpsert to allow easier file upload

CHANGELOG.md

@@ -1,3 +1,25 @@
+## (WIP) v0.10.0
+
+- Removed `rest.UploadedFile` struct (see below `filesystem.File`).
+
+- Added generic file resource struct that allows loading and uploading file content from
+  different sources (at the moment multipart/formdata requests and from the local filesystem).
+  ```
+  filesystem.File{}
+  filesystem.NewFileFromPath(path)
+  filesystem.NewFileFromMultipart(multipartHeader)
+  filesystem/System.UploadFile(file)
+  ```
+
+- Refactored `forms.RecordUpsert` to allow more easily loading and removing files programmatically.
+  ```
+  forms.RecordUpsert.LoadFiles(key, filesystem.File...) // add new filesystem.File to the form for upload
+  forms.RecordUpsert.RemoveFiles(key, filenames...)     // marks the filenames for deletion
+  ```
+
+- Optimized memory allocations (~20% improvement).
+
+
 ## v0.9.2
 
 - Fixed field column name conflict on record deletion ([#1220](https://github.com/pocketbase/pocketbase/discussions/1220)).

forms/record_upsert.go

@@ -52,7 +52,7 @@ type RecordUpsert struct {
 	OldPassword     string `json:"oldPassword"`
 	// ---
 
-	Data map[string]any `json:"data"`
+	data map[string]any
 }
 
 // NewRecordUpsert creates a new [RecordUpsert] form with initializer
@@ -75,6 +75,11 @@ func NewRecordUpsert(app core.App, record *models.Record) *RecordUpsert {
 	return form
 }
 
+// Data returns the loaded form's data.
+func (form *RecordUpsert) Data() map[string]any {
+	return form.data
+}
+
 // SetFullManageAccess sets the manageAccess bool flag of the current
 // form to enable/disable directly changing some system record fields
 // (often used with auth collection records).
@@ -97,9 +102,9 @@ func (form *RecordUpsert) loadFormDefaults() {
 		form.Verified = form.record.Verified()
 	}
 
-	form.Data = map[string]any{}
+	form.data = map[string]any{}
 	for _, field := range form.record.Collection().Schema.Fields() {
-		form.Data[field.Name] = form.record.Get(field.Name)
+		form.data[field.Name] = form.record.Get(field.Name)
 	}
 }
 
@@ -113,49 +118,56 @@ func (form *RecordUpsert) getContentType(r *http.Request) string {
 	return t
 }
 
-func (form *RecordUpsert) extractRequestData(r *http.Request, keyPrefix string) (map[string]any, error) {
+func (form *RecordUpsert) extractRequestData(
+	r *http.Request,
+	keyPrefix string,
+) (map[string]any, map[string][]*filesystem.File, error) {
 	switch form.getContentType(r) {
 	case "application/json":
 		return form.extractJsonData(r, keyPrefix)
 	case "multipart/form-data":
 		return form.extractMultipartFormData(r, keyPrefix)
 	default:
-		return nil, errors.New("Unsupported request Content-Type.")
+		return nil, nil, errors.New("unsupported request content-type")
 	}
 }
 
-func (form *RecordUpsert) extractJsonData(r *http.Request, keyPrefix string) (map[string]any, error) {
+func (form *RecordUpsert) extractJsonData(
-	result := map[string]any{}
+	r *http.Request,
+	keyPrefix string,
+) (map[string]any, map[string][]*filesystem.File, error) {
+	data := map[string]any{}
 
-	err := rest.CopyJsonBody(r, &result)
+	err := rest.CopyJsonBody(r, &data)
 
 	if keyPrefix != "" {
 		parts := strings.Split(keyPrefix, ".")
 		for _, part := range parts {
-			if result[part] == nil {
+			if data[part] == nil {
 				break
 			}
-			if v, ok := result[part].(map[string]any); ok {
+			if v, ok := data[part].(map[string]any); ok {
-				result = v
+				data = v
 			}
 		}
 	}
 
-	return result, err
+	return data, nil, err
 }
 
-func (form *RecordUpsert) extractMultipartFormData(r *http.Request, keyPrefix string) (map[string]any, error) {
+func (form *RecordUpsert) extractMultipartFormData(
-	result := map[string]any{}
+	r *http.Request,
-
+	keyPrefix string,
+) (map[string]any, map[string][]*filesystem.File, error) {
 	// parse form data (if not already)
 	if err := r.ParseMultipartForm(rest.DefaultMaxMemory); err != nil {
-		return result, err
+		return nil, nil, err
 	}
 
+	data := map[string]any{}
+	filesToUpload := map[string][]*filesystem.File{}
 	arrayValueSupportTypes := schema.ArraybleFieldTypes()
 
-	form.filesToUpload = map[string][]*filesystem.File{}
-
 	for fullKey, values := range r.PostForm {
 		key := fullKey
 		if keyPrefix != "" {
@@ -163,15 +175,15 @@ func (form *RecordUpsert) extractMultipartFormData(r *http.Request, keyPrefix st
 		}
 
 		if len(values) == 0 {
-			result[key] = nil
+			data[key] = nil
 			continue
 		}
 
 		field := form.record.Collection().Schema.GetFieldByName(key)
 		if field != nil && list.ExistInSlice(field.Type, arrayValueSupportTypes) {
-			result[key] = values
+			data[key] = values
 		} else {
-			result[key] = values[0]
+			data[key] = values[0]
 		}
 	}
 
@@ -197,32 +209,10 @@ func (form *RecordUpsert) extractMultipartFormData(r *http.Request, keyPrefix st
 			continue
 		}
 
-		options, ok := field.Options.(*schema.FileOptions)
+		filesToUpload[key] = append(filesToUpload[key], files...)
-		if !ok {
-			continue
 	}
 
-		if form.filesToUpload[key] == nil {
+	return data, filesToUpload, nil
-			form.filesToUpload[key] = []*filesystem.File{}
-		}
-
-		if options.MaxSelect == 1 {
-			form.filesToUpload[key] = append(form.filesToUpload[key], files[0])
-		} else if options.MaxSelect > 1 {
-			form.filesToUpload[key] = append(form.filesToUpload[key], files...)
-		}
-	}
-
-	return result, nil
-}
-
-func (form *RecordUpsert) normalizeData() error {
-	for _, field := range form.record.Collection().Schema.Fields() {
-		if v, ok := form.Data[field.Name]; ok {
-			form.Data[field.Name] = field.PrepareValue(v)
-		}
-	}
-	return nil
 }
 
 // LoadRequest extracts the json or multipart/form-data request data
@@ -235,15 +225,126 @@ func (form *RecordUpsert) normalizeData() error {
 // For single file upload fields, you can skip the index and directly
 // reset the field using its field name (eg. `myfile = null`).
 func (form *RecordUpsert) LoadRequest(r *http.Request, keyPrefix string) error {
-	requestData, err := form.extractRequestData(r, keyPrefix)
+	requestData, uploadedFiles, err := form.extractRequestData(r, keyPrefix)
 	if err != nil {
 		return err
 	}
 
-	return form.LoadData(requestData)
+	if err := form.LoadData(requestData); err != nil {
+		return err
+	}
+
+	for key, files := range uploadedFiles {
+		form.AddFiles(key, files...)
+	}
+
+	return nil
 }
 
-// LoadData loads and normalizes the provided data into the form.
+// AddFiles adds the provided file(s) to the specified file field.
+//
+// If the file field is a SINGLE-value file field (aka. "Max Select = 1"),
+// then the newly added file will REPLACE the existing one.
+// In this case if you pass more than 1 files only the first one will be assigned.
+//
+// If the file field is a MULTI-value file field (aka. "Max Select > 1"),
+// then the newly added file(s) will be APPENDED to the existing one(s).
+//
+// Example
+//
+// 	f1, _ := filesystem.NewFileFromPath("/path/to/file1.txt")
+// 	f2, _ := filesystem.NewFileFromPath("/path/to/file2.txt")
+// 	form.AddFiles("documents", f1, f2)
+func (form *RecordUpsert) AddFiles(key string, files ...*filesystem.File) error {
+	field := form.record.Collection().Schema.GetFieldByName(key)
+	if field == nil || field.Type != schema.FieldTypeFile {
+		return errors.New("invalid field key")
+	}
+
+	options, ok := field.Options.(*schema.FileOptions)
+	if !ok {
+		return errors.New("failed to initilize field options")
+	}
+
+	if len(files) == 0 {
+		return nil // nothing to upload
+	}
+
+	if form.filesToUpload == nil {
+		form.filesToUpload = map[string][]*filesystem.File{}
+	}
+
+	oldNames := list.ToUniqueStringSlice(form.data[key])
+
+	if options.MaxSelect == 1 {
+		// mark previous file(s) for deletion before replacing
+		if len(oldNames) > 0 {
+			form.filesToDelete = list.ToUniqueStringSlice(append(form.filesToDelete, oldNames...))
+		}
+
+		// replace
+		form.filesToUpload[key] = []*filesystem.File{files[0]}
+		form.data[key] = field.PrepareValue(files[0].Name)
+	} else {
+		// append
+		form.filesToUpload[key] = append(form.filesToUpload[key], files...)
+		for _, f := range files {
+			oldNames = append(oldNames, f.Name)
+		}
+		form.data[key] = field.PrepareValue(oldNames)
+	}
+
+	return nil
+}
+
+// RemoveFiles removes a single or multiple file from the specified file field.
+//
+// NB! If filesToDelete is not set it will remove all existing files
+// assigned to the file field (including those assigned with AddFiles)!
+//
+// Example
+//
+//  // mark only only 2 files for removal
+// 	form.AddFiles("documents", "file1_aw4bdrvws6.txt", "file2_xwbs36bafv.txt")
+//
+// 	// mark all "documents" files for removal
+// 	form.AddFiles("documents")
+func (form *RecordUpsert) RemoveFiles(key string, toDelete ...string) error {
+	field := form.record.Collection().Schema.GetFieldByName(key)
+	if field == nil || field.Type != schema.FieldTypeFile {
+		return errors.New("invalid field key")
+	}
+
+	existing := list.ToUniqueStringSlice(form.data[key])
+
+	// mark all files for deletion
+	if len(toDelete) == 0 {
+		toDelete = make([]string, len(existing))
+		copy(toDelete, existing)
+	}
+
+	// check for existing files
+	for i := len(existing) - 1; i >= 0; i-- {
+		if list.ExistInSlice(existing[i], toDelete) {
+			form.filesToDelete = append(form.filesToDelete, existing[i])
+			existing = append(existing[:i], existing[i+1:]...)
+		}
+	}
+
+	// check for newly uploaded files
+	for i := len(form.filesToUpload[key]) - 1; i >= 0; i-- {
+		f := form.filesToUpload[key][i]
+		if list.ExistInSlice(f.Name, toDelete) {
+			form.filesToUpload[key] = append(form.filesToUpload[key][:i], form.filesToUpload[key][i+1:]...)
+		}
+	}
+
+	form.data[key] = field.PrepareValue(existing)
+
+	return nil
+}
+
+// LoadData loads and normalizes the provided regular record data fields into the form.
 //
 // To DELETE previously uploaded file(s) you can suffix the field name
 // with the file index or filename (eg. `myfile.0`) and set it to null or empty string.
@@ -296,28 +397,26 @@ func (form *RecordUpsert) LoadData(requestData map[string]any) error {
 		value = field.PrepareValue(value)
 
 		if field.Type != schema.FieldTypeFile {
-			form.Data[key] = value
+			form.data[key] = value
 			continue
 		}
 
-		options, _ := field.Options.(*schema.FileOptions)
-		oldNames := list.ToUniqueStringSlice(form.Data[key])
-
 		// -----------------------------------------------------------
 		// Delete previously uploaded file(s)
 		// -----------------------------------------------------------
 
+		oldNames := list.ToUniqueStringSlice(form.data[key])
+
 		// if empty value was set, mark all previously uploaded files for deletion
 		if len(list.ToUniqueStringSlice(value)) == 0 && len(oldNames) > 0 {
-			form.filesToDelete = append(form.filesToDelete, oldNames...)
+			form.RemoveFiles(key)
-			form.Data[key] = []string{}
 		} else if len(oldNames) > 0 {
-			indexesToDelete := make([]int, 0, len(extendedData))
+			toDelete := []string{}
 
 			// search for individual file name to delete (eg. "file.test.png = null")
-			for i, name := range oldNames {
+			for _, name := range oldNames {
 				if v, ok := extendedData[key+"."+name]; ok && cast.ToString(v) == "" {
-					indexesToDelete = append(indexesToDelete, i)
+					toDelete = append(toDelete, name)
 				}
 			}
 
@@ -329,52 +428,17 @@ func (form *RecordUpsert) LoadData(requestData map[string]any) error {
 					if indexErr != nil || index >= len(oldNames) {
 						continue
 					}
-					indexesToDelete = append(indexesToDelete, index)
+					toDelete = append(toDelete, oldNames[index])
 				}
 			}
 
-			// slice to fill only with the non-deleted indexes
+			if len(toDelete) > 0 {
-			nonDeleted := make([]string, 0, len(oldNames))
+				form.RemoveFiles(key, toDelete...)
-			for i, name := range oldNames {
-				// not marked for deletion
-				if !list.ExistInSlice(i, indexesToDelete) {
-					nonDeleted = append(nonDeleted, name)
-					continue
 			}
-
-				// store the id to actually delete the file later
-				form.filesToDelete = append(form.filesToDelete, name)
-			}
-			form.Data[key] = nonDeleted
-		}
-
-		// -----------------------------------------------------------
-		// Check for new uploaded file
-		// -----------------------------------------------------------
-
-		if len(form.filesToUpload[key]) == 0 {
-			continue
-		}
-
-		// refresh oldNames list
-		oldNames = list.ToUniqueStringSlice(form.Data[key])
-
-		if options.MaxSelect == 1 {
-			// delete previous file(s) before replacing
-			if len(oldNames) > 0 {
-				form.filesToDelete = list.ToUniqueStringSlice(append(form.filesToDelete, oldNames...))
-			}
-			form.Data[key] = form.filesToUpload[key][0].Name
-		} else if options.MaxSelect > 1 {
-			// append the id of each uploaded file instance
-			for _, file := range form.filesToUpload[key] {
-				oldNames = append(oldNames, file.Name)
-			}
-			form.Data[key] = oldNames
 		}
 	}
 
-	return form.normalizeData()
+	return nil
 }
 
 // Validate makes the form validatable by implementing [validation.Validatable] interface.
@@ -464,7 +528,7 @@ func (form *RecordUpsert) Validate() error {
 		form.dao,
 		form.record,
 		form.filesToUpload,
-	).Validate(form.Data)
+	).Validate(form.data)
 }
 
 func (form *RecordUpsert) checkUniqueUsername(value any) error {
@@ -592,7 +656,7 @@ func (form *RecordUpsert) ValidateAndFill() error {
 	}
 
 	// bulk load the remaining form data
-	form.record.Load(form.Data)
+	form.record.Load(form.data)
 
 	return nil
 }

forms/record_upsert_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -16,6 +17,7 @@ import (
 	"github.com/pocketbase/pocketbase/forms"
 	"github.com/pocketbase/pocketbase/models"
 	"github.com/pocketbase/pocketbase/tests"
+	"github.com/pocketbase/pocketbase/tools/filesystem"
 	"github.com/pocketbase/pocketbase/tools/list"
 )
 
@@ -44,9 +46,9 @@ func TestNewRecordUpsert(t *testing.T) {
 
 	form := forms.NewRecordUpsert(app, record)
 
-	val := form.Data["title"]
+	val := form.Data()["title"]
 	if val != "test_value" {
-		t.Errorf("Expected record data to be loaded, got %v", form.Data)
+		t.Errorf("Expected record data to be loaded, got %v", form.Data())
 	}
 }
 
@@ -107,15 +109,15 @@ func TestRecordUpsertLoadRequestJson(t *testing.T) {
 		t.Fatalf("Expect id field to be %q, got %q", "test_id", form.Id)
 	}
 
-	if v, ok := form.Data["text"]; !ok || v != "test123" {
+	if v, ok := form.Data()["text"]; !ok || v != "test123" {
 		t.Fatalf("Expect title field to be %q, got %q", "test123", v)
 	}
 
-	if v, ok := form.Data["unknown"]; ok {
+	if v, ok := form.Data()["unknown"]; ok {
 		t.Fatalf("Didn't expect unknown field to be set, got %v", v)
 	}
 
-	fileOne, ok := form.Data["file_one"]
+	fileOne, ok := form.Data()["file_one"]
 	if !ok {
 		t.Fatal("Expect file_one field to be set")
 	}
@@ -123,7 +125,7 @@ func TestRecordUpsertLoadRequestJson(t *testing.T) {
 		t.Fatalf("Expect file_one field to be empty string, got %v", fileOne)
 	}
 
-	fileMany, ok := form.Data["file_many"]
+	fileMany, ok := form.Data()["file_many"]
 	if !ok || fileMany == nil {
 		t.Fatal("Expect file_many field to be set")
 	}
@@ -168,15 +170,15 @@ func TestRecordUpsertLoadRequestMultipart(t *testing.T) {
 		t.Fatalf("Expect id field to be %q, got %q", "test_id", form.Id)
 	}
 
-	if v, ok := form.Data["text"]; !ok || v != "test123" {
+	if v, ok := form.Data()["text"]; !ok || v != "test123" {
 		t.Fatalf("Expect text field to be %q, got %q", "test123", v)
 	}
 
-	if v, ok := form.Data["unknown"]; ok {
+	if v, ok := form.Data()["unknown"]; ok {
 		t.Fatalf("Didn't expect unknown field to be set, got %v", v)
 	}
 
-	fileOne, ok := form.Data["file_one"]
+	fileOne, ok := form.Data()["file_one"]
 	if !ok {
 		t.Fatal("Expect file_one field to be set")
 	}
@@ -184,7 +186,7 @@ func TestRecordUpsertLoadRequestMultipart(t *testing.T) {
 		t.Fatalf("Expect file_one field to be empty string, got %v", fileOne)
 	}
 
-	fileMany, ok := form.Data["file_many"]
+	fileMany, ok := form.Data()["file_many"]
 	if !ok || fileMany == nil {
 		t.Fatal("Expect file_many field to be set")
 	}
@@ -214,11 +216,11 @@ func TestRecordUpsertLoadData(t *testing.T) {
 		t.Fatal(loadErr)
 	}
 
-	if v, ok := form.Data["title"]; !ok || v != "test_new" {
+	if v, ok := form.Data()["title"]; !ok || v != "test_new" {
 		t.Fatalf("Expect title field to be %v, got %v", "test_new", v)
 	}
 
-	if v, ok := form.Data["active"]; !ok || v != true {
+	if v, ok := form.Data()["active"]; !ok || v != true {
 		t.Fatalf("Expect active field to be %v, got %v", true, v)
 	}
 }
@@ -498,7 +500,7 @@ func TestRecordUpsertSubmitInterceptors(t *testing.T) {
 	}
 
 	form := forms.NewRecordUpsert(app, record)
-	form.Data["title"] = "test_new"
+	form.Data()["title"] = "test_new"
 
 	testErr := errors.New("test_error")
 	interceptorRecordTitle := ""
@@ -533,7 +535,7 @@ func TestRecordUpsertSubmitInterceptors(t *testing.T) {
 		t.Fatalf("Expected interceptor2 to be called")
 	}
 
-	if interceptorRecordTitle != form.Data["title"].(string) {
+	if interceptorRecordTitle != form.Data()["title"].(string) {
 		t.Fatalf("Expected the form model to be filled before calling the interceptors")
 	}
 }
@@ -863,3 +865,75 @@ func TestRecordUpsertAuthRecord(t *testing.T) {
 		}
 	}
 }
+
+func TestRecordUpsertAddAndRemoveFiles(t *testing.T) {
+	app, _ := tests.NewTestApp()
+	defer app.Cleanup()
+
+	recordBefore, err := app.Dao().FindRecordById("demo1", "84nmscqy84lsi1t")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// create test temp files
+	tempDir := filepath.Join(app.DataDir(), "temp")
+	if err := os.MkdirAll(app.DataDir(), os.ModePerm); err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tempDir)
+	tmpFile, _ := os.CreateTemp(os.TempDir(), "tmpfile1-*.txt")
+	tmpFile.Close()
+
+	form := forms.NewRecordUpsert(app, recordBefore)
+
+	f1, err := filesystem.NewFileFromPath(tmpFile.Name())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f2, err := filesystem.NewFileFromPath(tmpFile.Name())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f3, err := filesystem.NewFileFromPath(tmpFile.Name())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	form.AddFiles("file_one", f1) // should replace the existin file
+
+	form.AddFiles("file_many", f2, f3) // should append
+
+	form.RemoveFiles("file_many", "300_WlbFWSGmW9.png", "logo_vcfJJG5TAh.svg") // should remove
+
+	if err := form.Submit(); err != nil {
+		t.Fatalf("Failed to submit the RecordUpsert form, got %v", err)
+	}
+
+	recordAfter, err := app.Dao().FindRecordById("demo1", "84nmscqy84lsi1t")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// ensure files deletion
+	if hasRecordFile(app, recordAfter, "test_d61b33QdDU.txt") {
+		t.Fatalf("Expected the old file_one file to be deleted")
+	}
+	if hasRecordFile(app, recordAfter, "300_WlbFWSGmW9.png") {
+		t.Fatalf("Expected 300_WlbFWSGmW9.png to be deleted")
+	}
+	if hasRecordFile(app, recordAfter, "logo_vcfJJG5TAh.svg") {
+		t.Fatalf("Expected logo_vcfJJG5TAh.svg to be deleted")
+	}
+
+	fileOne := recordAfter.GetStringSlice("file_one")
+	if len(fileOne) == 0 {
+		t.Fatalf("Expected new file_one file to be uploaded")
+	}
+
+	fileMany := recordAfter.GetStringSlice("file_many")
+	if len(fileMany) != 3 {
+		t.Fatalf("Expected file_many to be 3, got %v", fileMany)
+	}
+}

tests/request.go

@@ -29,12 +29,14 @@ func MockMultipartData(data map[string]string, fileFields ...string) (*bytes.Buf
 	// write file fields
 	for _, fileField := range fileFields {
 		// create a test temporary file
+		err := func() error {
 			tmpFile, err := os.CreateTemp(os.TempDir(), "tmpfile-*.txt")
 			if err != nil {
-			return nil, nil, err
+				return err
 			}
+
 			if _, err := tmpFile.Write([]byte("test")); err != nil {
-			return nil, nil, err
+				return err
 			}
 			tmpFile.Seek(0, 0)
 			defer tmpFile.Close()
@@ -43,10 +45,16 @@ func MockMultipartData(data map[string]string, fileFields ...string) (*bytes.Buf
 			// stub uploaded file
 			w, err := mp.CreateFormFile(fileField, tmpFile.Name())
 			if err != nil {
-			return nil, mp, err
+				return err
 			}
 			if _, err := io.Copy(w, tmpFile); err != nil {
-			return nil, mp, err
+				return err
+			}
+
+			return nil
+		}()
+		if err != nil {
+			return nil, nil, err
 		}
 	}