chore: Improve error messages in SaveFileToStorage#4173
chore: Improve error messages in SaveFileToStorage#4173pratikramteke wants to merge 8 commits intogofiber:mainfrom
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:
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 (1)
✅ Files skipped from review due to trivial changes (1)
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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>
| 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 |
|
@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.
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. |
| limitedReader := io.LimitReader(file, int64(maxUploadSize)+1) | ||
| if _, err = buf.ReadFrom(limitedReader); err != nil { | ||
| return fmt.Errorf("failed to read: %w", err) | ||
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, err) |
There was a problem hiding this comment.
buf.ReadFrom errors are formatted with %v, so they are not wrapped and can’t be matched with errors.Is/As. Wrap the underlying read error with %w (while still wrapping ErrFileRead).
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, err) | |
| return fmt.Errorf("%w: %q: %w", ErrFileRead, fileheader.Filename, err) |
| if buf.Len() > maxUploadSize { | ||
| return fmt.Errorf("failed to read: %w", fasthttp.ErrBodyTooLarge) | ||
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, fasthttp.ErrBodyTooLarge) | ||
| } |
There was a problem hiding this comment.
When buf.Len() > maxUploadSize, fasthttp.ErrBodyTooLarge is again included with %v instead of being wrapped. This prevents errors.Is(err, fasthttp.ErrBodyTooLarge) from working. Wrap it with %w (and keep wrapping ErrFileRead).
|
|
||
| if err := storage.SetWithContext(c.Context(), path, data, 0); err != nil { | ||
| return fmt.Errorf("failed to store: %w", err) | ||
| return fmt.Errorf("%w: %q to %q: %v", ErrFileStore, fileheader.Filename, path, err) |
There was a problem hiding this comment.
The storage SetWithContext error is formatted with %v, so the underlying storage error is not wrapped. Wrap the original error with %w so callers can errors.Is/As on the storage-specific error type (while still wrapping ErrFileStore).
| return fmt.Errorf("%w: %q to %q: %v", ErrFileStore, fileheader.Filename, path, err) | |
| return fmt.Errorf("%w: %q to %q: %w", ErrFileStore, fileheader.Filename, path, err) |
| @@ -529,25 +533,25 @@ func (c *DefaultCtx) SaveFileToStorage(fileheader *multipart.FileHeader, path st | |||
| } | |||
|
|
|||
| if fileheader.Size > 0 && fileheader.Size > int64(maxUploadSize) { | |||
| return fmt.Errorf("failed to read: %w", fasthttp.ErrBodyTooLarge) | |||
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, fasthttp.ErrBodyTooLarge) | |||
| } | |||
There was a problem hiding this comment.
PR description says errors should include both filename and path, but the open/read error paths only include fileheader.Filename (the path argument isn’t mentioned until the store failure case). Either include path in these earlier error messages too, or adjust the PR description/tests to match the intended behavior.
| err := ctx.SaveFileToStorage(fileHeader, "test-path", storage) | ||
|
|
||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "test-file.png") |
There was a problem hiding this comment.
This test asserts that the error message contains the filename, but it doesn’t validate the "path" part mentioned in the PR description. If the intent is to include both filename and destination path for debugging context, add an assertion for the path as well (and update the implementation accordingly so the failing code path includes it).
| require.Contains(t, err.Error(), "test-file.png") | |
| require.Contains(t, err.Error(), "test-file.png") | |
| require.Contains(t, err.Error(), "test-path") |
| 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.
The underlying err from fileheader.Open() is formatted with %v, so it is not wrapped. This breaks errors.Is/As for callers and makes it harder to detect the root cause programmatically. Wrap the underlying error (use %w) while still including the filename in the message (Go supports multiple %w).
| if fileheader.Size > 0 && fileheader.Size > int64(maxUploadSize) { | ||
| return fmt.Errorf("failed to read: %w", fasthttp.ErrBodyTooLarge) | ||
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, fasthttp.ErrBodyTooLarge) | ||
| } |
There was a problem hiding this comment.
fasthttp.ErrBodyTooLarge is included using %v, so the returned error no longer wraps it. Existing callers/tests that rely on errors.Is(err, fasthttp.ErrBodyTooLarge) will fail. Use %w for fasthttp.ErrBodyTooLarge (and keep wrapping ErrFileRead).
Improve error messages in SaveFileToStorage by including filename and path.
This provides better debugging context when handling file upload errors.