From dd90d476ad366b3da434d0cdae15f7d4307383dd Mon Sep 17 00:00:00 2001 From: Kostiantyn <32730812+kkovaletp@users.noreply.github.com> Date: Fri, 11 Oct 2024 12:31:49 +0300 Subject: [PATCH] Refactoring part 2.4: Long func refactoring in `graphql/resolvers` (#1076) * Split long functions and optimize code * Addressing comments * Remove unnecessary input param --------- Co-authored-by: Konstantin Koval --- api/graphql/resolvers/album.go | 20 ++++++-- api/graphql/resolvers/faces.go | 48 ++++++++++++------- api/graphql/resolvers/media.go | 5 +- api/graphql/resolvers/mediaGeoJson.go | 8 ++-- api/graphql/resolvers/scanner.go | 14 +++++- api/graphql/resolvers/search.go | 1 + api/graphql/resolvers/share_token.go | 10 +++- api/graphql/resolvers/thumbnails.go | 12 +++-- api/graphql/resolvers/timeline.go | 4 +- api/graphql/resolvers/user.go | 69 +++++++++++++++++---------- 10 files changed, 132 insertions(+), 59 deletions(-) diff --git a/api/graphql/resolvers/album.go b/api/graphql/resolvers/album.go index 799c6edf..789b4f21 100644 --- a/api/graphql/resolvers/album.go +++ b/api/graphql/resolvers/album.go @@ -11,7 +11,9 @@ import ( "gorm.io/gorm" ) -func (r *queryResolver) MyAlbums(ctx context.Context, order *models.Ordering, paginate *models.Pagination, onlyRoot *bool, showEmpty *bool, onlyWithFavorites *bool) ([]*models.Album, error) { +func (r *queryResolver) MyAlbums(ctx context.Context, order *models.Ordering, paginate *models.Pagination, onlyRoot *bool, + showEmpty *bool, onlyWithFavorites *bool) ([]*models.Album, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized @@ -21,6 +23,7 @@ func (r *queryResolver) MyAlbums(ctx context.Context, order *models.Ordering, pa } func (r *queryResolver) Album(ctx context.Context, id int, tokenCredentials *models.ShareTokenCredentials) (*models.Album, error) { + db := r.DB(ctx) if tokenCredentials != nil { @@ -34,7 +37,9 @@ func (r *queryResolver) Album(ctx context.Context, id int, tokenCredentials *mod return shareToken.Album, nil } - subAlbum, err := shareToken.Album.GetChildren(db, func(query *gorm.DB) *gorm.DB { return query.Where("sub_albums.id = ?", id) }) + subAlbum, err := shareToken.Album.GetChildren(db, func(query *gorm.DB) *gorm.DB { + return query.Where("sub_albums.id = ?", id) + }) if err != nil { return nil, errors.Wrapf(err, "find sub album of share token (%s)", tokenCredentials.Token) } @@ -59,12 +64,16 @@ func (r *Resolver) Album() api.AlbumResolver { type albumResolver struct{ *Resolver } -func (r *albumResolver) Media(ctx context.Context, album *models.Album, order *models.Ordering, paginate *models.Pagination, onlyFavorites *bool) ([]*models.Media, error) { +func (r *albumResolver) Media(ctx context.Context, album *models.Album, order *models.Ordering, + paginate *models.Pagination, onlyFavorites *bool) ([]*models.Media, error) { + db := r.DB(ctx) query := db. Where("media.album_id = ?", album.ID). - Where("media.id IN (?)", db.Model(&models.MediaURL{}).Select("media_urls.media_id").Where("media_urls.media_id = media.id")) + Where("media.id IN (?)", db.Model(&models.MediaURL{}). + Select("media_urls.media_id"). + Where("media_urls.media_id = media.id")) if onlyFavorites != nil && *onlyFavorites == true { user := auth.UserFromContext(ctx) @@ -93,7 +102,8 @@ func (r *albumResolver) Thumbnail(ctx context.Context, album *models.Album) (*mo return album.Thumbnail(r.DB(ctx)) } -func (r *albumResolver) SubAlbums(ctx context.Context, parent *models.Album, order *models.Ordering, paginate *models.Pagination) ([]*models.Album, error) { +func (r *albumResolver) SubAlbums(ctx context.Context, parent *models.Album, order *models.Ordering, + paginate *models.Pagination) ([]*models.Album, error) { var albums []*models.Album diff --git a/api/graphql/resolvers/faces.go b/api/graphql/resolvers/faces.go index 857fa328..d8d254c2 100644 --- a/api/graphql/resolvers/faces.go +++ b/api/graphql/resolvers/faces.go @@ -60,7 +60,8 @@ func (r imageFaceResolver) Media(ctx context.Context, obj *models.ImageFace) (*m return &obj.Media, nil } -func (r faceGroupResolver) ImageFaces(ctx context.Context, obj *models.FaceGroup, paginate *models.Pagination) ([]*models.ImageFace, error) { +func (r faceGroupResolver) ImageFaces(ctx context.Context, obj *models.FaceGroup, + paginate *models.Pagination) ([]*models.ImageFace, error) { db := r.DB(ctx) user := auth.UserFromContext(ctx) if user == nil { @@ -185,7 +186,8 @@ func (r *queryResolver) MyFaceGroups(ctx context.Context, paginate *models.Pagin faceGroupQuery := db. Joins("JOIN image_faces ON image_faces.face_group_id = face_groups.id"). - Where("image_faces.media_id IN (?)", db.Select("media.id").Table("media").Where(mediaAlbumIDinQuestion, userAlbumIDs)). + Where("image_faces.media_id IN (?)", + db.Select("media.id").Table("media").Where(mediaAlbumIDinQuestion, userAlbumIDs)). Group("image_faces.face_group_id"). Group("face_groups.id"). Order("CASE WHEN label IS NULL THEN 1 ELSE 0 END"). @@ -201,7 +203,8 @@ func (r *queryResolver) MyFaceGroups(ctx context.Context, paginate *models.Pagin return faceGroups, nil } -func (r *mutationResolver) SetFaceGroupLabel(ctx context.Context, faceGroupID int, label *string) (*models.FaceGroup, error) { +func (r *mutationResolver) SetFaceGroupLabel(ctx context.Context, faceGroupID int, + label *string) (*models.FaceGroup, error) { db := r.DB(ctx) user := auth.UserFromContext(ctx) if user == nil { @@ -224,7 +227,8 @@ func (r *mutationResolver) SetFaceGroupLabel(ctx context.Context, faceGroupID in return faceGroup, nil } -func (r *mutationResolver) CombineFaceGroups(ctx context.Context, destinationFaceGroupID int, sourceFaceGroupID int) (*models.FaceGroup, error) { +func (r *mutationResolver) CombineFaceGroups(ctx context.Context, destinationFaceGroupID int, + sourceFaceGroupID int) (*models.FaceGroup, error) { db := r.DB(ctx) user := auth.UserFromContext(ctx) if user == nil { @@ -246,7 +250,9 @@ func (r *mutationResolver) CombineFaceGroups(ctx context.Context, destinationFac } updateError := db.Transaction(func(tx *gorm.DB) error { - if err := tx.Model(&models.ImageFace{}).Where(faceGroupIDisQuestion, sourceFaceGroup.ID).Update("face_group_id", destinationFaceGroup.ID).Error; err != nil { + if err := tx.Model(&models.ImageFace{}). + Where(faceGroupIDisQuestion, sourceFaceGroup.ID). + Update("face_group_id", destinationFaceGroup.ID).Error; err != nil { return err } @@ -266,7 +272,8 @@ func (r *mutationResolver) CombineFaceGroups(ctx context.Context, destinationFac return destinationFaceGroup, nil } -func (r *mutationResolver) MoveImageFaces(ctx context.Context, imageFaceIDs []int, destinationFaceGroupID int) (*models.FaceGroup, error) { +func (r *mutationResolver) MoveImageFaces(ctx context.Context, imageFaceIDs []int, + destinationFaceGroupID int) (*models.FaceGroup, error) { db := r.DB(ctx) user := auth.UserFromContext(ctx) if user == nil { @@ -313,17 +320,8 @@ func (r *mutationResolver) MoveImageFaces(ctx context.Context, imageFaceIDs []in } // delete face groups if they have become empty - for _, faceGroup := range sourceFaceGroups { - var count int64 - if err := tx.Model(&models.ImageFace{}).Where(faceGroupIDisQuestion, faceGroup.ID).Count(&count).Error; err != nil { - return err - } - - if count == 0 { - if err := tx.Delete(&faceGroup).Error; err != nil { - return err - } - } + if err := deleteEmptyFaceGroups(sourceFaceGroups, tx); err != nil { + return err } return nil @@ -338,6 +336,22 @@ func (r *mutationResolver) MoveImageFaces(ctx context.Context, imageFaceIDs []in return destFaceGroup, nil } +func deleteEmptyFaceGroups(sourceFaceGroups []*models.FaceGroup, tx *gorm.DB) error { + for _, faceGroup := range sourceFaceGroups { + var count int64 + if err := tx.Model(&models.ImageFace{}).Where(faceGroupIDisQuestion, faceGroup.ID).Count(&count).Error; err != nil { + return err + } + + if count == 0 { + if err := tx.Delete(&faceGroup).Error; err != nil { + return err + } + } + } + return nil +} + func (r *mutationResolver) RecognizeUnlabeledFaces(ctx context.Context) ([]*models.ImageFace, error) { db := r.DB(ctx) user := auth.UserFromContext(ctx) diff --git a/api/graphql/resolvers/media.go b/api/graphql/resolvers/media.go index 939fd341..d874639d 100644 --- a/api/graphql/resolvers/media.go +++ b/api/graphql/resolvers/media.go @@ -14,6 +14,7 @@ import ( ) func (r *queryResolver) MyMedia(ctx context.Context, order *models.Ordering, paginate *models.Pagination) ([]*models.Media, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, errors.New("unauthorized") @@ -23,6 +24,7 @@ func (r *queryResolver) MyMedia(ctx context.Context, order *models.Ordering, pag } func (r *queryResolver) Media(ctx context.Context, id int, tokenCredentials *models.ShareTokenCredentials) (*models.Media, error) { + db := r.DB(ctx) if tokenCredentials != nil { @@ -46,7 +48,8 @@ func (r *queryResolver) Media(ctx context.Context, id int, tokenCredentials *mod err := db. Joins("Album"). Where("media.id = ?", id). - Where("EXISTS (SELECT * FROM user_albums WHERE user_albums.album_id = media.album_id AND user_albums.user_id = ?)", user.ID). + Where("EXISTS (SELECT * FROM user_albums WHERE user_albums.album_id = media.album_id AND user_albums.user_id = ?)", + user.ID). Where("media.id IN (?)", db.Model(&models.MediaURL{}).Select("media_id").Where("media_urls.media_id = media.id")). First(&media).Error diff --git a/api/graphql/resolvers/mediaGeoJson.go b/api/graphql/resolvers/mediaGeoJson.go index a5c10141..89985aa2 100644 --- a/api/graphql/resolvers/mediaGeoJson.go +++ b/api/graphql/resolvers/mediaGeoJson.go @@ -78,10 +78,10 @@ func (r *queryResolver) MyMediaGeoJSON(ctx context.Context) (interface{}, error) var media []*geoMedia err := r.DB(ctx).Table("media"). - Select( - "media.id AS media_id, media.title AS media_title, "+ - "media_urls.media_name AS thumbnail_name, media_urls.width AS thumbnail_width, media_urls.height AS thumbnail_height, "+ - "media_exif.gps_latitude AS latitude, media_exif.gps_longitude AS longitude"). + Select("media.id AS media_id, media.title AS media_title, "+ + "media_urls.media_name AS thumbnail_name, media_urls.width AS thumbnail_width, "+ + "media_urls.height AS thumbnail_height, media_exif.gps_latitude AS latitude, "+ + "media_exif.gps_longitude AS longitude"). Joins("INNER JOIN media_exif ON media.exif_id = media_exif.id"). Joins("INNER JOIN media_urls ON media.id = media_urls.media_id"). Joins("INNER JOIN user_albums ON media.album_id = user_albums.album_id"). diff --git a/api/graphql/resolvers/scanner.go b/api/graphql/resolvers/scanner.go index a5d3fca2..952724c9 100644 --- a/api/graphql/resolvers/scanner.go +++ b/api/graphql/resolvers/scanner.go @@ -49,7 +49,12 @@ func (r *mutationResolver) SetPeriodicScanInterval(ctx context.Context, interval return 0, errors.New("interval must be 0 or above") } - if err := db.Session(&gorm.Session{AllowGlobalUpdate: true}).Model(&models.SiteInfo{}).Update("periodic_scan_interval", interval).Error; err != nil { + if err := db. + Session(&gorm.Session{AllowGlobalUpdate: true}). + Model(&models.SiteInfo{}). + Update("periodic_scan_interval", interval). + Error; err != nil { + return 0, err } @@ -73,7 +78,12 @@ func (r *mutationResolver) SetScannerConcurrentWorkers(ctx context.Context, work return 0, errors.New("multiple workers not supported for SQLite databases") } - if err := db.Session(&gorm.Session{AllowGlobalUpdate: true}).Model(&models.SiteInfo{}).Update("concurrent_workers", workers).Error; err != nil { + if err := db. + Session(&gorm.Session{AllowGlobalUpdate: true}). + Model(&models.SiteInfo{}). + Update("concurrent_workers", workers). + Error; err != nil { + return 0, err } diff --git a/api/graphql/resolvers/search.go b/api/graphql/resolvers/search.go index f0366b5b..6e6d1d19 100644 --- a/api/graphql/resolvers/search.go +++ b/api/graphql/resolvers/search.go @@ -10,6 +10,7 @@ import ( ) func (r *Resolver) Search(ctx context.Context, query string, limitMedia *int, limitAlbums *int) (*models.SearchResult, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized diff --git a/api/graphql/resolvers/share_token.go b/api/graphql/resolvers/share_token.go index c156b201..cadc12f6 100644 --- a/api/graphql/resolvers/share_token.go +++ b/api/graphql/resolvers/share_token.go @@ -65,6 +65,7 @@ func (r *queryResolver) ShareToken(ctx context.Context, credentials models.Share } func (r *queryResolver) ShareTokenValidatePassword(ctx context.Context, credentials models.ShareTokenCredentials) (bool, error) { + var token models.ShareToken if err := r.DB(ctx).Where("value = ?", credentials.Token).First(&token).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { @@ -93,7 +94,9 @@ func (r *queryResolver) ShareTokenValidatePassword(ctx context.Context, credenti return true, nil } -func (r *mutationResolver) ShareAlbum(ctx context.Context, albumID int, expire *time.Time, password *string) (*models.ShareToken, error) { +func (r *mutationResolver) ShareAlbum(ctx context.Context, albumID int, expire *time.Time, + password *string) (*models.ShareToken, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized @@ -102,7 +105,9 @@ func (r *mutationResolver) ShareAlbum(ctx context.Context, albumID int, expire * return actions.AddAlbumShare(r.DB(ctx), user, albumID, expire, password) } -func (r *mutationResolver) ShareMedia(ctx context.Context, mediaID int, expire *time.Time, password *string) (*models.ShareToken, error) { +func (r *mutationResolver) ShareMedia(ctx context.Context, mediaID int, expire *time.Time, + password *string) (*models.ShareToken, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized @@ -121,6 +126,7 @@ func (r *mutationResolver) DeleteShareToken(ctx context.Context, tokenValue stri } func (r *mutationResolver) ProtectShareToken(ctx context.Context, tokenValue string, password *string) (*models.ShareToken, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized diff --git a/api/graphql/resolvers/thumbnails.go b/api/graphql/resolvers/thumbnails.go index 63051440..18163f03 100644 --- a/api/graphql/resolvers/thumbnails.go +++ b/api/graphql/resolvers/thumbnails.go @@ -8,14 +8,21 @@ import ( "gorm.io/gorm" ) -func (r *mutationResolver) SetThumbnailDownsampleMethod(ctx context.Context, method models.ThumbnailFilter) (models.ThumbnailFilter, error) { +func (r *mutationResolver) SetThumbnailDownsampleMethod(ctx context.Context, + method models.ThumbnailFilter) (models.ThumbnailFilter, error) { + db := r.DB(ctx) // if method > 5 { // return 0, errors.New("The requested filter is unsupported, defaulting to nearest neighbor") // } - if err := db.Session(&gorm.Session{AllowGlobalUpdate: true}).Model(&models.SiteInfo{}).Update("thumbnail_method", method).Error; err != nil { + if err := db. + Session(&gorm.Session{AllowGlobalUpdate: true}). + Model(&models.SiteInfo{}). + Update("thumbnail_method", method). + Error; err != nil { + return models.ThumbnailFilterNearestNeighbor, err } @@ -32,5 +39,4 @@ func (r *mutationResolver) SetThumbnailDownsampleMethod(ctx context.Context, met // langTrans = &lng // } - } diff --git a/api/graphql/resolvers/timeline.go b/api/graphql/resolvers/timeline.go index d78f70f6..76922910 100644 --- a/api/graphql/resolvers/timeline.go +++ b/api/graphql/resolvers/timeline.go @@ -9,7 +9,9 @@ import ( "github.com/photoview/photoview/api/graphql/models/actions" ) -func (r *queryResolver) MyTimeline(ctx context.Context, paginate *models.Pagination, onlyFavorites *bool, fromDate *time.Time) ([]*models.Media, error) { +func (r *queryResolver) MyTimeline(ctx context.Context, paginate *models.Pagination, onlyFavorites *bool, + fromDate *time.Time) ([]*models.Media, error) { + user := auth.UserFromContext(ctx) if user == nil { return nil, auth.ErrUnauthorized diff --git a/api/graphql/resolvers/user.go b/api/graphql/resolvers/user.go index 1c258235..d0d20b49 100644 --- a/api/graphql/resolvers/user.go +++ b/api/graphql/resolvers/user.go @@ -26,7 +26,8 @@ func (r *Resolver) User() api.UserResolver { return &userResolver{r} } -func (r *queryResolver) User(ctx context.Context, order *models.Ordering, paginate *models.Pagination) ([]*models.User, error) { +func (r *queryResolver) User(ctx context.Context, order *models.Ordering, + paginate *models.Pagination) ([]*models.User, error) { var users []*models.User @@ -72,7 +73,8 @@ func (r *queryResolver) MyUser(ctx context.Context) (*models.User, error) { return user, nil } -func (r *mutationResolver) AuthorizeUser(ctx context.Context, username string, password string) (*models.AuthorizeResult, error) { +func (r *mutationResolver) AuthorizeUser(ctx context.Context, username string, + password string) (*models.AuthorizeResult, error) { db := r.DB(ctx) user, err := models.AuthorizeUser(db, username, password) if err != nil { @@ -104,7 +106,8 @@ func (r *mutationResolver) AuthorizeUser(ctx context.Context, username string, p }, nil } -func (r *mutationResolver) InitialSetupWizard(ctx context.Context, username string, password string, rootPath string) (*models.AuthorizeResult, error) { +func (r *mutationResolver) InitialSetupWizard(ctx context.Context, username string, password string, + rootPath string) (*models.AuthorizeResult, error) { db := r.DB(ctx) siteInfo, err := models.GetSiteInfo(db) if err != nil { @@ -201,7 +204,8 @@ func (r *mutationResolver) ChangeUserPreferences(ctx context.Context, language * } // Admin queries -func (r *mutationResolver) UpdateUser(ctx context.Context, id int, username *string, password *string, admin *bool) (*models.User, error) { +func (r *mutationResolver) UpdateUser(ctx context.Context, id int, username *string, password *string, + admin *bool) (*models.User, error) { db := r.DB(ctx) if username == nil && password == nil && admin == nil { @@ -238,7 +242,8 @@ func (r *mutationResolver) UpdateUser(ctx context.Context, id int, username *str return &user, nil } -func (r *mutationResolver) CreateUser(ctx context.Context, username string, password *string, admin bool) (*models.User, error) { +func (r *mutationResolver) CreateUser(ctx context.Context, username string, password *string, + admin bool) (*models.User, error) { var user *models.User @@ -290,7 +295,6 @@ func (r *mutationResolver) UserRemoveRootAlbum(ctx context.Context, userID int, } var deletedAlbumIDs []int = nil - transactionError := db.Transaction(func(tx *gorm.DB) error { if err := tx.Raw("DELETE FROM user_albums WHERE user_id = ? AND album_id = ?", userID, albumID).Error; err != nil { return err @@ -316,22 +320,11 @@ func (r *mutationResolver) UserRemoveRootAlbum(ctx context.Context, userID int, } // Cleanup if no user owns the album anymore - var userAlbumCount int - if err := tx.Raw("SELECT COUNT(user_id) FROM user_albums WHERE album_id = ?", albumID).Scan(&userAlbumCount).Error; err != nil { + deletedAlbumIDs, err = cleanup(tx, albumID, childAlbumIDs) + if err != nil { return err } - if userAlbumCount == 0 { - deletedAlbumIDs = append(childAlbumIDs, albumID) - childAlbumIDs = nil - - // Delete albums from database - if err := tx.Delete(&models.Album{}, "id IN (?)", deletedAlbumIDs).Error; err != nil { - deletedAlbumIDs = nil - return err - } - } - return nil }) @@ -339,23 +332,51 @@ func (r *mutationResolver) UserRemoveRootAlbum(ctx context.Context, userID int, return nil, transactionError } + if err := clearCacheAndReloadFaces(db, deletedAlbumIDs); err != nil { + return nil, err + } + + return &album, nil +} + +func cleanup(tx *gorm.DB, albumID int, childAlbumIDs []int) ([]int, error) { + var userAlbumCount int + var deletedAlbumIDs []int = nil + + if err := tx.Raw("SELECT COUNT(user_id) FROM user_albums WHERE album_id = ?", + albumID).Scan(&userAlbumCount).Error; err != nil { + + return nil, err + } + + if userAlbumCount == 0 { + deletedAlbumIDs = append(childAlbumIDs, albumID) + childAlbumIDs = nil + // Delete albums from database + if err := tx.Delete(&models.Album{}, "id IN (?)", deletedAlbumIDs).Error; err != nil { + deletedAlbumIDs = nil + return nil, err + } + } + return deletedAlbumIDs, nil +} + +func clearCacheAndReloadFaces(db *gorm.DB, deletedAlbumIDs []int) error { if deletedAlbumIDs != nil { // Delete albums from cache for _, id := range deletedAlbumIDs { cacheAlbumPath := path.Join(utils.MediaCachePath(), strconv.Itoa(id)) if err := os.RemoveAll(cacheAlbumPath); err != nil { - return nil, err + return err } } - // Reload faces as media might have been deleted if face_detection.GlobalFaceDetector != nil { if err := face_detection.GlobalFaceDetector.ReloadFacesFromDatabase(db); err != nil { - return nil, err + return err } } } - - return &album, nil + return nil }