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

wrap-resource should set basic response headers #21

Closed
djpowell opened this issue Oct 1, 2012 · 4 comments
Closed

wrap-resource should set basic response headers #21

djpowell opened this issue Oct 1, 2012 · 4 comments

Comments

@djpowell
Copy link
Contributor

djpowell commented Oct 1, 2012

wrap-resource doesn't make an attempt to set any response headers, such as last-modified and content-length; and wrap-file-info isn't compatible with wrap-resource unless the resource came from a file on the class-path. If the resource came from a jar file, we end up without the response headers set.

I package my app as an uber-jar, so I'd like wrap-resource to support conditional gets.

It is easy enough to get the content-length and last-modified date for a resource URL, using something like the following. This works whether the resource comes from a file or a jar file (or anywhere else for that matter)

(defn last-modified-and-content-length
  [^java.net.URL res-url]
  (let [ucon (.openConnection res-url)]
    [(.getLastModified ucon) (.getContentLengthLong ucon)]))

Could we incorporate something like this into wrap-resource?

@weavejester
Copy link
Member

This is actually one of the things that is planned for Ring 1.2.0. The new not-modified middleware will handle "304 Not Modified" responses for handlers that return a response with a "Last Modified" or "Etag" header.

The next step is to change ring.util.response/resource-response function to return a more headers, such as Last-Modified and, as you pointed out, setting the Content-Length would be a good idea as well.

@djpowell
Copy link
Contributor Author

Here's a patch:
djpowell@89033af

  • Adds Content-Length and Last-Modified to all resource-responses.
  • Fixes a problem when directories paths are detected as resources.

@weavejester
Copy link
Member

Would it be possible to open up a pull request for that? It makes keeping track of patches a little easier.

@weavejester
Copy link
Member

This has been merged in.

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

No branches or pull requests

2 participants