second attempt at cache-control middleware #14

Closed
wants to merge 1 commit into
from

2 participants

@arohner

This patch adds a middleware for setting HTTP headers, and specifically setting the cache-control headers.

I made a first attempt at this last September, weavejester had comments on my patch, sorry it took so long. The first attempt was here: arohner@15c2817

It's been a long time since I wrote this, but I'm pretty sure it's a port of an existing middleware from pre-ring compojure.

@weavejester
Collaborator

There are a few things I'd like to change regarding the wrap-headers middleware. I'll need to have a bit of a think about it, but I don't think we want the header-options function or its singular namesake.

As you may already know, HTTP uses commas to separate multiple header values. So this:

Cache-Control: max-age=3600, must-revalidate

Is equivalent to:

Cache-Control: max-age=3600
Cache-Control: must-revalidate

Ring already supports multiple header values, so the above headers would look like this:

{"Cache-Control" ["max-age=3600" "must-revalidate"]}

That takes care of the commas; the next problem is the "=". We could change the ring.util.response/header function so that it supports map values, e.g.

(header {} "Cache-Control" {:max-age 3600, :must-revalidate true})
=> {:headers {"Cache-Control" ["max-age=3600" "must-revalidate"]}}

But I'm not quite sure what we want to do about ";", which is typically used to denote additional options for a value. Perhaps this:

(header {} "Accept-Language" [:da [:en-gb {:q 0.8}] [:en {:q 0.7}])
=> {:headers {"Accept-Language" ["da" "en-gb;q=0.8" "en;q=0.7"]}}

But I'm not sure I really like that. Perhaps a function in ring.util.response:

(header {} "Accept-Language" [:da (-> :en-gb (header-options {:q 0.8}))])
=> {:headers {"Accept-Language" ["da" "en-gb;q=0.8"]}}

I think I'll need to consider this for a little while longer. Suggestions and thoughts are welcome.

@weavejester
Collaborator

We also might want to take this discussion to the Ring Google Group.

@weavejester
Collaborator

This should probably be implemented as a third-party library first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment