Merge lp:~rharding/convoy/add_listmod_header into lp:convoy

[Richard Harding]

Add a last-modified header to the response so browsers/web servers can help prevent sending unchanged files.

= Summary =
Convoy should return a last-modified header to help allow browsers in sending 304 responses to the same combo files already stored.

== Proposed Fix ==
Track the mtime of the files as we process them and return that as a Last-Modified header in the response.

== Implementation Details ==
This was a bit intrusive. Since the response is really just the looping through the files I had to pull out the list of files in order to find the latest mtime. This means we add an additional loop of the files to the overall process.

The tests then had to be updated since the combine_files now expexts full paths vs the partials. Each of the tests either had to update the file paths themself, or I just used the new build_filelist to do it.

== Tests ==
- Updated the tests to call build_filelist
- Added the Last-Modified to the header checks
- Added a check to verify the Last-Header is correct.

[Robert Collins (lifeless)]

Personally I wouldn't send a last-mod, if you send a hash of the
content of the files - a strong validator - browsers will send an
if-none-match conditional, and you *won't* have to deal with the
fallout of fs datestamp skew (which would cause headache for LP, for
instance).

-Rob

Unmerged revisions

22. By Richard Harding on 2012-02-23

Garden

21. By Richard Harding on 2012-02-23

Update convoy to supply a last-modified header

=== modified file 'convoy/combo.py'
--- convoy/combo.py	2012-01-27 03:37:59 +0000
+++ convoy/combo.py	2012-02-23 18:57:20 +0000
@@ -19,7 +19,7 @@
 import os
 import re
 import urlparse
-
+from datetime import datetime
 
 CHUNK_SIZE = 2 << 12
 URL_RE = re.compile("url\([ \"\']*([^ \"\']+)[ \"\']*\)")
@@ -62,6 +62,26 @@
     return tuple([param for param, value in params])
 
 
+def build_filelist(root, fnames):
+    """Build a list of full paths and return that list with last modified time
+
+    :return: (files, mtime)
+
+    """
+    mtime = 0
+    files = []
+    for fname in fnames:
+        full = os.path.abspath(os.path.join(root, fname))
+        files.append(full)
+
+        # update the last modified time to the latest file
+        if os.path.isfile(full):
+            mod = os.path.getmtime(full)
+            mtime = mod if mod > mtime else mtime
+
+    return (files, mtime)
+
+
 def combine_files(fnames, root, resource_prefix="", rewrite_urls=True):
     """Combine many files into one.
 
@@ -71,18 +91,18 @@
     """
 
     resource_prefix = resource_prefix.rstrip("/")
-    for fname in fnames:
-        file_ext = os.path.splitext(fname)[-1]
-        basename = os.path.basename(fname)
-        full = os.path.abspath(os.path.join(root, fname))
-        yield "/* " + fname + " */\n"
+    comment_cleanup = root if root.endswith('/') else root + '/'
+    for full in fnames:
+        yield "/* " + full.replace(comment_cleanup, '') + " */\n"
         if not full.startswith(root) or not os.path.exists(full):
             yield "/* [missing] */\n"
         else:
             with open(full, "r") as f:
+                file_ext = os.path.splitext(full)[-1]
                 if file_ext == ".css" and rewrite_urls:
                     file_content = f.read()
                     src_dir = os.path.dirname(full)
+                    basename = os.path.basename(full)
                     relative_parts = relative_path(
                         os.path.join(root, basename), src_dir).split(
                         os.path.sep)
@@ -134,11 +154,16 @@
         else:
             start_response("404 Not Found", [("Content-Type", content_type)])
             return ("Not Found",)
-        start_response("200 OK", [("Content-Type", content_type)])
-
         # take any prefix in the url route into consideration for the root to
         # find files at
         updated_root = os.path.join(root, path_hint)
