Merge ~cjwatson/launchpad:scripts-utilities-js-future-imports into launchpad:master

[Colin Watson]

Convert lp.scripts.utilities.js to preferred __future__ imports

Also remove the unused combo_app.

This requires care because of WSGI's requirement for native strings in some places.

eb5bd07... by Colin Watson on 2019-12-31

Remove lp.scripts.utilities.js.combo.combo_app

It was unused; we use convoy for this instead.

[Colin Watson (cjwatson)]

At William's suggestion, I removed combo_app entirely, since it's unused. We might as well keep the rest of this port, though; I think the various type changes are still correct, and wsgi_native_string's new home is somewhat more appropriate.

[William Grant (wgrant)]

review: Approve (code)

diff --git a/lib/lp/scripts/utilities/js/combinecss.py b/lib/lp/scripts/utilities/js/combinecss.py
index 2047d25..1aa1072 100755
--- a/lib/lp/scripts/utilities/js/combinecss.py
+++ b/lib/lp/scripts/utilities/js/combinecss.py
@@ -1,6 +1,8 @@
 # Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 import os
 
 import scandir
diff --git a/lib/lp/scripts/utilities/js/combo.py b/lib/lp/scripts/utilities/js/combo.py
index 4d27293..f568bc2 100644
--- a/lib/lp/scripts/utilities/js/combo.py
+++ b/lib/lp/scripts/utilities/js/combo.py
@@ -1,6 +1,8 @@
 # Copyright 2011-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
 import os
@@ -8,7 +10,7 @@ import urlparse
 
 from six.moves.urllib.parse import parse_qsl
 
