Repository: mitmproxy/mitmproxy
Pull request: [#4235] Automatic view mode based on should_render method
#### Description

This fixes #4235.

This is second attempt to support `+json` content type suffix. The first attempt in #3353 was declined. I applied authors suggestions regarding using `should_render` method for automatic view selection.

#### Checklist

 - [x] I have updated tests where applicable.
 - [x] I have added an entry to the CHANGELOG.


Problem statement:
Recognize `+json` content type suffix
Currently mitmproxy uses json view automatically only when request/response content type is exactly `application/json`. Mitmproxy uses default raw view mode, when content mode has vendor-specific media-type with json suffix, e.g. `application/vnd.my.company+json`. User has to change view mode to json each time he/she opens the flow.

Mitmproxy should recognize `+json` suffix in request/response content types.

Hints from issue discussion:

Commit message: [#4235] Fix linter warnings

Patch to review:
diff --git a/CHANGELOG b/CHANGELOG
index b9bfb74de4..d2e9b35721 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -16,6 +16,7 @@ Unreleased: mitmproxy next
     * Fix file unlinking before external viewer finishes loading (@wchasekelley)
     * Add --cert-passphrase command line argument (@mirosyn)
     * Add interactive tutorials to the documentation (@mplattner)
+    * Automatic JSON view mode when `+json` suffix in content type (@kam800)
 
     * --- TODO: add new PRs above this line ---
 
diff --git a/examples/addons/contentview.py b/examples/addons/contentview.py
index 0b24daf77c..fde45603fa 100644
--- a/examples/addons/contentview.py
+++ b/examples/addons/contentview.py
@@ -10,11 +10,13 @@
 
 class ViewSwapCase(contentviews.View):
     name = "swapcase"
-    content_types = ["text/plain"]
 
     def __call__(self, data, **metadata) -> contentviews.TViewResult:
         return "case-swapped text", contentviews.format_text(data.swapcase())
 
+    def should_render(self, content_type):
+        return content_type == "text/plain"
+
 
 view = ViewSwapCase()
 
diff --git a/mitmproxy/contentviews/__init__.py b/mitmproxy/contentviews/__init__.py
index 49c41f2ff6..dc9e0105b1 100644
--- a/mitmproxy/contentviews/__init__.py
+++ b/mitmproxy/contentviews/__init__.py
@@ -27,7 +27,6 @@
 from .base import View, KEY_MAX, format_text, format_dict, TViewResult
 
 views: List[View] = []
-content_types_map: Dict[str, List[View]] = {}
 
 
 def get(name: str) -> Optional[View]:
@@ -45,19 +44,8 @@ def add(view: View) -> None:
 
     views.append(view)
 
-    for ct in view.content_types:
-        l = content_types_map.setdefault(ct, [])
-        l.append(view)
-
 
 def remove(view: View) -> None:
-    for ct in view.content_types:
-        l = content_types_map.setdefault(ct, [])
-        l.remove(view)
-
-        if not len(l):
-            del content_types_map[ct]
-
     views.remove(view)
 
 
diff --git a/mitmproxy/contentviews/auto.py b/mitmproxy/contentviews/auto.py
index 92d7c1f59f..e8f2a47565 100644
--- a/mitmproxy/contentviews/auto.py
+++ b/mitmproxy/contentviews/auto.py
@@ -13,12 +13,11 @@ def __call__(self, data, **metadata):
         if data and ctype:
             ct = http.parse_content_type(ctype) if ctype else None
             ct = "%s/%s" % (ct[0], ct[1])
-            if ct in contentviews.content_types_map:
-                return contentviews.content_types_map[ct][0](data, **metadata)
+            preferred_view = next((v for v in contentviews.views if v.should_render(ct)), None)
+            if preferred_view:
+                return preferred_view(data, **metadata)
             elif strutils.is_xml(data):
                 return contentviews.get("XML/HTML")(data, **metadata)
-            elif ct.startswith("image/"):
-                return contentviews.get("Image")(data, **metadata)
         if metadata.get("query"):
             return contentviews.get("Query")(data, **metadata)
         if data and strutils.is_mostly_bin(data):
diff --git a/mitmproxy/contentviews/base.py b/mitmproxy/contentviews/base.py
index 81f2e4879c..e54012b0fc 100644
--- a/mitmproxy/contentviews/base.py
+++ b/mitmproxy/contentviews/base.py
@@ -10,7 +10,6 @@
 
 class View:
     name: typing.ClassVar[str]
-    content_types: typing.ClassVar[typing.List[str]] = []
 
     def __call__(self, data: bytes, **metadata) -> TViewResult:
         """
@@ -34,6 +33,18 @@ def __call__(self, data: bytes, **metadata) -> TViewResult:
         """
         raise NotImplementedError()  # pragma: no cover
 
+    def should_render(self, content_type) -> bool:
+        """
+        Check if a ``View`` implementation should be used to render the data of provided content type.
+
+        Args:
+            content_type: content type string without parameters.
+
+        Returns:
+            A bool value. ``True`` indicates that the View can be used to render the data.
+        """
+        return False
+
 
 def format_pairs(
         items: typing.Iterable[typing.Tuple[TTextType, TTextType]]
diff --git a/mitmproxy/contentviews/css.py b/mitmproxy/contentviews/css.py
index 44b33761e1..215302bef2 100644
--- a/mitmproxy/contentviews/css.py
+++ b/mitmproxy/contentviews/css.py
@@ -50,15 +50,15 @@ def beautify(data: str, indent: str = "    "):
 
 class ViewCSS(base.View):
     name = "CSS"
-    content_types = [
-        "text/css"
-    ]
 
     def __call__(self, data, **metadata):
         data = data.decode("utf8", "surrogateescape")
         beautified = beautify(data)
         return "CSS", base.format_text(beautified)
 
+    def should_render(self, content_type):
+        return content_type == "text/css"
+
 
 if __name__ == "__main__":  # pragma: no cover
     with open("../tools/web/static/vendor.css") as f:
diff --git a/mitmproxy/contentviews/image/view.py b/mitmproxy/contentviews/image/view.py
index f6c37067c8..d6983c9223 100644
--- a/mitmproxy/contentviews/image/view.py
+++ b/mitmproxy/contentviews/image/view.py
@@ -16,16 +16,6 @@ def test_ico(h, f):
 class ViewImage(base.View):
     name = "Image"
 
-    # there is also a fallback in the auto view for image/*.
-    content_types = [
-        "image/png",
-        "image/jpeg",
-        "image/gif",
-        "image/vnd.microsoft.icon",
-        "image/x-icon",
-        "image/webp",
-    ]
-
     def __call__(self, data, **metadata):
         image_type = imghdr.what('', h=data)
         if image_type == 'png':
@@ -45,3 +35,6 @@ def __call__(self, data, **metadata):
         else:
             view_name = "Unknown Image"
         return view_name, base.format_dict(multidict.MultiDict(image_metadata))
+
+    def should_render(self, content_type):
+        return content_type.startswith("image/") and content_type != "image/svg+xml"
diff --git a/mitmproxy/contentviews/javascript.py b/mitmproxy/contentviews/javascript.py
index b5f09150f3..09543d0223 100644
--- a/mitmproxy/contentviews/javascript.py
+++ b/mitmproxy/contentviews/javascript.py
@@ -46,7 +46,7 @@ def beautify(data):
 
 class ViewJavaScript(base.View):
     name = "JavaScript"
-    content_types = [
+    __content_types = [
         "application/x-javascript",
         "application/javascript",
         "text/javascript"
@@ -56,3 +56,6 @@ def __call__(self, data, **metadata):
         data = data.decode("utf-8", "replace")
         res = beautify(data)
         return "JavaScript", base.format_text(res)
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
diff --git a/mitmproxy/contentviews/json.py b/mitmproxy/contentviews/json.py
index a5a7d03b4d..320fd22bab 100644
--- a/mitmproxy/contentviews/json.py
+++ b/mitmproxy/contentviews/json.py
@@ -38,13 +38,13 @@ def format_json(data: typing.Any) -> typing.Iterator[base.TViewLine]:
 
 class ViewJSON(base.View):
     name = "JSON"
-    content_types = [
-        "application/json",
-        "application/json-rpc",
-        "application/vnd.api+json"
-    ]
 
     def __call__(self, data, **metadata):
         data = parse_json(data)
         if data is not PARSE_ERROR:
             return "JSON", format_json(data)
+
+    def should_render(self, content_type):
+        return content_type == "application/json" \
+            or content_type == "application/json-rpc" \
+            or (content_type.startswith("application/") and content_type.endswith("+json"))
diff --git a/mitmproxy/contentviews/msgpack.py b/mitmproxy/contentviews/msgpack.py
index fec1f60c21..d55d3128ac 100644
--- a/mitmproxy/contentviews/msgpack.py
+++ b/mitmproxy/contentviews/msgpack.py
@@ -39,7 +39,7 @@ def format_msgpack(data):
 
 class ViewMsgPack(base.View):
     name = "MsgPack"
-    content_types = [
+    __content_types = [
         "application/msgpack",
         "application/x-msgpack",
     ]
@@ -48,3 +48,6 @@ def __call__(self, data, **metadata):
         data = parse_msgpack(data)
         if data is not PARSE_ERROR:
             return "MsgPack", format_msgpack(data)
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
diff --git a/mitmproxy/contentviews/multipart.py b/mitmproxy/contentviews/multipart.py
index e122df599f..e094f5046f 100644
--- a/mitmproxy/contentviews/multipart.py
+++ b/mitmproxy/contentviews/multipart.py
@@ -5,7 +5,6 @@
 
 class ViewMultipart(base.View):
     name = "Multipart Form"
-    content_types = ["multipart/form-data"]
 
     @staticmethod
     def _format(v):
@@ -18,3 +17,6 @@ def __call__(self, data, **metadata):
         v = http.multipart.decode(headers, data)
         if v:
             return "Multipart form", self._format(v)
+
+    def should_render(self, content_type):
+        return content_type == "multipart/form-data"
diff --git a/mitmproxy/contentviews/protobuf.py b/mitmproxy/contentviews/protobuf.py
index 50f8dcede2..80597cc214 100644
--- a/mitmproxy/contentviews/protobuf.py
+++ b/mitmproxy/contentviews/protobuf.py
@@ -66,7 +66,7 @@ class ViewProtobuf(base.View):
     """
 
     name = "Protocol Buffer"
-    content_types = [
+    __content_types = [
         "application/x-protobuf",
         "application/x-protobuffer",
     ]
@@ -77,3 +77,6 @@ def __call__(self, data, **metadata):
             raise ValueError("Failed to parse input.")
 
         return "Protobuf", base.format_text(decoded)
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
diff --git a/mitmproxy/contentviews/urlencoded.py b/mitmproxy/contentviews/urlencoded.py
index e35bbdb7ac..ed606c5fc0 100644
--- a/mitmproxy/contentviews/urlencoded.py
+++ b/mitmproxy/contentviews/urlencoded.py
@@ -4,7 +4,6 @@
 
 class ViewURLEncoded(base.View):
     name = "URL-encoded"
-    content_types = ["application/x-www-form-urlencoded"]
 
     def __call__(self, data, **metadata):
         try:
@@ -13,3 +12,6 @@ def __call__(self, data, **metadata):
             return None
         d = url.decode(data)
         return "URLEncoded form", base.format_pairs(d)
+
+    def should_render(self, content_type):
+        return content_type == "application/x-www-form-urlencoded"
diff --git a/mitmproxy/contentviews/wbxml.py b/mitmproxy/contentviews/wbxml.py
index b33f4ce894..4424b71f15 100644
--- a/mitmproxy/contentviews/wbxml.py
+++ b/mitmproxy/contentviews/wbxml.py
@@ -4,7 +4,7 @@
 
 class ViewWBXML(base.View):
     name = "WBXML"
-    content_types = [
+    __content_types = [
         "application/vnd.wap.wbxml",
         "application/vnd.ms-sync.wbxml"
     ]
@@ -17,3 +17,6 @@ def __call__(self, data, **metadata):
                 return "WBXML", base.format_text(parsedContent)
         except:
             return None
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
diff --git a/mitmproxy/contentviews/xml_html.py b/mitmproxy/contentviews/xml_html.py
index f2fa47cb19..04a7500dc8 100644
--- a/mitmproxy/contentviews/xml_html.py
+++ b/mitmproxy/contentviews/xml_html.py
@@ -214,7 +214,7 @@ def format_xml(tokens: Iterable[Token]) -> str:
 
 class ViewXmlHtml(base.View):
     name = "XML/HTML"
-    content_types = ["text/xml", "text/html"]
+    __content_types = ["text/xml", "text/html"]
 
     def __call__(self, data, **metadata):
         # TODO:
@@ -233,3 +233,6 @@ def __call__(self, data, **metadata):
         else:
             t = "XML"
         return t, pretty
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
diff --git a/test/mitmproxy/contentviews/image/test_view.py b/test/mitmproxy/contentviews/image/test_view.py
index 84dffcc714..8f5271dcad 100644
--- a/test/mitmproxy/contentviews/image/test_view.py
+++ b/test/mitmproxy/contentviews/image/test_view.py
@@ -15,3 +15,14 @@ def test_view_image(tdata):
             assert img.split(".")[-1].upper() in viewname
 
     assert v(b"flibble") == ('Unknown Image', [[('header', 'Image Format: '), ('text', 'unknown')]])
+
+
+def test_should_render():
+    v = image.ViewImage()
+    assert v.should_render("image/png")
+    assert v.should_render("image/jpeg")
+    assert v.should_render("image/gif")
+    assert v.should_render("image/vnd.microsoft.icon")
+    assert v.should_render("image/x-icon")
+    assert v.should_render("image/webp")
+    assert not v.should_render("image/svg+xml")
diff --git a/test/mitmproxy/contentviews/test_api.py b/test/mitmproxy/contentviews/test_api.py
index 82fd0672ed..ee132ed338 100644
--- a/test/mitmproxy/contentviews/test_api.py
+++ b/test/mitmproxy/contentviews/test_api.py
@@ -10,7 +10,9 @@
 
 class TestContentView(contentviews.View):
     name = "test"
-    content_types = ["test/123"]
+
+    def should_render(self, content_type):
+        return content_type == "test/123"
 
 
 def test_add_remove():
diff --git a/test/mitmproxy/contentviews/test_auto.py b/test/mitmproxy/contentviews/test_auto.py
index cd888a2d02..2cff15e382 100644
--- a/test/mitmproxy/contentviews/test_auto.py
+++ b/test/mitmproxy/contentviews/test_auto.py
@@ -36,6 +36,12 @@ def test_view_auto():
     )
     assert f[0].startswith("XML")
 
+    f = v(
+        b"{}",
+        headers=http.Headers(content_type="application/acme+json")
+    )
+    assert f[0].startswith("JSON")
+
     f = v(
         b"verybinary",
         headers=http.Headers(content_type="image/new-magic-image-format")
diff --git a/test/mitmproxy/contentviews/test_css.py b/test/mitmproxy/contentviews/test_css.py
index af1f776bf4..710654b96c 100644
--- a/test/mitmproxy/contentviews/test_css.py
+++ b/test/mitmproxy/contentviews/test_css.py
@@ -37,3 +37,9 @@ def test_simple():
     assert v(b"console.log('not really css')") == (
         'CSS', [[('text', "console.log('not really css')")]]
     )
+
+
+def test_should_render():
+    v = css.ViewCSS()
+    assert v.should_render("text/css")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_javascript.py b/test/mitmproxy/contentviews/test_javascript.py
index 8a10279785..f03d54727b 100644
--- a/test/mitmproxy/contentviews/test_javascript.py
+++ b/test/mitmproxy/contentviews/test_javascript.py
@@ -27,3 +27,11 @@ def test_format_xml(filename, tdata):
         expected = f.read()
     js = javascript.beautify(input)
     assert js == expected
+
+
+def test_should_render():
+    v = javascript.ViewJavaScript()
+    assert v.should_render("application/x-javascript")
+    assert v.should_render("application/javascript")
+    assert v.should_render("text/javascript")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_json.py b/test/mitmproxy/contentviews/test_json.py
index 2b5bf86a17..956f090ed6 100644
--- a/test/mitmproxy/contentviews/test_json.py
+++ b/test/mitmproxy/contentviews/test_json.py
@@ -41,3 +41,12 @@ def test_view_json():
 def test_view_json_doesnt_crash(data):
     v = full_eval(json.ViewJSON())
     v(data)
+
+
+def test_should_render():
+    v = json.ViewJSON()
+    assert v.should_render("application/json")
+    assert v.should_render("application/json-rpc")
+    assert v.should_render("application/vnd.api+json")
+    assert v.should_render("application/acme+json")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_msgpack.py b/test/mitmproxy/contentviews/test_msgpack.py
index d9ce81e80a..e97a462ebf 100644
--- a/test/mitmproxy/contentviews/test_msgpack.py
+++ b/test/mitmproxy/contentviews/test_msgpack.py
@@ -44,3 +44,10 @@ def test_view_msgpack():
 def test_view_msgpack_doesnt_crash(data):
     v = full_eval(msgpack.ViewMsgPack())
     v(data)
+
+
+def test_should_render():
+    v = msgpack.ViewMsgPack()
+    assert v.should_render("application/msgpack")
+    assert v.should_render("application/x-msgpack")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_multipart.py b/test/mitmproxy/contentviews/test_multipart.py
index 48a5ccc902..709a35f0c2 100644
--- a/test/mitmproxy/contentviews/test_multipart.py
+++ b/test/mitmproxy/contentviews/test_multipart.py
@@ -23,3 +23,9 @@ def test_view_multipart():
 
     h = http.Headers(content_type="unparseable")
     assert not view(v, headers=h)
+
+
+def test_should_render():
+    v = multipart.ViewMultipart()
+    assert v.should_render("multipart/form-data")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_protobuf.py b/test/mitmproxy/contentviews/test_protobuf.py
index f0a91fd15b..5241e68592 100644
--- a/test/mitmproxy/contentviews/test_protobuf.py
+++ b/test/mitmproxy/contentviews/test_protobuf.py
@@ -28,3 +28,10 @@ def test_format_pbuf(filename, tdata):
         expected = f.read()
 
     assert protobuf.format_pbuf(input) == expected
+
+
+def test_should_render():
+    v = protobuf.ViewProtobuf()
+    assert v.should_render("application/x-protobuf")
+    assert v.should_render("application/x-protobuffer")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_urlencoded.py b/test/mitmproxy/contentviews/test_urlencoded.py
index d01f9aaae1..c883cf9388 100644
--- a/test/mitmproxy/contentviews/test_urlencoded.py
+++ b/test/mitmproxy/contentviews/test_urlencoded.py
@@ -13,3 +13,9 @@ def test_view_urlencoded():
     assert v(d)
 
     assert not v(b"\xFF\x00")
+
+
+def test_should_render():
+    v = urlencoded.ViewURLEncoded()
+    assert v.should_render("application/x-www-form-urlencoded")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_wbxml.py b/test/mitmproxy/contentviews/test_wbxml.py
index 441a774978..54e906ce75 100644
--- a/test/mitmproxy/contentviews/test_wbxml.py
+++ b/test/mitmproxy/contentviews/test_wbxml.py
@@ -18,3 +18,10 @@ def test_wbxml(tdata):
 
     p = wbxml.ASCommandResponse.ASCommandResponse(input)
     assert p.xmlString == expected
+
+
+def test_should_render():
+    v = wbxml.ViewWBXML()
+    assert v.should_render("application/vnd.wap.wbxml")
+    assert v.should_render("application/vnd.ms-sync.wbxml")
+    assert not v.should_render("text/plain")
diff --git a/test/mitmproxy/contentviews/test_xml_html.py b/test/mitmproxy/contentviews/test_xml_html.py
index 82f85c155a..ae4f96de7c 100644
--- a/test/mitmproxy/contentviews/test_xml_html.py
+++ b/test/mitmproxy/contentviews/test_xml_html.py
@@ -34,3 +34,10 @@ def test_format_xml(filename, tdata):
         expected = f.read()
     tokens = xml_html.tokenize(input)
     assert xml_html.format_xml(tokens) == expected
+
+
+def test_should_render():
+    v = xml_html.ViewXmlHtml()
+    assert v.should_render("text/xml")
+    assert v.should_render("text/html")
+    assert not v.should_render("text/plain")


Code review comments:
diff_hunk: @@ -48,3 +48,6 @@ def __call__(self, data, **metadata):
         data = parse_msgpack(data)
         if data is not PARSE_ERROR:
             return "MsgPack", format_msgpack(data)
+
+    def should_render(self, content_type):
+        return content_type in self.__content_types
path: mitmproxy/contentviews/msgpack.py
text: @user1:
nitpick: why not inline the `__content_types` list directly here?
(same question for all the other content views with similar code style)

@user2:
Not inlining probably reduces the number of allocations, but this is not a hot path and we shouldn't care. Either option is fine.

More practically speaking, we should consider keeping an documented `content_types` ClassVar and have a default `View.should_render` implementation that checks just that. This way we could retain backward-compatibility with existing third-party views.

@author:
Well, I thought it could be misleading to have both `content_types` and `should_render`. `should_render` could do more than `content_types` does. Keeping `content_types` visible would encourage 3rd party users to still used, which could not be appropriate all the time. But the argument about keeping 3rd party views worries me and I am not sure how to handle it.

So, do you think we should have both `content_types` and `should_render` visible even when they are potentially incompatible?

diff_hunk: @@ -34,6 +33,18 @@ def __call__(self, data: bytes, **metadata) -> TViewResult:
         """
         raise NotImplementedError()  # pragma: no cover
 
+    def should_render(self, content_type) -> bool:
path: mitmproxy/contentviews/base.py
text: @user1:
While we are improving the API here, how would you all feel about having this function return an `int` as priority, where a value > 0 signals that the view can render this?

We could then selected the preferred view like this:

```python
priority, view = max(
    (v.render_priority(ct), v)
    for v in contentviews.views
)
```

A major benefit of this approach would be that I can write a custom contentview addon that pretty-prints data in a specific way for me. For example, let's say I'm only interested in a specific subproperty of a 2MB JSON blob, I could write an addon with a higher priority that only displays that part. I would have found that useful a couple of times in the past.

@author:
Great idea. But this would require changing `should_render` interface even further – passing the request/response contents as an argument so the extension could check if the JSON subproperty exists. I am afraid that this is going to grow the PR.

On the other hand, if we implemented your proposition, we could minimise the number of incompatible changes for 3-rd party views.

Do you think we should discuss the `should_render` interface now, or should we skip it for now?