-        return combine_files(fnames, updated_root, resource_prefix,
+
+        files, mtime = build_filelist(updated_root, fnames)
+        start_response("200 OK", [
+            ("Content-Type", content_type),
+            ("Last-Modified", datetime.ctime(datetime.fromtimestamp(mtime))),
+            ])
+
+        return combine_files(files, updated_root, resource_prefix,
                              rewrite_urls=rewrite_urls)
     return app

=== modified file 'convoy/tests/test_combo.py'
--- convoy/tests/test_combo.py	2012-01-27 03:37:59 +0000
+++ convoy/tests/test_combo.py	2012-02-23 18:57:20 +0000
@@ -18,11 +18,13 @@
 import os
 import difflib
 import textwrap
+from datetime import datetime
 from unittest import defaultTestLoader
 
 import mocker
 from paste.fixture import TestApp
 
+from convoy.combo import build_filelist
 from convoy.combo import combo_app
 from convoy.combo import combine_files
 from convoy.combo import parse_path_hint
@@ -94,11 +96,14 @@
                               "** oop-min **",
                               "/* event-custom/event-custom-min.js */",
                               "** event-custom-min **"))
+
+        testfiles, mtime = build_filelist(test_dir, [
+            "yui/yui-min.js",
+            "oop/oop-min.js",
+            "event-custom/event-custom-min.js"])
+
         self.assertEquals(
-            "".join(combine_files(["yui/yui-min.js",
-                                   "oop/oop-min.js",
-                                   "event-custom/event-custom-min.js"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)).strip(),
             expected)
 
     def test_combine_css_makes_relative_path(self):
@@ -140,10 +145,12 @@
                background: url(editor/assets/skins/sam/img/bg.png);
             }
             """
+        testfiles, mtime = build_filelist(test_dir, [
+           "widget/assets/skins/sam/widget.css",
+           "editor/assets/skins/sam/editor.css"])
+
         self.assertTextEquals(
-            "".join(combine_files(["widget/assets/skins/sam/widget.css",
-                                   "editor/assets/skins/sam/editor.css"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)).strip(),
             expected)
 
 
@@ -186,12 +193,76 @@
                background: url("http://foo/static/img/bg.png");
             }
             """
+        testfiles, mtime = build_filelist(test_dir, [
+           "widget/assets/skins/sam/widget.css",
+           "editor/assets/skins/sam/editor.css"])
+
         self.assertTextEquals(
-            "".join(combine_files(["widget/assets/skins/sam/widget.css",
-                                   "editor/assets/skins/sam/editor.css"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)).strip(),
             expected)
 
+
+    def test_file_builder_missing(self):
+        """
+        Given missing files, make sure our mtime is 0 and that the full paths
+        are based off the root we passed in.
+        """
+        test_dir = self.makeDir()
+        test_files = [
+            'widget/assets/skins/sam/widget.css',
+            'widget/assets/skins/sam/editor.css',
+        ]
+
+        files, mtime = build_filelist(test_dir, test_files)
+        self.assertEquals(0, mtime,
+            "mtime doesn't get updated for missing files")
+        self.assertEquals(files[0],
+                os.path.join(test_dir, test_files[0]))
+
+
+    def test_file_builder(self):
+        """
+        Given a list of filenames we should build a fullpath list and modified
+        time
+        """
+        test_dir = self.makeDir()
+        init_time = datetime.ctime(datetime.now())
+
+        self.makeSampleFile(
+            test_dir,
+            os.path.join("widget", "assets", "skins", "sam", "widget.css"),
+            """\
+            /* widget skin */
+            .yui-widget {
+               background: url("data:image/gif;base64,base64-data");
+            }
+            """)
+
+        self.makeSampleFile(
+            test_dir,
+            os.path.join("editor", "assets", "skins", "sam", "editor.css"),
+            """\
+            /* editor skin */
+            .yui-editor {
+               background: url("data:image/gif;base64,base64-data");
+            }
+            """)
+
+        test_files = [
+            "widget/assets/skins/sam/widget.css",
+            "editor/assets/skins/sam/editor.css",
+        ]
+
+        files, mtime = build_filelist(test_dir, test_files)
+
+        self.assertEquals(
+            os.path.join(test_dir, test_files[0]),
+            files[0])
+        self.assertEquals(files[1],
+                os.path.join(test_dir, test_files[1]))
+        self.assertNotEqual(0, mtime)
+
+
     def test_combine_css_leaves_data_uris_untouched(self):
         """
         Combining CSS files does not touch data uris in
@@ -231,10 +302,12 @@
                background: url("data:image/gif;base64,base64-data");
             }
             """
+        testfiles, mtime = build_filelist(test_dir, [
+           "widget/assets/skins/sam/widget.css",
+           "editor/assets/skins/sam/editor.css"])
+
         self.assertTextEquals(
-            "".join(combine_files(["widget/assets/skins/sam/widget.css",
-                                   "editor/assets/skins/sam/editor.css"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)).strip(),
             expected)
 
     def test_missing_file_is_ignored(self):
@@ -256,11 +329,14 @@
                               "/* [missing] */",
                               "/* event-custom/event-custom-min.js */",
                               "** event-custom-min **"))
