Fix file record and delete files (branding and avatar) (#1335)

- [x] create file_record table
- [x] avatar and branding files are added to file_record
- [x] branding files are being deleted
- [x] avatar files are being deleted
- [x] reload latest avatar (frontend) after backend state is being
updated

problems addressed in the pr:
- clean up job fails, because it cannot access file_record table
- avatar and branding files are not added to the file_record table
- avatar and branding files are never deleted
- after an avatar is being updated/deleted, the old file is still being
requested due to browser caching. This is causing error logs ("no such
file or directory") in the backend.

cf. conversation in [pr
1326](#1326)

---------

Co-authored-by: broccoli <[email protected]>
Co-authored-by: LinkinStars <[email protected]>

Author: 3 people

cmd/wire_gen.go

@@ -21,6 +21,7 @@ package middleware
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"os"
 	"path"
@@ -62,7 +63,8 @@ func (am *AvatarMiddleware) AvatarThumb() gin.HandlerFunc {
 				filePath, err = am.uploaderService.AvatarThumbFile(ctx, filename, size)
 				if err != nil {
 					log.Error(err)
-					ctx.Abort()
+					ctx.AbortWithStatus(http.StatusNotFound)
+					return
 				}
 			}
 			avatarFile, err := os.ReadFile(filePath)

internal/base/middleware/avatar.go

@@ -28,6 +28,7 @@ import (
 	"github.com/apache/answer/internal/schema"
 	"github.com/apache/answer/internal/service/siteinfo"
 	"github.com/gin-gonic/gin"
+	"github.com/segmentfault/pacman/log"
 )
 
 // SiteInfoController site info controller
@@ -274,8 +275,17 @@ func (sc *SiteInfoController) UpdateBranding(ctx *gin.Context) {
 	if handler.BindAndCheck(ctx, req) {
 		return
 	}
-	err := sc.siteInfoService.SaveSiteBranding(ctx, req)
-	handler.HandleResponse(ctx, err, nil)
+	currentBranding, getBrandingErr := sc.siteInfoService.GetSiteBranding(ctx)
+	if getBrandingErr == nil {
+		cleanUpErr := sc.siteInfoService.CleanUpRemovedBrandingFiles(ctx, req, currentBranding)
+		if cleanUpErr != nil {
+			log.Errorf("failed to clean up removed branding file(s): %v", cleanUpErr)
+		}
+	} else {
+		log.Errorf("failed to get current site branding: %v", getBrandingErr)
+	}
+	saveErr := sc.siteInfoService.SaveSiteBranding(ctx, req)
+	handler.HandleResponse(ctx, saveErr, nil)
 }
 
 // UpdateSiteWrite update site write info

internal/controller_admin/siteinfo_controller.go

@@ -74,6 +74,7 @@ var (
 		&entity.Badge{},
 		&entity.BadgeGroup{},
 		&entity.BadgeAward{},
+		&entity.FileRecord{},
 		&entity.PluginKVStorage{},
 	}
 

internal/migrations/init_data.go

@@ -82,3 +82,18 @@ func (fr *fileRecordRepo) UpdateFileRecord(ctx context.Context, fileRecord *enti
 	}
 	return
 }
+
+// GetFileRecordByURL gets a file record by its url
+func (fr *fileRecordRepo) GetFileRecordByURL(ctx context.Context, fileURL string) (record *entity.FileRecord, err error) {
+	record = &entity.FileRecord{}
+	session := fr.data.DB.Context(ctx)
+	exists, err := session.Where("file_url = ? AND status = ?", fileURL, entity.FileRecordStatusAvailable).Get(record)
+	if err != nil {
+		err = errors.InternalServer(reason.DatabaseError).WithError(err).WithStack()
+		return
+	}
+	if !exists {
+		return
+	}
+	return record, nil
+}

internal/repo/file_record/file_record_repo.go

@@ -101,3 +101,18 @@ func (sr *siteInfoRepo) setCache(ctx context.Context, siteType string, siteInfo
 		log.Error(err)
 	}
 }
+
+func (sr *siteInfoRepo) IsBrandingFileUsed(ctx context.Context, filePath string) (bool, error) {
+	siteInfo := &entity.SiteInfo{}
+	count, err := sr.data.DB.Context(ctx).
+		Table("site_info").
+		Where(builder.Eq{"type": "branding"}).
+		And(builder.Like{"content", "%" + filePath + "%"}).
+		Count(&siteInfo)
+
+	if err != nil {
+		return false, errors.InternalServer(reason.DatabaseError).WithError(err).WithStack()
+	}
+
+	return count > 0, nil
+}

internal/repo/site_info/siteinfo_repo.go

@@ -33,6 +33,7 @@ import (
 	"github.com/apache/answer/plugin"
 	"github.com/segmentfault/pacman/errors"
 	"github.com/segmentfault/pacman/log"
+	"xorm.io/builder"
 	"xorm.io/xorm"
 )
 
@@ -380,3 +381,17 @@ func decorateByUserCenterUser(original *entity.User, ucUser *plugin.UserCenterBa
 		original.Status = int(ucUser.Status)
 	}
 }
