chore: Improve error messages in SaveFileToStorage#4173
chore: Improve error messages in SaveFileToStorage#4173pratikramteke wants to merge 7 commits intogofiber:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances error reporting in the SaveFileToStorage function by including the filename and storage path in error messages. The reviewer recommends using the %q format verb instead of %s for these strings to ensure proper quoting and escaping of user-provided data, and highlights a potential information disclosure risk when including internal paths in errors.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded explicit nil check in DefaultCtx.SaveFileToStorage and introduced four new exported sentinel file errors; SaveFileToStorage now wraps and formats errors to include multipart filename (and destination path for store errors). Added tests for nil fileheader and BodyLimit filename propagation. No public signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ctx.go
Outdated
| file, err := fileheader.Open() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open: %w", err) | ||
| return fmt.Errorf("failed to open file %s: %w", fileheader.Filename, err) |
There was a problem hiding this comment.
If the fileader is nil, it is going to panic. you also need to add check for that. In addition to panic check, could you also unit test cases for error messages?
There was a problem hiding this comment.
Doesn't err have the filename already?
There was a problem hiding this comment.
no it only shows what the error is https://pkg.go.dev/internal/oserror#pkg-variables
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4173 +/- ##
=======================================
Coverage 91.22% 91.22%
=======================================
Files 123 123
Lines 11832 11834 +2
=======================================
+ Hits 10794 10796 +2
Misses 653 653
Partials 385 385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@pratikramteke pls check the hints |
gaby
changed the title
|
@pratikramteke The error needs to be defined in |
|
Please check unit tests and lintci errors |
| file, err := fileheader.Open() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open: %w", err) | ||
| return fmt.Errorf("%w: %q: %v", ErrFileOpen, fileheader.Filename, err) |
There was a problem hiding this comment.
i think we better sanitize filename for security reasons
There was a problem hiding this comment.
The filename comes from the Client Request, so they already know it.
4 participants
Improve error messages in SaveFileToStorage by including filename and path.
This provides better debugging context when handling file upload errors.