search

ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.75k stars 519 forks source link

`(wrap-not-modified)` is better off removing `Content-Length` header from 304 Not Modified responses #509

Open enspritz opened 1 month ago

enspritz commented 1 month ago

(wrap-not-modified) sets Content-Length is set to 0 on 304 Not Modified responses. I propose that it is better practice to completely remove the Content-Length header from those responses.

We can bench-press RFCs (now-superseded RFC 2616 §10.3.5, RFC 7230 §3.3.2, RFC 7232 §4.1 ); online discussion helps to untangle the abridged nature of those documents and elucidate its practical applications. There seems to be a rough consensus that content length should be omitted, as in 0 1.

The strongest signal for me comes from 3 places, downstream of the RFCs: Popular web server implementations like Apache HTTPD and NGINX, highly-trafficked web properties like amagooface, and intermediate services and equipment implementations like firewalls and caches. My personal experience is they will either omit Content-Length (easiest!) or more hazardously fill in the actual number of bytes ("octets") modulo a bunch of conditions I don't want to waste precious life-span agitating for.

Accord to a cursory inspection, high-traffic web properties, whose stable operations depend on getting such fine details right in the real world, uniformly omit Content-Length. For example:

curl -H 'If-Modified-Since: Tue, 1 Jan 2030 00:00:00 GMT' 'https://www.google.com/images/branding/googlelogo/2x/googlelogo_light_color_272x92dp.png'
curl -H 'If-Modified-Since: Tue, 1 Jan 2030 00:00:00 GMT' 'https://avatars.githubusercontent.com/u/317875?s=40&v=4'
curl -H 'If-Modified-Since: Tue, 1 Jan 2030 00:00:00 GMT' 'https://www.redditinc.com/assets/images/site/homepage/reddit-icon.svg'
curl -H 'If-Modified-Since: Tue, 1 Jan 2030 00:00:00 GMT' 'https://c.s-microsoft.com/static/fonts/segoe-ui/west-european/Semibold/latest.woff2'

So in terms of (wrap-not-modified), instead of adding Content-Length to the response headers at line 49: https://github.com/ring-clojure/ring/blob/f92911c26e029ef8579066e0d876dbc03b18b550/ring-core/src/ring/middleware/not_modified.clj#L46-L51

I propose a better course of action is to dissociate Content-Length from the headers. I'd submit a PR, but I didn't note any established precedent in the code base for the removal of a header based on its name (HTTP nomenclature) being either a Clojure keyword or a case-insensitive string, so I'm not sure how you'd like it done.

weavejester commented 1 month ago

That seems reasonable. The header removal can be achieved via find-header. A private function could be added to the not-modified namespace to remove the header:

(defn- remove-header [response header]
  (dissoc-in response [:headers (key (find-header response header))]))

This might be also useful generally, but better to not change the API when making a fix.