Fixes 4969 - Add parsing of filters

abp/filters/parser.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
-# Copyright (C) 2006-2017 eyeo GmbH
+# Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 from __future__ import unicode_literals
 
 import re
 from collections import namedtuple
 
-__all__ = ['parse_filterlist', 'parse_line', 'parse_filter']
+__all__ = [
+    'FILTER_ACTION',
+    'FILTER_OPTION',
+    'ParseError',
+    'SELECTOR_TYPE',
+    'parse_filterlist',
+    'parse_line',
+]
 
 
 class ParseError(Exception):
-    """Internal exception used by the parser to signal invalid input."""

[mathias, 2017/07/26 20:37:15]

Removing the custom __init__ function looks like an unrelated change to me.
Don't you think it should be part of a separate patch-set?

-
-
-def line_type(name, field_names, format_string):
+    """Exception thrown by the parser when it encounters invalid input.
+
+    :param error: Description of the error.
+    :param text: The text which was being parsed when an error occurred.
+    """
+
+    def __init__(self, error, text):
+        Exception.__init__(self, '{} in "{}"'.format(error, text))
+        self.text = text
+        self.error = error
+
+
+# Constants related to filters (see https://adblockplus.org/filters).
+class SELECTOR_TYPE:  # flake8: noqa (This class is an enumeration constant).
+    """Selector types"""
+    URL_PATTERN = 'url-pattern'  # Normal URL patterns.
+    URL_REGEXP = 'url-regexp'    # Regular expressions for URLs.
+    CSS = 'css'                  # CSS selectors for hiding filters.
+    XCSS = 'extended-css'        # Extended CSS selectors (to emulate CSS4).
+    ABP_SIMPLE = 'abp-simple'    # Simplified element hiding syntax.
+
+
+class FILTER_ACTION:  # flake8: noqa (This class is an enumeration constant).
+    """Filter actions"""
+    BLOCK = 'block'              # Block the request.
+    ALLOW = 'allow'              # Allow the request (whitelist).
+    HIDE = 'hide'                # Hide selected element(s).
+    SHOW = 'show'                # Show selected element(s) (whitelist).
+
+
+class FILTER_OPTION:  # flake8: noqa (This class is an enumeration constant).
+    """Filter options"""
+    # Resource types.
+    OTHER = 'other'
+    SCRIPT = 'script'
+    IMAGE = 'image'
+    STYLESHEET = 'stylesheet'
+    OBJECT = 'object'
+    SUBDOCUMENT = 'subdocument'
+    DOCUMENT = 'document'
+    WEBSOCKET = 'websocket'
+    WEBRTC = 'webrtc'
+    PING = 'ping'
+    XMLHTTPREQUEST = 'xmlhttprequest'
+    OBJECT_SUBREQUEST = 'object-subrequest'
+    MEDIA = 'media'
+    FONT = 'font'
+    POPUP = 'popup'
+    GENERICBLOCK = 'genericblock'
+    ELEMHIDE = 'elemhide'
+    GENERICHIDE = 'generichide'
+
+    # Deprecated resource types.
+    BACKGROUND = 'background'
+    XBL = 'xbl'
+    DTD = 'dtd'
+
+    # Other options.
+    MATCH_CASE = 'match-case'
+    DOMAIN = 'domain'
+    THIRD_PARTY = 'third-party'
+    COLLAPSE = 'collapse'
+    SITEKEY = 'sitekey'
+    DONOTTRACK = 'donottrack'
+
+
+def _line_type(name, field_names, format_string):
     """Define a line type.
 
     :param name: The name of the line type to define.
     :param field_names: A sequence of field names or one space-separated
         string that contains all field names.
     :param format_string: A format specifier for converting this line type

[mathias, 2017/07/26 20:37:15]

Fixing the missing format_string parameter documentation looks like an unrelated
change as well. So what about this one?

         back to string representation.
     :returns: Class created with `namedtuple` that has `.type` set to
         lowercased `name` and supports conversion back to string with
         `.to_string()` method.
     """
     lt = namedtuple(name, field_names)
     lt.type = name.lower()
     lt.to_string = lambda self: format_string.format(self)
     return lt
 
 
