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

DanielAuerX · 2025-05-09T13:31:10Z

create file_record table
avatar and branding files are added to file_record
branding files are being deleted
avatar files are being deleted
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

by registering the file record entitry in init_data.go, the table is being created during initialization.

+ uploaded avatar and branding files are added to the file_record table, so that the table provides a comprehensive overview of files in the file system

+ a file is being deleted after its reference has been removed from the site info. The user has no way to access the file again. Therefore the file would become an orphan if not deleted. + an if clause had to be added to CleanOrphanUploadFiles, because it would otherwise remove the branding and avatar files.

+ a file is being deleted after its reference has been removed from the user info. The user has no way to access the file again. Therefore the file would become an orphan if not deleted.

ensuring updated user info (e.g. avatar file path) is reloaded after changes. This prevents stale paths being cached by the browser and old resources being requested.

LinkinStars · 2025-05-13T08:11:41Z

Thanks for contributing. As a reminder, I see that all tasks have been completed, so if you need to review, please change the draft status to ready.

DanielAuerX · 2025-05-13T17:39:27Z

Hi @LinkinStars! yes, the tasks of this pr are finished, unless you see something I missed.
The clean up job (CleanOrphanUploadFiles) is running now. However, I am not sure that it works as intended when it was implemented. Maybe you want to double check that.

internal/controller_admin/siteinfo_controller.go

internal/service/content/user_service.go

internal/service/siteinfo/siteinfo_service.go

LinkinStars · 2025-05-15T09:15:50Z

Almost done. I find some small issues.

Did you forget to submit the wire gen.go? We can't run the code directly.
If a user switches avatars, for example from a customized avatar to the system default avatar, the file should not be deleted at this point. This is because the user may switch back later, and the deletion of the old avatar will only be processed when the user uploads a new custom avatar.
Let me explain why we use the CleanOrphanUploadFiles function to clean the file.

When a user uploads the file, whatever category(post or avatar), the file will be saved. But after that, if the user does not save the post or profile, the file will be an orphaned file. So we need to clean its background. Of course, we will think that your handling is enough. If you need to be more perfect, it is necessary to separately determine whether the avatar is using it by users(when isBrandingOrAvatarFile is true).

shuashuai · 2025-05-15T09:28:34Z

ui/src/pages/Users/Settings/Profile/index.tsx

-          ...user,
-          ...params,
-        });
+        getLoggedUserInfo().then(update);


Why should this be adjusted?

The whole PR problem has nothing to do with this, and the user information pulled back after editing the user information is also up to date, so there should be no need to adjust here.

the browser caches the stale path of the old avatar. Until a new session is started, the old resource is being requested in the backend, causing an error log by the avatar middleware (resource not found).
If you can live with that, ill take it out.

Can you give me a step to reproduce the problem?

I don't know where else will continue to request the old avatar after the user has updated his avatar

checkout branch

remove the commit in question

upload a custom avatar

update the custom avatar with another avatar

check the logs and/or the uploads/avatar/ get requests

@DanielAuerX This is the only problem left in this PR. Are there any suitable steps to reproduce this problem?

@DanielAuerX wait for your reply!

checkout branch

remove the commit in question

upload a custom avatar

update the custom avatar with another avatar

check the logs and/or the uploads/avatar/ get requests

I tested it and found that this modification still cannot eliminate browser cache unless the entire application is reloaded using window.reload, however, using this method will affect the user's experience.

Because from the perspective of generation implementation, updating user information after obtaining from get interface is the same as updating from params, and it cannot eliminate browser cache.

Here are the steps I tested with your code:

upload a custom avatar and save

reupload a custom avatar and save

Return to the question list page, and the last avatar will still be retrieved from the cache (you can see from the console's get request).

@DanielAuerX Thanks for the feedback, it made us realise that there is a bit of an issue with the backend here for errors as well. Maybe just adjust the error handling on the backend.

answer/internal/service/uploader/upload.go

Line 152 in 3f1ed50

return "", errors.InternalServer(reason.UnknownError).WithError(err).WithStack()

// There should be a 404 error here return "", errors.NotFound(reason.UnknownError).WithError(err)

answer/internal/base/middleware/avatar.go

Lines 62 to 68 in 3f1ed50

filePath, err = am.uploaderService.AvatarThumbFile(ctx, filename, size)

if err != nil {

log.Error(err)

ctx.Abort()

}

}

