Refactor readme rendering logic (#3760)

* Refactor readme rendering logic

* Add mock for readme render

* Fix silly error

Author: Jon Wayne Parrott

tests/functional/test_templates.py

@@ -40,7 +40,7 @@ def test_templates_for_empty_titles():
         "format_classifiers": "warehouse.filters:format_classifiers",
         "format_tags": "warehouse.filters:format_tags",
         "json": "warehouse.filters:tojson",
-        "readme": "warehouse.filters:readme",
+        "camoify": "warehouse.filters:camoify",
         "shorten_number": "warehouse.filters:shorten_number",
         "urlparse": "warehouse.filters:urlparse",
         "contains_valid_uris": "warehouse.filters:contains_valid_uris",

tests/unit/packaging/test_views.py

@@ -15,6 +15,7 @@
 from pyramid.httpexceptions import HTTPMovedPermanently, HTTPNotFound
 
 from warehouse.packaging import views
+from warehouse.utils import readme
 
 from ...common.db.accounts import UserFactory
 from ...common.db.classifiers import ClassifierFactory
@@ -143,15 +144,19 @@ def test_normalizing_version_redirects(self, db_request):
             pretend.call(name=release.project.name, version=release.version),
         ]
 
-    def test_detail_renders(self, db_request):
+    def test_detail_renders(self, monkeypatch, db_request):
         users = [
             UserFactory.create(),
             UserFactory.create(),
             UserFactory.create(),
         ]
         project = ProjectFactory.create()
         releases = [
-            ReleaseFactory.create(project=project, version=v)
+            ReleaseFactory.create(
+                project=project,
+                version=v,
+                description="unrendered description",
+                description_content_type="text/plain")
             for v in ["1.0", "2.0", "3.0", "4.0.dev0"]
         ]
         files = [
@@ -174,12 +179,18 @@ def test_detail_renders(self, db_request):
             role_name="another role",
         )
 