-InvalidLine = line_type('Invalid', 'text error', '{.text}')
-Header = line_type('Header', 'version', '[{.version}]')
-EmptyLine = line_type('EmptyLine', '', '')
-Comment = line_type('Comment', 'text', '! {.text}')
-Metadata = line_type('Metadata', 'key value', '! {0.key}: {0.value}')
-Include = line_type('Include', 'target', '%include {0.target}%')
-Filter = line_type('Filter', 'text selector action options', '{.text}')
+Header = _line_type('Header', 'version', '[{.version}]')
+EmptyLine = _line_type('EmptyLine', '', '')
+Comment = _line_type('Comment', 'text', '! {.text}')
+Metadata = _line_type('Metadata', 'key value', '! {0.key}: {0.value}')
+Filter = _line_type('Filter', 'text selector action options', '{.text}')
+Include = _line_type('Include', 'target', '%include {0.target}%')
 
 
 METADATA_REGEXP = re.compile(r'!\s*(\w+)\s*:\s*(.*)')
 METADATA_KEYS = {'Homepage', 'Title', 'Expires', 'Checksum', 'Redirect',
                  'Version'}
 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
 HEADER_REGEXP = re.compile(r'\[(Adblock(?:\s*Plus\s*[\d\.]+?)?)\]', flags=re.I)
