[go] net/http: remove Content-Encoding in writeNotModified
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling `http.ServeContent`.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 6dff7edd4f18a32144f4a14be11213b512a56b5b
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
1 file changed, 14 insertions(+), 0 deletions(-)
diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index 6caee9e..2f83358 100644
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -541,6 +541,7 @@
h := w.Header()
delete(h, "Content-Type")
delete(h, "Content-Length")
+ delete(h, "Content-Encoding")
if h.Get("Etag") != "" {
delete(h, "Last-Modified")
}
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Damien Neil.
Damien Neil uploaded patch set #2 to the change originally created by Gerrit Bot.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 6dff7edd4f18a32144f4a14be11213b512a56b5b
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
1 file changed, 14 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Patch set 2:Run-TryBot +1Code-Review +2Trust +1
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
Gerrit Bot uploaded patch set #3 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling `http.ServeContent`.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 6dff7edd4f18a32144f4a14be11213b512a56b5b
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
1 file changed, 14 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
1 comment:
Patchset:
Sure? I'm missing some context, like a bug or discussion.
A test might be nice too if this matters.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Mitar (Gerrit)
Patchset:
Sure? I'm missing some context, like a bug or discussion. […]
RFC is context. The comment at the beginning of `writeNotModified` function refers to RFC which states:
```
The server generating a 304 response MUST generate any of the
following header fields that would have been sent in a 200 (OK)
response to the same request: Cache-Control, Content-Location, Date,
ETag, Expires, and Vary.Since the goal of a 304 response is to minimize information transfer
when the recipient already has one or more cached representations, a
sender SHOULD NOT generate representation metadata other than the
above listed fields unless said metadata exists for the purpose of
guiding cache updates (e.g., Last-Modified might be useful if the
response does not have an ETag field).
```So, one more header to remove because it is unnecessary, but the API of `ServeContent` is that one should set `Content-Encoding` before calling it, if the content is encoded (e.g., compressed). But then, if content was not modified, that header should be removed.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Mitar, Brad Fitzpatrick.
Patch set 3:Trust +1
1 comment:
Patchset:
RFC is context. […]
Probably add this information to the CL description. Thanks.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Mitar, Brad Fitzpatrick.
Gerrit Bot uploaded patch set #4 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 6dff7edd4f18a32144f4a14be11213b512a56b5b
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
1 file changed, 16 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Mitar (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
2 comments:
Patchset:
Probably add this information to the CL description. Thanks.
Done
Patchset:
Updated description.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
Gerrit Bot uploaded patch set #5 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 3a97b6089c586ee8ed1913b8cfd03ffaaa29fa16
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
1 file changed, 16 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
Gerrit Bot uploaded patch set #6 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 6730b02bc7407e1d4bc024e280dd54e8dc372b6b
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 59 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Mitar (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
1 comment:
Patchset:
I have also added tests. But they are failing because there is difference between HTTP1 and HTTP2: it looks like on HTTP1 content-length header is added to "not modified" response, but it should not. Not sure why is that. That was a bug from before the test I added discovered. (There was no test for this before I could find.)
Not sure how to fix this, if somebody could point me to the place. I can also remove content-length check from tests I added and somebody else can fix it in a followup PR.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
Gerrit Bot uploaded patch set #7 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 03812c410b4d2be36ad3e88148111858ba34d64c
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 62 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Cherry Mui.
Gerrit Bot uploaded patch set #8 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: d10036ae6e89c3125198b37056a57ca6c61a3d0a
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 69 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
hopehook (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Mitar.
1 comment:
Patchset:
I have also added tests. […]
Hey, guys. I see the "Content-Length" of HTTP1 is 0 when status code is 304.
Where: `net/http/transfer.go:fixLength()`
Code:
```
if status/100 == 1 {
return 0, nil
}
switch status {
case 204, 304:
return 0, nil
}
```
RFC:
https://mirrors.nju.edu.cn/rfc/rfc7230.html#section-3.3
```
All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body. All other responses do
include a message body, although the body might be of zero length.
```
In my personal opinion, 0 or -1 has little effect.
I think you can pass the tests and add some comments for that.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Damien Neil, Mitar.
Gerrit Bot uploaded patch set #9 to this change.
The following approvals got outdated and were removed: Trust+1 by Cherry Mui, Trust+1 by Damien Neil
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 21744b96bd9534f0ab1792cd877fcd3b113bb92c
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 69 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Mitar (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Damien Neil, hopehook.
2 comments:
Patchset:
Hey, guys. I see the "Content-Length" of HTTP1 is 0 when status code is 304. […]
Tests now pass.
Patchset:
Please do another pass of reviews. There are tests now.
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Damien Neil, hopehook.
Gerrit Bot uploaded patch set #10 to this change.
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 53df6e73c44b63f351f7aeeb45cab82d706311eb
GitHub-Pull-Request: golang/go#50903
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 70 insertions(+), 0 deletions(-)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
hopehook (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Damien Neil.
Patch set 10:Run-TryBot +1
1 comment:
Patchset:
TryBots beginning. Status page: https://farmer.golang. […]
Ack
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.
Benny Siegert (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Brad Fitzpatrick, Cherry Mui, Damien Neil.
Patch set 10:Code-Review +1
Benny Siegert (Gerrit)
Benny Siegert submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/net/http/fs_test.go
Insertions: 54, Deletions: 0.
The diff is too large to show. Please review the diff.
```
net/http: remove Content-Encoding in writeNotModified
Additional header to remove if set before calling http.ServeContent.
The API of ServeContent is that one should set Content-Encoding before calling it, if the content is encoded (e.g., compressed). But then, if content has not been modified, that header should be removed, according to RFC 7232 section 4.1.
Change-Id: If51b35b7811a4dbb19de2ddb73f40c5e68fcec7e
GitHub-Last-Rev: 53df6e73c44b63f351f7aeeb45cab82d706311eb
GitHub-Pull-Request: golang/go#50903
Reviewed-on: https://go-review.googlesource.com/c/go/+/381955
Run-TryBot: hopehook <hope...@qq.com>
Reviewed-by: Damien Neil <dn...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Benny Siegert <bsie...@gmail.com>
---
M src/net/http/fs.go
M src/net/http/fs_test.go
2 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index 7a1d5f4..4f144eb 100644
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -541,6 +541,7 @@
h := w.Header()
delete(h, "Content-Type")
delete(h, "Content-Length")
+ delete(h, "Content-Encoding")
if h.Get("Etag") != "" {
delete(h, "Last-Modified")
}
diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index d627dfd..4be561c 100644
--- a/src/net/http/fs_test.go
+++ b/src/net/http/fs_test.go
@@ -564,6 +564,60 @@
}
}
+// Tests that ServeFile does not generate representation metadata when
+// file has not been modified, as per RFC 7232 section 4.1.
+func TestServeFileNotModified_h1(t *testing.T) { testServeFileNotModified(t, h1Mode) }
+func TestServeFileNotModified_h2(t *testing.T) { testServeFileNotModified(t, h2Mode) }
+func testServeFileNotModified(t *testing.T, h2 bool) {
+ defer afterTest(t)
+ cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
+ w.Header().Set("Content-Type", "application/json")
+ w.Header().Set("Content-Encoding", "foo")
+ w.Header().Set("Etag", `"123"`)
+ ServeFile(w, r, "testdata/file")
+
+ // Because the testdata is so small, it would fit in
+ // both the h1 and h2 Server's write buffers. For h1,
+ // sendfile is used, though, forcing a header flush at
+ // the io.Copy. http2 doesn't do a header flush so
+ // buffers all 11 bytes and then adds its own
+ // Content-Length. To prevent the Server's
+ // Content-Length and test ServeFile only, flush here.
+ w.(Flusher).Flush()
+ }))
+ defer cst.close()
+ req, err := NewRequest("GET", cst.ts.URL, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ req.Header.Set("If-None-Match", `"123"`)
+ resp, err := cst.c.Do(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ b, err := io.ReadAll(resp.Body)
+ resp.Body.Close()
+ if err != nil {
+ t.Fatal("reading Body:", err)
+ }
+ if len(b) != 0 {
+ t.Errorf("non-empty body")
+ }
+ if g, e := resp.StatusCode, StatusNotModified; g != e {
+ t.Errorf("status mismatch: got %d, want %d", g, e)
+ }
+ // HTTP1 transport sets ContentLength to 0.
+ if g, e1, e2 := resp.ContentLength, int64(-1), int64(0); g != e1 && g != e2 {
+ t.Errorf("Content-Length mismatch: got %d, want %d or %d", g, e1, e2)
+ }
+ if resp.Header.Get("Content-Type") != "" {
+ t.Errorf("Content-Type present, but it should not be")
+ }
+ if resp.Header.Get("Content-Encoding") != "" {
+ t.Errorf("Content-Encoding present, but it should not be")
+ }
+}
+
func TestServeIndexHtml(t *testing.T) {
defer afterTest(t)
To view, visit change 381955. To unsubscribe, or for help writing mail filters, visit settings.