Simplify copy file logic

server/filesystem/filesystem.go

@@ -2,7 +2,6 @@ package filesystem
 
 import (
 	"bufio"
-	"fmt"
 	"github.com/gabriel-vasile/mimetype"
 	"github.com/karrick/godirwalk"
 	"github.com/pkg/errors"
@@ -222,6 +221,42 @@ func (fs *Filesystem) Chown(path string) error {
 	})
 }
 
+// Begin looping up to 50 times to try and create a unique copy file name. This will take
+// an input of "file.txt" and generate "file copy.txt". If that name is already taken, it will
+// then try to write "file copy 2.txt" and so on, until reaching 50 loops. At that point we
+// won't waste anymore time, just use the current timestamp and make that copy.
+//
+// Could probably make this more efficient by checking if there are any files matching the copy
+// pattern, and trying to find the highest number and then incrementing it by one rather than
+// looping endlessly.
+func (fs *Filesystem) findCopySuffix(dir string, name string, extension string) (string, error) {
+	var i int
+	var suffix = " copy"
+
+	for i = 0; i < 51; i++ {
+		if i > 0 {
+			suffix = " copy " + strconv.Itoa(i)
+		}
+
+		n := name + suffix + extension
+		// If we stat the file and it does not exist that means we're good to create the copy. If it
+		// does exist, we'll just continue to the next loop and try again.
+		if _, err := fs.Stat(path.Join(dir, n)); err != nil {
+			if !os.IsNotExist(err) {
+				return "", err
+			}
+
+			break
+		}
+
+		if i == 50 {
+			suffix = "copy." + time.Now().Format(time.RFC3339)
+		}
+	}
+
+	return name + suffix + extension, nil
+}
+
 // Copies a given file to the same location and appends a suffix to the file to indicate that
 // it has been copied.
 func (fs *Filesystem) Copy(p string) error {
@@ -257,65 +292,15 @@ func (fs *Filesystem) Copy(p string) error {
 		name = strings.TrimSuffix(name, ".tar")
 	}
 
-	// Begin looping up to 50 times to try and create a unique copy file name. This will take
-	// an input of "file.txt" and generate "file copy.txt". If that name is already taken, it will
-	// then try to write "file copy 2.txt" and so on, until reaching 50 loops. At that point we
-	// won't waste anymore time, just use the current timestamp and make that copy.
-	//
-	// Could probably make this more efficient by checking if there are any files matching the copy
-	// pattern, and trying to find the highest number and then incrementing it by one rather than
-	// looping endlessly.
-	var i int
-	copySuffix := " copy"
-	for i = 0; i < 51; i++ {
-		if i > 0 {
-			copySuffix = " copy " + strconv.Itoa(i)
-		}
-
-		tryName := fmt.Sprintf("%s%s%s", name, copySuffix, extension)
-		tryLocation, err := fs.SafePath(path.Join(relative, tryName))
-		if err != nil {
-			return errors.WithStack(err)
-		}
-
-		// If the file exists, continue to the next loop, otherwise we're good to start a copy.
-		if _, err := os.Stat(tryLocation); err != nil && !os.IsNotExist(err) {
-			return errors.WithStack(err)
-		} else if os.IsNotExist(err) {
-			break
-		}
-
-		if i == 50 {
-			copySuffix = "." + time.Now().Format(time.RFC3339)
-		}
-	}
-
-	finalPath, err := fs.SafePath(path.Join(relative, fmt.Sprintf("%s%s%s", name, copySuffix, extension)))
-	if err != nil {
-		return errors.WithStack(err)
-	}
-
 	source, err := os.Open(cleaned)
 	if err != nil {
 		return errors.WithStack(err)
 	}
 	defer source.Close()
 
-	dest, err := os.Create(finalPath)
+	n, err := fs.findCopySuffix(relative, name, extension)
-	if err != nil {
-		return errors.WithStack(err)
-	}
-	defer dest.Close()
 
-	buf := make([]byte, 1024*4)
+	return fs.Writefile(path.Join(relative, n), source)
-	if _, err := io.CopyBuffer(dest, source, buf); err != nil {
-		return errors.WithStack(err)
-	}
-
-	// Once everything is done, increment the disk space used.
-	fs.addDisk(s.Size())
-
-	return nil
 }
 
 // Deletes a file or folder from the system. Prevents the user from accidentally

server/filesystem/filesystem_test.go

@@ -405,7 +405,7 @@ func TestFilesystem_Copy(t *testing.T) {
 
 			err = fs.Copy("../ext-source.txt")
 			g.Assert(err).IsNotNil()
-			g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
+			g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue()
 		})
 
 		g.It("should return an error if the source directory is outside the root", func() {
@@ -417,11 +417,11 @@ func TestFilesystem_Copy(t *testing.T) {
 
 			err = fs.Copy("../nested/in/dir/ext-source.txt")
 			g.Assert(err).IsNotNil()
-			g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
+			g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue()
 
 			err = fs.Copy("nested/in/../../../nested/in/dir/ext-source.txt")
 			g.Assert(err).IsNotNil()
-			g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
+			g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue()
 		})
 
 		g.It("should return an error if the source is a directory", func() {
@@ -512,7 +512,7 @@ func TestFilesystem_Delete(t *testing.T) {
 
 			err = fs.Delete("../ext-source.txt")
 			g.Assert(err).IsNotNil()
-			g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
+			g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue()
 		})
 
 		g.It("does not allow the deletion of the root directory", func() {