Download: Do not compress pictures added to zip archives #4298

Signed-off-by: Michael Mayer <michael@photoprism.app>

internal/api/download_album.go

@@ -54,21 +54,23 @@ func DownloadAlbum(router *gin.RouterGroup) {
 		AddDownloadHeader(c, zipFileName)
 
 		zipWriter := zip.NewWriter(c.Writer)
-		defer zipWriter.Close()
+		defer func(w *zip.Writer) {
+			logErr("zip", w.Close())
+		}(zipWriter)
 
 		var aliases = make(map[string]int)
 
 		for _, file := range files {
-			if file.FileHash == "" {
-				log.Warnf("download: empty file hash, skipped %s", clean.Log(file.FileName))
+			if file.FileName == "" {
+				log.Warnf("album: %s cannot be downloaded (empty file name)", clean.Log(file.FileUID))
 				continue
-			} else if file.FileName == "" {
-				log.Warnf("download: empty file name, skipped %s", clean.Log(file.FileUID))
+			} else if file.FileHash == "" {
+				log.Warnf("album: %s cannot be downloaded (empty file hash)", clean.Log(file.FileName))
 				continue
 			}
 
 			if file.FileSidecar {
-				log.Debugf("download: skipped sidecar %s", clean.Log(file.FileName))
+				log.Debugf("album: sidecar file %s not included in download", clean.Log(file.FileName))
 				continue
 			}
 
@@ -83,18 +85,18 @@ func DownloadAlbum(router *gin.RouterGroup) {
 			aliases[key] += 1
 
 			if fs.FileExists(fileName) {
-				if err := addFileToZip(zipWriter, fileName, alias); err != nil {
-					log.Errorf("download: failed adding %s to album zip (%s)", clean.Log(file.FileName), err)
+				if zipErr := fs.ZipFile(zipWriter, fileName, alias, false); zipErr != nil {
+					log.Errorf("download: failed to add %s (%s)", clean.Log(file.FileName), zipErr)
 					Abort(c, http.StatusInternalServerError, i18n.ErrZipFailed)
 					return
 				}
 
 				log.Infof("download: added %s as %s", clean.Log(file.FileName), clean.Log(alias))
 			} else {
-				log.Warnf("download: album file %s is missing", clean.Log(file.FileName))
+				log.Warnf("download: %s not found", clean.Log(file.FileName))
 			}
 		}
 
-		log.Infof("download: created %s [%s]", clean.Log(zipFileName), time.Since(start))
+		log.Infof("album: %s has been downloaded [%s]", clean.Log(a.AlbumTitle), time.Since(start))
 	})
 }

internal/api/import.go

@@ -117,7 +117,7 @@ func StartImport(router *gin.RouterGroup) {
 		// Delete empty import directory.
 		if srcFolder != "" && importPath != conf.ImportPath() && fs.DirIsEmpty(importPath) {
 			if err := os.Remove(importPath); err != nil {
-				log.Errorf("import: failed deleting empty folder %s: %s", clean.Log(importPath), err)
+				log.Errorf("import: failed to delete empty folder %s: %s", clean.Log(importPath), err)
 			} else {
 				log.Infof("import: deleted empty folder %s", clean.Log(importPath))
 			}
@@ -125,7 +125,7 @@ func StartImport(router *gin.RouterGroup) {
 
 		// Update moments if files have been imported.
 		if n := len(imported); n == 0 {
-			log.Infof("import: no new files found to import", clean.Log(importPath))
+			log.Infof("import: found no new files to import from %s", clean.Log(importPath))
 		} else {
 			log.Infof("import: imported %s", english.Plural(n, "file", "files"))
 			if moments := get.Moments(); moments == nil {

internal/api/users_upload.go

@@ -214,7 +214,7 @@ func ProcessUserUpload(router *gin.RouterGroup) {
 		// Delete empty import directory.
 		if fs.DirIsEmpty(uploadPath) {
 			if err := os.Remove(uploadPath); err != nil {
-				log.Errorf("upload: failed deleting empty folder %s: %s", clean.Log(uploadPath), err)
+				log.Errorf("upload: failed to delete empty folder %s: %s", clean.Log(uploadPath), err)
 			} else {
 				log.Infof("upload: deleted empty folder %s", clean.Log(uploadPath))
 			}
@@ -222,7 +222,7 @@ func ProcessUserUpload(router *gin.RouterGroup) {
 
 		// Update moments if files have been imported.
 		if n := len(imported); n == 0 {
-			log.Infof("upload: no new files imported", clean.Log(uploadPath))
+			log.Infof("upload: found no new files to import from %s", clean.Log(uploadPath))
 		} else {
 			log.Infof("upload: imported %s", english.Plural(n, "file", "files"))
 			if moments := get.Moments(); moments == nil {

internal/api/zip.go

@@ -3,7 +3,6 @@ package api
 import (
 	"archive/zip"
 	"fmt"
-	"io"
 	"net/http"
 	"os"
 	"path"
@@ -108,6 +107,14 @@ func ZipCreate(router *gin.RouterGroup) {
 
 		// Add files to zip.
 		for _, file := range files {
+			if file.FileName == "" {
+				log.Warnf("download: %s cannot be downloaded (empty file name)", clean.Log(file.FileUID))
+				continue
+			} else if file.FileHash == "" {
+				log.Warnf("download: %s cannot be downloaded (empty file hash)", clean.Log(file.FileName))
+				continue
+			}
+
 			fileName := photoprism.FileName(file.FileRoot, file.FileName)
 			alias := file.DownloadName(dlName, 0)
 			key := strings.ToLower(alias)
@@ -119,22 +126,22 @@ func ZipCreate(router *gin.RouterGroup) {
 			aliases[key] += 1
 
 			if fs.FileExists(fileName) {
-				if err := addFileToZip(zipWriter, fileName, alias); err != nil {
-					log.Errorf("zip: failed adding %s to zip (%s)", clean.Log(file.FileName), err)
+				if zipErr := fs.ZipFile(zipWriter, fileName, alias, false); zipErr != nil {
+					log.Errorf("download: failed to add %s (%s)", clean.Log(file.FileName), zipErr)
 					Abort(c, http.StatusInternalServerError, i18n.ErrZipFailed)
 					return
 				}
 
-				log.Infof("zip: added %s as %s", clean.Log(file.FileName), clean.Log(alias))
+				log.Infof("download: added %s as %s", clean.Log(file.FileName), clean.Log(alias))
 			} else {
-				log.Warnf("zip: media file %s is missing", clean.Log(file.FileName))
-				logErr("zip", file.Update("FileMissing", true))
+				log.Warnf("download: %s not found", clean.Log(file.FileName))
+				logErr("download", file.Update("FileMissing", true))
 			}
 		}
 
 		elapsed := int(time.Since(start).Seconds())
 
-		log.Infof("zip: created %s [%s]", clean.Log(zipBaseName), time.Since(start))
+		log.Infof("download: created %s [%s]", clean.Log(zipBaseName), time.Since(start))
 
 		c.JSON(http.StatusOK, gin.H{"code": http.StatusOK, "message": i18n.Msg(i18n.MsgZipCreatedIn, elapsed), "filename": zipBaseName})
 	})
@@ -146,7 +153,7 @@ func ZipCreate(router *gin.RouterGroup) {
 func ZipDownload(router *gin.RouterGroup) {
 	router.GET("/zip/:filename", func(c *gin.Context) {
 		if InvalidDownloadToken(c) {
-			log.Errorf("zip: %s", c.AbortWithError(http.StatusForbidden, fmt.Errorf("invalid download token")))
+			log.Errorf("download: %s", c.AbortWithError(http.StatusForbidden, fmt.Errorf("invalid download token")))
 			return
 		}
 
@@ -156,12 +163,12 @@ func ZipDownload(router *gin.RouterGroup) {
 		zipFileName := path.Join(zipPath, zipBaseName)
 
 		if !fs.FileExists(zipFileName) {
-			log.Errorf("zip: %s", c.AbortWithError(http.StatusNotFound, fmt.Errorf("%s not found", clean.Log(zipFileName))))
+			log.Errorf("download: %s", c.AbortWithError(http.StatusNotFound, fmt.Errorf("%s not found", clean.Log(zipFileName))))
 			return
 		}
 
 		defer func(fileName, baseName string) {
-			log.Debugf("zip: %s has been downloaded", clean.Log(baseName))
+			log.Infof("download: %s has been downloaded", clean.Log(baseName))
 
 			// Wait a moment before deleting the zip file, just to be sure:
 			// https://github.com/photoprism/photoprism/issues/2532
@@ -169,47 +176,12 @@ func ZipDownload(router *gin.RouterGroup) {
 
 			// Remove the zip file to free up disk space.
 			if err := os.Remove(fileName); err != nil {
-				log.Warnf("zip: failed deleting %s (%s)", clean.Log(fileName), err)
+				log.Warnf("download: failed to delete %s (%s)", clean.Log(fileName), err)
 			} else {
-				log.Debugf("zip: deleted %s", clean.Log(baseName))
+				log.Debugf("download: deleted %s", clean.Log(baseName))
 			}
 		}(zipFileName, zipBaseName)
 
-		log.Debugf("zip: submitting %s", clean.Log(zipBaseName))
-
 		c.FileAttachment(zipFileName, zipBaseName)
 	})
 }
-
-// addFileToZip adds a file to a zip archive.
-func addFileToZip(zipWriter *zip.Writer, fileName, fileAlias string) error {
-	fileToZip, err := os.Open(fileName)
-	if err != nil {
-		return err
-	}
-	defer fileToZip.Close()
-
-	// Get the file information
-	info, err := fileToZip.Stat()
-	if err != nil {
-		return err
-	}
-
-	header, err := zip.FileInfoHeader(info)
-	if err != nil {
-		return err
-	}
-
-	header.Name = fileAlias
-
-	// Change to deflate to gain better compression
-	// see http://golang.org/pkg/archive/zip/#pkg-constants
-	header.Method = zip.Deflate
-
-	writer, err := zipWriter.CreateHeader(header)
-	if err != nil {
-		return err
-	}
-	_, err = io.Copy(writer, fileToZip)
-	return err
-}

internal/entity/face.go

@@ -410,7 +410,7 @@ func FirstOrCreateFace(m *Face) *Face {
 		}
 		return &result
 	} else {
-		log.Errorf("faces: failed adding %s (%s)", m.ID, err)
+		log.Errorf("faces: failed to add %s (%s)", m.ID, err)
 	}
 
 	return nil

internal/entity/subject.go

@@ -227,7 +227,7 @@ func FirstOrCreateSubject(m *Subject) *Subject {
 	} else if found = FindSubjectByName(m.SubjName, true); found != nil {
 		return found
 	} else {
-		log.Errorf("subject: failed adding %s (%s)", clean.Log(m.SubjName), err)
+		log.Errorf("subject: failed to add %s (%s)", clean.Log(m.SubjName), err)
 	}
 
 	return nil

internal/photoprism/delete.go

@@ -37,7 +37,7 @@ func DeletePhoto(p *entity.Photo, mediaFiles bool, originals bool) (numFiles int
 	if !fs.FileExists(yamlFileName) {
 		return numFiles, nil
 	} else if err := os.Remove(yamlFileName); err != nil {
-		log.Warnf("photo: failed deleting sidecar file %s", clean.Log(yamlRelName))
+		log.Warnf("photo: failed to delete sidecar file %s", clean.Log(yamlRelName))
 	} else {
 		numFiles++
 		log.Infof("photo: deleted sidecar file %s", clean.Log(yamlRelName))
@@ -68,7 +68,7 @@ func DeleteFiles(files entity.Files, originals bool) (numFiles int) {
 		if jsonFile := f.FileName() + ".json"; !originals && f.Root() == entity.RootOriginals || !fs.FileExists(jsonFile) {
 			// Do nothing.
 		} else if err = os.Remove(jsonFile); err != nil {
-			log.Warnf("files: failed deleting sidecar %s", clean.Log(filepath.Base(jsonFile)))
+			log.Warnf("files: failed to delete sidecar %s", clean.Log(filepath.Base(jsonFile)))
 		} else {
 			numFiles++
 			log.Infof("files: deleted sidecar %s", clean.Log(filepath.Base(jsonFile)))
@@ -78,7 +78,7 @@ func DeleteFiles(files entity.Files, originals bool) (numFiles int) {
 		if exifJson, _ := ExifToolCacheName(file.FileHash); !fs.FileExists(exifJson) {
 			// Do nothing.
 		} else if err = os.Remove(exifJson); err != nil {
-			log.Warnf("files: failed deleting sidecar %s", clean.Log(filepath.Base(exifJson)))
+			log.Warnf("files: failed to delete sidecar %s", clean.Log(filepath.Base(exifJson)))
 		} else {
 			numFiles++
 			log.Infof("files: deleted sidecar %s", clean.Log(filepath.Base(exifJson)))
@@ -102,7 +102,7 @@ func DeleteFiles(files entity.Files, originals bool) (numFiles int) {
 			log.Debugf("files: skipped deleting %s", clean.Log(relName))
 			continue
 		} else if err = f.Remove(); err != nil {
-			log.Errorf("files: failed deleting %s", clean.Log(relName))
+			log.Errorf("files: failed to delete %s", clean.Log(relName))
 		} else {
 			numFiles++
 			log.Infof("files: deleted %s", clean.Log(relName))

internal/photoprism/import.go

@@ -239,7 +239,7 @@ func (imp *Import) Start(opt ImportOptions) fs.Done {
 		for _, directory := range directories {
 			if fs.DirIsEmpty(directory) {
 				if err := os.Remove(directory); err != nil {
-					log.Errorf("import: failed deleting empty folder %s (%s)", clean.Log(fs.RelName(directory, importPath)), err)
+					log.Errorf("import: failed to delete empty folder %s (%s)", clean.Log(fs.RelName(directory, importPath)), err)
 				} else {
 					log.Infof("import: deleted empty folder %s", clean.Log(fs.RelName(directory, importPath)))
 				}

internal/photoprism/mediafile.go

@@ -1352,7 +1352,7 @@ func (m *MediaFile) RemoveSidecarFiles() (numFiles int, err error) {
 
 	for _, sidecarName := range matches {
 		if err = os.Remove(sidecarName); err != nil {
-			log.Errorf("files: failed deleting sidecar %s", clean.Log(fs.RelName(sidecarName, sidecarPath)))
+			log.Errorf("files: failed to delete sidecar %s", clean.Log(fs.RelName(sidecarName, sidecarPath)))
 		} else {
 			numFiles++
 			log.Infof("files: deleted sidecar %s", clean.Log(fs.RelName(sidecarName, sidecarPath)))

internal/query/subjects.go

@@ -96,7 +96,7 @@ func CreateMarkerSubjects() (affected int64, err error) {
 			log.Errorf("faces: invalid subject %s", clean.Log(m.MarkerName))
 			continue
 		} else if subj = entity.FirstOrCreateSubject(subj); subj == nil {
-			log.Errorf("faces: failed adding subject %s", clean.Log(m.MarkerName))
+			log.Errorf("faces: failed to add subject %s", clean.Log(m.MarkerName))
 			continue
 		} else {
 			affected++

pkg/fs/fs.go

@@ -25,14 +25,12 @@ Additional information can be found in our Developer Guide:
 package fs
 
 import (
-	"archive/zip"
 	"fmt"
 	"io"
 	"net/http"
 	"os"
 	"os/user"
 	"path/filepath"
-	"strings"
 	"syscall"
 )
 
@@ -124,50 +122,6 @@ func Abs(name string) string {
 	return result
 }
 
-// copyToFile copies the zip file to destination
-// if the zip file is a directory, a directory is created at the destination.
-func copyToFile(f *zip.File, dest string) (fileName string, err error) {
-	rc, err := f.Open()
-	if err != nil {
-		return fileName, err
-	}
-
-	defer rc.Close()
-
-	// Store filename/path for returning and using later on
-	fileName = filepath.Join(dest, f.Name)
-
-	if f.FileInfo().IsDir() {
-		// Make Folder
-		return fileName, MkdirAll(fileName)
-	}
-
-	// Make File
-	var fdir string
-
-	if lastIndex := strings.LastIndex(fileName, string(os.PathSeparator)); lastIndex > -1 {
-		fdir = fileName[:lastIndex]
-	}
-
-	if err = MkdirAll(fdir); err != nil {
-		return fileName, err
-	}
-
-	fd, err := os.OpenFile(fileName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
-	if err != nil {
-		return fileName, err
-	}
-
-	defer fd.Close()
-
-	_, err = io.Copy(fd, rc)
-	if err != nil {
-		return fileName, err
-	}
-
-	return fileName, nil
-}
-
 // Download downloads a file from a URL.
 func Download(fileName string, url string) error {
 	if dir := filepath.Dir(fileName); dir == "" || dir == "/" || dir == "." || dir == ".." {

pkg/fs/zip.go

@@ -4,25 +4,35 @@ import (
 	"archive/zip"
 	"io"
 	"os"
+	"path/filepath"
 	"strings"
 )
 
-// ZipFiles compresses one or many files into a single zip archive file.
-// Param 1: filename is the output zip file's name.
-// Param 2: files is a list of files to add to the zip.
-func Zip(filename string, files []string) error {
-	newZipFile, err := os.Create(filename)
-	if err != nil {
+// Zip compresses one or many files into a single zip archive file.
+func Zip(zipName string, files []string, compress bool) (err error) {
+	// Create zip file directory if it does not yet exist.
+	if zipDir := filepath.Dir(zipName); zipDir != "" && zipDir != "." {
+		err = os.MkdirAll(zipDir, ModeDir)
+
+		if err != nil {
+			return err
+		}
+	}
+
+	var newZipFile *os.File
+
+	if newZipFile, err = os.Create(zipName); err != nil {
 		return err
 	}
+
 	defer newZipFile.Close()
 
 	zipWriter := zip.NewWriter(newZipFile)
 	defer zipWriter.Close()
 
-	// Add files to zip
-	for _, file := range files {
-		if err = AddToZip(zipWriter, file); err != nil {
+	// Add files to zip archive.
+	for _, fileName := range files {
+		if err = ZipFile(zipWriter, fileName, "", compress); err != nil {
 			return err
 		}
 	}
@@ -30,61 +40,122 @@ func Zip(filename string, files []string) error {
 	return nil
 }
 
-func AddToZip(zipWriter *zip.Writer, filename string) error {
-	fileToZip, err := os.Open(filename)
+// ZipFile adds a file to a zip archive, optionally with an alias and compression.
+func ZipFile(zipWriter *zip.Writer, fileName, fileAlias string, compress bool) (err error) {
+	// Open file.
+	fileToZip, err := os.Open(fileName)
 
 	if err != nil {
 		return err
 	}
 
+	// Close file when done.
 	defer fileToZip.Close()
 
-	// Get the file information
+	// Get file information.
 	info, err := fileToZip.Stat()
 	if err != nil {
 		return err
 	}
 
+	// Create file info header.
 	header, err := zip.FileInfoHeader(info)
 	if err != nil {
 		return err
 	}
 
-	// Change to deflate to gain better compression
-	// see http://golang.org/pkg/archive/zip/#pkg-constants
-	header.Method = zip.Deflate
+	// Set filename alias, if any.
+	if fileAlias != "" {
+		header.Name = fileAlias
+	}
 
+	// Set method to deflate to enable compression,
+	// see http://golang.org/pkg/archive/zip/#pkg-constants
+	if compress {
+		header.Method = zip.Deflate
+	} else {
+		header.Method = zip.Store
+	}
+
+	// Write file info header.
 	writer, err := zipWriter.CreateHeader(header)
 	if err != nil {
 		return err
 	}
+
+	// Copy file to zip.
 	_, err = io.Copy(writer, fileToZip)
+
+	// Return error, if any.
 	return err
 }
 
-// Extract Zip file in destination directory
-func Unzip(src, dest string) (fileNames []string, err error) {
-	r, err := zip.OpenReader(src)
+// Unzip extracts the contents of a zip file to the target directory.
+func Unzip(zipName, dir string) (files []string, err error) {
+	zipReader, err := zip.OpenReader(zipName)
 
 	if err != nil {
-		return fileNames, err
+		return files, err
 	}
 
-	defer r.Close()
+	defer zipReader.Close()
 
-	for _, f := range r.File {
+	for _, zipFile := range zipReader.File {
 		// Skip directories like __OSX and potentially malicious file names containing "..".
-		if strings.HasPrefix(f.Name, "__") || strings.Contains(f.Name, "..") {
+		if strings.HasPrefix(zipFile.Name, "__") || strings.Contains(zipFile.Name, "..") {
 			continue
 		}
 
-		fn, err := copyToFile(f, dest)
-		if err != nil {
-			return fileNames, err
+		fileName, unzipErr := UnzipFile(zipFile, dir)
+		if unzipErr != nil {
+			return files, unzipErr
 		}
 
-		fileNames = append(fileNames, fn)
+		files = append(files, fileName)
 	}
 
-	return fileNames, nil
+	return files, nil
+}
+
+// UnzipFile writes a file from a zip archive to the target destination.
+func UnzipFile(f *zip.File, dir string) (fileName string, err error) {
+	rc, err := f.Open()
+	if err != nil {
+		return fileName, err
+	}
+
+	defer rc.Close()
+
+	// Compose destination file or directory path.
+	fileName = filepath.Join(dir, f.Name)
+
+	// Create destination path if it is a directory.
+	if f.FileInfo().IsDir() {
+		return fileName, MkdirAll(fileName)
+	}
+
+	// If it is a file, make sure its destination directory exists.
+	var basePath string
+
+	if lastIndex := strings.LastIndex(fileName, string(os.PathSeparator)); lastIndex > -1 {
+		basePath = fileName[:lastIndex]
+	}
+
+	if err = MkdirAll(basePath); err != nil {
+		return fileName, err
+	}
+
+	fd, err := os.OpenFile(fileName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
+	if err != nil {
+		return fileName, err
+	}
+
+	defer fd.Close()
+
+	_, err = io.Copy(fd, rc)
+	if err != nil {
+		return fileName, err
+	}
+
+	return fileName, nil
 }

pkg/fs/zip_test.go

@@ -0,0 +1,76 @@
+package fs
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestZip(t *testing.T) {
+	t.Run("Compressed", func(t *testing.T) {
+		zipDir := filepath.Join(os.TempDir(), "pkg/fs")
+		zipName := filepath.Join(zipDir, "compressed.zip")
+		unzipDir := filepath.Join(zipDir, "compressed")
+		files := []string{"./testdata/directory/example.jpg"}
+
+		if err := Zip(zipName, files, true); err != nil {
+			t.Fatal(err)
+		}
+
+		assert.FileExists(t, zipName)
+
+		if info, err := os.Stat(zipName); err != nil {
+			t.Error(err)
+		} else {
+			t.Logf("%s: %d bytes", zipName, info.Size())
+		}
+
+		if unzipFiles, err := Unzip(zipName, unzipDir); err != nil {
+			t.Error(err)
+		} else {
+			t.Logf("%s: %#v", zipName, unzipFiles)
+		}
+
+		if err := os.Remove(zipName); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := os.RemoveAll(unzipDir); err != nil {
+			t.Fatal(err)
+		}
+	})
+	t.Run("Uncompressed", func(t *testing.T) {
+		zipDir := filepath.Join(os.TempDir(), "pkg/fs")
+		zipName := filepath.Join(zipDir, "uncompressed.zip")
+		unzipDir := filepath.Join(zipDir, "uncompressed")
+		files := []string{"./testdata/directory/example.jpg"}
+
+		if err := Zip(zipName, files, false); err != nil {
+			t.Fatal(err)
+		}
+
+		assert.FileExists(t, zipName)
+
+		if info, err := os.Stat(zipName); err != nil {
+			t.Error(err)
+		} else {
+			t.Logf("%s: %d bytes", zipName, info.Size())
+		}
+
+		if unzipFiles, err := Unzip(zipName, unzipDir); err != nil {
+			t.Error(err)
+		} else {
+			t.Logf("%s: %#v", zipName, unzipFiles)
+		}
+
+		if err := os.Remove(zipName); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := os.RemoveAll(unzipDir); err != nil {
+			t.Fatal(err)
+		}
+	})
+}