-from jsbuild import (
+from lp.scripts.utilities.js.jsbuild import (
     CSSComboFile,
     JSComboFile,
     )
@@ -32,7 +34,7 @@ def parse_qs(query):
     return tuple([param for param, value in params])
 
 
-def combine_files(fnames, root, resource_prefix="",
+def combine_files(fnames, root, resource_prefix=b"",
                   minify_css=True, rewrite_urls=True):
     """Combine many files into one.
 
@@ -56,33 +58,6 @@ def combine_files(fnames, root, resource_prefix="",
         if not full.startswith(root) or not os.path.exists(full):
             yield combo.get_comment("[missing]")
         else:
-            f = open(full, "r")
-            content = f.read()
-            f.close()
+            with open(full, "rb") as f:
+                content = f.read()
             yield combo.filter_file_content(content, full)
-
-
-def combo_app(root, resource_prefix="", minify_css=True, rewrite_urls=True):
-    """A simple YUI Combo Service WSGI app.
-
-    Serves any files under C{root}, setting an appropriate
-    C{Content-Type} header.
-    """
-    root = os.path.abspath(root)
-
-    def app(environ, start_response, root=root):
-        fnames = parse_qs(environ["QUERY_STRING"])
-        content_type = "text/plain"
-        if fnames:
-            if fnames[0].endswith(".js"):
-                content_type = "text/javascript"
-            elif fnames[0].endswith(".css"):
-                content_type = "text/css"
-        else:
-            start_response("404 Not Found", [("Content-Type", content_type)])
-            return ("Not Found",)
-        start_response("200 OK", [("Content-Type", content_type)])
-        return combine_files(fnames, root, resource_prefix,
-                             minify_css, rewrite_urls)
-
-    return app
diff --git a/lib/lp/scripts/utilities/js/jsbuild.py b/lib/lp/scripts/utilities/js/jsbuild.py
index 5499ea7..14841c0 100644
--- a/lib/lp/scripts/utilities/js/jsbuild.py
+++ b/lib/lp/scripts/utilities/js/jsbuild.py
@@ -3,6 +3,8 @@
 
 """build.py - Minifies and creates the JS build directory."""
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 __all__ = [
     'CSSComboFile',
@@ -26,9 +28,9 @@ BUILD_DIR = os.path.normpath(os.path.join(
 DEFAULT_SRC_DIR = os.path.normpath(os.path.join(
     HERE, os.pardir, os.pardir, os.pardir, 'app', 'javascript'))
 
-ESCAPE_STAR_PROPERTY_RE = re.compile(r'\*([a-zA-Z0-9_-]+):')
-UNESCAPE_STAR_PROPERTY_RE = re.compile(r'([a-zA-Z0-9_-]+)_ie_star_hack:')
-URL_RE = re.compile("url\([ \"\']*([^ \"\']+)[ \"\']*\)")
+ESCAPE_STAR_PROPERTY_RE = re.compile(br'\*([a-zA-Z0-9_-]+):')
+UNESCAPE_STAR_PROPERTY_RE = re.compile(br'([a-zA-Z0-9_-]+)_ie_star_hack:')
+URL_RE = re.compile(br"url\([ \"\']*([^ \"\']+)[ \"\']*\)")
 
 
 def relative_path(from_file, to_file):
@@ -77,37 +79,33 @@ class ComboFile:
 
     def update(self):
         """Update the file from its source files."""
-        target_fh = open(self.target_file, 'w')
-        try:
+        with open(self.target_file, 'wb') as target_fh:
             for src_file in self.src_files:
                 self.log("Processing '%s'" % os.path.basename(src_file))
                 target_fh.write(self.get_file_header(src_file))
-                fh = open(src_file)
-                content = fh.read()
-                fh.close()
+                with open(src_file, 'rb') as fh:
+                    content = fh.read()
                 try:
                     target_fh.write(
                         self.filter_file_content(content, src_file))
                 except Exception:
                     os.remove(self.target_file)
                     raise
-        finally:
-            target_fh.close()
 
     def get_comment(self, msg):
-        """Return a string wrapped in a comment to be include in the output.
+        """Return a byte string wrapped in a comment to include in the output.
 
         Can be used to help annotate the output file.
         """
-        return ''
+        return b''
 
     def get_file_header(self, path):
-        """Return a string to include before outputting a file.
+        """Return a byte string to include before outputting a file.
 
         Can be used by subclasses to output a file reference in the combined
         file. Default implementation returns nothing.
         """
-        return ''
+        return b''
 
     def filter_file_content(self, file_content, path):
         """Hook to process the file content before being combined."""
@@ -122,13 +120,13 @@ class JSComboFile(ComboFile):
     """
 
     def get_comment(self, msg):
-        return "// %s\n" % msg
+        return b"// %s\n" % msg.encode("UTF-8")
 
     def get_file_header(self, path):
         return self.get_comment(relative_path(self.target_file, path))
 
     def filter_file_content(self, file_content, path):
-        return file_content + '\n'
+        return file_content + b'\n'
 
 
 class CSSComboFile(ComboFile):
@@ -138,15 +136,15 @@ class CSSComboFile(ComboFile):
     to the new location, and minify the result.
     """
 
-    def __init__(self, src_files, target_file, resource_prefix="",
+    def __init__(self, src_files, target_file, resource_prefix=b"",
                  minify=True, rewrite_urls=True):
         super(CSSComboFile, self).__init__(src_files, target_file)
-        self.resource_prefix = resource_prefix.rstrip("/")
+        self.resource_prefix = resource_prefix.rstrip(b"/")
         self.minify = minify
         self.rewrite_urls = rewrite_urls
 
     def get_comment(self, msg):
-        return "/* %s */\n" % msg
+        return b"/* %s */\n" % msg.encode("UTF-8")
 
     def get_file_header(self, path):
         return self.get_comment(relative_path(self.target_file, path))
@@ -164,18 +162,18 @@ class CSSComboFile(ComboFile):
             def fix_relative_url(match):
                 url = match.group(1)
                 # Don't modify absolute URLs or 'data:' urls.
-                if (url.startswith("http") or
-                    url.startswith("/") or
-                    url.startswith("data:")):
+                if (url.startswith(b"http") or
+                        url.startswith(b"/") or
+                        url.startswith(b"data:")):
                     return match.group(0)
                 parts = relative_parts + url.split("/")
                 result = []
                 for part in parts:
-                    if part == ".." and result and result[-1] != "..":
+                    if part == b".." and result and result[-1] != b"..":
                         result.pop(-1)
                         continue
                     result.append(part)
-                return "url(%s)" % "/".join(
+                return b"url(%s)" % b"/".join(
                     filter(None, [self.resource_prefix] + result))
             file_content = URL_RE.sub(fix_relative_url, file_content)
 
@@ -186,16 +184,16 @@ class CSSComboFile(ComboFile):
                 cssutils.ser.prefs.useMinified()
 
                 stylesheet = ESCAPE_STAR_PROPERTY_RE.sub(
-                    r'\1_ie_star_hack:', file_content)
+                    br'\1_ie_star_hack:', file_content)
                 settings.set('DXImageTransform.Microsoft', True)
                 parser = cssutils.CSSParser(raiseExceptions=True)
                 css = parser.parseString(stylesheet)
                 stylesheet = UNESCAPE_STAR_PROPERTY_RE.sub(
-                    r'*\1:', css.cssText)
-                return stylesheet + "\n"
+                    br'*\1:', css.cssText)
+                return stylesheet + b"\n"
             finally:
                 cssutils.setSerializer(old_serializer)
-        return file_content + "\n"
+        return file_content + b"\n"
 
 
 class Builder:
diff --git a/lib/lp/scripts/utilities/js/tests/test_combo.py b/lib/lp/scripts/utilities/js/tests/test_combo.py
index b1c8bb7..c745a6c 100644
--- a/lib/lp/scripts/utilities/js/tests/test_combo.py
+++ b/lib/lp/scripts/utilities/js/tests/test_combo.py
@@ -1,17 +1,16 @@
 # Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
 import os
 import shutil
 import tempfile
 
-from paste.fixture import TestApp
-
 from lp.scripts.utilities.js.combo import (
     combine_files,
-    combo_app,
     parse_url,
     )
 from lp.testing import TestCase
@@ -435,7 +434,7 @@ class TestCombo(ComboTestBase):
             "".join(combine_files(["widget/assets/skins/sam/widget.css",
                                    "editor/assets/skins/sam/editor.css"],
                                   root=test_dir,
-                                  resource_prefix="/static/")).strip(),
+                                  resource_prefix=b"/static/")).strip(),
             expected)
 
     def test_missing_file_is_ignored(self):
@@ -513,60 +512,3 @@ class TestCombo(ComboTestBase):
         self.assertEqual(
             "".join(combine_files([hack], root=test_dir)).strip(),
             expected)
-
-
-class TestWSGICombo(ComboTestBase):
-
-    def setUp(self):
-        super(TestWSGICombo, self).setUp()
-        self.root = self.makeDir()
-        self.app = TestApp(combo_app(self.root))
-
-    def test_combo_app_sets_content_type_for_js(self):
-        """The WSGI App should set a proper Content-Type for Javascript."""
-        self.makeSampleFile(
-            self.root,
-            os.path.join("yui", "yui-min.js"),
-            "** yui-min **")
-        self.makeSampleFile(
-            self.root,
-            os.path.join("oop", "oop-min.js"),
-            "** oop-min **")
-        self.makeSampleFile(
-            self.root,
-            os.path.join("event-custom", "event-custom-min.js"),
-            "** event-custom-min **")
-
-        expected = "\n".join(("// yui/yui-min.js",
-                              "** yui-min **",
-                              "// oop/oop-min.js",
-                              "** oop-min **",
-                              "// event-custom/event-custom-min.js",
-                              "** event-custom-min **"))
-
-        res = self.app.get("/?" + "&".join(
-            ["yui/yui-min.js",
-             "oop/oop-min.js",
-             "event-custom/event-custom-min.js"]), status=200)
-        self.assertEqual(res.headers, [("Content-Type", "text/javascript")])
-        self.assertEqual(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."""
-        self.makeSampleFile(
-            self.root,
-            os.path.join("widget", "skin", "sam", "widget.css"),
-            "/* widget-skin-sam */")
-
-        expected = "/* widget/skin/sam/widget.css */"
-
-        res = self.app.get("/?" + "&".join(
-            ["widget/skin/sam/widget.css"]), status=200)
-        self.assertEqual(res.headers, [("Content-Type", "text/css")])
-        self.assertEqual(res.body.strip(), expected)
-
-    def test_no_filename_gives_404(self):
-        """If no filename is included, a 404 should be returned."""
-        res = self.app.get("/", status=404)
-        self.assertEqual(res.headers, [("Content-Type", "text/plain")])
-        self.assertEqual(res.body, "Not Found")
diff --git a/lib/lp/services/encoding.py b/lib/lp/services/encoding.py
index 2483176..7feb308 100644
--- a/lib/lp/services/encoding.py
+++ b/lib/lp/services/encoding.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Character encoding utilities"""
@@ -8,11 +8,14 @@ __all__ = [
     'escape_nonascii_uniquely',
     'guess',
     'is_ascii_only',
+    'wsgi_native_string',
     ]
 
 import codecs
 import re
 
+import six
+
 
 _boms = [
     (codecs.BOM_UTF16_BE, 'utf_16_be'),
@@ -212,3 +215,18 @@ def is_ascii_only(string):
         return False
     else:
         return True
+
+
+def wsgi_native_string(s):
+    """Make a native string suitable for use in WSGI.
+
+    PEP 3333 requires environment variables to be native strings that
+    contain only code points representable in ISO-8859-1.  To support
+    porting to Python 3 via an intermediate stage of Unicode literals in
+    Python 2, we enforce this here.
+    """
+    result = six.ensure_str(s, encoding='ISO-8859-1')
+    if six.PY3 and isinstance(s, six.text_type):
+        # Ensure we're limited to ISO-8859-1.
+        result.encode('ISO-8859-1')
+    return result
diff --git a/lib/lp/services/tests/test_encoding.py b/lib/lp/services/tests/test_encoding.py
index 1d91552..4a0e235 100644
--- a/lib/lp/services/tests/test_encoding.py
+++ b/lib/lp/services/tests/test_encoding.py
@@ -1,14 +1,44 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from doctest import (
     DocTestSuite,
     ELLIPSIS,
     )
+import unittest
+
+import six
 
 import lp.services.encoding
+from lp.services.encoding import wsgi_native_string
+from lp.testing import TestCase
+
+
+class TestWSGINativeString(TestCase):
+
+    def _toNative(self, s):
+        if six.PY3:
+            return s
+        else:
+            return s.encode('ISO-8859-1')
+
+    def test_not_bytes_or_unicode(self):
+        self.assertRaises(TypeError, wsgi_native_string, object())
+
+    def test_bytes_iso_8859_1(self):
+        self.assertEqual(
+            self._toNative(u'foo\xfe'), wsgi_native_string(b'foo\xfe'))
+
+    def test_unicode_iso_8859_1(self):
+        self.assertEqual(
+            self._toNative(u'foo\xfe'), wsgi_native_string(u'foo\xfe'))
+
+    def test_unicode_not_iso_8859_1(self):
+        self.assertRaises(UnicodeEncodeError, wsgi_native_string, u'foo\u2014')
 
 
 def test_suite():
-    suite = DocTestSuite(lp.services.encoding, optionflags=ELLIPSIS)
-    return suite
+    return unittest.TestSuite((
+        unittest.TestLoader().loadTestsFromName(__name__),
+        DocTestSuite(lp.services.encoding, optionflags=ELLIPSIS),
+        ))
diff --git a/lib/lp/services/webapp/servers.py b/lib/lp/services/webapp/servers.py
index c9fc0e6..e99fb05 100644
--- a/lib/lp/services/webapp/servers.py
+++ b/lib/lp/services/webapp/servers.py
@@ -64,6 +64,7 @@ from lp.app import versioninfo
 from lp.app.errors import UnexpectedFormData
 import lp.layers
 from lp.services.config import config
+from lp.services.encoding import wsgi_native_string
 from lp.services.features import get_relevant_feature_controller
 from lp.services.features.flags import NullFeatureController
 from lp.services.feeds.interfaces.application import IFeedsApplication
@@ -534,21 +535,6 @@ def get_query_string_params(request):
     return decoded_qs
 
 
-def wsgi_native_string(s):
-    """Make a native string suitable for use in WSGI.
-
-    PEP 3333 requires environment variables to be native strings that
-    contain only code points representable in ISO-8859-1.  To support
-    porting to Python 3 via an intermediate stage of Unicode literals in
-    Python 2, we enforce this here.
-    """
-    result = six.ensure_str(s, encoding='ISO-8859-1')
-    if six.PY3 and isinstance(s, six.text_type):
-        # Ensure we're limited to ISO-8859-1.
-        result.encode('ISO-8859-1')
-    return result
-
-
 class LaunchpadBrowserRequestMixin:
     """Provides methods used for both API and web browser requests."""
 
diff --git a/lib/lp/services/webapp/tests/test_servers.py b/lib/lp/services/webapp/tests/test_servers.py
index 79595e6..02afdb7 100644
--- a/lib/lp/services/webapp/tests/test_servers.py
+++ b/lib/lp/services/webapp/tests/test_servers.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -21,7 +21,6 @@ from lazr.restful.testing.webservice import (
     IGenericEntry,
     WebServiceTestCase,
     )
-import six
 from zope.component import (
     getGlobalSiteManager,
     getUtility,
@@ -48,7 +47,6 @@ from lp.services.webapp.servers import (
     WebServicePublication,
     WebServiceRequestPublicationFactory,
     WebServiceTestRequest,
-    wsgi_native_string,
     )
 from lp.testing import (
     EventRecorder,
@@ -355,29 +353,6 @@ class TestWebServiceRequest(WebServiceTestCase):
             request.response.getHeader('Vary'), 'Accept')
 
 
-class TestWSGINativeString(TestCase):
-
-    def _toNative(self, s):
-        if six.PY3:
-            return s
-        else:
-            return s.encode('ISO-8859-1')
-
-    def test_not_bytes_or_unicode(self):
-        self.assertRaises(TypeError, wsgi_native_string, object())
-
-    def test_bytes_iso_8859_1(self):
-        self.assertEqual(
-            self._toNative(u'foo\xfe'), wsgi_native_string(b'foo\xfe'))
-
-    def test_unicode_iso_8859_1(self):
-        self.assertEqual(
-            self._toNative(u'foo\xfe'), wsgi_native_string(u'foo\xfe'))
-
-    def test_unicode_not_iso_8859_1(self):
-        self.assertRaises(UnicodeEncodeError, wsgi_native_string, u'foo\u2014')
-
-
 class TestBasicLaunchpadRequest(TestCase):
     """Tests for the base request class"""
 
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index 59617f5..65fb63e 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -121,6 +121,7 @@ from lp.services.config.fixture import (
     )
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import session_store
+from lp.services.encoding import wsgi_native_string
 from lp.services.job.tests import celery_worker
 from lp.services.librarian.model import LibraryFileAlias
 from lp.services.librarianserver.testing.server import LibrarianServerFixture
@@ -147,7 +148,6 @@ from lp.services.webapp.interfaces import IOpenLaunchBag
 from lp.services.webapp.servers import (
     LaunchpadAccessLogger,
     register_launchpad_request_publication_factories,
-    wsgi_native_string,
     )
 import lp.services.webapp.session
 from lp.testing import (
diff --git a/lib/lp/testing/pages.py b/lib/lp/testing/pages.py
index 79bfba5..1c23980 100644
--- a/lib/lp/testing/pages.py
+++ b/lib/lp/testing/pages.py
@@ -79,6 +79,7 @@ from lp.services.beautifulsoup import (
     SoupStrainer,
     )
 from lp.services.config import config
+from lp.services.encoding import wsgi_native_string
 from lp.services.oauth.interfaces import (
     IOAuthConsumerSet,
     OAUTH_REALM,
@@ -86,10 +87,7 @@ from lp.services.oauth.interfaces import (
 from lp.services.webapp import canonical_url
 from lp.services.webapp.authorization import LaunchpadPermissiveSecurityPolicy
 from lp.services.webapp.interfaces import OAuthPermission
-from lp.services.webapp.servers import (
-    LaunchpadTestRequest,
-    wsgi_native_string,
-    )
+from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.services.webapp.url import urlsplit
 from lp.testing import (
     ANONYMOUS,