From b467f2ca61e46ee5937c657c35fa2641f4c0363f Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Sat, 6 Nov 2021 12:23:47 +0100 Subject: [PATCH 1/5] Add failing test for #198 --- api/go.mod | 1 + api/go.sum | 6 +++ api/graphql/resolvers/user.go | 2 +- api/scanner/cleanup_media.go | 2 +- api/scanner/cleanup_media_test.go | 81 +++++++++++++++++++++++++++++++ api/scanner/scanner_test.go | 12 +---- api/scanner/scanner_user.go | 2 +- api/test_utils/scanner_helpers.go | 36 ++++++++++++++ 8 files changed, 128 insertions(+), 14 deletions(-) create mode 100644 api/scanner/cleanup_media_test.go create mode 100644 api/test_utils/scanner_helpers.go diff --git a/api/go.mod b/api/go.mod index d433f0e4..cbd64d2a 100644 --- a/api/go.mod +++ b/api/go.mod @@ -19,6 +19,7 @@ require ( github.com/joho/godotenv v1.3.0 github.com/mattn/go-sqlite3 v1.14.6 // indirect github.com/mitchellh/mapstructure v1.4.1 // indirect + github.com/otiai10/copy v1.6.0 // indirect github.com/pkg/errors v0.9.1 github.com/sabhiram/go-gitignore v0.0.0-20201211210132-54b8a0bf510f github.com/stretchr/objx v0.3.0 // indirect diff --git a/api/go.sum b/api/go.sum index 73319927..108be1c4 100644 --- a/api/go.sum +++ b/api/go.sum @@ -437,6 +437,12 @@ github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJ github.com/openzipkin/zipkin-go v0.2.1/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4= github.com/openzipkin/zipkin-go v0.2.2 h1:nY8Hti+WKaP0cRsSeQ026wU03QsM762XBeCXBb9NAWI= github.com/openzipkin/zipkin-go v0.2.2/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4= +github.com/otiai10/copy v1.6.0 h1:IinKAryFFuPONZ7cm6T6E2QX/vcJwSnlaA5lfoaXIiQ= +github.com/otiai10/copy v1.6.0/go.mod h1:XWfuS3CrI0R6IE0FbgHsEazaXO8G0LpMp9o8tos0x4E= +github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95/go.mod h1:9qAhocn7zKJG+0mI8eUu6xqkFDYS2kb2saOteoSB3cE= +github.com/otiai10/curr v1.0.0/go.mod h1:LskTG5wDwr8Rs+nNQ+1LlxRjAtTZZjtJW4rMXl6j4vs= +github.com/otiai10/mint v1.3.0/go.mod h1:F5AjcsTsWUqX+Na9fpHb52P8pcRX2CI6A3ctIT91xUo= +github.com/otiai10/mint v1.3.2/go.mod h1:/yxELlJQ0ufhjUwhshSj+wFjZ78CnZ48/1wtmBH1OTc= github.com/pact-foundation/pact-go v1.0.4 h1:OYkFijGHoZAYbOIb1LWXrwKQbMMRUv1oQ89blD2Mh2Q= github.com/pact-foundation/pact-go v1.0.4/go.mod h1:uExwJY4kCzNPcHRj+hCR/HBbOOIwwtUjcrb0b5/5kLM= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c h1:Lgl0gzECD8GnQ5QCWA8o6BtfL6mDH5rQgM4/fX3avOs= diff --git a/api/graphql/resolvers/user.go b/api/graphql/resolvers/user.go index 2c52d4ef..24f222b8 100644 --- a/api/graphql/resolvers/user.go +++ b/api/graphql/resolvers/user.go @@ -343,7 +343,7 @@ func (r *mutationResolver) UserRemoveRootAlbum(ctx context.Context, userID int, } // Reload faces as media might have been deleted - if face_detection.GlobalFaceDetector == nil { + if face_detection.GlobalFaceDetector != nil { if err := face_detection.GlobalFaceDetector.ReloadFacesFromDatabase(r.Database); err != nil { return nil, err } diff --git a/api/scanner/cleanup_media.go b/api/scanner/cleanup_media.go index 94926721..de2250d9 100644 --- a/api/scanner/cleanup_media.go +++ b/api/scanner/cleanup_media.go @@ -126,7 +126,7 @@ func deleteOldUserAlbums(db *gorm.DB, scannedAlbums []*models.Album, user *model } // Reload faces after deleting albums - if face_detection.GlobalFaceDetector == nil { + if face_detection.GlobalFaceDetector != nil { if err := face_detection.GlobalFaceDetector.ReloadFacesFromDatabase(db); err != nil { deleteErrors = append(deleteErrors, err) } diff --git a/api/scanner/cleanup_media_test.go b/api/scanner/cleanup_media_test.go new file mode 100644 index 00000000..7df7b1ec --- /dev/null +++ b/api/scanner/cleanup_media_test.go @@ -0,0 +1,81 @@ +package scanner_test + +import ( + "os" + "path" + "testing" + + "github.com/otiai10/copy" + "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/test_utils" + "github.com/stretchr/testify/assert" +) + +func TestCleanupMedia(t *testing.T) { + test_utils.FilesystemTest(t) + db := test_utils.DatabaseTest(t) + + test_dir := t.TempDir() + copy.Copy("./test_data", test_dir) + + countAllMedia := func() int { + var all_media []*models.Media + if !assert.NoError(t, db.Find(&all_media).Error) { + return -1 + } + return len(all_media) + } + + countAllMediaURLs := func() int { + var all_media_urls []*models.MediaURL + if !assert.NoError(t, db.Find(&all_media_urls).Error) { + return -1 + } + return len(all_media_urls) + } + + pass := "1234" + user1, err := models.RegisterUser(db, "user1", &pass, true) + if !assert.NoError(t, err) { + return + } + + user2, err := models.RegisterUser(db, "user2", &pass, true) + if !assert.NoError(t, err) { + return + } + + root_album := models.Album{ + Title: "root album", + Path: test_dir, + } + + if !assert.NoError(t, db.Save(&root_album).Error) { + return + } + + err = db.Model(user1).Association("Albums").Append(&root_album) + if !assert.NoError(t, err) { + return + } + err = db.Model(user2).Association("Albums").Append(&root_album) + if !assert.NoError(t, err) { + return + } + + test_utils.RunScannerOnUser(t, db, user1) + assert.Equal(t, 9, countAllMedia()) + assert.Equal(t, 18, countAllMediaURLs()) + + // move faces directory + assert.NoError(t, os.Rename(path.Join(test_dir, "faces"), path.Join(test_dir, "faces_moved"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 9, countAllMedia()) + assert.Equal(t, 18, countAllMediaURLs()) + + // remove faces_moved directory + assert.NoError(t, os.RemoveAll(path.Join(test_dir, "faces_moved"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 3, countAllMedia()) + assert.Equal(t, 6, countAllMediaURLs()) +} diff --git a/api/scanner/scanner_test.go b/api/scanner/scanner_test.go index 4ccd45d2..e4d66ecc 100644 --- a/api/scanner/scanner_test.go +++ b/api/scanner/scanner_test.go @@ -6,7 +6,6 @@ import ( "time" "github.com/photoview/photoview/api/graphql/models" - "github.com/photoview/photoview/api/scanner" "github.com/photoview/photoview/api/scanner/face_detection" "github.com/photoview/photoview/api/test_utils" "github.com/stretchr/testify/assert" @@ -40,20 +39,11 @@ func TestFullScan(t *testing.T) { return } - if !assert.NoError(t, scanner.InitializeScannerQueue(db)) { - return - } - if !assert.NoError(t, face_detection.InitializeFaceDetector(db)) { return } - if !assert.NoError(t, scanner.AddUserToQueue(user)) { - return - } - - // wait for all jobs to finish - scanner.CloseScannerQueue() + test_utils.RunScannerOnUser(t, db, user) var all_media []*models.Media if !assert.NoError(t, db.Find(&all_media).Error) { diff --git a/api/scanner/scanner_user.go b/api/scanner/scanner_user.go index ab0d1e13..63be22aa 100644 --- a/api/scanner/scanner_user.go +++ b/api/scanner/scanner_user.go @@ -23,7 +23,7 @@ func getPhotoviewIgnore(ignorePath string) ([]string, error) { // Open .photoviewignore file, if exists photoviewIgnoreFile, err := os.Open(path.Join(ignorePath, ".photoviewignore")) if err != nil { - if err == os.ErrNotExist { + if os.IsNotExist(err) { return photoviewIgnore, nil } return photoviewIgnore, err diff --git a/api/test_utils/scanner_helpers.go b/api/test_utils/scanner_helpers.go new file mode 100644 index 00000000..b143fc65 --- /dev/null +++ b/api/test_utils/scanner_helpers.go @@ -0,0 +1,36 @@ +package test_utils + +import ( + "testing" + + "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/scanner" + "github.com/stretchr/testify/assert" + "gorm.io/gorm" +) + +func RunScannerOnUser(t *testing.T, db *gorm.DB, user *models.User) { + if !assert.NoError(t, scanner.InitializeScannerQueue(db)) { + return + } + + if !assert.NoError(t, scanner.AddUserToQueue(user)) { + return + } + + // wait for all jobs to finish + scanner.CloseScannerQueue() +} + +func RunScannerAll(t *testing.T, db *gorm.DB) { + if !assert.NoError(t, scanner.InitializeScannerQueue(db)) { + return + } + + if !assert.NoError(t, scanner.AddAllToQueue()) { + return + } + + // wait for all jobs to finish + scanner.CloseScannerQueue() +} From 3040daafb6a7f29da82b84bccc7d1ee84ee7c0bb Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Sat, 6 Nov 2021 12:38:05 +0100 Subject: [PATCH 2/5] Fix tests --- api/scanner/cleanup_media.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/scanner/cleanup_media.go b/api/scanner/cleanup_media.go index de2250d9..96f72cc3 100644 --- a/api/scanner/cleanup_media.go +++ b/api/scanner/cleanup_media.go @@ -83,7 +83,6 @@ func deleteOldUserAlbums(db *gorm.DB, scannedAlbums []*models.Album, user *model Table("user_albums"). Joins("JOIN albums ON user_albums.album_id = albums.id"). Where("user_id = ?", user.ID). - // Where("album_id IN (?)", userAlbumIDs). Where("album_id NOT IN (?)", scannedAlbumIDs) if err := query.Find(&deleteAlbums).Error; err != nil { @@ -109,7 +108,7 @@ func deleteOldUserAlbums(db *gorm.DB, scannedAlbums []*models.Album, user *model // Delete old albums from database err := db.Transaction(func(tx *gorm.DB) error { - if err := tx.Model(&user).Association("Albums").Delete(deleteAlbums); err != nil { + if err := tx.Where("album_id IN ?", deleteAlbumIDs).Delete(&models.UserAlbums{}).Error; err != nil { return err } From c48ede175a05d83bd05b2c7af34d22a98382727e Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Sat, 6 Nov 2021 13:01:01 +0100 Subject: [PATCH 3/5] Fix exif circular imports in tests --- api/routes/authenticate_routes_test.go | 16 ++++++++-------- api/scanner/exif/exif.go | 8 ++++---- api/scanner/exif/exif_parser_external.go | 2 +- api/scanner/exif/exif_parser_internal.go | 6 +++++- api/scanner/exif/exif_test.go | 11 ++++++----- 5 files changed, 24 insertions(+), 19 deletions(-) diff --git a/api/routes/authenticate_routes_test.go b/api/routes/authenticate_routes_test.go index 5677b1ba..9f67b0bf 100644 --- a/api/routes/authenticate_routes_test.go +++ b/api/routes/authenticate_routes_test.go @@ -88,8 +88,8 @@ func TestAuthenticateRoute(t *testing.T) { assert.NoError(t, err) assert.True(t, success) - assert.Equal(t, responseMessage, "success") - assert.Equal(t, responseStatus, http.StatusAccepted) + assert.Equal(t, "success", responseMessage) + assert.Equal(t, http.StatusAccepted, responseStatus) }) }) @@ -103,8 +103,8 @@ func TestAuthenticateRoute(t *testing.T) { assert.NoError(t, err) assert.True(t, success) - assert.Equal(t, responseMessage, "success") - assert.Equal(t, responseStatus, http.StatusAccepted) + assert.Equal(t, "success", responseMessage) + assert.Equal(t, http.StatusAccepted, responseStatus) }) t.Run("Request without access token", func(t *testing.T) { @@ -114,8 +114,8 @@ func TestAuthenticateRoute(t *testing.T) { assert.Error(t, err) assert.False(t, success) - assert.Equal(t, responseMessage, "unauthorized") - assert.Equal(t, responseStatus, http.StatusForbidden) + assert.Equal(t, "unauthorized", responseMessage) + assert.Equal(t, http.StatusForbidden, responseStatus) }) expire := time.Now().Add(time.Hour * 24 * 30) @@ -139,8 +139,8 @@ func TestAuthenticateRoute(t *testing.T) { assert.NoError(t, err) assert.True(t, success) - assert.Equal(t, responseMessage, "success") - assert.Equal(t, responseStatus, http.StatusAccepted) + assert.Equal(t, "success", responseMessage) + assert.Equal(t, http.StatusAccepted, responseStatus) }) }) diff --git a/api/scanner/exif/exif.go b/api/scanner/exif/exif.go index 838fb10e..06f509f4 100644 --- a/api/scanner/exif/exif.go +++ b/api/scanner/exif/exif.go @@ -9,19 +9,19 @@ import ( "github.com/photoview/photoview/api/graphql/models" ) -type exifParser interface { +type ExifParser interface { ParseExif(media_path string) (*models.MediaEXIF, error) } -var globalExifParser exifParser +var globalExifParser ExifParser func InitializeEXIFParser() { // Decide between internal or external Exif parser - exiftoolParser, err := newExiftoolParser() + exiftoolParser, err := NewExiftoolParser() if err != nil { log.Printf("Failed to get exiftool, using internal exif parser instead: %v\n", err) - globalExifParser = &internalExifParser{} + globalExifParser = NewInternalExifParser() } else { log.Println("Found exiftool") globalExifParser = exiftoolParser diff --git a/api/scanner/exif/exif_parser_external.go b/api/scanner/exif/exif_parser_external.go index 29940fea..2d71391b 100644 --- a/api/scanner/exif/exif_parser_external.go +++ b/api/scanner/exif/exif_parser_external.go @@ -15,7 +15,7 @@ type externalExifParser struct { dataLoader *dataloader.ExiftoolLoader } -func newExiftoolParser() (*externalExifParser, error) { +func NewExiftoolParser() (ExifParser, error) { et, err := exiftool.NewExiftool(exiftool.NoPrintConversion()) if err != nil { diff --git a/api/scanner/exif/exif_parser_internal.go b/api/scanner/exif/exif_parser_internal.go index 1cb8a42f..11c8ef80 100644 --- a/api/scanner/exif/exif_parser_internal.go +++ b/api/scanner/exif/exif_parser_internal.go @@ -16,7 +16,11 @@ import ( // internalExifParser is an exif parser that parses the media without the use of external tools type internalExifParser struct{} -func (p *internalExifParser) ParseExif(media_path string) (returnExif *models.MediaEXIF, returnErr error) { +func NewInternalExifParser() ExifParser { + return internalExifParser{} +} + +func (p internalExifParser) ParseExif(media_path string) (returnExif *models.MediaEXIF, returnErr error) { photoFile, err := os.Open(media_path) if err != nil { return nil, err diff --git a/api/scanner/exif/exif_test.go b/api/scanner/exif/exif_test.go index e586c2c6..9c2d32a8 100644 --- a/api/scanner/exif/exif_test.go +++ b/api/scanner/exif/exif_test.go @@ -1,4 +1,4 @@ -package exif +package exif_test import ( "fmt" @@ -9,6 +9,7 @@ import ( "github.com/barasher/go-exiftool" "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/scanner/exif" "github.com/photoview/photoview/api/test_utils" "github.com/stretchr/testify/assert" ) @@ -22,18 +23,18 @@ func TestExifParsers(t *testing.T) { parsers := []struct { name string - parser exifParser + parser exif.ExifParser }{ { name: "internal", - parser: &internalExifParser{}, + parser: exif.NewInternalExifParser(), }, } - if externalParser, err := newExiftoolParser(); err == nil { + if externalParser, err := exif.NewExiftoolParser(); err == nil { parsers = append(parsers, struct { name string - parser exifParser + parser exif.ExifParser }{ name: "external", parser: externalParser, From 0618b7011032aa792f7da47ccd36cb13a66bd8da Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Sat, 6 Nov 2021 13:23:59 +0100 Subject: [PATCH 4/5] Enable foreign keys for sqlite --- api/database/database.go | 5 +++++ api/scanner/cleanup_media.go | 4 ++-- api/scanner/cleanup_media_test.go | 33 ++++++++++++++++++------------ api/utils/environment_variables.go | 4 ++-- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/api/database/database.go b/api/database/database.go index ac94e125..b13564dd 100644 --- a/api/database/database.go +++ b/api/database/database.go @@ -103,6 +103,11 @@ func ConfigureDatabase(config *gorm.Config) (*gorm.DB, error) { return nil, err } + // Manually enable foreign keys for sqlite, as this isn't done by default + if drivers.DatabaseDriver() == drivers.DatabaseDriverSqlite { + db.Exec("PRAGMA foreign_keys = ON") + } + return db, nil } diff --git a/api/scanner/cleanup_media.go b/api/scanner/cleanup_media.go index 96f72cc3..7dff49bd 100644 --- a/api/scanner/cleanup_media.go +++ b/api/scanner/cleanup_media.go @@ -108,11 +108,11 @@ func deleteOldUserAlbums(db *gorm.DB, scannedAlbums []*models.Album, user *model // Delete old albums from database err := db.Transaction(func(tx *gorm.DB) error { - if err := tx.Where("album_id IN ?", deleteAlbumIDs).Delete(&models.UserAlbums{}).Error; err != nil { + if err := tx.Where("album_id IN (?)", deleteAlbumIDs).Delete(&models.UserAlbums{}).Error; err != nil { return err } - if err := tx.Where("id IN ?", deleteAlbumIDs).Delete(models.Album{}).Error; err != nil { + if err := tx.Where("id IN (?)", deleteAlbumIDs).Delete(models.Album{}).Error; err != nil { return err } diff --git a/api/scanner/cleanup_media_test.go b/api/scanner/cleanup_media_test.go index 7df7b1ec..ed5e708e 100644 --- a/api/scanner/cleanup_media_test.go +++ b/api/scanner/cleanup_media_test.go @@ -63,19 +63,26 @@ func TestCleanupMedia(t *testing.T) { return } - test_utils.RunScannerOnUser(t, db, user1) - assert.Equal(t, 9, countAllMedia()) - assert.Equal(t, 18, countAllMediaURLs()) + t.Run("Modify albums", func(t *testing.T) { - // move faces directory - assert.NoError(t, os.Rename(path.Join(test_dir, "faces"), path.Join(test_dir, "faces_moved"))) - test_utils.RunScannerAll(t, db) - assert.Equal(t, 9, countAllMedia()) - assert.Equal(t, 18, countAllMediaURLs()) + test_utils.RunScannerOnUser(t, db, user1) + assert.Equal(t, 9, countAllMedia()) + assert.Equal(t, 18, countAllMediaURLs()) - // remove faces_moved directory - assert.NoError(t, os.RemoveAll(path.Join(test_dir, "faces_moved"))) - test_utils.RunScannerAll(t, db) - assert.Equal(t, 3, countAllMedia()) - assert.Equal(t, 6, countAllMediaURLs()) + // move faces directory + assert.NoError(t, os.Rename(path.Join(test_dir, "faces"), path.Join(test_dir, "faces_moved"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 9, countAllMedia()) + assert.Equal(t, 18, countAllMediaURLs()) + + // remove faces_moved directory + assert.NoError(t, os.RemoveAll(path.Join(test_dir, "faces_moved"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 3, countAllMedia()) + assert.Equal(t, 6, countAllMediaURLs()) + }) + + // t.Run("Modify images", func(t *testing.T) { + + // }) } diff --git a/api/utils/environment_variables.go b/api/utils/environment_variables.go index 612ff793..76d5861a 100644 --- a/api/utils/environment_variables.go +++ b/api/utils/environment_variables.go @@ -66,13 +66,13 @@ func (v EnvironmentVariable) GetBool() bool { // ShouldServeUI whether or not the "serve ui" option is enabled func ShouldServeUI() bool { - return EnvServeUI.GetValue() == "1" + return EnvServeUI.GetBool() } // DevelopmentMode describes whether or not the server is running in development mode, // and should thus print debug informations and enable other features related to developing. func DevelopmentMode() bool { - return EnvDevelopmentMode.GetValue() == "1" + return EnvDevelopmentMode.GetBool() } // UIPath returns the value from where the static UI files are located if SERVE_UI=1 From 6545a042250f4ce1f48982542a4cd6a24355db3a Mon Sep 17 00:00:00 2001 From: viktorstrate Date: Sat, 6 Nov 2021 13:30:23 +0100 Subject: [PATCH 5/5] Add tests for modifying media on disk --- api/scanner/cleanup_media_test.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/api/scanner/cleanup_media_test.go b/api/scanner/cleanup_media_test.go index ed5e708e..63c2476f 100644 --- a/api/scanner/cleanup_media_test.go +++ b/api/scanner/cleanup_media_test.go @@ -7,6 +7,7 @@ import ( "github.com/otiai10/copy" "github.com/photoview/photoview/api/graphql/models" + "github.com/photoview/photoview/api/scanner/face_detection" "github.com/photoview/photoview/api/test_utils" "github.com/stretchr/testify/assert" ) @@ -15,6 +16,10 @@ func TestCleanupMedia(t *testing.T) { test_utils.FilesystemTest(t) db := test_utils.DatabaseTest(t) + if !assert.NoError(t, face_detection.InitializeFaceDetector(db)) { + return + } + test_dir := t.TempDir() copy.Copy("./test_data", test_dir) @@ -82,7 +87,15 @@ func TestCleanupMedia(t *testing.T) { assert.Equal(t, 6, countAllMediaURLs()) }) - // t.Run("Modify images", func(t *testing.T) { + t.Run("Modify images", func(t *testing.T) { + assert.NoError(t, os.Rename(path.Join(test_dir, "buttercup_close_summer_yellow.jpg"), path.Join(test_dir, "yellow-flower.jpg"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 3, countAllMedia()) + assert.Equal(t, 6, countAllMediaURLs()) - // }) + assert.NoError(t, os.Remove(path.Join(test_dir, "lilac_lilac_bush_lilac.jpg"))) + test_utils.RunScannerAll(t, db) + assert.Equal(t, 2, countAllMedia()) + assert.Equal(t, 4, countAllMediaURLs()) + }) }