+
+        testfiles, mtime = build_filelist(test_dir, [
+            "yui/yui-min.js",
+            "oop/oop-min.js",
+            "event-custom/event-custom-min.js"])
+
         self.assertEquals(
-            "".join(combine_files(["yui/yui-min.js",
-                                   "oop/oop-min.js",
-                                   "event-custom/event-custom-min.js"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)).strip(),
             expected)
 
     def test_no_parent_hack(self):
@@ -360,9 +436,12 @@
                background: url(/static/editor/assets/skins/sam/img/bg.png);
             }
             """
+        testfiles, mtime = build_filelist(test_dir, [
+           "widget/assets/skins/sam/widget.css",
+           "editor/assets/skins/sam/editor.css"])
+
         self.assertTextEquals(
-            "".join(combine_files(["widget/assets/skins/sam/widget.css",
-                                   "editor/assets/skins/sam/editor.css"],
+            "".join(combine_files(testfiles,
                                   root=test_dir,
                                   resource_prefix="/static/")).strip(),
             expected)
@@ -381,9 +460,9 @@
             /* yui/base/base.css */
             .foo{background-image:url(img.png)}
             """
+        testfiles, mtime = build_filelist(test_dir, ["yui/base/base.css"])
         self.assertTextEquals(
-            "".join(combine_files(["yui/base/base.css"],
-                                  root=test_dir)).strip(),
+            "".join(combine_files(testfiles, root=test_dir)),
             expected)
 
 
@@ -419,11 +498,12 @@
             ["yui/yui-min.js",
              "oop/oop-min.js",
              "event-custom/event-custom-min.js"]), status=200)
-        self.assertEquals(res.headers, [("Content-Type", "text/javascript")])
+        self.assertEquals(res.headers[0], ("Content-Type", "text/javascript"))
         self.assertEquals(res.body.strip(), expected)
 
     def test_combo_app_sets_content_type_for_css(self):
         """The WSGI App should set a proper Content-Type for CSS."""
+        init_time = datetime.ctime(datetime.now())
         self.makeSampleFile(
             self.root,
             os.path.join("widget", "skin", "sam", "widget.css"),
@@ -433,7 +513,10 @@
 
         res = self.app.get("/?" + "&".join(
             ["widget/skin/sam/widget.css"]), status=200)
-        self.assertEquals(res.headers, [("Content-Type", "text/css")])
+        self.assertEquals(res.headers[0], ("Content-Type", "text/css"))
+        self.assertEqual(res.headers[1][0], 'Last-Modified')
+        self.assertEqual(res.headers[1][1], init_time, (res.headers[1][1],
+            init_time))
         self.assertEquals(res.body.strip(), expected)
 
     def test_no_filename_gives_404(self):