Conversation
WalkthroughRefactors IO utilities from ioutil to io in tests and server. Introduces ReaderFunc and SimpleGet for FileEntry, replacing URL storage with a reader abstraction. Updates zip_streamer to stream entries via FileEntry.reader instead of direct HTTP GET, adjusting lifecycle to use r.Close() and io.Copy. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as ZipServer
participant Z as ZipStreamer
participant F as FileEntry.reader (ReaderFunc)
participant H as HTTP Origin (via SimpleGet)
C->>S: Request ZIP
S->>Z: Build and stream ZIP
loop For each FileEntry
Z->>F: reader()
activate F
F->>H: HTTP GET (SimpleGet)
H-->>F: Body stream
F-->>Z: io.ReadCloser
deactivate F
Z->>Z: io.Copy(entryWriter, reader)
Z-->>Z: defer reader.Close()
end
Z-->>S: ZIP bytes
S-->>C: ZIP response
note over Z: ReaderFunc abstracts retrieval\n(old direct http.Get removed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/zip_streamer_test.go(2 hunks)zip_streamer/file_entry.go(2 hunks)zip_streamer/server.go(3 hunks)zip_streamer/zip_streamer.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/zip_streamer_test.go (2)
zip_streamer/zip_streamer.go (1)
NewZipStream(17-30)zip_streamer/file_entry.go (1)
FileEntry(15-18)
🔇 Additional comments (5)
zip_streamer/file_entry.go (2)
13-13: LGTM: Clean abstraction for lazy data retrieval.The
ReaderFunctype provides a good abstraction that decouplesFileEntryfrom direct HTTP access, enabling better testability and flexibility.
32-58: LGTM: URL validation preserved with new reader abstraction.The function correctly validates URL scheme and prefix requirements before creating the
FileEntrywith the newSimpleGet-based reader. The validation logic remains intact while transitioning to the reader abstraction.zip_streamer/server.go (1)
66-66: LGTM: Standard migration from deprecated ioutil to io.The changes correctly update
ioutil.ReadAlltoio.ReadAll, aligning with Go 1.16+ best practices. No functional changes.Also applies to: 124-124
test/zip_streamer_test.go (1)
16-16: LGTM: Standard migration from deprecated ioutil to io.The changes correctly update
ioutil.Discardtoio.Discard, aligning with Go 1.16+ best practices. No functional changes.Also applies to: 24-24
zip_streamer/zip_streamer.go (1)
53-53: LGTM: Clean streaming via io.Copy.The transition from direct HTTP response handling to the reader abstraction is well-implemented.
io.Copyefficiently streams data from the reader to the zip entry writer.
| func SimpleGet(url string) ReaderFunc { | ||
| return func() (io.ReadCloser, error) { | ||
| resp, err := http.Get(url) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return resp.Body, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: HTTP status code not validated.
SimpleGet returns the response body without checking resp.StatusCode. A 404, 500, or any non-2xx response will return an open body that may contain error HTML/JSON instead of the expected content, causing corrupt zip entries.
Apply this diff to validate the status code:
func SimpleGet(url string) ReaderFunc {
return func() (io.ReadCloser, error) {
resp, err := http.Get(url)
if err != nil {
return nil, err
}
+ if resp.StatusCode != http.StatusOK {
+ resp.Body.Close()
+ return nil, errors.New("HTTP request failed with status: " + resp.Status)
+ }
return resp.Body, nil
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In zip_streamer/file_entry.go around lines 20 to 28, SimpleGet currently returns
resp.Body without validating resp.StatusCode; update the function to check that
resp.StatusCode is in the 2xx range (e.g., resp.StatusCode >= 200 &&
resp.StatusCode < 300), and if not, read/close the body and return an error that
includes the status code (and optionally the response body or status text) so
callers do not receive error pages as valid content; ensure the response body is
closed on error to avoid leaks and only return resp.Body when the status is
successful.
| r, err := entry.reader() | ||
| if err != nil { | ||
| continue | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusOK { | ||
| continue | ||
| } | ||
| defer r.Close() |
There was a problem hiding this comment.
Critical: defer in loop causes resource leak.
The defer r.Close() on Line 41 is inside a loop, which means all readers remain open until StreamAllFiles() returns. For large file lists, this will exhaust file descriptors and HTTP connections.
Apply this diff to close readers immediately after use:
for _, entry := range z.entries {
r, err := entry.reader()
if err != nil {
continue
}
- defer r.Close()
header := &zip.FileHeader{
Name: entry.ZipPath(),
Method: z.CompressionMethod,
Modified: time.Now(),
}
entryWriter, err := zipWriter.CreateHeader(header)
if err != nil {
+ r.Close()
return err
}
_, err = io.Copy(entryWriter, r)
+ r.Close()
if err != nil {
return err
}Alternatively, extract the processing into a separate function where defer will execute after each iteration.
🤖 Prompt for AI Agents
In zip_streamer/zip_streamer.go around lines 37 to 41, the defer r.Close() is
declared inside a loop so closes are deferred until function exit (leaking file
descriptors); instead close each reader immediately after its use — either
replace the defer with an explicit r.Close() call right after processing that
iteration (ensuring it runs even on early returns/errors), or move the per-entry
logic into a small helper/anonymous function and keep defer r.Close() inside
that function so it executes at the end of each iteration.
1 participant
Summary by CodeRabbit