From aecaec3e104fc8aebb4f2e3e9ce29bb7dee4dc0c Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 2 Aug 2016 18:25:53 +0100
Subject: [PATCH 1/7] Change the way we summarize URLs

Using XPath is slow on some machines (for unknown reasons), so use a
different approach to get a list of text nodes.

Try to generate a summary that respect paragraph and then word
boundaries, adding ellipses when appropriate.
---
 synapse/rest/media/v1/preview_url_resource.py | 78 ++++++++++++++++---
 1 file changed, 67 insertions(+), 11 deletions(-)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index 74c64f137159..ea46b8aa1bbd 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -29,6 +29,8 @@
 from synapse.util.async import ObservableDeferred
 from synapse.util.stringutils import is_ascii
 
+from copy import deepcopy
+
 import os
 import re
 import fnmatch
@@ -329,20 +331,74 @@ def _calc_og(self, tree, media_info, requester):
                 # ...or if they are within a <script/> or <style/> tag.
                 # This is a very very very coarse approximation to a plain text
                 # render of the page.
-                text_nodes = tree.xpath("//text()[not(ancestor::header | ancestor::nav | "
-                                        "ancestor::aside | ancestor::footer | "
-                                        "ancestor::script | ancestor::style)]" +
-                                        "[ancestor::body]")
-                text = ''
+
+                # We don't just use XPATH here as that is slow on some machines.
+
+                cloned_tree = deepcopy(tree.find("body"))
+
+                TAGS_TO_REMOVE = ("header", "nav", "aside", "footer", "script", "style",)
+                for el in cloned_tree.iter(TAGS_TO_REMOVE):
+                    el.getparent().remove(el)
+
+                # Split all the text nodes into paragraphs (by splitting on new
+                # lines)
+                text_nodes = (
+                    line.strip()
+                    for line in el.text.splitlines()
+                    for el in el.iter() if el.text
+                )
+
+                # Try to get a summary of between 200 and 500 words, respecting
+                # first paragraph and then word boundaries.
+                # TODO: Respect sentences?
+                MIN_SIZE = 200
+                MAX_SIZE = 500
+
+                description = ''
+
+                # Keep adding paragraphs until we get to the MIN_SIZE.
                 for text_node in text_nodes:
-                    if len(text) < 500:
-                        text += text_node + ' '
+                    if len(description) < MIN_SIZE:
+                        description += text_node + '\n'
                     else:
                         break
-                text = re.sub(r'[\t ]+', ' ', text)
-                text = re.sub(r'[\t \r\n]*[\r\n]+', '\n', text)
-                text = text.strip()[:500]
-                og['og:description'] = text if text else None
+
+                description = description.strip()
+                description = re.sub(r'[\t ]+', ' ', description)
+                description = re.sub(r'[\t \r\n]*[\r\n]+', '\n', description)
+
+                # If the concatenation of paragraphs to get above MIN_SIZE
+                # took us over MAX_SIZE, then we need to truncate mid paragraph
+                if len(description) > MAX_SIZE:
+                    new_desc = ""
+
+                    # This splits the paragraph into words, but keeping the
+                    # (proceeding) whitespace intact so we can easily concat
+                    # words back together.
+                    for match in re.finditer("\s*\S+", description):
+                        word = match.group()
+
+                        # Keep adding words while the total length is less than
+                        # MAX_SIZE.
+                        if len(word) + len(new_desc) < MAX_SIZE:
+                            new_desc += word
+                        else:
+                            # At thi point the next word *will* take us over
+                            # MAX_SIZE, but we also want to ensure that its not
+                            # a huge word. If it is add it anyway and we'll
+                            # truncate later.
+                            if len(new_desc) < MIN_SIZE:
+                                new_desc += word
+                            break
+
+                    # Double check that we're not over the limit
+                    if len(new_desc) > MAX_SIZE:
+                        new_desc = new_desc[:MAX_SIZE]
+
+                    # We always add an ellipsis because at the very least
+                    # we chooped mid paragraph.
+                    description = new_desc.strip() + "…"
+                og['og:description'] = description if description else None
 
         # TODO: delete the url downloads to stop diskfilling,
         # as we only ever cared about its OG

From f83f5fbce8dc2a2d05df7c2731d8e698e14d3385 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 2 Aug 2016 18:32:42 +0100
Subject: [PATCH 2/7] Make it actually compile

---
 synapse/rest/media/v1/preview_url_resource.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index ea46b8aa1bbd..4624b890146f 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -344,8 +344,8 @@ def _calc_og(self, tree, media_info, requester):
                 # lines)
                 text_nodes = (
                     line.strip()
+                    for el in cloned_tree.iter() if el.text
                     for line in el.text.splitlines()
-                    for el in el.iter() if el.text
                 )
 
                 # Try to get a summary of between 200 and 500 words, respecting

