diff --git a/pkg/files/filehandling.go b/pkg/files/filehandling.go index 91f0d55e6..1d6f59c07 100644 --- a/pkg/files/filehandling.go +++ b/pkg/files/filehandling.go @@ -20,7 +20,6 @@ import ( "os" "testing" - "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/modules/keyvalue" @@ -54,9 +53,8 @@ func initFixtures(t *testing.T) { // InitTestFileFixtures initializes file fixtures func InitTestFileFixtures(t *testing.T) { - // Init fixture files - filename := config.FilesBasePath.GetString() + "/1" - err := afero.WriteFile(afs, filename, []byte("testfile1"), 0644) + testfile := &File{ID: 1} + err := afero.WriteFile(afs, testfile.getAbsoluteFilePath(), []byte("testfile1"), 0644) require.NoError(t, err) } @@ -84,6 +82,6 @@ func InitTests() { } // FileStat stats a file. This is an exported function to be able to test this from outide of the package -func FileStat(filename string) (os.FileInfo, error) { - return afs.Stat(filename) +func FileStat(file *File) (os.FileInfo, error) { + return afs.Stat(file.getAbsoluteFilePath()) } diff --git a/pkg/files/files.go b/pkg/files/files.go index 1c2f018fa..8d7a363f8 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -128,11 +128,8 @@ func CreateWithMimeAndSession(s *xorm.Session, f io.Reader, realname string, rea } // Delete removes a file from the DB and the file system -func (f *File) Delete() (err error) { - s := db.NewSession() - defer s.Close() - - deleted, err := s.Where("id = ?", f.ID).Delete(f) +func (f *File) Delete(s *xorm.Session) (err error) { + deleted, err := s.Where("id = ?", f.ID).Delete(&File{}) if err != nil { _ = s.Rollback() return err diff --git a/pkg/files/files_test.go b/pkg/files/files_test.go index 22af6fe43..439634fe7 100644 --- a/pkg/files/files_test.go +++ b/pkg/files/files_test.go @@ -21,6 +21,8 @@ import ( "os" "testing" + "code.vikunja.io/api/pkg/db" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -84,15 +86,20 @@ func TestCreate(t *testing.T) { func TestFile_Delete(t *testing.T) { t.Run("Normal", func(t *testing.T) { + s := db.NewSession() + defer s.Close() initFixtures(t) f := &File{ID: 1} - err := f.Delete() + err := f.Delete(s) require.NoError(t, err) }) t.Run("Nonexisting", func(t *testing.T) { + s := db.NewSession() + defer s.Close() initFixtures(t) f := &File{ID: 9999} - err := f.Delete() + + err := f.Delete(s) require.Error(t, err) assert.True(t, IsErrFileDoesNotExist(err)) }) diff --git a/pkg/models/export.go b/pkg/models/export.go index ad75f6d73..d41dac9c4 100644 --- a/pkg/models/export.go +++ b/pkg/models/export.go @@ -398,7 +398,7 @@ func RegisterOldExportCleanupCron() { log.Debugf(logPrefix+"Removing %d old user data exports...", len(fs)) for _, f := range fs { - err = f.Delete() + err = f.Delete(s) if err != nil { log.Errorf(logPrefix+"Could not remove user export file %d: %s", f.ID, err) return diff --git a/pkg/models/project.go b/pkg/models/project.go index 8d4cdbbbe..d7045de60 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1204,8 +1204,11 @@ func (p *Project) DeleteBackgroundFileIfExists() (err error) { return } + s := db.NewSession() + defer s.Close() + file := files.File{ID: p.BackgroundFileID} - err = file.Delete() + err = file.Delete(s) if err != nil && files.IsErrFileDoesNotExist(err) { return nil } diff --git a/pkg/models/task_attachment.go b/pkg/models/task_attachment.go index 4a9247d80..74dcbe78e 100644 --- a/pkg/models/task_attachment.go +++ b/pkg/models/task_attachment.go @@ -77,7 +77,7 @@ func (ta *TaskAttachment) NewAttachment(s *xorm.Session, f io.ReadCloser, realna ta.CreatedBy, err = GetUserOrLinkShareUser(s, a) if err != nil { // remove the uploaded file if adding it to the db fails - if err2 := file.Delete(); err2 != nil { + if err2 := file.Delete(s); err2 != nil { return err2 } return err @@ -87,7 +87,7 @@ func (ta *TaskAttachment) NewAttachment(s *xorm.Session, f io.ReadCloser, realna _, err = s.Insert(ta) if err != nil { // remove the uploaded file if adding it to the db fails - if err2 := file.Delete(); err2 != nil { + if err2 := file.Delete(s); err2 != nil { return err2 } return err @@ -326,7 +326,7 @@ func (ta *TaskAttachment) Delete(s *xorm.Session, a web.Auth) error { } // Delete the underlying file - err = ta.File.Delete() + err = ta.File.Delete(s) // If the file does not exist, we don't want to error out if err != nil && files.IsErrFileDoesNotExist(err) { return nil diff --git a/pkg/models/task_attachment_test.go b/pkg/models/task_attachment_test.go index d9770a000..2ede7544c 100644 --- a/pkg/models/task_attachment_test.go +++ b/pkg/models/task_attachment_test.go @@ -19,7 +19,7 @@ package models import ( "io" "os" - "strconv" + "path/filepath" "testing" "code.vikunja.io/api/pkg/config" @@ -51,7 +51,7 @@ func TestTaskAttachment_ReadOne(t *testing.T) { // Load the actual attachment file and check its content err = ta.File.LoadFileByID() require.NoError(t, err) - assert.Equal(t, config.FilesBasePath.GetString()+"/1", ta.File.File.Name()) + assert.Equal(t, filepath.Join(config.ServiceRootpath.GetString(), config.FilesBasePath.GetString(), "/1"), ta.File.File.Name()) content := make([]byte, 9) read, err := ta.File.File.Read(content) require.NoError(t, err) @@ -122,7 +122,7 @@ func TestTaskAttachment_NewAttachment(t *testing.T) { err := ta.NewAttachment(s, tf, "testfile", 100, testuser) require.NoError(t, err) assert.NotEqual(t, 0, ta.FileID) - _, err = files.FileStat("files/" + strconv.FormatInt(ta.FileID, 10)) + _, err = files.FileStat(ta.File) require.NoError(t, err) assert.False(t, os.IsNotExist(err)) assert.Equal(t, testuser.ID, ta.CreatedByID) @@ -158,23 +158,28 @@ func TestTaskAttachment_ReadAll(t *testing.T) { } func TestTaskAttachment_Delete(t *testing.T) { - db.LoadAndAssertFixtures(t) - s := db.NewSession() - defer s.Close() - u := &user.User{ID: 1} - files.InitTestFileFixtures(t) t.Run("Normal", func(t *testing.T) { + files.InitTestFileFixtures(t) + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + ta := &TaskAttachment{ID: 1} err := ta.Delete(s, u) require.NoError(t, err) // Check if the file itself was deleted - _, err = files.FileStat("/1") // The new file has the id 2 since it's the second attachment + _, err = files.FileStat(ta.File) // The new file has the id 2 since it's the second attachment + require.Error(t, err) assert.True(t, os.IsNotExist(err)) }) t.Run("Nonexisting", func(t *testing.T) { files.InitTestFileFixtures(t) + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + ta := &TaskAttachment{ID: 9999} err := ta.Delete(s, u) require.Error(t, err) @@ -182,6 +187,10 @@ func TestTaskAttachment_Delete(t *testing.T) { }) t.Run("Existing attachment, nonexisting file", func(t *testing.T) { files.InitTestFileFixtures(t) + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + ta := &TaskAttachment{ID: 2} err := ta.Delete(s, u) require.NoError(t, err) diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 65ebb3f30..514ea195c 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -79,7 +79,7 @@ type Task struct { EndDate time.Time `xorm:"DATETIME INDEX null 'end_date'" json:"end_date" query:"-"` // An array of users who are assigned to this task Assignees []*user.User `xorm:"-" json:"assignees"` - // An array of labels which are associated with this task. This property is read-only, you must use the seperate endpoint to add labels to a task. + // An array of labels which are associated with this task. This property is read-only, you must use the separate endpoint to add labels to a task. Labels []*Label `xorm:"-" json:"labels"` // The task color in hex HexColor string `xorm:"varchar(6) null" json:"hex_color" valid:"runelength(0|7)" maxLength:"7"` diff --git a/pkg/modules/background/unsplash/unsplash.go b/pkg/modules/background/unsplash/unsplash.go index 7a72aea9b..8f406c461 100644 --- a/pkg/modules/background/unsplash/unsplash.go +++ b/pkg/modules/background/unsplash/unsplash.go @@ -291,7 +291,7 @@ func (p *Provider) Set(s *xorm.Session, image *background.Image, project *models // Remove the old background if one exists if project.BackgroundFileID != 0 { file := files.File{ID: project.BackgroundFileID} - err = file.Delete() + err = file.Delete(s) if err != nil && !files.IsErrFileDoesNotExist(err) { return err } diff --git a/pkg/modules/background/upload/upload.go b/pkg/modules/background/upload/upload.go index d2b98d289..32aa9f161 100644 --- a/pkg/modules/background/upload/upload.go +++ b/pkg/modules/background/upload/upload.go @@ -56,7 +56,7 @@ func (p *Provider) Set(s *xorm.Session, img *background.Image, project *models.P // Remove the old background if one exists if project.BackgroundFileID != 0 { file := files.File{ID: project.BackgroundFileID} - err := file.Delete() + err := file.Delete(s) if err != nil && !files.IsErrFileDoesNotExist(err) { return err } diff --git a/pkg/routes/api/v1/avatar.go b/pkg/routes/api/v1/avatar.go index fc536d314..137e583a2 100644 --- a/pkg/routes/api/v1/avatar.go +++ b/pkg/routes/api/v1/avatar.go @@ -167,7 +167,7 @@ func UploadAvatar(c echo.Context) (err error) { // Remove the old file if one exists if u.AvatarFileID != 0 { f := &files.File{ID: u.AvatarFileID} - if err := f.Delete(); err != nil { + if err := f.Delete(s); err != nil { if !files.IsErrFileDoesNotExist(err) { _ = s.Rollback() return handler.HandleHTTPError(err)