Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Reduce complexity of StaticFileHandler.get() #476

Merged
merged 4 commits into from

2 participants

@birknilson

This commit ensures StaticFileHandler.get() will delegate the handling of setting the header and body response to two new methods. This reduces the complexity of the initial method, but also eases subclassing of the class and the handling of how the GET response should be generated. The commit passes all the available tests.

My reason for doing this is mainly to make it easier to change how the response body is set. In a project where I use Mustache templates I need to retrieve a static resource called templates.json which will contain the contents of multiple templates which will be JSON encoded and allow me to utilize Mustache templates on the client-side without additional HTTP requests.

@bdarnell
Owner

Looks good, although I'm worried about the proliferation of method signatures. The existing methods all take path, but get_content takes an abspath and set_headers takes both. For consistency's sake I think it would be better to specify all methods in terms of path, since abspath is a function of path (we could store the abspath in an attribute so it doesn't need to be recomputed every time).

@birknilson birknilson Cache the computed absolute path to the requested resource in
StaticFileHandler. The cache is cleared on_finish of each request
in order to ensure recomputation on each request.
38cccf8
@birknilson

I agree @bdarnell and started out with storing the absolute path in an attribute, but dropped it since the original implementation handled the cleanup in a way I found uglier than passing it to the two mentioned methods. However, looking at it again I think the solution suggested in commit 38cccf8 is a nice one. It passes all the available tests and I tried using it with some existing tornado applications of mine and it works as intended.

@bdarnell bdarnell merged commit 38cccf8 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 9, 2012
  1. @birknilson

    Introduce StaticFileHandler.(get_content|set_headers) which reduces the

    birknilson authored
    complexity of the HTTP GET handler of static resources.
  2. @birknilson
Commits on Mar 11, 2012
  1. @birknilson
Commits on Mar 26, 2012
  1. @birknilson

    Cache the computed absolute path to the requested resource in

    birknilson authored
    StaticFileHandler. The cache is cleared on_finish of each request
    in order to ensure recomputation on each request.
This page is out of date. Refresh to see the latest.
Showing with 56 additions and 10 deletions.
  1. +56 −10 tornado/web.py
View
66 tornado/web.py
@@ -1489,6 +1489,43 @@ def head(self, path):
def get(self, path, include_body=True):
path = self.parse_url_path(path)
+ if not self.set_headers(path):
+ return
+
+ body = self.get_content(path)
+ if not body:
+ return
+
+ hasher = hashlib.sha1()
+ hasher.update(body)
+ self.set_header("Etag", '"%s"' % hasher.hexdigest())
+ if include_body:
+ self.write(body)
+ else:
+ assert self.request.method == "HEAD"
+ self.set_header("Content-Length", len(body))
+
+ def on_finish(self):
+ # Cleanup the cached computation of the absolute path associated with
+ # the requested resource. This is crucial in order to ensure accurate
+ # responses in upcoming requests which would otherwise utilize the same
+ # absolute path as the initial one - which would be chaotic.
+ self._abspath = None
+
+ def get_absolute_path(self, path):
+ """Retrieve the absolute path on the filesystem where the resource
+ corresponding to the given URL ``path`` can be found.
+
+ This method also handles the validation of the given path and ensures
+ resources outside of the static directory cannot be accessed.
+ """
+ # In case the ``_abspath`` attribute exists and contains a value
+ # other than None the abspath has already been computed and verified.
+ # It can be returned instantly in order to avoid recomputation.
+ abspath = getattr(self, '_abspath', None)
+ if abspath is not None:
+ return abspath
+
abspath = os.path.abspath(os.path.join(self.root, path))
# os.path.abspath strips a trailing /
# it needs to be temporarily added back for requests to root/
@@ -1507,6 +1544,15 @@ def get(self, path, include_body=True):
if not os.path.isfile(abspath):
raise HTTPError(403, "%s is not a file", path)
+ self._abspath = abspath
+ return abspath
+
+ def set_headers(self, path):
+ """Set the response headers in order to ensure that client browsers
+ will cache the requested resource and not proceed to retrieve its content
+ in the case of a 304 response.
+ """
+ abspath = self.get_absolute_path(path)
stat_result = os.stat(abspath)
modified = datetime.datetime.fromtimestamp(stat_result[stat.ST_MTIME])
@@ -1535,18 +1581,18 @@ def get(self, path, include_body=True):
if_since = datetime.datetime.fromtimestamp(time.mktime(date_tuple))
if if_since >= modified:
self.set_status(304)
- return
+ return False
+ return True
+
+ def get_content(self, path):
+ """Retrieve the content of the requested resource which is located
+ at the given absolute ``path``.
+ """
+ abspath = self.get_absolute_path(path)
with open(abspath, "rb") as file:
- data = file.read()
- hasher = hashlib.sha1()
- hasher.update(data)
- self.set_header("Etag", '"%s"' % hasher.hexdigest())
- if include_body:
- self.write(data)
- else:
- assert self.request.method == "HEAD"
- self.set_header("Content-Length", len(data))
+ return file.read()
+ return None
def set_extra_headers(self, path):
"""For subclass to add extra headers to the response"""
Something went wrong with that request. Please try again.