From 9e66c58cebfd219a3a777944194cef292fcc5aad Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 2 Aug 2016 18:37:31 +0100
Subject: [PATCH 3/7] Spelling.

---
 synapse/rest/media/v1/preview_url_resource.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index 4624b890146f..e1212313e1d4 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -373,7 +373,7 @@ def _calc_og(self, tree, media_info, requester):
                     new_desc = ""
 
                     # This splits the paragraph into words, but keeping the
-                    # (proceeding) whitespace intact so we can easily concat
+                    # (preceeding) whitespace intact so we can easily concat
                     # words back together.
                     for match in re.finditer("\s*\S+", description):
                         word = match.group()
@@ -383,7 +383,7 @@ def _calc_og(self, tree, media_info, requester):
                         if len(word) + len(new_desc) < MAX_SIZE:
                             new_desc += word
                         else:
-                            # At thi point the next word *will* take us over
+                            # At this point the next word *will* take us over
                             # MAX_SIZE, but we also want to ensure that its not
                             # a huge word. If it is add it anyway and we'll
                             # truncate later.
@@ -396,7 +396,7 @@ def _calc_og(self, tree, media_info, requester):
                         new_desc = new_desc[:MAX_SIZE]
 
                     # We always add an ellipsis because at the very least
-                    # we chooped mid paragraph.
+                    # we chopped mid paragraph.
                     description = new_desc.strip() + "…"
                 og['og:description'] = description if description else None
 

From 6b58ade2f066beb7fa1339e494c09120b7e02976 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 2 Aug 2016 18:41:22 +0100
Subject: [PATCH 4/7] Comment on why we clone

---
 synapse/rest/media/v1/preview_url_resource.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index e1212313e1d4..64ad218f7028 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -334,6 +334,7 @@ def _calc_og(self, tree, media_info, requester):
 
                 # We don't just use XPATH here as that is slow on some machines.
 
+                # We clone `tree` as we modify it.
                 cloned_tree = deepcopy(tree.find("body"))
 
                 TAGS_TO_REMOVE = ("header", "nav", "aside", "footer", "script", "style",)

From 58c9653c6b7f258ed24fe509e2db1664d90ae866 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 2 Aug 2016 18:50:03 +0100
Subject: [PATCH 5/7] Don't infer paragrahs from newlines

---
 synapse/rest/media/v1/preview_url_resource.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index 64ad218f7028..ebd07d696b25 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -344,9 +344,8 @@ def _calc_og(self, tree, media_info, requester):
                 # Split all the text nodes into paragraphs (by splitting on new
                 # lines)
                 text_nodes = (
-                    line.strip()
+                    re.sub(r'\s+', '\n', el.text).strip()
                     for el in cloned_tree.iter() if el.text
-                    for line in el.text.splitlines()
                 )
 
                 # Try to get a summary of between 200 and 500 words, respecting

From e97648c4e219d56fa7b46cdd763a588e84a23f02 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 4 Aug 2016 16:08:32 +0100
Subject: [PATCH 6/7] Test summarization

---
 synapse/rest/media/v1/preview_url_resource.py | 106 ++++++-------
 tests/test_preview.py                         | 139 ++++++++++++++++++
 2 files changed, 193 insertions(+), 52 deletions(-)
 create mode 100644 tests/test_preview.py

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index ebd07d696b25..b2e2e7e62440 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -347,58 +347,7 @@ def _calc_og(self, tree, media_info, requester):
                     re.sub(r'\s+', '\n', el.text).strip()
                     for el in cloned_tree.iter() if el.text
                 )
-
-                # Try to get a summary of between 200 and 500 words, respecting
-                # first paragraph and then word boundaries.
-                # TODO: Respect sentences?
-                MIN_SIZE = 200
-                MAX_SIZE = 500
-
-                description = ''
-
-                # Keep adding paragraphs until we get to the MIN_SIZE.
-                for text_node in text_nodes:
-                    if len(description) < MIN_SIZE:
-                        description += text_node + '\n'
-                    else:
-                        break
-
-                description = description.strip()
-                description = re.sub(r'[\t ]+', ' ', description)
-                description = re.sub(r'[\t \r\n]*[\r\n]+', '\n', description)
-
-                # If the concatenation of paragraphs to get above MIN_SIZE
-                # took us over MAX_SIZE, then we need to truncate mid paragraph
-                if len(description) > MAX_SIZE:
-                    new_desc = ""
-
-                    # This splits the paragraph into words, but keeping the
-                    # (preceeding) whitespace intact so we can easily concat
-                    # words back together.
-                    for match in re.finditer("\s*\S+", description):
-                        word = match.group()
-
-                        # Keep adding words while the total length is less than
-                        # MAX_SIZE.
-                        if len(word) + len(new_desc) < MAX_SIZE:
-                            new_desc += word
-                        else:
-                            # At this point the next word *will* take us over
-                            # MAX_SIZE, but we also want to ensure that its not
-                            # a huge word. If it is add it anyway and we'll
-                            # truncate later.
-                            if len(new_desc) < MIN_SIZE:
-                                new_desc += word
-                            break
-
-                    # Double check that we're not over the limit
-                    if len(new_desc) > MAX_SIZE:
-                        new_desc = new_desc[:MAX_SIZE]
-
-                    # We always add an ellipsis because at the very least
-                    # we chopped mid paragraph.
-                    description = new_desc.strip() + "…"
-                og['og:description'] = description if description else None
+                og['og:description'] = _summarize_paragraphs(text_nodes)
 
         # TODO: delete the url downloads to stop diskfilling,
         # as we only ever cared about its OG
