fix: resolve issue with S3 objects not deleting on /full route#269
fix: resolve issue with S3 objects not deleting on /full route#269asternic merged 7 commits intoasternic:mainfrom
Conversation
This reorders the logic in DeleteUserComplete so the S3 config is queried before the user is deleted from the database. Fixes asternic#268
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug preventing S3 objects from being properly deleted when a user's account was fully removed. It achieves this by resequencing database operations to ensure S3 configuration is retrieved before user data is purged. Additionally, it introduces a more resilient lazy initialization strategy for S3 clients, ensuring that S3 operations can proceed even if clients weren't explicitly set up during initial startup or after service restarts, thereby enhancing the overall stability and correctness of S3 integrations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where S3 objects were not being deleted upon user removal by reordering the database queries. The introduction of a lazy-initialization mechanism for the S3 client is also a good structural improvement. However, the changes introduce a new critical bug in handlers.go that could prevent S3 object deletion under certain error conditions. Additionally, there is some code redundancy in wmiau.go that could be refactored. My review includes comments on these points.
| // 5. Remove files from S3 (if enabled) | ||
| var s3Enabled bool | ||
| err = s.db.QueryRow("SELECT s3_enabled FROM users WHERE id = $1", id).Scan(&s3Enabled) | ||
| // 6. Remove files from S3 (if enabled) |
There was a problem hiding this comment.
There's a subtle bug in the if condition on the next line (if err == nil && s3Enabled). The err variable it checks is from the os.RemoveAll operation on line 5296. If local file deletion fails, this will prevent S3 objects from being deleted.
To fix this, the condition on line 5303 should be changed to just if s3Enabled. The s3Enabled flag will correctly be false if the initial database query for it failed, so checking err here is both incorrect and unnecessary.
|
|
||
| // Process S3 upload if enabled | ||
| if s3Config.Enabled == "true" && (s3Config.MediaDelivery == "s3" || s3Config.MediaDelivery == "both") { | ||
| ensureS3ClientForUser(txtid) |
There was a problem hiding this comment.
This call to ensureS3ClientForUser(txtid) is redundant because an identical check and call is already performed on line 787 for all message events. This redundant call is repeated for all media types (audio, document, video, sticker).
Please remove this line and the similar ones for other media types to improve clarity and avoid unnecessary function calls.
This PR fixes the bug described in #268 .
It reorders the logic in
handlers.go -> DeleteUserComplete()so that the database is queried for the user'ss3_enabledstatus before their record is deleted from theuserstable. This prevents the S3 deletion step from being silently skipped due to an empty database result.