fix(filters): separate each match on its own line for source: watches (#4492)

* fix(filters): separate each match on its own line for source: watches

#4477

A `source:` watch passes append_pretty_line_formatting=False to the filter
helpers, and then keeps the filtered text verbatim instead of running it
through html_to_text(). TEXT_FILTER_LIST_LINE_SUFFIX ('<br>') only works
because Inscriptis turns it into a newline, so for that path no separator was
emitted at all and every match ran into the next on a single line.

Add filter_match_separator(), which picks '\n' instead of '<br>' when the
result will not be passed through Inscriptis, and use it from
xpath_filter(), xpath1_filter() and include_filters() - all three carried the
same condition. The first match still never gets a separator, so no leading
newline is introduced, and the Inscriptis path is byte-for-byte unchanged.

* fix(filters): don't double-space xpath element matches on source: watches

xpath_filter/xpath1_filter serialise whole elements with
etree.tostring(pretty_print=True), which already terminates every match
with a newline. Adding the new source: separator unconditionally put a
second newline between matched elements, so each one landed on its own
line separated by a blank line.

Only emit the separator when the accumulated output does not already end
on a line break, which leaves attribute and text() matches (plain
strings, no newline of their own) separated as before.

Found while producing the before/after output requested on #4492.

---------

Co-authored-by: Yi-111-a <yi-111-a@users.noreply.github.com>
Co-authored-by: Yi-111-a <41898262+github-actions[bot]@users.noreply.github.com>

changedetectionio/html_tools.py

@@ -95,6 +95,10 @@ TEXT_FILTER_LIST_LINE_SUFFIX = "<br>"
 TRANSLATE_WHITESPACE_TABLE = str.maketrans('', '', '\r\n\t ')
 PERL_STYLE_REGEX = r'^/(.*?)/([a-z]*)?$'
 
+# 'br', 'hr', 'div' and 'p' already make Inscriptis break the line, so a filter match on one of
+# them is never suffixed with TEXT_FILTER_LIST_LINE_SUFFIX (a div becomes 4 whitespaces).
+FILTER_TAGS_WITH_OWN_NEWLINE = ('br', 'hr', 'div', 'p')
+
 # Whitespace is allowed after the tag name ("</title >") but not after "<" - "< title>" is text, not a tag
 TITLE_RE = re.compile(r"<title[^>]*>(.*?)</title\s*>", re.I | re.S)
 # Locate the tag with pos/endpos instead of data.lower(), which copies the whole document (67MB seen
@@ -237,6 +241,38 @@ def perl_style_slash_enclosed_regex_to_options(regex):
 
     return regex
 
+def filter_match_separator(append_pretty_line_formatting, html_block, tag):
+    """What to append after a filter match so that each match gets its own line in the diff.
+
+    Always empty for the first match, so no leading separator is ever emitted.
+
+    :param append_pretty_line_formatting: True while the result still has to survive
+        html_to_text() (Inscriptis). `source:` watches pass False - see processor.py,
+        `append_pretty_line_formatting=not self.watch.is_source_type_url`.
+    :param html_block: the output accumulated so far, used to skip the first match
+    :param tag: the matched element's tag name, or None when the match is a plain string
+               (an attribute or text() node) and so is not HTML at all
+    """
+    if not append_pretty_line_formatting:
+        # A `source:` watch keeps the filtered text verbatim and never runs html_to_text(),
+        # so a literal '<br>' would reach the snapshot as visible text. A real newline is
+        # the only separator that survives there - and with no separator at all every match
+        # ran straight into the next on a single line, see #4477.
+        if not len(html_block):
+            return ""
+
+        # xpath_filter/xpath1_filter serialise whole elements with
+        # etree.tostring(pretty_print=True), which already ends every match with a newline.
+        # Adding one here as well would double-space those matches, so only separate when
+        # the accumulated output does not already end on a line break. Attribute and
+        # text() matches return plain strings and do need the separator.
+        return "" if html_block.endswith("\n") else "\n"
+
+    if not len(html_block) or tag in FILTER_TAGS_WITH_OWN_NEWLINE:
+        return ""
+
+    return TEXT_FILTER_LIST_LINE_SUFFIX
+
 # Given a CSS Rule, and a blob of HTML, return the blob of HTML that matches
 def include_filters(include_filters, html_content, append_pretty_line_formatting=False):
     from bs4 import BeautifulSoup
@@ -249,8 +285,7 @@ def include_filters(include_filters, html_content, append_pretty_line_formatting
         # And where the matched result doesn't include something that will cause Inscriptis to add a newline
         # (This way each 'match' reliably has a new-line in the diff)
         # Divs are converted to 4 whitespaces by inscriptis
-        if append_pretty_line_formatting and len(html_block) and not element.name in (['br', 'hr', 'div', 'p']):
-            html_block += TEXT_FILTER_LIST_LINE_SUFFIX
+        html_block += filter_match_separator(append_pretty_line_formatting, html_block, getattr(element, 'name', None))
 
         html_block += str(element)
 
@@ -422,8 +457,7 @@ def xpath_filter(xpath_filter, html_content, append_pretty_line_formatting=False
             # And where the matched result doesn't include something that will cause Inscriptis to add a newline
             # (This way each 'match' reliably has a new-line in the diff)
             # Divs are converted to 4 whitespaces by inscriptis
-            if append_pretty_line_formatting and len(html_block) and (not hasattr( element, 'tag' ) or not element.tag in (['br', 'hr', 'div', 'p'])):
-                html_block += TEXT_FILTER_LIST_LINE_SUFFIX
+            html_block += filter_match_separator(append_pretty_line_formatting, html_block, getattr(element, 'tag', None))
 
             if type(element) == str:
                 html_block += element
@@ -483,8 +517,7 @@ def xpath1_filter(xpath_filter, html_content, append_pretty_line_formatting=Fals
             # And where the matched result doesn't include something that will cause Inscriptis to add a newline
             # (This way each 'match' reliably has a new-line in the diff)
             # Divs are converted to 4 whitespaces by inscriptis
-            if append_pretty_line_formatting and len(html_block) and (not hasattr(element, 'tag') or not element.tag in (['br', 'hr', 'div', 'p'])):
-                html_block += TEXT_FILTER_LIST_LINE_SUFFIX
+            html_block += filter_match_separator(append_pretty_line_formatting, html_block, getattr(element, 'tag', None))
 
             # Some kind of text, UTF-8 or other
             if isinstance(element, (str, bytes)):

changedetectionio/tests/unit/test_filter_match_separator.py

@@ -0,0 +1,148 @@
+#!/usr/bin/env python3
+import unittest
+
+from changedetectionio import html_tools
+
+# A page whose <link> elements give five distinct href attribute values, so a multi-match
+# filter has something to separate. Paraphrased from #4477.
+LINK_PAGE = """<html><head>
+<link rel="apple-touch-icon" href="/static/apple-touch/wikipedia.png">
+<link rel="icon" href="/static/favicon/wikipedia.ico">
+<link rel="license" href="//creativecommons.org/licenses/by-sa/4.0/">
+<link rel="stylesheet" href="//upload.wikimedia.org/style.css">
+<link rel="alternate" href="https://wikis.world/@wikipedia">
+</head><body>Page body</body></html>"""
+
+HREFS = ['/static/apple-touch/wikipedia.png',
+         '/static/favicon/wikipedia.ico',
+         '//creativecommons.org/licenses/by-sa/4.0/',
+         '//upload.wikimedia.org/style.css',
+         'https://wikis.world/@wikipedia']
+
+
+class TestSourceTypeFilterSeparator(unittest.TestCase):
+    """#4477: a `source:` watch runs an XPath/CSS filter over more than one match.
+
+    processor.py passes append_pretty_line_formatting=not self.watch.is_source_type_url, and
+    for a `source:` watch the filtered text is then kept verbatim instead of going through
+    html_to_text(). So TEXT_FILTER_LIST_LINE_SUFFIX ('<br>') cannot be used - it would show
+    up as literal text - and the previous code added no separator at all, which ran every
+    match into the next on a single line.
+    """
+
+    def test_xpath_multiple_matches_are_one_per_line(self):
+        filtered = html_tools.xpath_filter(
+            xpath_filter='//link/@href',
+            html_content=LINK_PAGE,
+            append_pretty_line_formatting=False,
+        )
+
+        self.assertNotIn('<br>', filtered, "'<br>' would leak into a source: snapshot as text")
+        self.assertEqual(HREFS, filtered.splitlines())
+
+    def test_xpath1_multiple_matches_are_one_per_line(self):
+        filtered = html_tools.xpath1_filter(
+            xpath_filter='//link/@href',
+            html_content=LINK_PAGE,
+            append_pretty_line_formatting=False,
+        )
+
+        self.assertNotIn('<br>', filtered)
+        self.assertEqual(HREFS, filtered.splitlines())
+
+    def test_css_multiple_matches_are_one_per_line(self):
+        filtered = html_tools.include_filters(
+            include_filters='link',
+            html_content=LINK_PAGE,
+            append_pretty_line_formatting=False,
+        )
+
+        self.assertNotIn('<br>', filtered)
+        self.assertEqual(5, len(filtered.splitlines()))
+        for href in HREFS:
+            self.assertIn(href, filtered)
+
+    def test_single_match_gets_no_separator(self):
+        """One match means there is nothing to separate, so no leading newline either."""
+        filtered = html_tools.xpath_filter(
+            xpath_filter='//link[@rel="icon"]/@href',
+            html_content=LINK_PAGE,
+            append_pretty_line_formatting=False,
+        )
+
+        self.assertEqual('/static/favicon/wikipedia.ico', filtered)
+
+    def test_no_matches_gets_empty_output(self):
+        filtered = html_tools.xpath_filter(
+            xpath_filter='//link[@rel="does-not-exist"]/@href',
+            html_content=LINK_PAGE,
+            append_pretty_line_formatting=False,
+        )
+
+        self.assertEqual('', filtered)
+
+    def test_xpath_element_matches_are_not_double_spaced(self):
+        """xpath_filter serialises elements with etree.tostring(pretty_print=True), which
+        already ends each match with a newline. The separator must not add a second one,
+        which would leave every matched element separated by a blank line.
+
+        The page is written without whitespace between the <link> elements on purpose: the
+        newline between them in LINK_PAGE is the element's own tail, and tostring emits it,
+        so the output is single-spaced regardless of which separator is used."""
+        compact = '<html><head>' + ''.join(
+            f'<link rel="x" href="{href}">' for href in HREFS) + '</head></html>'
+
+        for filtered in (
+            html_tools.xpath_filter(xpath_filter='//link', html_content=compact,
+                                    append_pretty_line_formatting=False),
+            html_tools.xpath1_filter(xpath_filter='//link', html_content=compact,
+                                     append_pretty_line_formatting=False),
+        ):
+            self.assertNotIn('<br>', filtered)
+            self.assertNotIn('\n\n', filtered, 'matched elements must not be double-spaced')
+            self.assertEqual(5, len(filtered.splitlines()))
+            for href in HREFS:
+                self.assertIn(href, filtered)
+
+    def test_xpath_element_matches_match_the_pre_fix_spacing(self):
+        """Element matches must come out exactly as they did before this branch, i.e. the
+        separator is a no-op for them and only attribute/text() matches are affected."""
+        compact = '<html><head>' + ''.join(
+            f'<link rel="x" href="{href}">' for href in HREFS) + '</head></html>'
+
+        filtered = html_tools.xpath_filter(xpath_filter='//link', html_content=compact,
+                                          append_pretty_line_formatting=False)
+        self.assertEqual('\n'.join(f'<link rel="x" href="{href}">' for href in HREFS) + '\n',
+                         filtered)
+
+
+class TestFilterSeparatorHelper(unittest.TestCase):
+    """The separator decision itself, including the unchanged Inscriptis path."""
+
+    def test_first_match_never_gets_a_separator(self):
+        for pretty in (True, False):
+            self.assertEqual('', html_tools.filter_match_separator(pretty, "", None))
+            self.assertEqual('', html_tools.filter_match_separator(pretty, "", 'div'))
+
+    def test_pretty_path_keeps_using_the_br_marker(self):
+        self.assertEqual(html_tools.TEXT_FILTER_LIST_LINE_SUFFIX,
+                         html_tools.filter_match_separator(True, 'something', None))
+
+    def test_pretty_path_leaves_tags_that_break_a_line_alone(self):
+        for tag in html_tools.FILTER_TAGS_WITH_OWN_NEWLINE:
+            self.assertEqual('', html_tools.filter_match_separator(True, 'something', tag))
+
+    def test_non_pretty_path_uses_a_newline(self):
+        self.assertEqual("\n", html_tools.filter_match_separator(False, 'something', None))
+        # Even for tags that would break a line under Inscriptis - it never runs here, and a
+        # '<br>' is not a newline in the verbatim text.
+        self.assertEqual("\n", html_tools.filter_match_separator(False, 'something', 'div'))
+
+    def test_non_pretty_path_adds_nothing_when_output_already_ends_on_a_newline(self):
+        """xpath element matches are already newline-terminated by etree.tostring."""
+        self.assertEqual('', html_tools.filter_match_separator(False, '<link>\n', 'link'))
+        self.assertEqual('', html_tools.filter_match_separator(False, '<link>\n', None))
+
+
+if __name__ == '__main__':
+    unittest.main()