From a94a8d0ab1f6c9d63ae607dfec866c47ae3e4b5c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 13 May 2023 22:04:57 +0800 Subject: [PATCH] Use standard HTTP library to serve files (#24693) `http.ServeFile/ServeContent` handles `If-xxx`, `Content-Length`, `Range` and `Etag` correctly After this PR, storage files (eg: avatar) could be responded with correct Content-Length. --- modules/context/context_test.go | 4 +- modules/httpcache/httpcache.go | 37 ++----------------- modules/httpcache/httpcache_test.go | 57 ----------------------------- modules/httplib/mock.go | 35 ------------------ modules/httplib/serve_test.go | 13 ++++--- modules/public/public.go | 8 +--- routers/web/base.go | 16 +++----- routers/web/misc/misc.go | 9 ++--- 8 files changed, 22 insertions(+), 157 deletions(-) delete mode 100644 modules/httplib/mock.go diff --git a/modules/context/context_test.go b/modules/context/context_test.go index a6facc9788..033ce2ef0a 100644 --- a/modules/context/context_test.go +++ b/modules/context/context_test.go @@ -5,16 +5,16 @@ package context import ( "net/http" + "net/http/httptest" "testing" - "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" ) func TestRemoveSessionCookieHeader(t *testing.T) { - w := httplib.NewMockResponseWriter() + w := httptest.NewRecorder() w.Header().Add("Set-Cookie", (&http.Cookie{Name: setting.SessionConfig.CookieName, Value: "foo"}).String()) w.Header().Add("Set-Cookie", (&http.Cookie{Name: "other", Value: "bar"}).String()) assert.Len(t, w.Header().Values("Set-Cookie"), 2) diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index 46e0152ef4..001ac06415 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -4,10 +4,8 @@ package httpcache import ( - "encoding/base64" - "fmt" + "io" "net/http" - "os" "strconv" "strings" "time" @@ -37,38 +35,9 @@ func SetCacheControlInHeader(h http.Header, maxAge time.Duration, additionalDire h.Set("Cache-Control", strings.Join(append(directives, additionalDirectives...), ", ")) } -// generateETag generates an ETag based on size, filename and file modification time -func generateETag(fi os.FileInfo) string { - etag := fmt.Sprint(fi.Size()) + fi.Name() + fi.ModTime().UTC().Format(http.TimeFormat) - return `"` + base64.StdEncoding.EncodeToString([]byte(etag)) + `"` -} - -// HandleTimeCache handles time-based caching for a HTTP request -func HandleTimeCache(req *http.Request, w http.ResponseWriter, fi os.FileInfo) (handled bool) { - return HandleGenericTimeCache(req, w, fi.ModTime()) -} - -// HandleGenericTimeCache handles time-based caching for a HTTP request -func HandleGenericTimeCache(req *http.Request, w http.ResponseWriter, lastModified time.Time) (handled bool) { +func ServeContentWithCacheControl(w http.ResponseWriter, req *http.Request, name string, modTime time.Time, content io.ReadSeeker) { SetCacheControlInHeader(w.Header(), setting.StaticCacheTime) - - ifModifiedSince := req.Header.Get("If-Modified-Since") - if ifModifiedSince != "" { - t, err := time.Parse(http.TimeFormat, ifModifiedSince) - if err == nil && lastModified.Unix() <= t.Unix() { - w.WriteHeader(http.StatusNotModified) - return true - } - } - - w.Header().Set("Last-Modified", lastModified.Format(http.TimeFormat)) - return false -} - -// HandleFileETagCache handles ETag-based caching for a HTTP request -func HandleFileETagCache(req *http.Request, w http.ResponseWriter, fi os.FileInfo) (handled bool) { - etag := generateETag(fi) - return HandleGenericETagCache(req, w, etag) + http.ServeContent(w, req, name, modTime, content) } // HandleGenericETagCache handles ETag-based caching for a HTTP request. diff --git a/modules/httpcache/httpcache_test.go b/modules/httpcache/httpcache_test.go index 624d2f4d4b..d81f06097c 100644 --- a/modules/httpcache/httpcache_test.go +++ b/modules/httpcache/httpcache_test.go @@ -6,23 +6,12 @@ package httpcache import ( "net/http" "net/http/httptest" - "os" "strings" "testing" - "time" "github.com/stretchr/testify/assert" ) -type mockFileInfo struct{} - -func (m mockFileInfo) Name() string { return "gitea.test" } -func (m mockFileInfo) Size() int64 { return int64(10) } -func (m mockFileInfo) Mode() os.FileMode { return os.ModePerm } -func (m mockFileInfo) ModTime() time.Time { return time.Time{} } -func (m mockFileInfo) IsDir() bool { return false } -func (m mockFileInfo) Sys() interface{} { return nil } - func countFormalHeaders(h http.Header) (c int) { for k := range h { // ignore our headers for internal usage @@ -34,52 +23,6 @@ func countFormalHeaders(h http.Header) (c int) { return c } -func TestHandleFileETagCache(t *testing.T) { - fi := mockFileInfo{} - etag := `"MTBnaXRlYS50ZXN0TW9uLCAwMSBKYW4gMDAwMSAwMDowMDowMCBHTVQ="` - - t.Run("No_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - handled := HandleFileETagCache(req, w, fi) - - assert.False(t, handled) - assert.Equal(t, 2, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Cache-Control") - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - }) - t.Run("Wrong_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", `"wrong etag"`) - - handled := HandleFileETagCache(req, w, fi) - - assert.False(t, handled) - assert.Equal(t, 2, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Cache-Control") - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - }) - t.Run("Correct_If-None-Match", func(t *testing.T) { - req := &http.Request{Header: make(http.Header)} - w := httptest.NewRecorder() - - req.Header.Set("If-None-Match", etag) - - handled := HandleFileETagCache(req, w, fi) - - assert.True(t, handled) - assert.Equal(t, 1, countFormalHeaders(w.Header())) - assert.Contains(t, w.Header(), "Etag") - assert.Equal(t, etag, w.Header().Get("Etag")) - assert.Equal(t, http.StatusNotModified, w.Code) - }) -} - func TestHandleGenericETagCache(t *testing.T) { etag := `"test"` diff --git a/modules/httplib/mock.go b/modules/httplib/mock.go deleted file mode 100644 index 7d284e86fb..0000000000 --- a/modules/httplib/mock.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2023 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package httplib - -import ( - "bytes" - "net/http" -) - -type MockResponseWriter struct { - header http.Header - - StatusCode int - BodyBuffer bytes.Buffer -} - -func (m *MockResponseWriter) Header() http.Header { - return m.header -} - -func (m *MockResponseWriter) Write(bytes []byte) (int, error) { - if m.StatusCode == 0 { - m.StatusCode = http.StatusOK - } - return m.BodyBuffer.Write(bytes) -} - -func (m *MockResponseWriter) WriteHeader(statusCode int) { - m.StatusCode = statusCode -} - -func NewMockResponseWriter() *MockResponseWriter { - return &MockResponseWriter{header: http.Header{}} -} diff --git a/modules/httplib/serve_test.go b/modules/httplib/serve_test.go index 0768f1c713..fed4611d21 100644 --- a/modules/httplib/serve_test.go +++ b/modules/httplib/serve_test.go @@ -6,6 +6,7 @@ package httplib import ( "fmt" "net/http" + "net/http/httptest" "net/url" "os" "strings" @@ -25,12 +26,12 @@ func TestServeContentByReader(t *testing.T) { r.Header.Set("Range", fmt.Sprintf("bytes=%s", rangeStr)) } reader := strings.NewReader(data) - w := NewMockResponseWriter() + w := httptest.NewRecorder() ServeContentByReader(r, w, "test", int64(len(data)), reader) - assert.Equal(t, expectedStatusCode, w.StatusCode) + assert.Equal(t, expectedStatusCode, w.Code) if expectedStatusCode == http.StatusPartialContent || expectedStatusCode == http.StatusOK { assert.Equal(t, fmt.Sprint(len(expectedContent)), w.Header().Get("Content-Length")) - assert.Equal(t, expectedContent, w.BodyBuffer.String()) + assert.Equal(t, expectedContent, w.Body.String()) } } @@ -76,12 +77,12 @@ func TestServeContentByReadSeeker(t *testing.T) { } defer seekReader.Close() - w := NewMockResponseWriter() + w := httptest.NewRecorder() ServeContentByReadSeeker(r, w, "test", time.Time{}, seekReader) - assert.Equal(t, expectedStatusCode, w.StatusCode) + assert.Equal(t, expectedStatusCode, w.Code) if expectedStatusCode == http.StatusPartialContent || expectedStatusCode == http.StatusOK { assert.Equal(t, fmt.Sprint(len(expectedContent)), w.Header().Get("Content-Length")) - assert.Equal(t, expectedContent, w.BodyBuffer.String()) + assert.Equal(t, expectedContent, w.Body.String()) } } diff --git a/modules/public/public.go b/modules/public/public.go index 0c0e6dc1cc..ed38d85cfa 100644 --- a/modules/public/public.go +++ b/modules/public/public.go @@ -97,10 +97,6 @@ func handleRequest(w http.ResponseWriter, req *http.Request, fs http.FileSystem, return true } - if httpcache.HandleFileETagCache(req, w, fi) { - return true - } - serveContent(w, req, fi, fi.ModTime(), f) return true } @@ -124,11 +120,11 @@ func serveContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modt w.Header().Set("Content-Type", "application/octet-stream") } w.Header().Set("Content-Encoding", "gzip") - http.ServeContent(w, req, fi.Name(), modtime, rdGzip) + httpcache.ServeContentWithCacheControl(w, req, fi.Name(), modtime, rdGzip) return } } - http.ServeContent(w, req, fi.Name(), modtime, content) + httpcache.ServeContentWithCacheControl(w, req, fi.Name(), modtime, content) return } diff --git a/routers/web/base.go b/routers/web/base.go index b9b5958389..1f6c4fbfc5 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -6,7 +6,6 @@ package web import ( "errors" "fmt" - "io" "net/http" "os" "path" @@ -76,12 +75,6 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor } fi, err := objStore.Stat(rPath) - if err == nil && httpcache.HandleTimeCache(req, w, fi) { - return - } - - // If we have matched and access to release or issue - fr, err := objStore.Open(rPath) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { log.Warn("Unable to find %s %s", prefix, rPath) @@ -92,14 +85,15 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) return } - defer fr.Close() - _, err = io.Copy(w, fr) + fr, err := objStore.Open(rPath) if err != nil { - log.Error("Error whilst rendering %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst rendering %s %s", prefix, rPath), http.StatusInternalServerError) + log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) + http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) return } + defer fr.Close() + httpcache.ServeContentWithCacheControl(w, req, path.Base(rPath), fi.ModTime(), fr) }) } } diff --git a/routers/web/misc/misc.go b/routers/web/misc/misc.go index 582179990a..6ed3b5c3ad 100644 --- a/routers/web/misc/misc.go +++ b/routers/web/misc/misc.go @@ -5,13 +5,13 @@ package misc import ( "net/http" - "os" "path" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/httpcache" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) func SSHInfo(rw http.ResponseWriter, req *http.Request) { @@ -34,11 +34,8 @@ func DummyOK(w http.ResponseWriter, req *http.Request) { } func RobotsTxt(w http.ResponseWriter, req *http.Request) { - filePath := path.Join(setting.CustomPath, "robots.txt") - fi, err := os.Stat(filePath) - if err == nil && httpcache.HandleTimeCache(req, w, fi) { - return - } + filePath := util.FilePathJoinAbs(setting.CustomPath, "robots.txt") + httpcache.SetCacheControlInHeader(w.Header(), setting.StaticCacheTime) http.ServeFile(w, req, filePath) }