+
+func (ur *userRepo) IsAvatarFileUsed(ctx context.Context, filePath string) (bool, error) {
+	user := &entity.User{}
+	count, err := ur.data.DB.Context(ctx).
+		Table("user").
+		Where(builder.Like{"avatar", "%" + filePath + "%"}).
+		Count(&user)
+
+	if err != nil {
+		return false, errors.InternalServer(reason.DatabaseError).WithError(err).WithStack()
+	}
+
+	return count > 0, nil
+}

internal/repo/user/user_repo.go

@@ -23,9 +23,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"time"
+
 	"github.com/apache/answer/internal/service/event_queue"
 	"github.com/apache/answer/pkg/token"
-	"time"
 
 	"github.com/apache/answer/internal/base/constant"
 	questioncommon "github.com/apache/answer/internal/service/question_common"
@@ -41,6 +42,7 @@ import (
 	"github.com/apache/answer/internal/service/activity_common"
 	"github.com/apache/answer/internal/service/auth"
 	"github.com/apache/answer/internal/service/export"
+	"github.com/apache/answer/internal/service/file_record"
 	"github.com/apache/answer/internal/service/role"
 	"github.com/apache/answer/internal/service/siteinfo_common"
 	usercommon "github.com/apache/answer/internal/service/user_common"
@@ -67,6 +69,7 @@ type UserService struct {
 	userNotificationConfigService *user_notification_config.UserNotificationConfigService
 	questionService               *questioncommon.QuestionCommon
 	eventQueueService             event_queue.EventQueueService
+	fileRecordService             *file_record.FileRecordService
 }
 
 func NewUserService(userRepo usercommon.UserRepo,
@@ -82,6 +85,7 @@ func NewUserService(userRepo usercommon.UserRepo,
 	userNotificationConfigService *user_notification_config.UserNotificationConfigService,
 	questionService *questioncommon.QuestionCommon,
 	eventQueueService event_queue.EventQueueService,
+	fileRecordService *file_record.FileRecordService,
 ) *UserService {
 	return &UserService{
 		userCommonService:             userCommonService,
@@ -97,6 +101,7 @@ func NewUserService(userRepo usercommon.UserRepo,
 		userNotificationConfigService: userNotificationConfigService,
 		questionService:               questionService,
 		eventQueueService:             eventQueueService,
+		fileRecordService:             fileRecordService,
 	}
 }
 
@@ -355,6 +360,9 @@ func (us *UserService) UpdateInfo(ctx context.Context, req *schema.UpdateInfoReq
 	}
 
 	cond := us.formatUserInfoForUpdateInfo(oldUserInfo, req, siteUsers)
+
+	us.cleanUpRemovedAvatar(ctx, oldUserInfo.Avatar, cond.Avatar)
+
 	err = us.userRepo.UpdateInfo(ctx, cond)
 	if err != nil {
 		return nil, err
@@ -363,6 +371,41 @@ func (us *UserService) UpdateInfo(ctx context.Context, req *schema.UpdateInfoReq
 	return nil, err
 }
 
+func (us *UserService) cleanUpRemovedAvatar(
+	ctx context.Context,
+	oldAvatarJSON string,
+	newAvatarJSON string,
+) {
+	if oldAvatarJSON == newAvatarJSON {
+		return
+	}
+
+	var oldAvatar, newAvatar schema.AvatarInfo
+
+	_ = json.Unmarshal([]byte(oldAvatarJSON), &oldAvatar)
+	_ = json.Unmarshal([]byte(newAvatarJSON), &newAvatar)
+
+	if len(oldAvatar.Custom) == 0 {
+		return
+	}
+
+	// clean up if old is custom and it's either removed or replaced
+	if oldAvatar.Custom != newAvatar.Custom {
+		fileRecord, err := us.fileRecordService.GetFileRecordByURL(ctx, oldAvatar.Custom)
+		if err != nil {
+			log.Error(err)
+			return
+		}
+		if fileRecord == nil {
+			log.Warn("no file record found for old avatar url:", oldAvatar.Custom)
+			return
+		}
+		if err := us.fileRecordService.DeleteAndMoveFileRecord(ctx, fileRecord); err != nil {
+			log.Error(err)
+		}
+	}
+}
+
 func (us *UserService) formatUserInfoForUpdateInfo(
 	oldUserInfo *entity.User, req *schema.UpdateInfoRequest, siteUsersConf *schema.SiteUsersResp) *entity.User {
 	avatar, _ := json.Marshal(req.Avatar)

internal/service/content/user_service.go

@@ -24,13 +24,15 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/apache/answer/internal/base/constant"
 	"github.com/apache/answer/internal/entity"
 	"github.com/apache/answer/internal/service/revision"
 	"github.com/apache/answer/internal/service/service_config"
 	"github.com/apache/answer/internal/service/siteinfo_common"
+	usercommon "github.com/apache/answer/internal/service/user_common"
 	"github.com/apache/answer/pkg/checker"
 	"github.com/apache/answer/pkg/dir"
 	"github.com/apache/answer/pkg/writer"
@@ -44,6 +46,7 @@ type FileRecordRepo interface {
 	GetFileRecordPage(ctx context.Context, page, pageSize int, cond *entity.FileRecord) (
 		fileRecordList []*entity.FileRecord, total int64, err error)
 	DeleteFileRecord(ctx context.Context, id int) (err error)
+	GetFileRecordByURL(ctx context.Context, fileURL string) (record *entity.FileRecord, err error)
 }
 
 // FileRecordService file record service
@@ -52,6 +55,7 @@ type FileRecordService struct {
 	revisionRepo    revision.RevisionRepo
 	serviceConfig   *service_config.ServiceConfig
 	siteInfoService siteinfo_common.SiteInfoCommonService
+	userService     *usercommon.UserCommon
 }
 
 // NewFileRecordService new file record service
@@ -60,12 +64,14 @@ func NewFileRecordService(
 	revisionRepo revision.RevisionRepo,
 	serviceConfig *service_config.ServiceConfig,
 	siteInfoService siteinfo_common.SiteInfoCommonService,
+	userService *usercommon.UserCommon,
 ) *FileRecordService {
 	return &FileRecordService{
 		fileRecordRepo:  fileRecordRepo,
 		revisionRepo:    revisionRepo,
 		serviceConfig:   serviceConfig,
 		siteInfoService: siteInfoService,
+		userService:     userService,
 	}
 }
 
@@ -104,6 +110,21 @@ func (fs *FileRecordService) CleanOrphanUploadFiles(ctx context.Context) {
 			if fileRecord.CreatedAt.AddDate(0, 0, 2).After(time.Now()) {
 				continue
 			}
+			if isBrandingOrAvatarFile(fileRecord.FilePath) {
+				if strings.Contains(fileRecord.FilePath, constant.BrandingSubPath+"/") {
+					if fs.siteInfoService.IsBrandingFileUsed(ctx, fileRecord.FilePath) {
+						continue
+					}
+				} else if strings.Contains(fileRecord.FilePath, constant.AvatarSubPath+"/") {
+					if fs.userService.IsAvatarFileUsed(ctx, fileRecord.FilePath) {
+						continue
+					}
+				}
+				if err := fs.DeleteAndMoveFileRecord(ctx, fileRecord); err != nil {
+					log.Error(err)
+				}
+				continue
+			}
 			if checker.IsNotZeroString(fileRecord.ObjectID) {
 				_, exist, err := fs.revisionRepo.GetLastRevisionByObjectID(ctx, fileRecord.ObjectID)
 				if err != nil {
@@ -129,14 +150,18 @@ func (fs *FileRecordService) CleanOrphanUploadFiles(ctx context.Context) {
 				}
 			}
 			// Delete and move the file record
-			if err := fs.deleteAndMoveFileRecord(ctx, fileRecord); err != nil {
+			if err := fs.DeleteAndMoveFileRecord(ctx, fileRecord); err != nil {
 				log.Error(err)
 			}
 		}
 		page++
 	}
 }
 
+func isBrandingOrAvatarFile(filePath string) bool {
+	return strings.Contains(filePath, constant.BrandingSubPath+"/") || strings.Contains(filePath, constant.AvatarSubPath+"/")
+}
+
 func (fs *FileRecordService) PurgeDeletedFiles(ctx context.Context) {
 	deletedPath := filepath.Join(fs.serviceConfig.UploadPath, constant.DeletedSubPath)
 	log.Infof("purge deleted files: %s", deletedPath)
@@ -152,7 +177,7 @@ func (fs *FileRecordService) PurgeDeletedFiles(ctx context.Context) {
 	return
 }
 
-func (fs *FileRecordService) deleteAndMoveFileRecord(ctx context.Context, fileRecord *entity.FileRecord) error {
+func (fs *FileRecordService) DeleteAndMoveFileRecord(ctx context.Context, fileRecord *entity.FileRecord) error {
 	// Delete the file record
 	if err := fs.fileRecordRepo.DeleteFileRecord(ctx, fileRecord.ID); err != nil {
 		return fmt.Errorf("delete file record error: %v", err)
@@ -170,3 +195,12 @@ func (fs *FileRecordService) deleteAndMoveFileRecord(ctx context.Context, fileRe
 	log.Debugf("delete and move file: %s", fileRecord.FileURL)
 	return nil
 }
+
+func (fs *FileRecordService) GetFileRecordByURL(ctx context.Context, fileURL string) (record *entity.FileRecord, err error) {
+	record, err = fs.fileRecordRepo.GetFileRecordByURL(ctx, fileURL)
+	if err != nil {
+		log.Errorf("error retrieving file record by URL: %v", err)
+		return
+	}
+	return
+}