From d8e6acda8cfe7e823ce49a20a4587b2641a90bf4 Mon Sep 17 00:00:00 2001 From: burbon Date: Mon, 11 May 2020 09:37:59 +0100 Subject: [PATCH] Support Range header end in lfs (#11314) * Initial support of end Range header option in lfs * Allow end range option to be unspecified * Declare toByte for consistency * Factor out content encoding tests from doLfs This is so Range tests could still use doLfs but without repeating not related tests * Add Range header test * implemented extraHeader * parametrized expectedStatus * Add more test cases of Range header * Fix S1030: should use resp.Body.String() * Add more tests for edge cases * Fix tests Signed-off-by: Andrew Thornton Co-authored-by: zeripath --- integrations/lfs_getobject_test.go | 117 +++++++++++++++++++++++++---- modules/lfs/server.go | 20 +++-- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/integrations/lfs_getobject_test.go b/integrations/lfs_getobject_test.go index 15d9c52a756e..45e94406a598 100644 --- a/integrations/lfs_getobject_test.go +++ b/integrations/lfs_getobject_test.go @@ -12,6 +12,7 @@ import ( "io" "io/ioutil" "net/http" + "net/http/httptest" "testing" "code.gitea.io/gitea/models" @@ -56,13 +57,7 @@ func storeObjectInRepo(t *testing.T, repositoryID int64, content *[]byte) string return oid } -func doLfs(t *testing.T, content *[]byte, expectGzip bool) { - defer prepareTestEnv(t)() - setting.CheckLFSVersion() - if !setting.LFS.StartServer { - t.Skip() - return - } +func storeAndGetLfs(t *testing.T, content *[]byte, extraHeader *http.Header, expectedStatus int) *httptest.ResponseRecorder { repo, err := models.GetRepositoryByOwnerAndName("user2", "repo1") assert.NoError(t, err) oid := storeObjectInRepo(t, repo.ID, content) @@ -73,8 +68,19 @@ func doLfs(t *testing.T, content *[]byte, expectGzip bool) { // Request OID req := NewRequest(t, "GET", "/user2/repo1.git/info/lfs/objects/"+oid+"/test") req.Header.Set("Accept-Encoding", "gzip") - resp := session.MakeRequest(t, req, http.StatusOK) + if extraHeader != nil { + for key, values := range *extraHeader { + for _, value := range values { + req.Header.Add(key, value) + } + } + } + resp := session.MakeRequest(t, req, expectedStatus) + return resp +} + +func checkResponseTestContentEncoding(t *testing.T, content *[]byte, resp *httptest.ResponseRecorder, expectGzip bool) { contentEncoding := resp.Header().Get("Content-Encoding") if !expectGzip || !setting.EnableGzip { assert.NotContains(t, contentEncoding, "gzip") @@ -89,23 +95,44 @@ func doLfs(t *testing.T, content *[]byte, expectGzip bool) { assert.NoError(t, err) assert.Equal(t, *content, result) } - } func TestGetLFSSmall(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } content := []byte("A very small file\n") - doLfs(t, &content, false) + + resp := storeAndGetLfs(t, &content, nil, http.StatusOK) + checkResponseTestContentEncoding(t, &content, resp, false) } func TestGetLFSLarge(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } content := make([]byte, gzip.MinSize*10) for i := range content { content[i] = byte(i % 256) } - doLfs(t, &content, true) + + resp := storeAndGetLfs(t, &content, nil, http.StatusOK) + checkResponseTestContentEncoding(t, &content, resp, true) } func TestGetLFSGzip(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } b := make([]byte, gzip.MinSize*10) for i := range b { b[i] = byte(i % 256) @@ -115,10 +142,18 @@ func TestGetLFSGzip(t *testing.T) { gzippWriter.Write(b) gzippWriter.Close() content := outputBuffer.Bytes() - doLfs(t, &content, false) + + resp := storeAndGetLfs(t, &content, nil, http.StatusOK) + checkResponseTestContentEncoding(t, &content, resp, false) } func TestGetLFSZip(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } b := make([]byte, gzip.MinSize*10) for i := range b { b[i] = byte(i % 256) @@ -130,5 +165,61 @@ func TestGetLFSZip(t *testing.T) { fileWriter.Write(b) zipWriter.Close() content := outputBuffer.Bytes() - doLfs(t, &content, false) + + resp := storeAndGetLfs(t, &content, nil, http.StatusOK) + checkResponseTestContentEncoding(t, &content, resp, false) +} + +func TestGetLFSRangeNo(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } + content := []byte("123456789\n") + + resp := storeAndGetLfs(t, &content, nil, http.StatusOK) + assert.Equal(t, content, resp.Body.Bytes()) +} + +func TestGetLFSRange(t *testing.T) { + defer prepareTestEnv(t)() + setting.CheckLFSVersion() + if !setting.LFS.StartServer { + t.Skip() + return + } + content := []byte("123456789\n") + + tests := []struct { + in string + out string + status int + }{ + {"bytes=0-0", "1", http.StatusPartialContent}, + {"bytes=0-1", "12", http.StatusPartialContent}, + {"bytes=1-1", "2", http.StatusPartialContent}, + {"bytes=1-3", "234", http.StatusPartialContent}, + {"bytes=1-", "23456789\n", http.StatusPartialContent}, + // end-range smaller than start-range is ignored + {"bytes=1-0", "23456789\n", http.StatusPartialContent}, + {"bytes=0-10", "123456789\n", http.StatusPartialContent}, + // end-range bigger than length-1 is ignored + {"bytes=0-11", "123456789\n", http.StatusPartialContent}, + {"bytes=11-", "", http.StatusPartialContent}, + // incorrect header value cause whole header to be ignored + {"bytes=-", "123456789\n", http.StatusOK}, + {"foobar", "123456789\n", http.StatusOK}, + } + + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + h := http.Header{ + "Range": []string{tt.in}, + } + resp := storeAndGetLfs(t, &content, &h, tt.status) + assert.Equal(t, tt.out, resp.Body.String()) + }) + } } diff --git a/modules/lfs/server.go b/modules/lfs/server.go index fda5ed7452bc..c39bf40d69b8 100644 --- a/modules/lfs/server.go +++ b/modules/lfs/server.go @@ -165,15 +165,24 @@ func getContentHandler(ctx *context.Context) { } // Support resume download using Range header - var fromByte int64 + var fromByte, toByte int64 + toByte = meta.Size - 1 statusCode := 200 if rangeHdr := ctx.Req.Header.Get("Range"); rangeHdr != "" { - regex := regexp.MustCompile(`bytes=(\d+)\-.*`) + regex := regexp.MustCompile(`bytes=(\d+)\-(\d*).*`) match := regex.FindStringSubmatch(rangeHdr) if len(match) > 1 { statusCode = 206 fromByte, _ = strconv.ParseInt(match[1], 10, 32) - ctx.Resp.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", fromByte, meta.Size-1, meta.Size-fromByte)) + + if match[2] != "" { + _toByte, _ := strconv.ParseInt(match[2], 10, 32) + if _toByte >= fromByte && _toByte < toByte { + toByte = _toByte + } + } + + ctx.Resp.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", fromByte, toByte, meta.Size-fromByte)) } } @@ -186,7 +195,8 @@ func getContentHandler(ctx *context.Context) { } defer content.Close() - ctx.Resp.Header().Set("Content-Length", strconv.FormatInt(meta.Size-fromByte, 10)) + contentLength := toByte + 1 - fromByte + ctx.Resp.Header().Set("Content-Length", strconv.FormatInt(contentLength, 10)) ctx.Resp.Header().Set("Content-Type", "application/octet-stream") filename := ctx.Params("filename") @@ -198,7 +208,7 @@ func getContentHandler(ctx *context.Context) { } ctx.Resp.WriteHeader(statusCode) - if written, err := io.Copy(ctx.Resp, content); err != nil { + if written, err := io.CopyN(ctx.Resp, content, contentLength); err != nil { log.Error("Error whilst copying LFS OID[%s] to the response after %d bytes. Error: %v", meta.Oid, written, err) } logRequest(ctx.Req, statusCode)