Skip to content

Abstract reader #39

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.

Sign up for GitHub

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

Draft
vbmithr wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Abstract reader #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2c6b86
fix deprecated ioutil -> io
vbmithr Oct 1, 2025
7aa7653
FileEntry: use abstract source
vbmithr Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/zip_streamer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package testing

import (
"io/ioutil"
"io"
"os"
"testing"

Expand All @@ -13,15 +13,15 @@ var validFileEntry2, _ = zip_streamer.NewFileEntry("https://gist.githubuserconte
var invalidFileEntry, _ = zip_streamer.NewFileEntry("https://gist.githubusercontent.com/scosman/265617b16bb395850d1f0eefd25cc8e3/raw/ade5a9006493a16a57c6d24884b1e1d3978800eksfjmsdklfjsdlkfjsdlkfj/fake.jpg", "invalid.jpg")

func TestZipStreamCosntructorEmpty(t *testing.T) {
z, err := zip_streamer.NewZipStream(make([]*zip_streamer.FileEntry, 0), ioutil.Discard)
z, err := zip_streamer.NewZipStream(make([]*zip_streamer.FileEntry, 0), io.Discard)

if err == nil || z != nil {
t.Fatal("allowed empty streamer")
}
}

func TestZipStreamCosntructor(t *testing.T) {
z, err := zip_streamer.NewZipStream([]*zip_streamer.FileEntry{validFileEntry}, ioutil.Discard)
z, err := zip_streamer.NewZipStream([]*zip_streamer.FileEntry{validFileEntry}, io.Discard)

if err != nil || z == nil {
t.Fatal("constructor failed")
Expand Down
22 changes: 16 additions & 6 deletions zip_streamer/file_entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,31 @@ package zip_streamer

import (
"errors"
"io"
"net/http"
"net/url"
"os"
"path"
"strings"
)

type ReaderFunc func() (io.ReadCloser, error)

type FileEntry struct {
url *url.URL
reader ReaderFunc
zipPath string
}

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
}
}
Comment on lines +20 to +28
Copy link

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.


const UrlPrefixEnvVar = "ZS_URL_PREFIX"

func NewFileEntry(urlString string, zipPath string) (*FileEntry, error) {
Expand All @@ -37,16 +51,12 @@ func NewFileEntry(urlString string, zipPath string) (*FileEntry, error) {
}

f := FileEntry{
url: url,
reader: SimpleGet(urlString),
zipPath: zipPath,
}
return &f, nil
}

func (f *FileEntry) Url() *url.URL {
return f.url
}

func (f *FileEntry) ZipPath() string {
return f.zipPath
}
6 changes: 3 additions & 3 deletions zip_streamer/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"archive/zip"
"errors"
"fmt"
"io/ioutil"
"io"
"net/http"
"time"

Expand Down Expand Up @@ -63,7 +63,7 @@ func (s *Server) HandleCreateLink(w http.ResponseWriter, req *http.Request) {
}

func (s *Server) parseZipRequest(w http.ResponseWriter, req *http.Request) (*ZipDescriptor, error) {
body, err := ioutil.ReadAll(req.Body)
body, err := io.ReadAll(req.Body)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(`{"status":"error","error":"missing body"}`))
Expand Down Expand Up @@ -121,7 +121,7 @@ func retrieveZipDescriptorFromUrl(listfileUrl string) (*ZipDescriptor, error) {
if listfileResp.StatusCode != http.StatusOK {
return nil, errors.New("List File Server Error")
}
body, err := ioutil.ReadAll(listfileResp.Body)
body, err := io.ReadAll(listfileResp.Body)
if err != nil {
return nil, err
}
Expand Down
9 changes: 3 additions & 6 deletions zip_streamer/zip_streamer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,11 @@ func (z *ZipStream) StreamAllFiles() error {
success := 0

for _, entry := range z.entries {
resp, err := http.Get(entry.Url().String())
r, err := entry.reader()
if err != nil {
continue
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
continue
}
defer r.Close()
Comment on lines +37 to +41
Copy link

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.


header := &zip.FileHeader{
Name: entry.ZipPath(),
Expand All @@ -53,7 +50,7 @@ func (z *ZipStream) StreamAllFiles() error {
return err
}

_, err = io.Copy(entryWriter, resp.Body)
_, err = io.Copy(entryWriter, r)
if err != nil {
return err
}
Expand Down