-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix file record and delete files (branding and avatar) #1335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
LinkinStars
commented
May 13, 2025
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
commented
May 13, 2025
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.
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
CleanOrphanUploadFilesfunction 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielAuerX This is the only problem left in this PR. Are there any suitable steps to reproduce this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
// 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.AbortWithStatus(http.StatusNotFound) // This should not be continued here, but should be returned directly, because the file no longer exists. return }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielAuerX So, let's just remove the commit and adjust the error handling on the backend.
DanielAuerX
commented
May 15, 2025
@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
isBrandingOrAvatarFileis 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
commented
May 16, 2025
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) }
DanielAuerX
commented
May 16, 2025
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
+ 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.
+ 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
+ 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.
339eeed to
55dd8e3
Compare
Uh oh!
There was an error while loading. Please reload this page.
problems addressed in the pr:
cf. conversation in pr 1326