Skip to content

Commit 36cde08

Browse files
sheeengsheeeng
committed
fix: download contents
Fix #3810, #2480. Signed-off-by: Leonard Sheng Sheng Lee <[email protected]>
1 parent 9c16433 commit 36cde08

File tree

2 files changed

+75
-120
lines changed

2 files changed

+75
-120
lines changed

‎github/repos_contents.go‎

Lines changed: 61 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"io"
1818
"net/http"
1919
"net/url"
20-
"path"
2120
"strings"
2221
)
2322

@@ -98,6 +97,54 @@ func (r *RepositoryContent) GetContent() (string, error) {
9897
}
9998
}
10099

100+
// DownloadContent downloads the content of a file using the information from
101+
// the RepositoryContent. This method supports files of any size and works with
102+
// RepositoryContent entries obtained from GetContents calls on both individual
103+
// files and directories.
104+
//
105+
// It returns an io.ReadCloser for the file content and an *http.Response for
106+
// the download request (which may be nil if content was obtained without an
107+
// HTTP request). It is the caller's responsibility to close the ReadCloser.
108+
func (r *RepositoryContent) DownloadContent(ctx context.Context, httpClient *http.Client) (io.ReadCloser, *http.Response, error) {
109+
// Check if this is a file.
110+
if r.Type == nil || *r.Type != "file" {
111+
return nil, nil, fmt.Errorf("cannot download content for type %q (only files are supported)", r.GetType())
112+
}
113+
114+
// Handle empty files.
115+
if r.Size != nil && *r.Size == 0 {
116+
return io.NopCloser(strings.NewReader("")), nil, nil
117+
}
118+
119+
// Try to use the inline content if available.
120+
if r.Content != nil && *r.Content != "" {
121+
content, err := r.GetContent()
122+
if err == nil {
123+
return io.NopCloser(strings.NewReader(content)), nil, nil
124+
}
125+
// If GetContent fails (e.g., encoding "none"), fall through to use DownloadURL.
126+
}
127+
128+
// Use the download URL.
129+
if r.DownloadURL == nil || *r.DownloadURL == "" {
130+
return nil, nil, errors.New("no download URL available for this file")
131+
}
132+
133+
req, err := http.NewRequestWithContext(ctx, "GET", *r.DownloadURL, nil)
134+
if err != nil {
135+
return nil, nil, err
136+
}
137+
138+
resp, err := httpClient.Do(req)
139+
if err != nil {
140+
return nil, nil, err
141+
}
142+
143+
// Return the response body even for non-2xx status codes.
144+
// Callers should check resp.StatusCode to verify success.
145+
return resp.Body, resp, nil
146+
}
147+
101148
// GetReadme gets the Readme file for the repository.
102149
//
103150
// GitHub API docs: https://docs.github.com/rest/repos/contents#get-a-repository-readme
@@ -137,40 +184,8 @@ func (s *RepositoriesService) GetReadme(ctx context.Context, owner, repo string,
137184
//
138185
//meta:operation GET /repos/{owner}/{repo}/contents/{path}
139186
func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *Response, error) {
140-
dir := path.Dir(filepath)
141-
filename := path.Base(filepath)
142-
fileContent, _, resp, err := s.GetContents(ctx, owner, repo, filepath, opts)
143-
if err == nil && fileContent != nil {
144-
content, err := fileContent.GetContent()
145-
if err == nil && content != "" {
146-
return io.NopCloser(strings.NewReader(content)), resp, nil
147-
}
148-
}
149-
150-
_, dirContents, resp, err := s.GetContents(ctx, owner, repo, dir, opts)
151-
if err != nil {
152-
return nil, resp, err
153-
}
154-
155-
for _, contents := range dirContents {
156-
if contents.GetName() == filename {
157-
if contents.GetDownloadURL() == "" {
158-
return nil, resp, fmt.Errorf("no download link found for %v", filepath)
159-
}
160-
dlReq, err := http.NewRequestWithContext(ctx, "GET", *contents.DownloadURL, nil)
161-
if err != nil {
162-
return nil, resp, err
163-
}
164-
dlResp, err := s.client.client.Do(dlReq)
165-
if err != nil {
166-
return nil, &Response{Response: dlResp}, err
167-
}
168-
169-
return dlResp.Body, &Response{Response: dlResp}, nil
170-
}
171-
}
172-
173-
return nil, resp, fmt.Errorf("no file named %v found in %v", filename, dir)
187+
reader, _, resp, err := s.DownloadContentsWithMeta(ctx, owner, repo, filepath, opts)
188+
return reader, resp, err
174189
}
175190

