From cf2d8b57ae2ba4bf57c37810153cfe88c7e6b10f Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 5 Jun 2024 09:10:42 +0200 Subject: [PATCH] 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) + }) }