fix(file): fixed Last-Modified/Etag issue on embed file system #33
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue where the Sequence diagram for FileViewer handling embedded filessequenceDiagram
participant Client
participant FileViewer
participant FileSystem
Client->>FileViewer: GET request with If-None-Match
alt is embedded file
FileViewer->>FileViewer: Check ETag
alt ETag matches
FileViewer-->>Client: 304 Not Modified
else ETag doesn't match
FileViewer->>FileSystem: Open file
FileSystem-->>FileViewer: File content
FileViewer-->>Client: 200 OK with content and ETag
end
else not embedded file
FileViewer->>FileSystem: Open file
FileSystem-->>FileViewer: File content
FileViewer-->>Client: Serve content with headers
end
Class diagram showing FileViewer and StaticViewEngine changesclassDiagram
class FileViewer {
-fsys fs.FS
-path string
-isEmbed bool
-etag string
+MimeType() *MimeType
+Render(w http.ResponseWriter, r *http.Request, data any) error
-serveContent(w http.ResponseWriter, r *http.Request) error
}
class StaticViewEngine {
-isEmbedFsys bool
+Load(fsys fs.FS, app *App) error
-handle(fsys fs.FS, app *App, path string)
}
note for FileViewer "Added ETag support for embedded files"
note for StaticViewEngine "Added embed filesystem detection"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 90.66% 90.97% +0.30%
==========================================
Files 37 37
Lines 1286 1330 +44
==========================================
+ Hits 1166 1210 +44
Misses 83 83
Partials 37 37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return err | ||
| } | ||
|
|
||
| http.ServeContent(w, r, v.path, fi.ModTime(), f.(io.ReadSeeker)) |
There was a problem hiding this comment.
issue (bug_risk): Unsafe type assertion to io.ReadSeeker could panic
The type assertion f.(io.ReadSeeker) will panic if the file system doesn't support seeking. Consider checking the type assertion with ok value and handle non-seeker case gracefully.
viewer_file.go
Outdated
|
|
||
| w.Header().Set("ETag", v.etag) | ||
| if match := r.Header.Get("If-None-Match"); match != "" { | ||
| if match == v.etag { |
There was a problem hiding this comment.
issue: ETag comparison doesn't handle all HTTP spec cases
The If-None-Match header can contain multiple ETags and weak validators (W/ prefix). Consider using a more robust ETag comparison that handles these cases.
There was a problem hiding this comment.
Sure! I'm generating a new review now.
There was a problem hiding this comment.
Hey @cnlangzi, I've posted a new review for you!
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding error logging in NewFileViewer when file open or ETag calculation fails, rather than silently continuing. This would help with debugging if issues occur.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return err | ||
| } | ||
|
|
||
| http.ServeContent(w, r, v.path, fi.ModTime(), f.(io.ReadSeeker)) |
There was a problem hiding this comment.
issue (bug_risk): Add type assertion check for io.ReadSeeker
This type assertion could panic if the file doesn't implement io.ReadSeeker. Consider checking the type assertion with the comma-ok pattern.
| @@ -39,6 +71,40 @@ | |||
| // Render serves a file from the file system using the FileViewer. | |||
| // It writes the file to the http.ResponseWriter. | |||
| func (v *FileViewer) Render(w http.ResponseWriter, r *http.Request, data any) error { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the file serving logic by using http.ServeContent and setting the ETag conditionally for embedded files.
The current implementation adds unnecessary complexity by custom ETag handling and branching logic. We can simplify while preserving the embedded file optimization:
func (v *FileViewer) Render(w http.ResponseWriter, r *http.Request, data any) error {
f, err := v.fsys.Open(v.path)
if err != nil {
w.WriteHeader(http.StatusNotFound)
return nil
}
defer f.Close()
fi, err := f.Stat()
if err != nil {
return err
}
if v.isEmbed {
w.Header().Set("ETag", v.etag)
}
http.ServeContent(w, r, v.path, fi.ModTime(), f.(io.ReadSeeker))
return nil
}This simplification:
- Removes duplicate code paths and nested conditionals
- Uses http.ServeContent's built-in ETag handling for non-embedded files
- Preserves the pre-computed ETag optimization for embedded files
- Eliminates manual ETag matching since http.ServeContent handles it
Changed
Fixed
Last-Modified/ETagissue on embed file system.Added
Tests
Tasks to complete before merging PR:
make unit-testto check for any regressions 📋make lintto check for any issuesSummary by Sourcery
Bug Fixes: