-
-
Notifications
You must be signed in to change notification settings - Fork 2k
chore: Improve error messages in SaveFileToStorage #4173
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
base: main
Are you sure you want to change the base?
Changes from all commits
80e0203
485129f
23f1d53
240b1ad
d310941
3364650
3d08591
3cf3871
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -517,9 +517,13 @@ | |||||
|
|
||||||
| // SaveFileToStorage saves any multipart file to an external storage system. | ||||||
| func (c *DefaultCtx) SaveFileToStorage(fileheader *multipart.FileHeader, path string, storage Storage) error { | ||||||
| if fileheader == nil { | ||||||
| return ErrFileHeaderNil | ||||||
| } | ||||||
|
|
||||||
| 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) | ||||||
| } | ||||||
|
Comment on lines
524
to
527
|
||||||
| defer file.Close() //nolint:errcheck // not needed | ||||||
|
|
||||||
|
|
@@ -529,25 +533,25 @@ | |||||
| } | ||||||
|
|
||||||
| 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) | ||||||
| } | ||||||
|
Comment on lines
524
to
537
|
||||||
|
|
||||||
| buf := bytebufferpool.Get() | ||||||
| defer bytebufferpool.Put(buf) | ||||||
|
|
||||||
| 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) | ||||||
|
||||||
| return fmt.Errorf("%w: %q: %v", ErrFileRead, fileheader.Filename, err) | |
| return fmt.Errorf("%w: %q: %w", ErrFileRead, fileheader.Filename, err) |
Copilot
AI
Apr 26, 2026
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.
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).
Copilot
AI
Apr 26, 2026
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 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) |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -5281,6 +5281,42 @@ func (s *mockContextAwareStorage) Close() error { | |||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| func Test_Ctx_SaveFileToStorage_NilFileHeader(t *testing.T) { | ||||||||
| t.Parallel() | ||||||||
|
|
||||||||
| app := New() | ||||||||
| storage := memory.New() | ||||||||
|
|
||||||||
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | ||||||||
| defer app.ReleaseCtx(ctx) | ||||||||
|
|
||||||||
| err := ctx.SaveFileToStorage(nil, "test", storage) | ||||||||
|
|
||||||||
| require.Error(t, err) | ||||||||
| require.ErrorIs(t, err, ErrFileHeaderNil) | ||||||||
| } | ||||||||
|
|
||||||||
| func Test_Ctx_SaveFileToStorage_ErrorMessageContainsFilename(t *testing.T) { | ||||||||
| t.Parallel() | ||||||||
|
|
||||||||
| app := New(Config{BodyLimit: 10}) // small limit to force error | ||||||||
| storage := memory.New() | ||||||||
|
|
||||||||
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) | ||||||||
| defer app.ReleaseCtx(ctx) | ||||||||
|
|
||||||||
| fileHeader := createMultipartFileHeader( | ||||||||
| t, | ||||||||
| "test-file.png", | ||||||||
| bytes.Repeat([]byte{'a'}, 100), // bigger than limit | ||||||||
| ) | ||||||||
|
|
||||||||
| err := ctx.SaveFileToStorage(fileHeader, "test-path", storage) | ||||||||
|
|
||||||||
| require.Error(t, err) | ||||||||
| require.Contains(t, err.Error(), "test-file.png") | ||||||||
|
||||||||
| require.Contains(t, err.Error(), "test-file.png") | |
| require.Contains(t, err.Error(), "test-file.png") | |
| require.Contains(t, err.Error(), "test-path") |
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 think we better sanitize filename for security reasons
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 filename comes from the Client Request, so they already know it.