Compress with empty content

[lammel]

This is a follow-up for PR #2044.

In RFC9110 is stated:

A sender that generates a message containing content SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender. If a Content-Type header field is not present, the recipient MAY either assume a media type of "application/octet-stream" ([RFC2046], Section 4.5.1) or examine the data to determine its type.

So this PR adds a check for the response size to ensure no Content-Type header is set if no payload is set.

[codecov]

Codecov Report

Base: 92.69% // Head: 92.69% // No change to project coverage 👍

Coverage data is based on head (bc9d1a2) compared to base (135c511).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2355   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files          37       37           
  Lines        4462     4462           
=======================================
  Hits         4136     4136           
  Misses        238      238           
  Partials       88       88           

Impacted Files	Coverage Δ
middleware/compress.go	81.15% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lammel]

@aldas This looks to be an easy fix. Not sure if streaming responses are negatively affected as they also should set the header when a write occured unless it's zero sized.

[aldas]

I do not think this is actually working. Seems that it is something to do with response.gw.header which are somehow disconnected from res.Header().Del(echo.HeaderContentEncoding)

Probably this header is not uptodate with handlerHeader https://github.com/golang/go/blob/dfd13ce59d74638baff4f94d64851cda53e63bdc/src/net/http/server.go#L1171

Try this example:

func main() {
	e := echo.New()

	e.Use(middleware.Gzip())
	e.GET("/", func(c echo.Context) error {
		return c.String(http.StatusOK, c.QueryParam("test"))
	})

	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		log.Fatal(err)
	}
}

x@x:~/code/$ curl -v --compressed -H "Accept-Encoding: gzip" "http://localhost:8080?test="
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /?test= HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.81.0
> Accept: */*
> Accept-Encoding: gzip
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Encoding: gzip
< Content-Type: text/plain; charset=UTF-8
< Vary: Accept-Encoding
< Date: Tue, 06 Dec 2022 19:08:23 GMT
< Content-Length: 10
< 
* Connection #0 to host localhost left intact

that < Content-Encoding: gzip is still there when we send empty response.

[aldas]

I think mutating Headers in that block does not work as by that point the response has already been written to the client. In unit-tests it might be working but unittests do not consider what was actually written to the wire. Cleanup at that point is ok but there is no point to mess with headers as that is too late.

it might work if we could intercept func (r *Response) WriteHeader(code int) { before func (r *Response) Write(b []byte) (n int, err error) { call and check if that b is empty. This is because WriteHeader will cause headers to be "cloned" for that chunkedwriter and afterthat our mutations do not reach chunkedwriter and cw headers are those that are written to the client.

It might be that this "feature" is not worth fixing as RFC does not say that it must not be there. it says that it should be there for X conditions

[lammel]

it might work if we could intercept func (r *Response) WriteHeader(code int) { before func (r *Response) Write(b []byte) (n int, err error) { call and check if that b is empty. This is because WriteHeader will cause headers to be "cloned" for that chunkedwriter and afterthat our mutations do not reach chunkedwriter and cw headers are those that are written to the client.

It might be that this "feature" is not worth fixing as RFC does not say that it must not be there. it says that it should be there for X conditions

Due to the added complexity I think we should close this PR for now and revisit the topic when additional usecases come up, that require to intercept the WriteHeader code.

So let's call this one for now and live in the gray zone of internet specs for a while. No harm is done if the header is sent anyway.