+        # patch the readme rendering logic.
+        render_description = pretend.call_recorder(
+            lambda raw, content_type: "rendered description")
+        monkeypatch.setattr(readme, "render", render_description)
+
         result = views.release_detail(releases[1], db_request)
 
         assert result == {
             "project": project,
             "release": releases[1],
             "files": [files[1]],
+            "description": "rendered description",
             "latest_version": project.latest_version,
             "all_versions": [
                 (r.version, r.created, r.is_prerelease)
@@ -189,6 +200,10 @@ def test_detail_renders(self, db_request):
             "license": None
         }
 
+        assert render_description.calls == [
+            pretend.call("unrendered description", "text/plain")
+        ]
+
     def test_license_from_classifier(self, db_request):
         """A license label is added when a license classifier exists."""
         other_classifier = ClassifierFactory.create(

tests/unit/test_filters.py

@@ -14,11 +14,9 @@
 
 from functools import partial
 
-import jinja2
 import packaging.version
 import pretend
 import pytest
-import readme_renderer.rst
 
 from warehouse import filters
 
@@ -42,134 +40,9 @@ def test_camo_url():
     )
 
 
-class TestReadmeRender:
-
-    def test_can_render_rst(self, monkeypatch):
-        renderer = pretend.call_recorder(lambda raw: "rendered")
-        monkeypatch.setattr(readme_renderer.rst, "render", renderer)
-
-        request = pretend.stub(
-            registry=pretend.stub(
-                settings={
-                    "camo.url": "https://camo.example.net/",
-                    "camo.key": "fake key",
-                },
-            ),
-        )
-        camo_url = partial(filters._camo_url, request)
-        request.camo_url = camo_url
-
-        ctx = {"request": request}
-
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/x-rst",
-        )
-
-        assert result == jinja2.Markup("rendered")
-        assert renderer.calls == [pretend.call('raw thing')]
-
-    def test_cant_render_rst(self, monkeypatch):
-        rst_renderer = pretend.call_recorder(lambda raw: None)
-        txt_renderer = pretend.call_recorder(lambda raw: "rendered<br>thing")
-        monkeypatch.setattr(readme_renderer.rst, "render", rst_renderer)
-        monkeypatch.setattr(readme_renderer.txt, "render", txt_renderer)
-
-        request = pretend.stub(
-            registry=pretend.stub(
-                settings={
-                    "camo.url": "https://camo.example.net/",
-                    "camo.key": "fake key",
-                },
-            ),
-        )
-        camo_url = partial(filters._camo_url, request)
-        request.camo_url = camo_url
-
-        ctx = {"request": request}
-
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/x-rst",
-        )
-
-        assert result == jinja2.Markup("rendered<br>thing")
-        assert rst_renderer.calls == [pretend.call('raw thing')]
-        assert txt_renderer.calls == [pretend.call('raw thing')]
-
-    def test_can_render_plaintext(self, monkeypatch):
-        renderer = pretend.call_recorder(lambda raw: "rendered")
-        monkeypatch.setattr(readme_renderer.txt, "render", renderer)
-
-        request = pretend.stub(
-            registry=pretend.stub(
-                settings={
-                    "camo.url": "https://camo.example.net/",
-                    "camo.key": "fake key",
-                },
-            ),
-        )
-        camo_url = partial(filters._camo_url, request)
-        request.camo_url = camo_url
-
-        ctx = {"request": request}
-
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/plain",
-        )
-
-        assert result == jinja2.Markup("rendered")
-        assert renderer.calls == [pretend.call('raw thing')]
-
-    def test_can_render_markdown(self, monkeypatch):
-        renderer = pretend.call_recorder(lambda raw: "rendered")
-        monkeypatch.setattr(readme_renderer.markdown, "render", renderer)
-
-        request = pretend.stub(
-            registry=pretend.stub(
-                settings={
-                    "camo.url": "https://camo.example.net/",
-                    "camo.key": "fake key",
-                },
-            ),
-        )
-        camo_url = partial(filters._camo_url, request)
-        request.camo_url = camo_url
-
-        ctx = {"request": request}
-
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/markdown",
-        )
-
-        assert result == jinja2.Markup("rendered")
-        assert renderer.calls == [pretend.call('raw thing')]
-
-    def test_can_render_missing_content_type(self, monkeypatch):
-        renderer = pretend.call_recorder(lambda raw: "rendered")
-        monkeypatch.setattr(readme_renderer.rst, "render", renderer)
-
-        request = pretend.stub(
-            registry=pretend.stub(
-                settings={
-                    "camo.url": "https://camo.example.net/",
-                    "camo.key": "fake key",
-                },
-            ),
-        )
-        camo_url = partial(filters._camo_url, request)
-        request.camo_url = camo_url
-
-        ctx = {"request": request}
-
-        result = filters.readme(
-            ctx, "raw thing", description_content_type=None,
-        )
-
-        assert result == jinja2.Markup("rendered")
-        assert renderer.calls == [pretend.call('raw thing')]
-
-    def test_renders_camo(self, monkeypatch):
+class TestCamoify:
+    def test_camoify(self):
         html = "<img src=http://example.com/image.jpg>"
