From afba61f55d8daa03c96aa0c3f01025740ab073b4 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 4 Jun 2024 23:20:20 +0200 Subject: [PATCH 1/4] test(storage): export UninitializedStorage to simulate failure (cherry picked from commit 20148e061aff1e63f0549aa444d41b12d6cc041f) --- modules/storage/helper.go | 2 +- modules/storage/helper_test.go | 2 +- modules/storage/storage.go | 16 ++++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/modules/storage/helper.go b/modules/storage/helper.go index f8dff9e64d..4c24209f4f 100644 --- a/modules/storage/helper.go +++ b/modules/storage/helper.go @@ -10,7 +10,7 @@ import ( "os" ) -var uninitializedStorage = discardStorage("uninitialized storage") +var UninitializedStorage = discardStorage("uninitialized storage") type discardStorage string diff --git a/modules/storage/helper_test.go b/modules/storage/helper_test.go index f4c2d0467f..d0665b6dc9 100644 --- a/modules/storage/helper_test.go +++ b/modules/storage/helper_test.go @@ -12,7 +12,7 @@ import ( func Test_discardStorage(t *testing.T) { tests := []discardStorage{ - uninitializedStorage, + UninitializedStorage, discardStorage("empty"), } for _, tt := range tests { diff --git a/modules/storage/storage.go b/modules/storage/storage.go index 8f970b5dfc..90f74eb2bd 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -109,26 +109,26 @@ func SaveFrom(objStorage ObjectStorage, p string, callback func(w io.Writer) err var ( // Attachments represents attachments storage - Attachments ObjectStorage = uninitializedStorage + Attachments ObjectStorage = UninitializedStorage // LFS represents lfs storage - LFS ObjectStorage = uninitializedStorage + LFS ObjectStorage = UninitializedStorage // Avatars represents user avatars storage - Avatars ObjectStorage = uninitializedStorage + Avatars ObjectStorage = UninitializedStorage // RepoAvatars represents repository avatars storage - RepoAvatars ObjectStorage = uninitializedStorage + RepoAvatars ObjectStorage = UninitializedStorage // RepoArchives represents repository archives storage - RepoArchives ObjectStorage = uninitializedStorage + RepoArchives ObjectStorage = UninitializedStorage // Packages represents packages storage - Packages ObjectStorage = uninitializedStorage + Packages ObjectStorage = UninitializedStorage // Actions represents actions storage - Actions ObjectStorage = uninitializedStorage + Actions ObjectStorage = UninitializedStorage // Actions Artifacts represents actions artifacts storage - ActionsArtifacts ObjectStorage = uninitializedStorage + ActionsArtifacts ObjectStorage = UninitializedStorage ) // Init init the stoarge From 3ba58114c79b7938aca2090a6c57409fda4c66bc Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 4 Jun 2024 23:21:00 +0200 Subject: [PATCH 2/4] test(avatar): deleting a user avatar and file is atomic The avatar must not be unset in the database if there is a failure to remove the avatar file from storage (file or S3). The two operations are wrapped in a transaction for that purpose and this test verifies it is effective. See 1be797faba301503e29db9de7eb32335a684464c Fix bug on avatar (cherry picked from commit c139efb1e9e65998ad557159b0c93ddc871b6b59) --- services/user/avatar_test.go | 47 ++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 services/user/avatar_test.go diff --git a/services/user/avatar_test.go b/services/user/avatar_test.go new file mode 100644 index 0000000000..0dc4dec651 --- /dev/null +++ b/services/user/avatar_test.go @@ -0,0 +1,47 @@ +// Copyright The Forgejo Authors. +// SPDX-License-Identifier: MIT + +package user + +import ( + "bytes" + "image" + "image/png" + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/storage" + "code.gitea.io/gitea/modules/test" + + "github.com/stretchr/testify/assert" +) + +func TestUserDeleteAvatar(t *testing.T) { + myImage := image.NewRGBA(image.Rect(0, 0, 1, 1)) + var buff bytes.Buffer + png.Encode(&buff, myImage) + + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + err := UploadAvatar(db.DefaultContext, user, buff.Bytes()) + assert.NoError(t, err) + verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.NotEqual(t, "", verification.Avatar) + + t.Run("AtomicStorageFailure", func(t *testing.T) { + defer test.MockVariableValue[storage.ObjectStorage](&storage.Avatars, storage.UninitializedStorage)() + err = DeleteAvatar(db.DefaultContext, user) + assert.Error(t, err) + verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.True(t, verification.UseCustomAvatar) + }) + + err = DeleteAvatar(db.DefaultContext, user) + assert.NoError(t, err) + + verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.Equal(t, "", verification.Avatar) +} From 32d8ada0e712070615b22aabe4ef01b65e7b0133 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 19 May 2024 12:58:39 +0800 Subject: [PATCH 3/4] Fix bug on avatar (#31008) Co-authored-by: silverwind (cherry picked from commit 58a03e9fadb345de5653345c2a68ecfd0750940a) (cherry picked from commit 1be797faba301503e29db9de7eb32335a684464c) --- services/user/avatar.go | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/services/user/avatar.go b/services/user/avatar.go index 2d6c3faf9a..3f87466eaa 100644 --- a/services/user/avatar.go +++ b/services/user/avatar.go @@ -5,8 +5,10 @@ package user import ( "context" + "errors" "fmt" "io" + "os" "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" @@ -48,16 +50,24 @@ func UploadAvatar(ctx context.Context, u *user_model.User, data []byte) error { func DeleteAvatar(ctx context.Context, u *user_model.User) error { aPath := u.CustomAvatarRelativePath() log.Trace("DeleteAvatar[%d]: %s", u.ID, aPath) - if len(u.Avatar) > 0 { - if err := storage.Avatars.Delete(aPath); err != nil { - return fmt.Errorf("Failed to remove %s: %w", aPath, err) - } - } - u.UseCustomAvatar = false - u.Avatar = "" - if _, err := db.GetEngine(ctx).ID(u.ID).Cols("avatar, use_custom_avatar").Update(u); err != nil { - return fmt.Errorf("DeleteAvatar: %w", err) - } - return nil + return db.WithTx(ctx, func(ctx context.Context) error { + hasAvatar := len(u.Avatar) > 0 + u.UseCustomAvatar = false + u.Avatar = "" + if _, err := db.GetEngine(ctx).ID(u.ID).Cols("avatar, use_custom_avatar").Update(u); err != nil { + return fmt.Errorf("DeleteAvatar: %w", err) + } + + if hasAvatar { + if err := storage.Avatars.Delete(aPath); err != nil { + if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to remove %s: %w", aPath, err) + } + log.Warn("Deleting avatar %s but it doesn't exist", aPath) + } + } + + return nil + }) } From cf2d8b57ae2ba4bf57c37810153cfe88c7e6b10f Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 5 Jun 2024 09:10:42 +0200 Subject: [PATCH 4/4] test(avatar): deleting a user avatar is idempotent If the avatar file in storage does not exist, it is not an error and the database can be updated. See 1be797faba301503e29db9de7eb32335a684464c Fix bug on avatar (cherry picked from commit d2c4d833f4e0ef62a095f9829f927667e9dcca7e) --- modules/storage/helper.go | 16 ++++----- modules/storage/helper_test.go | 4 +-- modules/storage/storage.go | 10 +++--- services/user/avatar_test.go | 61 ++++++++++++++++++++++++++-------- 4 files changed, 62 insertions(+), 29 deletions(-) diff --git a/modules/storage/helper.go b/modules/storage/helper.go index 4c24209f4f..95f1c7b9a8 100644 --- a/modules/storage/helper.go +++ b/modules/storage/helper.go @@ -10,30 +10,30 @@ import ( "os" ) -var UninitializedStorage = discardStorage("uninitialized storage") +var UninitializedStorage = DiscardStorage("uninitialized storage") -type discardStorage string +type DiscardStorage string -func (s discardStorage) Open(_ string) (Object, error) { +func (s DiscardStorage) Open(_ string) (Object, error) { return nil, fmt.Errorf("%s", s) } -func (s discardStorage) Save(_ string, _ io.Reader, _ int64) (int64, error) { +func (s DiscardStorage) Save(_ string, _ io.Reader, _ int64) (int64, error) { return 0, fmt.Errorf("%s", s) } -func (s discardStorage) Stat(_ string) (os.FileInfo, error) { +func (s DiscardStorage) Stat(_ string) (os.FileInfo, error) { return nil, fmt.Errorf("%s", s) } -func (s discardStorage) Delete(_ string) error { +func (s DiscardStorage) Delete(_ string) error { return fmt.Errorf("%s", s) } -func (s discardStorage) URL(_, _ string) (*url.URL, error) { +func (s DiscardStorage) URL(_, _ string) (*url.URL, error) { return nil, fmt.Errorf("%s", s) } -func (s discardStorage) IterateObjects(_ string, _ func(string, Object) error) error { +func (s DiscardStorage) IterateObjects(_ string, _ func(string, Object) error) error { return fmt.Errorf("%s", s) } diff --git a/modules/storage/helper_test.go b/modules/storage/helper_test.go index d0665b6dc9..f1f9791044 100644 --- a/modules/storage/helper_test.go +++ b/modules/storage/helper_test.go @@ -11,9 +11,9 @@ import ( ) func Test_discardStorage(t *testing.T) { - tests := []discardStorage{ + tests := []DiscardStorage{ UninitializedStorage, - discardStorage("empty"), + DiscardStorage("empty"), } for _, tt := range tests { t.Run(string(tt), func(t *testing.T) { diff --git a/modules/storage/storage.go b/modules/storage/storage.go index 90f74eb2bd..b83b1c7929 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -170,7 +170,7 @@ func initAvatars() (err error) { func initAttachments() (err error) { if !setting.Attachment.Enabled { - Attachments = discardStorage("Attachment isn't enabled") + Attachments = DiscardStorage("Attachment isn't enabled") return nil } log.Info("Initialising Attachment storage with type: %s", setting.Attachment.Storage.Type) @@ -180,7 +180,7 @@ func initAttachments() (err error) { func initLFS() (err error) { if !setting.LFS.StartServer { - LFS = discardStorage("LFS isn't enabled") + LFS = DiscardStorage("LFS isn't enabled") return nil } log.Info("Initialising LFS storage with type: %s", setting.LFS.Storage.Type) @@ -202,7 +202,7 @@ func initRepoArchives() (err error) { func initPackages() (err error) { if !setting.Packages.Enabled { - Packages = discardStorage("Packages isn't enabled") + Packages = DiscardStorage("Packages isn't enabled") return nil } log.Info("Initialising Packages storage with type: %s", setting.Packages.Storage.Type) @@ -212,8 +212,8 @@ func initPackages() (err error) { func initActions() (err error) { if !setting.Actions.Enabled { - Actions = discardStorage("Actions isn't enabled") - ActionsArtifacts = discardStorage("ActionsArtifacts isn't enabled") + Actions = DiscardStorage("Actions isn't enabled") + ActionsArtifacts = DiscardStorage("ActionsArtifacts isn't enabled") return nil } log.Info("Initialising Actions storage with type: %s", setting.Actions.LogStorage.Type) diff --git a/services/user/avatar_test.go b/services/user/avatar_test.go index 0dc4dec651..557ddccec0 100644 --- a/services/user/avatar_test.go +++ b/services/user/avatar_test.go @@ -7,6 +7,7 @@ import ( "bytes" "image" "image/png" + "os" "testing" "code.gitea.io/gitea/models/db" @@ -18,30 +19,62 @@ import ( "github.com/stretchr/testify/assert" ) +type alreadyDeletedStorage struct { + storage.DiscardStorage +} + +func (s alreadyDeletedStorage) Delete(_ string) error { + return os.ErrNotExist +} + func TestUserDeleteAvatar(t *testing.T) { myImage := image.NewRGBA(image.Rect(0, 0, 1, 1)) var buff bytes.Buffer png.Encode(&buff, myImage) - assert.NoError(t, unittest.PrepareTestDatabase()) - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - - err := UploadAvatar(db.DefaultContext, user, buff.Bytes()) - assert.NoError(t, err) - verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - assert.NotEqual(t, "", verification.Avatar) - t.Run("AtomicStorageFailure", func(t *testing.T) { - defer test.MockVariableValue[storage.ObjectStorage](&storage.Avatars, storage.UninitializedStorage)() + defer test.MockProtect[storage.ObjectStorage](&storage.Avatars)() + + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + err := UploadAvatar(db.DefaultContext, user, buff.Bytes()) + assert.NoError(t, err) + verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.NotEqual(t, "", verification.Avatar) + + // fail to delete ... + storage.Avatars = storage.UninitializedStorage err = DeleteAvatar(db.DefaultContext, user) assert.Error(t, err) - verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + // ... the avatar is not removed from the database + verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) assert.True(t, verification.UseCustomAvatar) + + // already deleted ... + storage.Avatars = alreadyDeletedStorage{} + err = DeleteAvatar(db.DefaultContext, user) + assert.NoError(t, err) + + // ... the avatar is removed from the database + verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.Equal(t, "", verification.Avatar) }) - err = DeleteAvatar(db.DefaultContext, user) - assert.NoError(t, err) + t.Run("Success", func(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - assert.Equal(t, "", verification.Avatar) + err := UploadAvatar(db.DefaultContext, user, buff.Bytes()) + assert.NoError(t, err) + verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.NotEqual(t, "", verification.Avatar) + + err = DeleteAvatar(db.DefaultContext, user) + assert.NoError(t, err) + + verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.Equal(t, "", verification.Avatar) + }) }