valyala / fasthttp

Fast HTTP package for Go. Tuned for high performance. Zero memory allocations in hot paths. Up to 10x faster than net/http
MIT License
21.91k stars 1.76k forks source link

fasthttp.FS won't return Content-Encoding when files are too small #1795

Closed gaby closed 5 months ago

gaby commented 5 months ago

While writing unit-tests for Gofiber, we noticed that when using fasthttp.FS if the file is too small and we send a request with Accept-Encoding to gzip the handler will not set the encoding header.

Request example:

req := httptest.NewRequest(fiber.MethodGet, "/index.html", nil)
req.Header.Set("Accept-Encoding", "gzip")

Example file:

h1 {
    color: red;
    text-align: center;
}

Returned headers:

http.Header(
    http.Header{"Content-Length":[]string{"46"},
    "Content-Type":[]string{"text/css; charset=utf-8"},
    "Date":[]string{"Thu, 20 Jun 2024 12:11:58 GMT"},
    "Last-Modified":[]string{"Wed, 19 Jun 2024 20:44:53 GMT"}}
)

When using a bigger file like the index.html here https://github.com/gofiber/fiber/blob/main/.github/testdata/fs/index.html

The following headers are returned:

http.Header(
    http.Header{"Content-Encoding":[]string{"gzip"},
    "Content-Length":[]string{"228"},
    "Content-Type":[]string{"text/html; charset=utf-8"},
    "Date":[]string{"Thu, 20 Jun 2024 12:13:47 GMT"},
    "Last-Modified":[]string{"Wed, 19 Jun 2024 20:44:53 GMT"}}
)

Is this intended, or should fasthttp always set the Content-Encoding header even if the file is now bigger ?

ReneWerner87 commented 5 months ago

https://github.com/valyala/fasthttp/blob/497922a21ef4b314f393887e9c6147b8c3e3eda4/http.go#L1767-L1773

gaby commented 5 months ago

@erikdubbelboer Not sure if this should be better documented. Closing since it's not an issue.

gaby commented 5 months ago

@erikdubbelboer So fasthttp.FS only supports gzip/brotli, but it requires adding 2 params for it? Shouldn't it all just be "compress=True" and use the Accept-Encoding to decide which one to use?

erikdubbelboer commented 5 months ago

I'm not sure I understand. Yes it supports those if you enable them. You don't need a query param.