-BFILTER_OPTIONS_REGEXP = re.compile(
-    r'\$(~?[\w\-]+(?:=[^,\s]+)?(?:,~?[\w\-]+(?:=[^,\s]+)?)*)$'
+HIDING_FILTER_REGEXP = re.compile(r'^([^/*|@"!]*?)#([@?])?#(.+)$')
+FILTER_OPTIONS_REGEXP = re.compile(
+    r'\$(~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)$'
 )
-HFILTER_REGEXP = re.compile(
-    r'^([^\/\*\|\@"!]*?)#(\@)?(?:([\w\-]+|\*)'
-    r'((?:\([\w\-]+(?:[$^*]?=[^\(\)"]*)?\))*)|#([^{}]+))$'
-)
-
-# Types of resources to block (based on adblockpluscore/lib/filterClasses.js).
-TYPES = {
-    'font', 'websocket', 'object-subrequest', 'script', 'elemhide', 'media',
-    'image', 'object', 'ping', 'genericblock', 'stylesheet', 'other', 'popup',
-    'xmlhttprequest', 'document', 'webrtc', 'subdocument', 'generichide',
-}
-
-# Special types used for whitelisting.
-TYPES_WHITELIST = {
-    'document', 'elemhide', 'generichide', 'genericblock',
-}
-
-# By default blocking filters apply to everything except whitelist-only types
-# and popups (based on adblockpluscore/lib/filterClasses.js).
-TYPES_DEFAULT = {t for t in TYPES if t not in TYPES_WHITELIST}
-
-# Type options that are synonyms for other types.
-TYPE_SYNONYMS = {
-    'xbl': 'other',
-    'dtd': 'other',
-    'background': 'image',
-}
 
 
 def _parse_comment(text):
     match = METADATA_REGEXP.match(text)
     if match and match.group(1) in METADATA_KEYS:
         return Metadata(match.group(1), match.group(2))
     return Comment(text[1:].strip())
 
 
 def _parse_header(text):
     match = HEADER_REGEXP.match(text)
     if not match:
-        raise ParseError('Malformed header')

[mathias, 2017/07/26 20:37:15]

Please explain why you don't include the malformed text in the error message
here (and the similar functions below). I was under the impression that such
kind of information is highly appreciated because Python exception trace output
does not include invocation parameters, hence reproduction without can is often
quite tricky.

[Vasily Kuznetsov, 2017/07/27 11:05:02]

My reasoning was that you can get to this place in two ways:
1. by calling `parse_line(stuff)` directly: in this case the caller should
expect to get the exception (it's documented in the docstring of `parse_line`)
and handle it more or less in the same scope, where they know what was passed to
`parse_line`.
2. by calling `parse_filterlist(list_of_stuff)`: in this case the exception gets
caught inside of `parse_filterlist` and the result is an `InvalidLine` object,
that contains the original line.

Looking at it now, it seems to me that the assumption that the user will handle
the exception before they lose track of what was passed to `parse_line`, that I
made in considering case 1, is too optimistic. I will change the exception to be
more user-friendly. This should also resolve your concern about the disappearing
`__init__` in line 25.

+        raise ParseError('Malformed header', text)
     return Header(match.group(1))
 
 
 def _parse_instruction(text):
     match = INCLUDE_REGEXP.match(text)
     if not match:
-        raise ParseError('Unrecognized instruction')
+        raise ParseError('Unrecognized instruction', text)
     return Include(match.group(1))
 
 
-def _separate_domains(domains):
-    options = {}
-    for d in domains:
-        if d.startswith('~'):
-            options.setdefault('domains-exclude', []).append(d.lstrip('~'))
-        else:
-            options.setdefault('domains-include', []).append(d)
-    if 'domains-include' in options:
-        options['domains-none'] = True
-    return options
-
-
-def _separate_types(types):
-    """Convert a list of `(type, on_off)` tuples to options:
-
-    - types-none: True if we start with nothing included, absent if we start
-                  with TYPES_DEFAULT included.
-    - types-include: List of additional included types.
-    - types-exclude: List of excluded types.
-    """
-    if not types:
-        return {}
-
-    if types[0][1]:  # If the first type is ON, we start with nothing...
-        types_default = set()
-        options = {'types-none': True}
-    else:            # ...otherwise we start with default type set.
-        types_default = TYPES_DEFAULT
-        options = {}
-
-    # Include/exclude any deviations from default.
-    for name, value in dict(types).items():
-        if value and name not in types_default:
-            options.setdefault('types-include', []).append(name)
-        if not value and name in types_default:
-            options.setdefault('types-exclude', []).append(name)
-
-    return options
-
-
-def _parse_hiding_filter(text, match):
-    if match.group(5):
-        selector = {'type': 'css', 'value': match.group(5)}
-    else:
-        selector = {
-            'type': 'abp-simple',
-            'value': match.group(3) + match.group(4),
-        }
-    action = 'show' if match.group(2) else 'hide'
-    options = _separate_domains(list(filter(None, match.group(1).split(','))))
-    return Filter(text, selector, action, options)
+def _parse_option(option):
+    if '=' in option:
+        return option.split('=', 1)
+    if option.startswith('~'):
+        return option[1:], False
+    return option, True
+
+
+def _parse_filter_option(option):
+    name, value = _parse_option(option)
+
+    # Handle special cases of multivalued options.
+    if name == FILTER_OPTION.DOMAIN:
+        value = [_parse_option(o) for o in value.split('|')]
+    elif name == FILTER_OPTION.SITEKEY:
+        value = value.split('|')
+
+    return name, value
 
 
 def _parse_filter_options(options):
-    # Based on RegExpFilter.fromText in lib/filterClasses.js
-    # in adblockpluscore.
-    parsed_options = {}
-    type_options = []
-
-    for option in options.split(','):
-        if '=' in option:
-            name, value = option.split('=', 1)
-        elif option.startswith('~'):
-            name, value = option[1:], False
-        else:
-            name, value = option, True
-
-        if name in TYPE_SYNONYMS:
-            name = TYPE_SYNONYMS[name]
-        if name in TYPES:
-            type_options.append((name, value))
-        elif name == 'domain':
-            parsed_options.update(_separate_domains(value.split('|')))
-        elif name == 'sitekey':
-            parsed_options['sitekeys'] = value.split('|')
-        else:
-            parsed_options[name] = value
-
-    parsed_options.update(_separate_types(type_options))
-    return parsed_options
+    return [_parse_filter_option(o) for o in options.split(',')]
 
 
 def _parse_blocking_filter(text):
     # Based on RegExpFilter.fromText in lib/filterClasses.js
-    # in adblockpluscore.
-    action = 'block'
-    options = {}
+    # in https://hg.adblockplus.org/adblockpluscore.
+    action = FILTER_ACTION.BLOCK
+    options = []
     selector = text
 
     if selector.startswith('@@'):
-        action = 'allow'

[mathias, 2017/07/26 20:37:15]

I think we should have symbols like BFILTER_ACTION_ALLOW and
BFILTER_ACTION_BLOCK for actions asslociated with blocking filters, analogous to
HFILTER_ACTION_HIDE and HFILTER_ACTION_SHOW for element hiding filters above.

[Vasily Kuznetsov, 2017/07/27 11:05:02]

Probably not these exact names for the constants, but in general I agree,
constants are better than magic strings.

+        action = FILTER_ACTION.ALLOW
         selector = selector[2:]
 
     if '$' in selector:
-        opt_match = BFILTER_OPTIONS_REGEXP.search(selector)
+        opt_match = FILTER_OPTIONS_REGEXP.search(selector)
         if opt_match:
             selector = selector[:opt_match.start(0)]
             options = _parse_filter_options(opt_match.group(1))
 
     if (len(selector) > 1 and
             selector.startswith('/') and selector.endswith('/')):
-        selector = {'type': 'url-regexp', 'value': selector[1:-1]}

[mathias, 2017/07/26 20:37:15]

I also think we should have symbols like SELECTOR_TYPE_REGEXP and
SELECTOR_TYPE_PATTERN. And be it just to have a place to document them like #
http://link/to/explanation or an actual definition or something, or for IDE's to
recognize them symbols, or plain old helping humans with the association by
creating an official item.

[Vasily Kuznetsov, 2017/07/27 11:05:02]

Acknowledged.

+        selector = {'type': SELECTOR_TYPE.URL_REGEXP, 'value': selector[1:-1]}
     else:
-        selector = {'type': 'url-pattern', 'value': selector}
+        selector = {'type': SELECTOR_TYPE.URL_PATTERN, 'value': selector}
+
+    return Filter(text, selector, action, options)
+
+
+def _parse_hiding_filter(text, domain, type_flag, selector_value):
+    selector = {'type': SELECTOR_TYPE.CSS, 'value': selector_value}
+    action = FILTER_ACTION.HIDE
+    options = []
+
+    if type_flag == '@':
+        action = FILTER_ACTION.SHOW
+    elif type_flag == '?':
+        selector['type'] = SELECTOR_TYPE.XCSS
+
+    if domain:
+        domains = [_parse_option(d) for d in domain.split(',')]
+        options.append((FILTER_OPTION.DOMAIN, domains))
 
     return Filter(text, selector, action, options)
 
 
 def parse_filter(text):
     """Parse one filter.
 
     :param text: Text representation of a filter.
-    :returns: filter object.
-    """
-    match = HFILTER_REGEXP.match(text) if '#' in text else False

[mathias, 2017/07/26 20:37:15]

Call me old-fashioned but I seriously dislike changing the type of a variable
(especially when the former and new value types quack quite differently). You
could just use None instead of False here, so it'll be match or no match, not
match or untruth.

[Vasily Kuznetsov, 2017/07/27 11:05:02]

Completely agree about changing the type of the variable. Here, as far as we're
concerned (the variable will only be used in the following `if`, if it's not a
proper match), the quacking is the same, but the following code could
potentially change, leading to subtle bugs. Thanks for catching this, I will fix
it.

-    if match:
-        return _parse_hiding_filter(text, match)
+    :returns: Filter object.
+    """
+    if '#' in text:
+        match = HIDING_FILTER_REGEXP.search(text)
+        if match:
+            return _parse_hiding_filter(text, *match.groups())
     return _parse_blocking_filter(text)
 
 
 def parse_line(line_text):
     """Parse one line of a filter list.
 
     :param line_text: Line of a filter list (must be a unicode string).
-    :returns: Parsed line object (see `line_type`).
+    :returns: Parsed line object (see `_line_type`).
     :raises ParseError: If the line can't be successfully parsed.
     """
     content = line_text.strip()
 
     if content == '':
         line = EmptyLine()
     elif content.startswith('!'):
         line = _parse_comment(content)
     elif content.startswith('%') and content.endswith('%'):
         line = _parse_instruction(content)
     elif content.startswith('[') and content.endswith(']'):
         line = _parse_header(content)
     else:
         line = parse_filter(content)
 
     assert line.to_string().replace(' ', '') == content.replace(' ', '')
     return line
 
 
 def parse_filterlist(lines):
     """Parse filter list from an iterable.
 
     :param lines: List of strings or file or other iterable.
     :returns: Iterator over parsed lines.
     :raises ParseError: Can be thrown during iteration for invalid lines.
     """
     for line in lines:
-        try:
-            yield parse_line(line)
-        except ParseError as pe:
-            yield InvalidLine(line.strip(), str(pe))
+        yield parse_line(line)