From 5c88e96b2c2b2e466e9e5dff8af4c001677704ec Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Thu, 21 Sep 2023 22:58:06 +0200 Subject: [PATCH 1/7] made the logging path of fileDownload.go more straight-forward --- client/go.mod | 2 +- client/go.sum | 4 +- server/backend/cron/fileDownload.go | 71 +++++++++++++++-------------- 3 files changed, 39 insertions(+), 38 deletions(-) diff --git a/client/go.mod b/client/go.mod index 0b101708..b52b76d1 100644 --- a/client/go.mod +++ b/client/go.mod @@ -3,7 +3,7 @@ module github.com/TUM-Dev/Campus-Backend/client go 1.21 require ( - github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230920223017-73df1e4061c8 + github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230921201035-301a0c7bab98 github.com/sirupsen/logrus v1.9.3 google.golang.org/grpc v1.58.1 ) diff --git a/client/go.sum b/client/go.sum index 72830782..5a226406 100644 --- a/client/go.sum +++ b/client/go.sum @@ -1,5 +1,5 @@ -github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230920223017-73df1e4061c8 h1:FYhgX9DA9JvEyP06VX/Ot/8Oav9fS8hwHhGOyLfvX6E= -github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230920223017-73df1e4061c8/go.mod h1:fjoLL3rbdY6wTRJIksekT2p3OUp5ocFfXjB/avV/TVI= +github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230921201035-301a0c7bab98 h1:cifzCP3eUZgN+iqfhfGPTL1JFz4g/qjBNpwhcjlKOWk= +github.com/TUM-Dev/Campus-Backend/server v0.0.0-20230921201035-301a0c7bab98/go.mod h1:fjoLL3rbdY6wTRJIksekT2p3OUp5ocFfXjB/avV/TVI= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/server/backend/cron/fileDownload.go b/server/backend/cron/fileDownload.go index f70e1f10..809f26eb 100644 --- a/server/backend/cron/fileDownload.go +++ b/server/backend/cron/fileDownload.go @@ -2,11 +2,12 @@ package cron import ( "bytes" - "fmt" + "errors" "image" "io" "net/http" "os" + "path" "strings" "github.com/TUM-Dev/Campus-Backend/server/model" @@ -19,13 +20,24 @@ import ( // fileDownloadCron Downloads all files that are not marked as finished in the database. func (c *CronService) fileDownloadCron() error { var files []model.Files - err := c.db.Find(&files, "downloaded = 0").Scan(&files).Error - if err != nil && err != gorm.ErrRecordNotFound { + err := c.db.Find(&files, "downloaded = 0").Error + if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { + log.WithError(err).Error("Could not get files from database") return err } - for i := range files { - if files[i].URL.Valid { - c.downloadFile(files[i]) + for _, file := range files { + if !file.URL.Valid { + log.WithField("fileId", file.File).Info("skipping file without url") + continue + } + log.WithField("url", file.URL.String).Info("downloading file") + if err := downloadFile(&file); err != nil { + log.WithError(err).WithField("url", file.URL.String).Warn("Could not download file") + continue + } + if err = c.db.Model(&model.Files{URL: file.URL}).Update("downloaded", true).Error; err != nil { + log.WithError(err).Error("Could not set image to downloaded.") + continue } } return nil @@ -34,51 +46,40 @@ func (c *CronService) fileDownloadCron() error { // downloadFile Downloads a file, marks it downloaded and resizes it if it's an image. // url: download url of the file // name: target name of the file -func (c *CronService) downloadFile(file model.Files) { - if !file.URL.Valid { - log.WithField("fileId", file.File).Info("skipping file without url") - } - url := file.URL.String - log.WithField("url", url).Info("downloading file") - resp, err := http.Get(url) +func downloadFile(file *model.Files) error { + resp, err := http.Get(file.URL.String) if err != nil { - log.WithError(err).WithField("url", url).Warn("Could not download image") - return + log.WithError(err).WithField("url", file.URL.String).Warn("Could not download image") + return err } // read body here because we can't exhaust the io.reader twice body, err := io.ReadAll(resp.Body) if err != nil { log.WithError(err).Warn("Unable to read http body") - return + return err } - // resize if file is image - mime := mimetype.Detect(body) - if strings.HasPrefix(mime.String(), "image/") { + // in our case resolves to /Storage/news/newspread/1234abc.jpg + dstPath := path.Join(StorageDir, file.Path, file.Name) + if strings.HasPrefix(mimetype.Detect(body).String(), "image/") { + // save with resizing downloadedImg, _, err := image.Decode(bytes.NewReader(body)) if err != nil { - log.WithError(err).WithField("url", url).Warn("Couldn't decode source image") - return + log.WithError(err).WithField("url", file.URL.String).Warn("Couldn't decode source image") + return err } - // in our case resolves to /Storage/news/newspread/1234abc.jpg - dstFileName := fmt.Sprintf("%s%s", file.Path, file.Name) - dstImage := imaging.Resize(downloadedImg, 1280, 0, imaging.Lanczos) - err = imaging.Save(dstImage, StorageDir+dstFileName, imaging.JPEGQuality(75)) - if err != nil { - log.WithError(err).WithField("url", url).Warn("Could not save image file") - return + resizedImage := imaging.Resize(downloadedImg, 1280, 0, imaging.Lanczos) + if err = imaging.Save(resizedImage, dstPath, imaging.JPEGQuality(75)); err != nil { + log.WithError(err).WithField("url", file.URL.String).Warn("Could not save image file") + return err } } else { // save without resizing image - err = os.WriteFile(fmt.Sprintf("%s%s", file.Path, file.Name), body, 0644) - if err != nil { + if err := os.WriteFile(dstPath, body, 0644); err != nil { log.WithError(err).Error("Can't save file to disk") - return + return err } } - err = c.db.Model(&model.Files{}).Where("url = ?", url).Update("downloaded", true).Error - if err != nil { - log.WithError(err).Error("Could not set image to downloaded.") - } + return nil } From 5df64b4d0cfcbf3af57a766e8f8a0fae2bf874c6 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Thu, 21 Sep 2023 23:30:28 +0200 Subject: [PATCH 2/7] refactored the file downloader into separate testable functions --- server/backend/cron/fileDownload.go | 102 ++++++++++++++++++---------- 1 file changed, 66 insertions(+), 36 deletions(-) diff --git a/server/backend/cron/fileDownload.go b/server/backend/cron/fileDownload.go index 809f26eb..0dd9db41 100644 --- a/server/backend/cron/fileDownload.go +++ b/server/backend/cron/fileDownload.go @@ -1,9 +1,7 @@ package cron import ( - "bytes" "errors" - "image" "io" "net/http" "os" @@ -20,66 +18,98 @@ import ( // fileDownloadCron Downloads all files that are not marked as finished in the database. func (c *CronService) fileDownloadCron() error { var files []model.Files - err := c.db.Find(&files, "downloaded = 0").Error + err := c.db.Find(&files, "downloaded = 0 AND url IS NOT NULL").Error if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { log.WithError(err).Error("Could not get files from database") return err } for _, file := range files { - if !file.URL.Valid { - log.WithField("fileId", file.File).Info("skipping file without url") + // in our case resolves to /Storage/news/newspread/1234abc.jpg + dstPath := path.Join(StorageDir, file.Path, file.Name) + fields := log.Fields{"url": file.URL.String, "dstPath": dstPath} + log.WithFields(fields).Info("downloading file") + + if err := ensureFileDoesNotExist(dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not ensure file does not exist") + continue + } + if err := downloadFile(file.URL.String, dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not download file") continue } - log.WithField("url", file.URL.String).Info("downloading file") - if err := downloadFile(&file); err != nil { - log.WithError(err).WithField("url", file.URL.String).Warn("Could not download file") + if err := maybeResizeImage(dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not resize image") continue } + // everything went well => we can mark the file as downloaded if err = c.db.Model(&model.Files{URL: file.URL}).Update("downloaded", true).Error; err != nil { - log.WithError(err).Error("Could not set image to downloaded.") + log.WithError(err).WithFields(fields).Error("Could not set image to downloaded.") continue } } return nil } -// downloadFile Downloads a file, marks it downloaded and resizes it if it's an image. -// url: download url of the file -// name: target name of the file -func downloadFile(file *model.Files) error { - resp, err := http.Get(file.URL.String) +// ensureFileDoesNotExist makes sure that the file does not exist, but the directory in which it should be does +func ensureFileDoesNotExist(dstPath string) error { + if _, err := os.Stat(dstPath); err == nil { + // file already exists + return os.Remove(dstPath) + } + return os.MkdirAll(path.Dir(dstPath), 0755) +} + +// maybeResizeImage resizes the image if it's an image and larger than 1280px +func maybeResizeImage(dstPath string) error { + mime, err := mimetype.DetectFile(dstPath) if err != nil { - log.WithError(err).WithField("url", file.URL.String).Warn("Could not download image") return err } - // read body here because we can't exhaust the io.reader twice - body, err := io.ReadAll(resp.Body) + if !strings.HasPrefix(mime.String(), "image/") { + return nil + } + + img, err := imaging.Open(dstPath) if err != nil { - log.WithError(err).Warn("Unable to read http body") return err } + resizedImage := imaging.Resize(img, 1280, 0, imaging.Lanczos) + return imaging.Save(resizedImage, dstPath, imaging.JPEGQuality(75)) +} - // in our case resolves to /Storage/news/newspread/1234abc.jpg - dstPath := path.Join(StorageDir, file.Path, file.Name) - if strings.HasPrefix(mimetype.Detect(body).String(), "image/") { - // save with resizing - downloadedImg, _, err := image.Decode(bytes.NewReader(body)) - if err != nil { - log.WithError(err).WithField("url", file.URL.String).Warn("Couldn't decode source image") - return err +// downloadFile Downloads a file from the given url and saves it to the given path +func downloadFile(url string, dstPath string) error { + fields := log.Fields{"url": url, "dstPath": dstPath} + resp, err := http.Get(url) + if err != nil { + log.WithError(err).WithFields(fields).Warn("Could not download image") + return err + } + defer func(Body io.ReadCloser) { + if err := Body.Close(); err != nil { + log.WithError(err).WithFields(fields).Error("Error while closing body") } + }(resp.Body) + if resp.StatusCode != http.StatusOK { + log.WithError(err).WithFields(fields).Warn("Could not download image") + return err + } - resizedImage := imaging.Resize(downloadedImg, 1280, 0, imaging.Lanczos) - if err = imaging.Save(resizedImage, dstPath, imaging.JPEGQuality(75)); err != nil { - log.WithError(err).WithField("url", file.URL.String).Warn("Could not save image file") - return err - } - } else { - // save without resizing image - if err := os.WriteFile(dstPath, body, 0644); err != nil { - log.WithError(err).Error("Can't save file to disk") - return err + // save the file to disk + out, err := os.Create(dstPath) + if err != nil { + log.WithError(err).WithFields(fields).Warn("Could not create file") + return err + } + defer func(out *os.File) { + err := out.Close() + if err != nil { + log.WithError(err).WithFields(fields).Error("Error while closing file") } + }(out) + if _, err := io.Copy(out, resp.Body); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not copy file") + return err } return nil } From 710f0fec1d432a4162a9c5f70cde6c08125a4707 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Thu, 21 Sep 2023 23:56:31 +0200 Subject: [PATCH 3/7] added a testcase for `maybeResizeImage` --- server/backend/cron/fileDownload.go | 2 +- server/backend/cron/fileDownload_test.go | 63 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 server/backend/cron/fileDownload_test.go diff --git a/server/backend/cron/fileDownload.go b/server/backend/cron/fileDownload.go index 0dd9db41..0555da95 100644 --- a/server/backend/cron/fileDownload.go +++ b/server/backend/cron/fileDownload.go @@ -59,7 +59,7 @@ func ensureFileDoesNotExist(dstPath string) error { return os.MkdirAll(path.Dir(dstPath), 0755) } -// maybeResizeImage resizes the image if it's an image and larger than 1280px +// maybeResizeImage resizes the image if it's an image to 1280px width keeping the aspect ratio func maybeResizeImage(dstPath string) error { mime, err := mimetype.DetectFile(dstPath) if err != nil { diff --git a/server/backend/cron/fileDownload_test.go b/server/backend/cron/fileDownload_test.go new file mode 100644 index 00000000..a029ddce --- /dev/null +++ b/server/backend/cron/fileDownload_test.go @@ -0,0 +1,63 @@ +package cron + +import ( + "image" + "os" + "testing" + + "github.com/disintegration/imaging" + "github.com/stretchr/testify/require" +) + +func TestMaybeResizeImage(t *testing.T) { + t.Run("Resize Image", func(t *testing.T) { + dstPath := "test_image.jpg" + require.NoError(t, createDummyImage(dstPath, 2000, 1000)) + defer os.Remove(dstPath) + require.NoError(t, maybeResizeImage(dstPath)) + img, err := imaging.Open(dstPath) + require.NoError(t, err) + require.Equal(t, 1280, img.Bounds().Dx()) + require.Equal(t, 640, img.Bounds().Dy()) + }) + t.Run("Do not Resize smaller Image", func(t *testing.T) { + dstPath := "test_image.jpg" + require.NoError(t, createDummyImage(dstPath, 1000, 2000)) + defer os.Remove(dstPath) + require.NoError(t, maybeResizeImage(dstPath)) + img, err := imaging.Open(dstPath) + require.NoError(t, err) + require.Equal(t, 1280, img.Bounds().Dx()) + require.Equal(t, 2560, img.Bounds().Dy()) + }) + + t.Run("Skip Non-Image", func(t *testing.T) { + nonImageFile := "non_image.txt" + content := []byte("Dummy Text") + require.NoError(t, createDummyFile(nonImageFile, content)) + defer os.Remove(nonImageFile) + require.NoError(t, maybeResizeImage(nonImageFile)) + contentAfterExecution, err := os.ReadFile(nonImageFile) + require.NoError(t, err) + require.Equal(t, content, contentAfterExecution) + }) +} + +// createDummyImage creates a dummy image file with the specified dimensions +func createDummyImage(filePath string, width, height int) error { + img := image.NewRGBA(image.Rect(0, 0, width, height)) + return imaging.Save(img, filePath, imaging.JPEGQuality(75)) +} + +// createDummyFile creates a dummy non-image file +func createDummyFile(filePath string, content []byte) error { + file, err := os.Create(filePath) + if err != nil { + return err + } + defer file.Close() + if _, err := file.Write(content); err != nil { + return err + } + return nil +} From 59af397d4242fb94ed211f179a0da7d4ac9962da Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 22 Sep 2023 00:03:37 +0200 Subject: [PATCH 4/7] made shure that the file-downloading runs in a transaction --- server/backend/cron/fileDownload.go | 69 +++++++++++++++-------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/server/backend/cron/fileDownload.go b/server/backend/cron/fileDownload.go index 0555da95..8e9711a6 100644 --- a/server/backend/cron/fileDownload.go +++ b/server/backend/cron/fileDownload.go @@ -17,37 +17,44 @@ import ( // fileDownloadCron Downloads all files that are not marked as finished in the database. func (c *CronService) fileDownloadCron() error { - var files []model.Files - err := c.db.Find(&files, "downloaded = 0 AND url IS NOT NULL").Error - if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { - log.WithError(err).Error("Could not get files from database") - return err - } - for _, file := range files { - // in our case resolves to /Storage/news/newspread/1234abc.jpg - dstPath := path.Join(StorageDir, file.Path, file.Name) - fields := log.Fields{"url": file.URL.String, "dstPath": dstPath} - log.WithFields(fields).Info("downloading file") - - if err := ensureFileDoesNotExist(dstPath); err != nil { - log.WithError(err).WithFields(fields).Warn("Could not ensure file does not exist") - continue - } - if err := downloadFile(file.URL.String, dstPath); err != nil { - log.WithError(err).WithFields(fields).Warn("Could not download file") - continue - } - if err := maybeResizeImage(dstPath); err != nil { - log.WithError(err).WithFields(fields).Warn("Could not resize image") - continue + return c.db.Transaction(func(tx *gorm.DB) error { + var files []model.Files + err := tx.Find(&files, "downloaded = 0 AND url IS NOT NULL").Error + if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { + log.WithError(err).Error("Could not get files from database") + return err } - // everything went well => we can mark the file as downloaded - if err = c.db.Model(&model.Files{URL: file.URL}).Update("downloaded", true).Error; err != nil { - log.WithError(err).WithFields(fields).Error("Could not set image to downloaded.") - continue + for _, file := range files { + // in our case resolves to /Storage/news/newspread/1234abc.jpg + dstPath := path.Join(StorageDir, file.Path, file.Name) + fields := log.Fields{"url": file.URL.String, "dstPath": dstPath} + log.WithFields(fields).Info("downloading file") + + if err = tx.Model(&model.Files{File: file.File}).Update("downloads", file.Downloads+1).Error; err != nil { + log.WithError(err).WithFields(fields).Error("Could not set update the download-count") + continue + } + + if err := ensureFileDoesNotExist(dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not ensure file does not exist") + continue + } + if err := downloadFile(file.URL.String, dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not download file") + continue + } + if err := maybeResizeImage(dstPath); err != nil { + log.WithError(err).WithFields(fields).Warn("Could not resize image") + continue + } + // everything went well => we can mark the file as downloaded + if err = tx.Model(&model.Files{URL: file.URL}).Update("downloaded", true).Error; err != nil { + log.WithError(err).WithFields(fields).Error("Could not set image to downloaded.") + continue + } } - } - return nil + return nil + }) } // ensureFileDoesNotExist makes sure that the file does not exist, but the directory in which it should be does @@ -82,7 +89,6 @@ func downloadFile(url string, dstPath string) error { fields := log.Fields{"url": url, "dstPath": dstPath} resp, err := http.Get(url) if err != nil { - log.WithError(err).WithFields(fields).Warn("Could not download image") return err } defer func(Body io.ReadCloser) { @@ -91,14 +97,12 @@ func downloadFile(url string, dstPath string) error { } }(resp.Body) if resp.StatusCode != http.StatusOK { - log.WithError(err).WithFields(fields).Warn("Could not download image") return err } // save the file to disk out, err := os.Create(dstPath) if err != nil { - log.WithError(err).WithFields(fields).Warn("Could not create file") return err } defer func(out *os.File) { @@ -108,7 +112,6 @@ func downloadFile(url string, dstPath string) error { } }(out) if _, err := io.Copy(out, resp.Body); err != nil { - log.WithError(err).WithFields(fields).Warn("Could not copy file") return err } return nil From 71674516fa6a8b367867869101d366e5856bb84f Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 22 Sep 2023 00:07:47 +0200 Subject: [PATCH 5/7] added a testcase for `ensureFileDoesNotExist` --- server/backend/cron/fileDownload_test.go | 28 ++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/server/backend/cron/fileDownload_test.go b/server/backend/cron/fileDownload_test.go index a029ddce..18e0e0c1 100644 --- a/server/backend/cron/fileDownload_test.go +++ b/server/backend/cron/fileDownload_test.go @@ -43,6 +43,34 @@ func TestMaybeResizeImage(t *testing.T) { }) } +func TestEnsureFileDoesNotExist(t *testing.T) { + tmpFilePath := "test_dir/test_file.txt" + defer func() { _ = os.RemoveAll("test_dir") }() + + t.Run("FileDoesNotExist", func(t *testing.T) { + require.NoError(t, ensureFileDoesNotExist(tmpFilePath)) + + _, dirErr := os.Stat("test_dir") + require.NoError(t, dirErr) + + _, fileErr := os.Stat(tmpFilePath) + require.True(t, os.IsNotExist(fileErr)) + }) + + t.Run("FileExists", func(t *testing.T) { + _, createErr := os.Create(tmpFilePath) + require.NoError(t, createErr) + + require.NoError(t, ensureFileDoesNotExist(tmpFilePath)) + + _, dirErr := os.Stat("test_dir") + require.NoError(t, dirErr) + + _, fileErr := os.Stat(tmpFilePath) + require.True(t, os.IsNotExist(fileErr)) + }) +} + // createDummyImage creates a dummy image file with the specified dimensions func createDummyImage(filePath string, width, height int) error { img := image.NewRGBA(image.Rect(0, 0, width, height)) From 1147baa5a12e18f90ae1f8fdb24808ba6d936410 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 22 Sep 2023 00:17:00 +0200 Subject: [PATCH 6/7] fixed a typo --- server/backend/cron/fileDownload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/backend/cron/fileDownload.go b/server/backend/cron/fileDownload.go index 8e9711a6..3e16f5a0 100644 --- a/server/backend/cron/fileDownload.go +++ b/server/backend/cron/fileDownload.go @@ -15,7 +15,7 @@ import ( "gorm.io/gorm" ) -// fileDownloadCron Downloads all files that are not marked as finished in the database. +// fileDownloadCron downloads all files that are not marked as finished in the database func (c *CronService) fileDownloadCron() error { return c.db.Transaction(func(tx *gorm.DB) error { var files []model.Files From e7425ed9dadb8ebe135816cffd843a9c4f45fca3 Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 22 Sep 2023 01:38:03 +0200 Subject: [PATCH 7/7] rebase --- server/backend/rpcserver.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/backend/rpcserver.go b/server/backend/rpcserver.go index fa26e1ed..3544fef1 100644 --- a/server/backend/rpcserver.go +++ b/server/backend/rpcserver.go @@ -3,9 +3,10 @@ package backend import ( "context" "errors" - "github.com/TUM-Dev/Campus-Backend/server/env" "net" + "github.com/TUM-Dev/Campus-Backend/server/env" + pb "github.com/TUM-Dev/Campus-Backend/server/api/tumdev" "github.com/TUM-Dev/Campus-Backend/server/backend/ios_notifications/ios_apns" "github.com/TUM-Dev/Campus-Backend/server/backend/ios_notifications/ios_apns/ios_apns_jwt"