Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto add 'Vary' header after compression #1585

Merged
merged 4 commits into from
Jul 2, 2023

Conversation

AutumnSun1996
Copy link
Contributor

Add config SetAddVaryHeaderForCompression to enable 'Vary: Accept-Encoding' header when compression is used.

Added a config variable addVaryHeaderForCompress to control this feature, with setter SetAddVaryHeaderForCompression.
To remain backward compatible, it's false by default.
When enabled, 'Vary: Accept-Encoding' header will be added when compression is used.

fix #1583

Add config `SetAddVaryHeaderForCompression` to enable
'Vary: Accept-Encoding' header when compression is used.
@erikdubbelboer
Copy link
Collaborator

I have thought about it and I think it's better to remove SetAddVaryHeaderForCompression and always set the Vary header. We also always set Content-Encoding. Can you change that, then I can merge it. Thanks.

@AutumnSun1996
Copy link
Contributor Author

Hi, the code is updated, please review again. Thanks.

http.go Outdated
@@ -1723,6 +1723,7 @@ func (resp *Response) brotliBody(level int) error {
resp.bodyRaw = nil
}
resp.Header.SetContentEncodingBytes(strBr)
resp.Header.Set("Vary", "Accept-Encoding")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm thinking now it's probably better to use this. In case the user already set the Vary header before themselves.

	if v := resp.Header.Peek(HeaderVary); !bytes.Contains(v, strAcceptEncoding) {
		resp.Header.SetBytesV(HeaderVary, append(v, (","+HeaderAcceptEncoding)...))
	} else {
		resp.Header.Add(HeaderVary, HeaderAcceptEncoding)
	}

@AutumnSun1996
Copy link
Contributor Author

See what you mean. But this leads to another question: should we also handle the case where the user has added 'Vary: Accept-Encoding'?

@erikdubbelboer
Copy link
Collaborator

That's exactly what my example does.

@AutumnSun1996
Copy link
Contributor Author

oh sorry, I just missed the second check. going to update it now

@AutumnSun1996
Copy link
Contributor Author

AutumnSun1996 commented Jul 1, 2023

I created such a function to check and update the Header, instead of copy-paste. any more comments?

// AddVaryBytes add value to the 'Vary' header if it's not included
func (h *ResponseHeader) AddVaryBytes(value []byte) 

header.go Outdated
@@ -344,6 +344,18 @@ func (h *ResponseHeader) SetContentEncodingBytes(contentEncoding []byte) {
h.contentEncoding = append(h.contentEncoding[:0], contentEncoding...)
}

// AddVaryBytes add value to the 'Vary' header if it's not included
func (h *ResponseHeader) AddVaryBytes(value []byte) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a specific function and our API surface is already huge. Lets not export this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts was, it can also be used to add other values into 'Vary' header in application code.
Any way, it's ok to me to change it to 'addVaryBytes'

@erikdubbelboer erikdubbelboer merged commit 0d0bbfe into valyala:master Jul 2, 2023
14 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should add 'Vary: Accept-Encoding' header when compress is used
2 participants