From d2c4d833f4e0ef62a095f9829f927667e9dcca7e Mon Sep 17 00:00:00 2001
From: Earl Warren <contact@earl-warren.org>
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)
+ })
}