diff --git a/pkg/files/filehandling.go b/pkg/files/filehandling.go index c0c3dcfcd..d2acb8278 100644 --- a/pkg/files/filehandling.go +++ b/pkg/files/filehandling.go @@ -17,12 +17,14 @@ package files import ( + "bytes" "errors" "fmt" "os" "path/filepath" "strings" "testing" + "time" "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" @@ -107,6 +109,7 @@ func initS3FileHandler() error { func initLocalFileHandler() { fs = afero.NewOsFs() afs = &afero.Afero{Fs: fs} + s3Client = nil setDefaultLocalConfig() } @@ -116,13 +119,20 @@ func InitFileHandler() error { switch fileType { case "s3": - return initS3FileHandler() + if err := initS3FileHandler(); err != nil { + return err + } case "local": initLocalFileHandler() - return nil default: return fmt.Errorf("invalid file storage type '%s': must be 'local' or 's3'", fileType) } + + if err := ValidateFileStorage(); err != nil { + return fmt.Errorf("storage validation failed: %w", err) + } + + return nil } // InitTestFileHandler initializes a new memory file system for testing @@ -175,3 +185,22 @@ func InitTests() { func FileStat(file *File) (os.FileInfo, error) { return afs.Stat(file.getAbsoluteFilePath()) } + +// ValidateFileStorage checks that the configured file storage is writable +// by creating and removing a temporary file. +func ValidateFileStorage() error { + filename := fmt.Sprintf(".vikunja-check-%d", time.Now().UnixNano()) + path := filepath.Join(config.FilesBasePath.GetString(), filename) + + err := writeToStorage(path, bytes.NewReader([]byte{}), 0) + if err != nil { + return fmt.Errorf("failed to create test file at %s: %w", path, err) + } + + err = afs.Remove(path) + if err != nil { + return fmt.Errorf("failed to remove test file at %s: %w", path, err) + } + + return nil +} diff --git a/pkg/files/files.go b/pkg/files/files.go index 2f25a6c83..01b5cfd16 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -152,22 +152,13 @@ func (f *File) Delete(s *xorm.Session) (err error) { return keyvalue.DecrBy(metrics.FilesCountKey, 1) } -// Save saves a file to storage -func (f *File) Save(fcontent io.Reader) (err error) { - +// writeToStorage writes content to the given path, handling both local and S3 backends +func writeToStorage(path string, content io.Reader, size uint64) error { if s3Client == nil { - err = afs.WriteReader(f.getAbsoluteFilePath(), fcontent) - if err != nil { - return fmt.Errorf("failed to save file: %w", err) - } - - return keyvalue.IncrBy(metrics.FilesCountKey, 1) + return afs.WriteReader(path, content) } - // For S3 storage, use PutObject directly with Content-Length to enable streaming - // without buffering the entire file in memory. Some S3-compatible services - // (like MinIO) require Content-Length to be set explicitly. - body, contentLength, cleanup, err := prepareS3UploadBody(fcontent, f.Size) + body, contentLength, cleanup, err := prepareS3UploadBody(content, size) if err != nil { return err } @@ -177,14 +168,22 @@ func (f *File) Save(fcontent io.Reader) (err error) { _, err = s3Client.PutObject(&s3.PutObjectInput{ Bucket: aws.String(s3Bucket), - Key: aws.String(f.getAbsoluteFilePath()), + Key: aws.String(path), Body: body, ContentLength: aws.Int64(contentLength), }) if err != nil { return fmt.Errorf("failed to upload file to S3: %w", err) } + return nil +} +// Save saves a file to storage +func (f *File) Save(fcontent io.Reader) error { + err := writeToStorage(f.getAbsoluteFilePath(), fcontent, f.Size) + if err != nil { + return fmt.Errorf("failed to save file: %w", err) + } return keyvalue.IncrBy(metrics.FilesCountKey, 1) } diff --git a/pkg/files/s3_test.go b/pkg/files/s3_test.go index badcde106..1f2ef5a72 100644 --- a/pkg/files/s3_test.go +++ b/pkg/files/s3_test.go @@ -220,9 +220,12 @@ func TestInitFileHandler_S3Configuration(t *testing.T) { config.FilesS3AccessKey.Set("test-access-key") config.FilesS3SecretKey.Set("test-secret-key") - // This should not return an error with valid configuration + // With valid configuration, InitFileHandler will succeed at config parsing + // but fail at storage validation (since the S3 endpoint isn't real). + // The error should be from validation, not from config parsing. err := InitFileHandler() - assert.NoError(t, err) + require.Error(t, err) + assert.Contains(t, err.Error(), "storage validation failed") }) t.Run("missing S3 endpoint", func(t *testing.T) { @@ -281,14 +284,20 @@ func TestInitFileHandler_S3Configuration(t *testing.T) { func TestInitFileHandler_LocalFilesystem(t *testing.T) { // Save original config values originalType := config.FilesType.GetString() + originalBasePath := config.FilesBasePath.GetString() + + // Create a temp directory for the test + tempDir := t.TempDir() // Restore config after test defer func() { config.FilesType.Set(originalType) + config.FilesBasePath.Set(originalBasePath) }() - // Test with local filesystem + // Test with local filesystem using writable temp directory config.FilesType.Set("local") + config.FilesBasePath.Set(tempDir) // This should not return an error err := InitFileHandler()