chore: Improve error messages in SaveFileToStorage#4173
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded explicit nil check in DefaultCtx.SaveFileToStorage and four exported sentinel file errors; SaveFileToStorage now returns/wraps these errors with filenames (and destination path for store failures). Tests added for nil fileheader and BodyLimit filename propagation. No public signatures changed. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4173 +/- ##
==========================================
+ Coverage 91.22% 91.26% +0.03%
==========================================
Files 130 130
Lines 12756 12760 +4
==========================================
+ Hits 11637 11645 +8
+ Misses 706 704 -2
+ Partials 413 411 -2
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 |
|
@pratikramteke The error needs to be defined in |
|
Please check unit tests and lintci errors |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve debugging for SaveFileToStorage failures by adding file-related sentinel errors and including more context (e.g., filename/path) in returned errors.
Changes:
- Added new exported file-related sentinel errors in
error.go. - Updated
DefaultCtx.SaveFileToStorageto return those sentinel errors and to include filename/path details in error messages. - Added tests covering nil file header handling and verifying that error messages include the filename.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
error.go |
Adds exported sentinel errors for file operations used by SaveFileToStorage. |
ctx.go |
Updates SaveFileToStorage to use new sentinel errors and enrich error messages. |
ctx_test.go |
Adds tests for nil file headers and filename presence in error messages. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@pratikramteke Can you fix the review issues? Thanks |
Address review feedback on PR gofiber#4173: - Use %w for all underlying errors in SaveFileToStorage so callers can errors.Is/As fasthttp.ErrBodyTooLarge and storage-specific errors; also satisfies errorlint. - Restore Test_Ctx_SaveFileToStorage_LimitExceeded(_UnknownSize) which rely on errors.Is(err, fasthttp.ErrBodyTooLarge). - Drop the misplaced "test-path" assertion from Test_Ctx_SaveFileToStorage_ErrorMessageContainsFilename: the BodyLimit branch intentionally does not include path. Add ErrorIs assertions on the new sentinels instead. - Add Test_Ctx_SaveFileToStorage_StoreErrorIncludesPath that triggers the storage failure branch and asserts both filename and path appear in the error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SaveFile had the same nil-panic vector as SaveFileToStorage: fasthttp.SaveMultipartFile -> fh.Open() panics on nil. Return ErrFileHeaderNil for consistency with SaveFileToStorage and add Test_Ctx_SaveFile_NilFileHeader. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add Test_Ctx_SaveFileToStorage_DefaultBodyLimitFallback to cover the defensive maxUploadSize <= 0 branch (raises SaveFileToStorage coverage from 86.4% to 90.9%). - Drop unused method receiver names on failingStorage helpers to satisfy revive (unused-receiver). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Improve error messages in SaveFileToStorage by including filename and path.
This provides better debugging context when handling file upload errors.