search

tower-rs / tower-http

HTTP specific Tower utilities.
680 stars 159 forks source link

Vary header in compression #399

Closed Dragonink closed 7 months ago

Dragonink commented 1 year ago

Motivation

From the "Compression in HTTP" MDN page:

As content negotiation has been used to choose a representation based on its encoding, the server must send a Vary header containing at least Accept-Encoding alongside this header in the response; that way, caches will be able to cache the different representations of the resource.

Solution

We just append Accept-Encoding to the Vary header when constructing the compression response.

jplatte commented 1 year ago

I wonder if we should also send the header in some of the conditions where we early return without a content-encoding, to indicate to caches that a different response body is available when accept-encoding is set. Wdyt?

Dragonink commented 1 year ago

So we always send the Vary header whatever the body will be? You're right, we should do this. I will fix that.

I have a question about branches: why has the v0.4.x branch (and the 0.4.3 version) diverged from master? So what base branch should I target with this PR? master or v0.4.x?

jplatte commented 1 year ago

So we always send the Vary header whatever the body will be? You're right, we should do this. I will fix that.

No, not in every case. If should_compress is false, the body will never be compressed no matter which headers the client sends AFAIU. However, for Encoding::Identity, the body was not compressed specifically of what headers the client sent (again, AFAIU).

I have a question about branches: why has the v0.4.x branch (and the 0.4.3 version) diverged from master? So what base branch should I target with this PR? master or v0.4.x?

It's okay to make PRs against either. I guess if you want this PR to be released (soon-ish) as part of 0.4.x, it makes sense to make the PR against that. There's (so far) only a tiny difference between the branches, so it will be trivial to cherry-pick changes such that they affect both branches.

Dragonink commented 1 year ago

So we should always send the Vary except when should_compress is false?

jplatte commented 1 year ago

No, I think there's another early-return branch (body already compressed, i.e. content-encoding already set) in which the middleware should essentially do nothing.

cc @davidpdrsn @Nehliin wdyt about when we should set vary?

Dragonink commented 1 year ago

The Content-Encoding already set is tested in should_compress:

let should_compress = !res.headers().contains_key(header::CONTENT_ENCODING)
    && self.predicate.should_compress(&res);

Thus if I guard the Vary header with an if should_compress, the header will be added unless the response is already compressed or the response is never be compressed.

jplatte commented 11 months ago

Ping @davidpdrsn @Nehliin any opinions about this PR / the question of when vary should be sent?

Also @Dragonink have you looked into the test failure? (MSRV job failing was fixed in #418)

Nehliin commented 10 months ago

sorry won't have time to review in the near future