avatarFile, err := os.ReadFile(filePath)

filePath, err = am.uploaderService.AvatarThumbFile(ctx, filename, size) if err != nil { log.Error(err) ctx.AbortWithStatus(http.StatusNotFound) // This should not be continued here, but should be returned directly, because the file no longer exists. return }

I tested it and found that this modification still cannot eliminate browser cache unless the entire application is reloaded using window.reload, however, using this method will affect the user's experience.

@shuashuai Just for clarification, this change is not supposed to eliminate browser cache. Its supposed to be updating the user information, so an old avatar file is not being requested after it has been removed from the user information. This is not new behavior. However, previously requesting an old resource was successful, because files were not being deleted.

Do you have a better solution? I can also just remove the commit, if you think this is acceptable.

Just for clarification, this change is not supposed to eliminate browser cache. Its supposed to be updating the user information, so an old avatar file is not being requested after it has been removed from the user information. This is not new behavior. However, previously requesting an old resource was successful, because files were not being deleted.

Both ways of updating the user's information are the same and there is no practical difference between them!

Do you have a better solution? I can also just remove the commit, if you think this is acceptable.

There is no better way at the moment, so let's keep the previous logic for the time being.

+ using existing AvatarInfo schema instead of redundant schema + improved error handling, so that the branding information is always being updated, even if its not possible to remove the old files. + added updated wire_gen.go file

DanielAuerX · 2025-05-15T14:02:45Z

@LinkinStars First of all, thanks for the review and the explanation!

Almost done. I find some small issues.
1. Did you forget to submit the `wire gen.go`? We can't run the code directly.

now committed

2. If a user **switches** avatars, for example from a customized avatar to the system default avatar, the file **should not be deleted** at this point. This is because the user may switch back later, and the deletion of the old avatar will only be processed when the user **uploads a new custom avatar**.

the avatar file is only being deleted, if the new custom avatar is an empty string or the file name is unlike the old one. Therefore the desired behavior you are describing should already be the case. I also tested it manually. Unless i understand you wrong.

3. Let me explain why we use the `CleanOrphanUploadFiles` function to clean the file.
When a user uploads the file, whatever category(post or avatar), the file will be saved. But after that, if the user does not save the post or profile, the file will be an orphaned file. So we need to clean its background. Of course, we will think that your handling is enough. If you need to be more perfect, it is necessary to separately determine whether the avatar is using it by users(when isBrandingOrAvatarFile is true).

you are right, when one uploads an avatar/branding file and never clicks save, the file wont be deleted. I will add some logic to the clean up job, thanks.
I just noticed that the avatar_thumb files arent deleted either. I will also look into that.

LinkinStars · 2025-05-16T02:55:52Z

@DanielAuerX

For issue 2, here are my reproduction steps.

Upload an avatar and save the profile.
Check the avatar dir. (The file exists)
Switch the avatar to system and save the profile.
Check the avatar dir. (The file not exists)

1. ➜  avatar git:(fix_file_record) ls
2. ➜  avatar git:(fix_file_record) ls
5sVCXwyhsbm.png
4. ➜  avatar git:(fix_file_record) ls
➜  avatar git:(fix_file_record)

This is the reason for the problem.

if oldAvatar.Type == "custom" && (newAvatar.Type != "custom" || oldAvatar.Custom != newAvatar.Custom) {

At this time, newAvatar.Type != "custom" so the file will be deleted.

To make the logic simple, perhaps it could be implemented like this.

	// As long as there are no original files, there must be no need to delete
	if len(oldAvatar.Custom) == 0 {
		return
	}
	// At this point, it turns out that the old file must exist. 
	// Then delete as long as the new file and the old file are inconsistent as you say. 
	// Whatever type the user chooses.
	if oldAvatar.Custom != newAvatar.Custom {
		fileRecord, err := us.fileRecordService.GetFileRecordByURL(ctx, oldAvatar.Custom)
	}

+ added logic that removed orphaned within the clean up job. Files may be orphaned if a user uploads a file, but never saves it (registering it with the user profile or site info). + if a user switches avatars, for example from a customized avatar to the system default avatar, the file is not deleted anymore.

DanielAuerX · 2025-05-16T21:28:23Z

@DanielAuerX

For issue 2, here are my reproduction steps.

Upload an avatar and save the profile.

Check the avatar dir. (The file exists)

Switch the avatar to system and save the profile.

Check the avatar dir. (The file not exists)
1. ➜  avatar git:(fix_file_record) ls
2. ➜  avatar git:(fix_file_record) ls
5sVCXwyhsbm.png
4. ➜  avatar git:(fix_file_record) ls
➜  avatar git:(fix_file_record) 
This is the reason for the problem.
if oldAvatar.Type == "custom" && (newAvatar.Type != "custom" || oldAvatar.Custom != newAvatar.Custom) {
At this time, newAvatar.Type != "custom" so the file will be deleted.

To make the logic simple, perhaps it could be implemented like this.
	// As long as there are no original files, there must be no need to delete
	if len(oldAvatar.Custom) == 0 {
		return
	}
	// At this point, it turns out that the old file must exist. 
	// Then delete as long as the new file and the old file are inconsistent as you say. 
	// Whatever type the user chooses.
	if oldAvatar.Custom != newAvatar.Custom {
		fileRecord, err := us.fileRecordService.GetFileRecordByURL(ctx, oldAvatar.Custom)
	}

Should be done now

broccoli and others added 3 commits May 3, 2025 12:26

Fix: creating file_record table

aebb7d4

by registering the file record entitry in init_data.go, the table is being created during initialization.

fix: all uploaded files are added to file_record

2dc8368

+ uploaded avatar and branding files are added to the file_record table, so that the table provides a comprehensive overview of files in the file system

DanielAuerX marked this pull request as draft May 9, 2025 13:31

broccoli added 2 commits May 11, 2025 12:29

feat: delete avatar files

404db43

+ a file is being deleted after its reference has been removed from the user info. The user has no way to access the file again. Therefore the file would become an orphan if not deleted.

fix: reload latest user info after backend state is being updated

97d773f

ensuring updated user info (e.g. avatar file path) is reloaded after changes. This prevents stale paths being cached by the browser and old resources being requested.

DanielAuerX marked this pull request as ready for review May 13, 2025 17:39

LinkinStars self-requested a review May 14, 2025 11:20

LinkinStars self-assigned this May 14, 2025

LinkinStars requested changes May 15, 2025

View reviewed changes

internal/controller_admin/siteinfo_controller.go Outdated Show resolved Hide resolved

internal/service/content/user_service.go Outdated Show resolved Hide resolved

internal/service/siteinfo/siteinfo_service.go Show resolved Hide resolved

shuashuai reviewed May 15, 2025

View reviewed changes

improved error handling, using existing schema

d024853

+ using existing AvatarInfo schema instead of redundant schema + improved error handling, so that the branding information is always being updated, even if its not possible to remove the old files. + added updated wire_gen.go file

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DanielAuerX commented May 9, 2025 •

edited

Loading

LinkinStars commented May 13, 2025

DanielAuerX commented May 13, 2025

LinkinStars commented May 15, 2025 •

edited

Loading

shuashuai May 15, 2025

DanielAuerX May 15, 2025

shuashuai May 15, 2025

DanielAuerX May 15, 2025

LinkinStars May 19, 2025

shuashuai May 19, 2025

shuashuai May 20, 2025

LinkinStars May 20, 2025

DanielAuerX May 20, 2025

shuashuai May 20, 2025

DanielAuerX commented May 15, 2025

LinkinStars commented May 16, 2025

DanielAuerX commented May 16, 2025

	filePath, err = am.uploaderService.AvatarThumbFile(ctx, filename, size)
	if err != nil {
	log.Error(err)
	ctx.Abort()
	}
	}
	avatarFile, err := os.ReadFile(filePath)

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

Are you sure you want to change the base?

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

Conversation

DanielAuerX commented May 9, 2025 • edited Loading

LinkinStars commented May 13, 2025

DanielAuerX commented May 13, 2025

LinkinStars commented May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielAuerX commented May 15, 2025

LinkinStars commented May 16, 2025

DanielAuerX commented May 16, 2025

DanielAuerX commented May 9, 2025 •

edited

Loading

LinkinStars commented May 15, 2025 •

edited

Loading