Response: optimize writeFile (#234)

- Open file once instead of thrice - fsutil: expanded the list of known extensions for content type detection - fsutil: add DetectContentType to sniff from a reader directly - fsutil: add DetectContentTypeByExtension for content type detection fallback
go-goyave · Dec 1, 2024 · 448e78f · 448e78f
1 parent 5836bff
commit 448e78f
Show file tree

Hide file tree

Showing 7 changed files with 514 additions and 111 deletions.
diff --git a/resources/empty b/resources/empty
diff --git a/resources/empty.json b/resources/empty.json
diff --git a/resources/empty.txt b/resources/empty.txt
diff --git a/response.go b/response.go
@@ -12,6 +12,7 @@ import (
 	"strconv"
 	"sync"
 
+	"github.com/samber/lo"
 	"gorm.io/gorm"
 	errorutil "goyave.dev/goyave/v5/util/errors"
 	"goyave.dev/goyave/v5/util/fsutil"
@@ -328,32 +329,37 @@ func (r *Response) String(responseCode int, message string) {
 }
 
 func (r *Response) writeFile(fs fs.StatFS, file string, disposition string) {
-	if !fsutil.FileExists(fs, file) {
+	f, err := fs.Open(file)
+	if err != nil {
 		r.Status(http.StatusNotFound)
 		return
 	}
-	r.status = http.StatusOK
-	mime, size, err := fsutil.GetMIMEType(fs, file)
-	if err != nil {
-		r.Error(errorutil.NewSkip(err, 4))
+	defer func() {
+		_ = f.Close()
+	}()
+	stat, err := f.Stat()
+	if err != nil || stat.IsDir() {
+		r.Status(http.StatusNotFound)
 		return
 	}
+	size := stat.Size()
 	header := r.responseWriter.Header()
-	header.Set("Content-Disposition", disposition)
 
+	contentType, _ := lo.Coalesce(header.Get("Content-Type"), "application/octet-stream")
 	if header.Get("Content-Type") == "" {
-		header.Set("Content-Type", mime)
+		readSeeker, ok := f.(io.ReadSeeker)
+		if size == 0 || !ok {
+			contentType = fsutil.DetectContentTypeByExtension(file)
+		} else {
+			contentType, err = fsutil.DetectContentType(readSeeker, file)
+			if err != nil {
+				panic(errorutil.NewSkip(err, 4))
+			}
+		}
 	}
-
+	header.Set("Content-Disposition", disposition)
 	header.Set("Content-Length", strconv.FormatInt(size, 10))
-
-	f, _ := fs.Open(file)
-	// FIXME: the file is opened thrice here, can we optimize this so it's only opened once?
-	// No need to check for errors, fsutil.FileExists(fs, file) and
-	// fsutil.GetMIMEType(fs, file) already handled that.
-	defer func() {
-		_ = f.Close()
-	}()
+	header.Set("Content-Type", contentType)
 	if _, err := io.Copy(r, f); err != nil {
 		panic(errorutil.NewSkip(err, 4))
 	}

diff --git a/response_test.go b/response_test.go
@@ -143,53 +143,204 @@ func TestResponse(t *testing.T) {
 	})
 
 	t.Run("File", func(t *testing.T) {
-		resp, recorder := newTestReponse()
-
-		resp.File(&osfs.FS{}, "resources/test_file.txt")
-		res := recorder.Result()
-
-		assert.Equal(t, http.StatusOK, res.StatusCode)
-		assert.Equal(t, "inline", res.Header.Get("Content-Disposition"))
-		assert.Equal(t, "25", res.Header.Get("Content-Length"))
-		assert.Equal(t, "text/plain; charset=utf-8", res.Header.Get("Content-Type"))
+		cases := []struct {
+			setup           func(resp *Response)
+			desc            string
+			filename        string
+			wantLength      string
+			wantContentType string
+			wantDisposition string
+			wantBody        []byte
+			wantStatus      int
+		}{
+			{
+				desc:            "content-type_detection",
+				filename:        "resources/test_file.txt",
+				wantLength:      "25",
+				wantContentType: "text/plain; charset=utf-8",
+				wantDisposition: "inline",
+				wantStatus:      http.StatusOK,
+				wantBody:        append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), // utf-8 BOM + text content
+			},
+			{
+				desc: "content-type_provided",
+				setup: func(resp *Response) {
+					resp.Header().Set("Content-Type", "provided")
+				},
+				filename:        "resources/test_file.txt",
+				wantLength:      "25",
+				wantContentType: "provided",
+				wantDisposition: "inline",
+				wantStatus:      http.StatusOK,
+				wantBody:        append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), // utf-8 BOM + text content
+			},
+			{
+				desc:            "empty_txt_file",
+				filename:        "resources/empty.txt",
+				wantLength:      "0",
+				wantContentType: "text/plain",
+				wantDisposition: "inline",
+				wantStatus:      0, // Will be set to 200 in finalization step, which isn't executed here.
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "empty_file",
+				filename:        "resources/empty",
+				wantLength:      "0",
+				wantContentType: "application/octet-stream",
+				wantDisposition: "inline",
+				wantStatus:      0, // Will be set to 200 in finalization step, which isn't executed here.
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "not_found",
+				filename:        "not_a_file",
+				wantLength:      "",
+				wantContentType: "",
+				wantDisposition: "",
+				wantStatus:      http.StatusNotFound,
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "directory",
+				filename:        ".",
+				wantLength:      "",
+				wantContentType: "",
+				wantDisposition: "",
+				wantStatus:      http.StatusNotFound,
+				wantBody:        []byte{},
+			},
+		}
 
-		body, err := io.ReadAll(res.Body)
-		assert.NoError(t, res.Body.Close())
-		require.NoError(t, err)
+		for _, c := range cases {
+			t.Run(c.desc, func(t *testing.T) {
+				resp, recorder := newTestReponse()
+				if c.setup != nil {
+					c.setup(resp)
+				}
+				resp.File(&osfs.FS{}, c.filename)
+				res := recorder.Result()
 
-		// utf-8 BOM + text content
-		assert.Equal(t, append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), body)
+				assert.Equal(t, c.wantStatus, resp.status)
+				assert.Equal(t, c.wantDisposition, res.Header.Get("Content-Disposition"))
+				assert.Equal(t, c.wantLength, res.Header.Get("Content-Length"))
+				assert.Equal(t, c.wantContentType, res.Header.Get("Content-Type"))
 
-		t.Run("not_found", func(t *testing.T) {
-			resp, _ := newTestReponse()
-			resp.File(&osfs.FS{}, "not_a_file")
-			assert.Equal(t, http.StatusNotFound, resp.status)
-		})
+				body, err := io.ReadAll(res.Body)
+				assert.NoError(t, res.Body.Close())
+				require.NoError(t, err)
+				assert.Equal(t, c.wantBody, body)
+			})
+		}
 	})
 
 	t.Run("Download", func(t *testing.T) {
-		resp, recorder := newTestReponse()
-
-		resp.Download(&osfs.FS{}, "resources/test_file.txt", "test_file.txt")
-		res := recorder.Result()
-
-		assert.Equal(t, http.StatusOK, res.StatusCode)
-		assert.Equal(t, "attachment; filename=\"test_file.txt\"", res.Header.Get("Content-Disposition"))
-		assert.Equal(t, "25", res.Header.Get("Content-Length"))
-		assert.Equal(t, "text/plain; charset=utf-8", res.Header.Get("Content-Type"))
+		cases := []struct {
+			setup           func(resp *Response)
+			desc            string
+			path            string
+			filename        string
+			wantLength      string
+			wantContentType string
+			wantDisposition string
+			wantBody        []byte
+			wantStatus      int
+		}{
+			{
+				desc:            "content-type_detection",
+				path:            "resources/test_file.txt",
+				filename:        "name.txt",
+				wantLength:      "25",
+				wantContentType: "text/plain; charset=utf-8",
+				wantDisposition: "attachment; filename=\"name.txt\"",
+				wantStatus:      http.StatusOK,
+				wantBody:        append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), // utf-8 BOM + text content
+			},
+			{
+				desc: "content-type_provided",
+				setup: func(resp *Response) {
+					resp.Header().Set("Content-Type", "provided")
+				},
+				path:            "resources/test_file.txt",
+				filename:        "name.txt",
+				wantLength:      "25",
+				wantContentType: "provided",
+				wantDisposition: "attachment; filename=\"name.txt\"",
+				wantStatus:      http.StatusOK,
+				wantBody:        append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), // utf-8 BOM + text content
+			},
+			{
+				desc:            "empty_txt_file",
+				path:            "resources/empty.txt",
+				filename:        "name.txt",
+				wantLength:      "0",
+				wantContentType: "text/plain",
+				wantDisposition: "attachment; filename=\"name.txt\"",
+				wantStatus:      0, // Will be set to 200 in finalization step, which isn't executed here.
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "empty_file",
+				path:            "resources/empty",
+				filename:        "empty",
+				wantLength:      "0",
+				wantContentType: "application/octet-stream",
+				wantDisposition: "attachment; filename=\"empty\"",
+				wantStatus:      0, // Will be set to 200 in finalization step, which isn't executed here.
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "empty_json_file", // The content-type should be detected from the file extension as a fallback
+				path:            "resources/empty.json",
+				filename:        "name.json",
+				wantLength:      "0",
+				wantContentType: "application/json",
+				wantDisposition: "attachment; filename=\"name.json\"",
+				wantStatus:      0, // Will be set to 200 in finalization step, which isn't executed here.
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "not_found",
+				path:            "not_a_file",
+				filename:        "name.txt",
+				wantLength:      "",
+				wantContentType: "",
+				wantDisposition: "",
+				wantStatus:      http.StatusNotFound,
+				wantBody:        []byte{},
+			},
+			{
+				desc:            "directory",
+				path:            ".",
+				filename:        "name.txt",
+				wantLength:      "",
+				wantContentType: "",
+				wantDisposition: "",
+				wantStatus:      http.StatusNotFound,
+				wantBody:        []byte{},
+			},
+		}
 
-		body, err := io.ReadAll(res.Body)
-		assert.NoError(t, res.Body.Close())
-		require.NoError(t, err)
+		for _, c := range cases {
+			t.Run(c.desc, func(t *testing.T) {
+				resp, recorder := newTestReponse()
+				if c.setup != nil {
+					c.setup(resp)
+				}
+				resp.Download(&osfs.FS{}, c.path, c.filename)
+				res := recorder.Result()
 
-		// utf-8 BOM + text content
-		assert.Equal(t, append([]byte{0xef, 0xbb, 0xbf}, []byte("utf-8 with BOM content")...), body)
+				assert.Equal(t, c.wantStatus, resp.status)
+				assert.Equal(t, c.wantDisposition, res.Header.Get("Content-Disposition"))
+				assert.Equal(t, c.wantLength, res.Header.Get("Content-Length"))
+				assert.Equal(t, c.wantContentType, res.Header.Get("Content-Type"))
 
-		t.Run("not_found", func(t *testing.T) {
-			resp, _ := newTestReponse()
-			resp.Download(&osfs.FS{}, "not_a_file", "file.txt")
-			assert.Equal(t, http.StatusNotFound, resp.status)
-		})
+				body, err := io.ReadAll(res.Body)
+				assert.NoError(t, res.Body.Close())
+				require.NoError(t, err)
+				assert.Equal(t, c.wantBody, body)
+			})
+		}
 	})
 
 	t.Run("JSON", func(t *testing.T) {