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 1be797faba Fix bug on avatar

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)
 }

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) {

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")
+		Actions = DiscardStorage("Actions isn't enabled")
-		ActionsArtifacts = discardStorage("ActionsArtifacts isn't enabled")
+		ActionsArtifacts = DiscardStorage("ActionsArtifacts isn't enabled")
 		return nil
 	}
 	log.Info("Initialising Actions storage with type: %s", setting.Actions.LogStorage.Type)

services/user/avatar_test.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"image"
 	"image/png"
+	"os"
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
@@ -18,11 +19,22 @@ 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)
 
+	t.Run("AtomicStorageFailure", func(t *testing.T) {
+		defer test.MockProtect[storage.ObjectStorage](&storage.Avatars)()
+
 		assert.NoError(t, unittest.PrepareTestDatabase())
 		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 
@@ -31,17 +43,38 @@ func TestUserDeleteAvatar(t *testing.T) {
 		verification := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		assert.NotEqual(t, "", verification.Avatar)
 
-	t.Run("AtomicStorageFailure", func(t *testing.T) {
+		// fail to delete ...
-		defer test.MockVariableValue[storage.ObjectStorage](&storage.Avatars, storage.UninitializedStorage)()
+		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)
 	})
 
+	t.Run("Success", func(t *testing.T) {
+		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)
+
 		err = DeleteAvatar(db.DefaultContext, user)
 		assert.NoError(t, err)
 
 		verification = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 		assert.Equal(t, "", verification.Avatar)
+	})
 }