-        monkeypatch.setattr(readme_renderer.rst, "render", lambda raw: html)
 
         request = pretend.stub(
             registry=pretend.stub(
@@ -184,19 +57,17 @@ def test_renders_camo(self, monkeypatch):
 
         ctx = {"request": request}
 
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/x-rst",
+        result = filters.camoify(
+            ctx, html
         )
 
-        assert result == jinja2.Markup(
+        assert result == (
             '<img src="https://camo.example.net/'
             'b410d235a3d2fc44b50ccab827e531dece213062/'
-            '687474703a2f2f6578616d706c652e636f6d2f696d6167652e6a7067">'
-        )
+            '687474703a2f2f6578616d706c652e636f6d2f696d6167652e6a7067">')
 
-    def test_renders_camo_no_src(self, monkeypatch):
+    def test_camoify_no_src(self, monkeypatch):
         html = "<img>"
-        monkeypatch.setattr(readme_renderer.rst, "render", lambda raw: html)
 
         request = pretend.stub(
             registry=pretend.stub(
@@ -216,11 +87,9 @@ def test_renders_camo_no_src(self, monkeypatch):
         )
         monkeypatch.setattr(filters, "_camo_url", gen_camo_url)
 
-        result = filters.readme(
-            ctx, "raw thing", description_content_type="text/x-rst",
-        )
+        result = filters.camoify(ctx, html)
 
-        assert result == jinja2.Markup("<img>")
+        assert result == "<img>"
         assert gen_camo_url.calls == []
 
 

tests/unit/utils/test_readme.py

@@ -0,0 +1,47 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from warehouse.utils import readme
+
+
+def test_render_with_none():
+    result = readme.render(None)
+    assert result is None
+
+
+def test_can_render_rst():
+    result = readme.render("raw thing", "text/x-rst")
+    assert result == "<p>raw thing</p>\n"
+
+
+def test_cant_render_rst():
+    result = readme.render("raw `<thing", "text/x-rst")
+    assert result == "raw `&lt;thing"
+
+
+def test_can_render_plaintext():
+    result = readme.render("raw thing", "text/plain")
+    assert result == "raw thing"
+
+
+def test_can_render_markdown():
+    result = readme.render("raw thing", "text/markdown")
+    assert result == "<p>raw thing</p>\n"
+
+
+def test_can_render_missing_content_type():
+    result = readme.render("raw thing")
+    assert result == "<p>raw thing</p>\n"
+
+
+def test_renderer_version():
+    assert readme.renderer_version() is not None

warehouse/config.py

@@ -327,7 +327,7 @@ def configure(settings=None):
     )
     filters.setdefault("format_tags", "warehouse.filters:format_tags")
     filters.setdefault("json", "warehouse.filters:tojson")
-    filters.setdefault("readme", "warehouse.filters:readme")
+    filters.setdefault("camoify", "warehouse.filters:camoify")
     filters.setdefault("shorten_number", "warehouse.filters:shorten_number")
     filters.setdefault("urlparse", "warehouse.filters:urlparse")
     filters.setdefault(

warehouse/filters.py

@@ -11,7 +11,6 @@
 # limitations under the License.
 
 import binascii
-import cgi
 import collections
 import enum
 import hmac
@@ -25,21 +24,11 @@
 import jinja2
 
 import packaging.version
-import readme_renderer.markdown
-import readme_renderer.rst
-import readme_renderer.txt
 
 from pyramid.threadlocal import get_current_request
 
 from warehouse.utils.http import is_valid_uri
 
-_renderers = {
-    '': readme_renderer.rst,  # Default if description_content_type is None
-    'text/plain': readme_renderer.txt,
-    'text/x-rst': readme_renderer.rst,
-    'text/markdown': readme_renderer.markdown,
-}
-
 
 class PackageType(enum.Enum):
     bdist_dmg = "OSX Disk Image"
@@ -73,27 +62,14 @@ def _camo_url(request, url):
 
 
 @jinja2.contextfilter
-def readme(ctx, value, *, description_content_type):
+def camoify(ctx, value):
     request = ctx.get("request") or get_current_request()
 
-    content_type, parameters = cgi.parse_header(description_content_type or '')
-
-    # Get the appropriate renderer
-    renderer = _renderers[content_type]
-
-    # Actually render the given value, this will not only render the value, but
-    # also ensure that it's had any disallowed markup removed.
-    rendered = renderer.render(value, **parameters)
-
-    # If the content was not rendered, we'll render as plaintext instead
-    if rendered is None:
-        rendered = readme_renderer.txt.render(value)
-
     # Parse the rendered output and replace any inline images that don't point
     # to HTTPS with camouflaged images.
     tree_builder = html5lib.treebuilders.getTreeBuilder("dom")
     parser = html5lib.html5parser.HTMLParser(tree=tree_builder)
-    dom = parser.parse(rendered)
+    dom = parser.parse(value)
 
     for element in dom.getElementsByTagName("img"):
         src = element.getAttribute("src")
@@ -102,9 +78,9 @@ def readme(ctx, value, *, description_content_type):
 
     tree_walker = html5lib.treewalkers.getTreeWalker("dom")
     html_serializer = html5lib.serializer.HTMLSerializer()
-    rendered = "".join(html_serializer.serialize(tree_walker(dom)))
+    camoed = "".join(html_serializer.serialize(tree_walker(dom)))
 
-    return jinja2.Markup(rendered)
+    return camoed
 
 
 _SI_SYMBOLS = ["k", "M", "G", "T", "P", "E", "Z", "Y"]