176191
// DownloadContentsWithMeta is identical to DownloadContents but additionally
@@ -186,40 +201,25 @@ func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo,
186201
//
187202
//meta:operation GET /repos/{owner}/{repo}/contents/{path}
188203
func (s *RepositoriesService) DownloadContentsWithMeta(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *RepositoryContent, *Response, error) {
189-
dir := path.Dir(filepath)
190-
filename := path.Base(filepath)
191204
fileContent, _, resp, err := s.GetContents(ctx, owner, repo, filepath, opts)
192-
if err == nil && fileContent != nil {
193-
content, err := fileContent.GetContent()
194-
if err == nil && content != "" {
195-
return io.NopCloser(strings.NewReader(content)), fileContent, resp, nil
196-
}
205+
if err != nil {
206+
return nil, nil, resp, err
207+
}
208+
if fileContent == nil {
209+
return nil, nil, resp, fmt.Errorf("no file found at %v", filepath)
197210
}
198211

199-
_, dirContents, resp, err := s.GetContents(ctx, owner, repo, dir, opts)
212+
reader, httpResp, err := fileContent.DownloadContent(ctx, s.client.client)
200213
if err != nil {
201-
return nil, nil, resp, err
214+
return nil, fileContent, resp, err
202215
}
203216

204-
for _, contents := range dirContents {
205-
if contents.GetName() == filename {
206-
if contents.GetDownloadURL() == "" {
207-
return nil, contents, resp, fmt.Errorf("no download link found for %v", filepath)
208-
}
209-
dlReq, err := http.NewRequestWithContext(ctx, "GET", *contents.DownloadURL, nil)
210-
if err != nil {
211-
return nil, contents, resp, err
212-
}
213-
dlResp, err := s.client.client.Do(dlReq)
214-
if err != nil {
215-
return nil, contents, &Response{Response: dlResp}, err
216-
}
217-
218-
return dlResp.Body, contents, &Response{Response: dlResp}, nil
219-
}
217+
// If we got an HTTP response from the download, wrap it in a Response.
218+
if httpResp != nil {
219+
return reader, fileContent, &Response{Response: httpResp}, nil
220220
}
221221

222-
return nil, nil, resp, fmt.Errorf("no file named %v found in %v", filename, dir)
222+
return reader, fileContent, resp, nil
223223
}
224224

225225
// GetContents can return either the metadata and content of a single file

‎github/repos_contents_test.go‎

Lines changed: 14 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -182,17 +182,10 @@ func TestRepositoriesService_DownloadContents_SuccessForDirectory(t *testing.T)
182182
mux.HandleFunc("/repos/o/r/contents/d/f", func(w http.ResponseWriter, r *http.Request) {
183183
testMethod(t, r, "GET")
184184
fmt.Fprint(w, `{
185-
"type": "file",
186-
"name": "f"
187-
}`)
188-
})
189-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
190-
testMethod(t, r, "GET")
191-
fmt.Fprint(w, `[{
192185
"type": "file",
193186
"name": "f",
194187
"download_url": "`+serverURL+baseURLPath+`/download/f"
195-
}]`)
188+
}`)
196189
})
197190
mux.HandleFunc("/download/f", func(w http.ResponseWriter, r *http.Request) {
198191
testMethod(t, r, "GET")
@@ -240,18 +233,12 @@ func TestRepositoriesService_DownloadContents_FailedResponse(t *testing.T) {
240233

241234
mux.HandleFunc("/repos/o/r/contents/d/f", func(w http.ResponseWriter, r *http.Request) {
242235
testMethod(t, r, "GET")
243-
fmt.Fprint(w, `{
244-
"type": "file",
245-
"name": "f"
246-
}`)
247-
})
248-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
249-
testMethod(t, r, "GET")
250-
fmt.Fprint(w, `[{
236+
fmt.Fprint(w,
237+
`{
251238
"type": "file",
252239
"name": "f",
253240
"download_url": "`+serverURL+baseURLPath+`/download/f"
254-
}]`)
241+
}`)
255242
})
256243
mux.HandleFunc("/download/f", func(w http.ResponseWriter, r *http.Request) {
257244
testMethod(t, r, "GET")
@@ -288,18 +275,9 @@ func TestRepositoriesService_DownloadContents_NoDownloadURL(t *testing.T) {
288275
testMethod(t, r, "GET")
289276
fmt.Fprint(w, `{
290277
"type": "file",
291-
"name": "f",
292-
"content": ""
278+
"name": "f"
293279
}`)
294280
})
295-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
296-
testMethod(t, r, "GET")
297-
fmt.Fprint(w, `[{
298-
"type": "file",
299-
"name": "f",
300-
"content": ""
301-
}]`)
302-
})
303281

304282
ctx := t.Context()
305283
reader, resp, err := client.Repositories.DownloadContents(ctx, "o", "r", "d/f", nil)
@@ -322,16 +300,8 @@ func TestRepositoriesService_DownloadContents_NoFile(t *testing.T) {
322300

323301
mux.HandleFunc("/repos/o/r/contents/d/f", func(w http.ResponseWriter, r *http.Request) {
324302
testMethod(t, r, "GET")
325-
fmt.Fprint(w, `{
326-
"type": "file",
327-
"name": "f",
328-
"content": ""
329-
}`)
330-
})
331-
332-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
333-
testMethod(t, r, "GET")
334-
fmt.Fprint(w, `[]`)
303+
w.WriteHeader(http.StatusNotFound)
304+
fmt.Fprint(w, `{"message":"Not Found"}`)
335305
})
336306

337307
ctx := t.Context()
@@ -413,13 +383,13 @@ func TestRepositoriesService_DownloadContentsWithMeta_SuccessForDirectory(t *tes
413383
t.Parallel()
414384
client, mux, serverURL := setup(t)
415385

416-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
386+
mux.HandleFunc("/repos/o/r/contents/d/f", func(w http.ResponseWriter, r *http.Request) {
417387
testMethod(t, r, "GET")
418-
fmt.Fprint(w, `[{
388+
fmt.Fprint(w, `{
419389
"type": "file",
420390
"name": "f",
421391
"download_url": "`+serverURL+baseURLPath+`/download/f"
422-
}]`)
392+
}`)
423393
})
424394
mux.HandleFunc("/download/f", func(w http.ResponseWriter, r *http.Request) {
425395
testMethod(t, r, "GET")
@@ -474,14 +444,6 @@ func TestRepositoriesService_DownloadContentsWithMeta_FailedResponse(t *testing.
474444
w.WriteHeader(http.StatusInternalServerError)
475445
fmt.Fprint(w, "foo error")
476446
})
477-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
478-
testMethod(t, r, "GET")
479-
fmt.Fprint(w, `[{
480-
"type": "file",
481-
"name": "f",
482-
"download_url": "`+downloadURL+`"
483-
}]`)
484-
})
485447

486448
ctx := t.Context()
487449
r, c, resp, err := client.Repositories.DownloadContentsWithMeta(ctx, "o", "r", "d/f", nil)
@@ -520,17 +482,9 @@ func TestRepositoriesService_DownloadContentsWithMeta_NoDownloadURL(t *testing.T
520482
testMethod(t, r, "GET")
521483
fmt.Fprint(w, `{
522484
"type": "file",
523-
"name": "f",
485+
"name": "f"
524486
}`)
525487
})
526-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
527-
testMethod(t, r, "GET")
528-
fmt.Fprint(w, `[{
529-
"type": "file",
530-
"name": "f",
531-
"content": ""
532-
}]`)
533-
})
534488

535489
ctx := t.Context()
536490
reader, contents, resp, err := client.Repositories.DownloadContentsWithMeta(ctx, "o", "r", "d/f", nil)
@@ -555,9 +509,10 @@ func TestRepositoriesService_DownloadContentsWithMeta_NoFile(t *testing.T) {
555509
t.Parallel()
556510
client, mux, _ := setup(t)
557511

558-
mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) {
512+
mux.HandleFunc("/repos/o/r/contents/d/f", func(w http.ResponseWriter, r *http.Request) {
559513
testMethod(t, r, "GET")
560-
fmt.Fprint(w, `[]`)
514+
w.WriteHeader(http.StatusNotFound)
515+
fmt.Fprint(w, `{"message":"Not Found"}`)
561516
})
562517

563518
ctx := t.Context()

0 commit comments

Comments
 (0)