@@ -506,3 +455,56 @@ def _is_html(self, content_type):
             content_type.startswith("application/xhtml")
         ):
             return True
+
+
+def summarize_paragraphs(text_nodes, min_size=200, max_size=500):
+    # Try to get a summary of between 200 and 500 words, respecting
+    # first paragraph and then word boundaries.
+    # TODO: Respect sentences?
+
+    description = ''
+
+    # Keep adding paragraphs until we get to the MIN_SIZE.
+    for text_node in text_nodes:
+        if len(description) < min_size:
+            text_node = re.sub(r'[\t \r\n]+', ' ', text_node)
+            description += text_node + '\n\n'
+        else:
+            break
+
+    description = description.strip()
+    description = re.sub(r'[\t ]+', ' ', description)
+    description = re.sub(r'[\t \r\n]*[\r\n]+', '\n\n', description)
+
+    # If the concatenation of paragraphs to get above MIN_SIZE
+    # took us over MAX_SIZE, then we need to truncate mid paragraph
+    if len(description) > max_size:
+        new_desc = ""
+
+        # This splits the paragraph into words, but keeping the
+        # (preceeding) whitespace intact so we can easily concat
+        # words back together.
+        for match in re.finditer("\s*\S+", description):
+            word = match.group()
+
+            # Keep adding words while the total length is less than
+            # MAX_SIZE.
+            if len(word) + len(new_desc) < max_size:
+                new_desc += word
+            else:
+                # At this point the next word *will* take us over
+                # MAX_SIZE, but we also want to ensure that its not
+                # a huge word. If it is add it anyway and we'll
+                # truncate later.
+                if len(new_desc) < min_size:
+                    new_desc += word
+                break
+
+        # Double check that we're not over the limit
+        if len(new_desc) > max_size:
+            new_desc = new_desc[:max_size]
+
+        # We always add an ellipsis because at the very least
+        # we chopped mid paragraph.
+        description = new_desc.strip() + "…"
+    return description if description else None
diff --git a/tests/test_preview.py b/tests/test_preview.py
new file mode 100644
index 000000000000..2a801173a032
--- /dev/null
+++ b/tests/test_preview.py
@@ -0,0 +1,139 @@
+# -*- coding: utf-8 -*-
+# Copyright 2014-2016 OpenMarket Ltd
+#
+# 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 . import unittest
+
+from synapse.rest.media.v1.preview_url_resource import summarize_paragraphs
+
+
+class PreviewTestCase(unittest.TestCase):
+
+    def test_long_summarize(self):
+        example_paras = [
+            """Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:
+            Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in
+            Troms county, Norway. The administrative centre of the municipality is
+            the city of Tromsø. Outside of Norway, Tromso and Tromsö are
+            alternative spellings of the city.Tromsø is considered the northernmost
+            city in the world with a population above 50,000. The most populous town
+            north of it is Alta, Norway, with a population of 14,272 (2013).""",
+
+            """Tromsø lies in Northern Norway. The municipality has a population of
+            (2015) 72,066, but with an annual influx of students it has over 75,000
+            most of the year. It is the largest urban area in Northern Norway and the
+            third largest north of the Arctic Circle (following Murmansk and Norilsk).
+            Most of Tromsø, including the city centre, is located on the island of
+            Tromsøya, 350 kilometres (217 mi) north of the Arctic Circle. In 2012,
+            Tromsøya had a population of 36,088. Substantial parts of the urban area
+            are also situated on the mainland to the east, and on parts of Kvaløya—a
+            large island to the west. Tromsøya is connected to the mainland by the Tromsø
+            Bridge and the Tromsøysund Tunnel, and to the island of Kvaløya by the
+            Sandnessund Bridge. Tromsø Airport connects the city to many destinations
+            in Europe. The city is warmer than most other places located on the same
+            latitude, due to the warming effect of the Gulf Stream.""",
+
+            """The city centre of Tromsø contains the highest number of old wooden
+            houses in Northern Norway, the oldest house dating from 1789. The Arctic
+            Cathedral, a modern church from 1965, is probably the most famous landmark
+            in Tromsø. The city is a cultural centre for its region, with several
+            festivals taking place in the summer. Some of Norway's best-known
+             musicians, Torbjørn Brundtland and Svein Berge of the electronica duo
+             Röyksopp and Lene Marlin grew up and started their careers in Tromsø.
+             Noted electronic musician Geir Jenssen also hails from Tromsø.""",
+        ]
+
+        desc = summarize_paragraphs(example_paras, min_size=200, max_size=500)
+
+        self.assertEquals(
+            desc,
+            "Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:"
+            " Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in"
+            " Troms county, Norway. The administrative centre of the municipality is"
+            " the city of Tromsø. Outside of Norway, Tromso and Tromsö are"
+            " alternative spellings of the city.Tromsø is considered the northernmost"
+            " city in the world with a population above 50,000. The most populous town"
+            " north of it is Alta, Norway, with a population of 14,272 (2013)."
+        )
+
+        desc = summarize_paragraphs(example_paras[1:], min_size=200, max_size=500)
+
+        self.assertEquals(
+            desc,
+            "Tromsø lies in Northern Norway. The municipality has a population of"
+            " (2015) 72,066, but with an annual influx of students it has over 75,000"
+            " most of the year. It is the largest urban area in Northern Norway and the"
+            " third largest north of the Arctic Circle (following Murmansk and Norilsk)."
+            " Most of Tromsø, including the city centre, is located on the island of"
+            " Tromsøya, 350 kilometres (217 mi) north of the Arctic Circle. In 2012,"
+            " Tromsøya had a population of 36,088. Substantial parts of the…"
+        )
+
+    def test_short_summarize(self):
+        example_paras = [
+            "Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:"
+            " Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in"
+            " Troms county, Norway.",
+
+            "Tromsø lies in Northern Norway. The municipality has a population of"
+            " (2015) 72,066, but with an annual influx of students it has over 75,000"
+            " most of the year.",
+
+            "The city centre of Tromsø contains the highest number of old wooden"
+            " houses in Northern Norway, the oldest house dating from 1789. The Arctic"
+            " Cathedral, a modern church from 1965, is probably the most famous landmark"
+            " in Tromsø.",
+        ]
+
+        desc = summarize_paragraphs(example_paras, min_size=200, max_size=500)
+
+        self.assertEquals(
+            desc,
+            "Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:"
+            " Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in"
+            " Troms county, Norway.\n"
+            "\n"
+            "Tromsø lies in Northern Norway. The municipality has a population of"
+            " (2015) 72,066, but with an annual influx of students it has over 75,000"
+            " most of the year."
+        )
+
+    def test_small_then_large_summarize(self):
+        example_paras = [
+            "Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:"
+            " Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in"
+            " Troms county, Norway.",
+
+            "Tromsø lies in Northern Norway. The municipality has a population of"
+            " (2015) 72,066, but with an annual influx of students it has over 75,000"
+            " most of the year."
+            " The city centre of Tromsø contains the highest number of old wooden"
+            " houses in Northern Norway, the oldest house dating from 1789. The Arctic"
+            " Cathedral, a modern church from 1965, is probably the most famous landmark"
+            " in Tromsø.",
+        ]
+
+        desc = summarize_paragraphs(example_paras, min_size=200, max_size=500)
+        self.assertEquals(
+            desc,
+            "Tromsø (Norwegian pronunciation: [ˈtrʊmsœ] ( listen); Northern Sami:"
+            " Romsa; Finnish: Tromssa[2] Kven: Tromssa) is a city and municipality in"
+            " Troms county, Norway.\n"
+            "\n"
+            "Tromsø lies in Northern Norway. The municipality has a population of"
+            " (2015) 72,066, but with an annual influx of students it has over 75,000"
+            " most of the year. The city centre of Tromsø contains the highest number"
+            " of old wooden houses in Northern Norway, the oldest house dating from"
+            " 1789. The Arctic Cathedral, a modern church…"
+        )

From b5525c76d1993038bd0343970ab41611401fc128 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 4 Aug 2016 16:10:08 +0100
Subject: [PATCH 7/7] Typo

---
 synapse/rest/media/v1/preview_url_resource.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index b2e2e7e62440..4060593f7f73 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -347,7 +347,7 @@ def _calc_og(self, tree, media_info, requester):
                     re.sub(r'\s+', '\n', el.text).strip()
                     for el in cloned_tree.iter() if el.text
                 )
-                og['og:description'] = _summarize_paragraphs(text_nodes)
+                og['og:description'] = summarize_paragraphs(text_nodes)
 
         # TODO: delete the url downloads to stop diskfilling,
         # as we only ever cared about its OG