From d2c4d833f4e0ef62a095f9829f927667e9dcca7e 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 --- 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) + }) }