Issue 6877 - Only parse headers in the first line of the filter list

abp/filters/parser.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # 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
@@ skipping 124 matching lines @@
 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*!\s*(.*?)\s*:\s*(.*)')
 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
-HEADER_REGEXP = re.compile(r'\[(?:(Adblock(?:\s*Plus\s*[\d\.]+?)?)|.*)\]$',

[Sebastian Noack, 2018/09/15 16:08:32]

I changed this regular epxressions like this in my patch so that I don't have to
first check whether the line starts and ends with square brackets. With your
implementation this seems redundant.

[Vasily Kuznetsov, 2018/09/17 10:40:27]

Yeah, you're right. I think the logic of parse_line() is more clear the way it
is so I will undo the regexp change.

[Vasily Kuznetsov, 2018/09/18 12:41:14]

Done.

+HEADER_REGEXP = re.compile(r'\[(?:(Adblock(?:\s*Plus\s*[\d\.]+?)?)|.*)\]',
                            flags=re.I)
 HIDING_FILTER_REGEXP = re.compile(r'^([^/*|@"!]*?)#([@?])?#(.+)$')
 FILTER_OPTIONS_REGEXP = re.compile(
     r'\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$'
 )
-
-
-def _parse_header(text):
-    match = HEADER_REGEXP.match(text)
-    version = match.group(1) if match else None
-    if not version:
-        raise ParseError('Malformed header', text)
-    return Header(version)
 
 
 def _parse_instruction(text):
     match = INCLUDE_REGEXP.match(text)
     if not match:
         raise ParseError('Unrecognized instruction', text)
     return Include(match.group(1))
 
 
 def _parse_option(option):
@@ skipping 77 matching lines @@
         Parsed filter.
 
     """
     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, mode='body'):

[Sebastian Noack, 2018/09/15 16:08:32]

Having the "mode" as part of the public API, requires to document it (you did
that below), and probably also calls for more tests than we currently have. By
not exposing this implementation detail, the public API (and it's documentation
and tests) can be simpler. Also we wouldn't need to validate the value below.

[Vasily Kuznetsov, 2018/09/17 10:40:27]

I would like to keep the mode in the public API. This way full functionality or
line parsing is available to the users and I think it's a good thing. You're
right about the tests, however, I will add them.

+def parse_line(line, position='body'):
     """Parse one line of a filter list.
 
-    The types of lines that that the parser recognizes depend on the mode. In
-    body mode the parser only recognizes filters, comments, processing
-    instructions and empty lines. In medata mode it in addition recognizes
-    metadata. In start mode it also recognizes headers.
-
-    Note: checksum metadata lines are recognized in all modes for backwards
+    The types of lines that that the parser recognizes depend on the position.
+    If position="body", the parser only recognizes filters, comments,
+    processing instructions and empty lines. If position="metadata", it in
+    addition recognizes metadata. If position="start", it also recognizes
+    headers.
+
+    Note: Checksum metadata lines are recognized in all positions for backwards
     compatibility. Historically, checksums can occur at the bottom of the
     filter list. They are are no longer used by Adblock Plus, but in order to
     strip them (in abp.filters.renderer), we have to make sure to still parse
     them regardless of their position in the filter list.
 
     Parameters
     ----------
-    line_text : str
+    line : str
         Line of a filter list.
-    mode : str
-        Parsing mode, one of "start", "metadata" or "body" (default).
+    position : str
+        Position in the filter list, one of "start", "metadata" or "body"
+        (default is "body").
 
     Returns
     -------
     namedtuple
         Parsed line (see `_line_type`).
 
     Raises
     ------
     ParseError
         ParseError: If the line can't be parsed.
 
     """
-    MODES = {'body', 'start', 'metadata'}
-    if mode not in MODES:
-        raise ValueError('mode should be one of {}'.format(MODES))
-
-    if isinstance(line_text, type(b'')):
-        line_text = line_text.decode('utf-8')
-
-    content = line_text.strip()
-
-    if content == '':
+    POSITIONS = {'body', 'start', 'metadata'}
+    if position not in POSITIONS:
+        raise ValueError('position should be one of {}'.format(POSITIONS))
+
+    if isinstance(line, type(b'')):
+        line = line.decode('utf-8')
+
+    stripped = line.strip()
+
+    if stripped == '':
         return EmptyLine()
 
-    if content.startswith('!'):
-        match = METADATA_REGEXP.match(line_text)
+    if position == 'start':
+        match = HEADER_REGEXP.search(line)
+        if match:
+            version = match.group(1)
+            if not version:
+                raise ParseError('Malformed header', line)
+            return Header(version)
+
+    if stripped.startswith('!'):
+        match = METADATA_REGEXP.match(line)
         if match:
             key, value = match.groups()
-            if mode != 'body' or key.lower() == 'checksum':

[Sebastian Noack, 2018/09/15 16:08:32]

We probably should keep the comment why we treat checksums special here.

[Vasily Kuznetsov, 2018/09/17 10:40:27]

I would like to also keep the note about checksums in the docstring also. Do you
think it would be ok to just refer to the docstring here?

[Sebastian Noack, 2018/09/17 18:11:52]

I didn't notice that you moved that note to the docstring. I personally, would
rather kept it here, but fair enough.

[Vasily Kuznetsov, 2018/09/18 12:41:14]

It needs to be in the docstring because it's part of the external behavior /
API. I agree that mentioning it here is helpful, so I'll do both.

+            if position != 'body' or key.lower() == 'checksum':
                 return Metadata(key, value)
-        return Comment(content[1:].lstrip())
-
-    if content.startswith('%') and content.endswith('%'):
-        return _parse_instruction(content)
-
-    if mode == 'start' and content.startswith('[') and content.endswith(']'):
-        return _parse_header(content)
-
-    return parse_filter(content)
+        return Comment(stripped[1:].lstrip())
+
+    if stripped.startswith('%') and stripped.endswith('%'):
+        return _parse_instruction(stripped)
+
+    return parse_filter(stripped)
 
 
 def parse_filterlist(lines):
     """Parse filter list from an iterable.
 
     Parameters
     ----------
     lines: iterable of str
         Lines of the filter list.
 
     Returns
     -------
     iterator of namedtuple
         Parsed lines of the filter list.
 
     Raises
     ------
     ParseError
         Thrown during iteration for invalid filter list lines.
     TypeError
         If `lines` is not iterable.
 
     """
-    mode = 'start'

[Sebastian Noack, 2018/09/17 18:11:52]

Maybe "position" would be more accurate name for this variable? Also instead of
"start" maybe "header" would be more in line with the other values.

[Vasily Kuznetsov, 2018/09/18 12:41:14]

Yeah, "position" is a better name. I changed it.

As for "start" vs. "header" -- I have chosen the former because there's not
always a header in the file, so giving the impression that we're only looking
for a header seems misleading. 

+    position = 'start'
 
     for line in lines:
-        parsed_line = parse_line(line, mode)
+        parsed_line = parse_line(line, position)
         yield parsed_line
 
-        if mode != 'body' and parsed_line.type in {'header', 'metadata'}:
-            # Continue parsing metadata if it's not over...
-            mode = 'metadata'
+        if position != 'body' and parsed_line.type in {'header', 'metadata'}:
+            # Continue parsing metadata until it's over...
+            position = 'metadata'
         else:
-            # ...otherwise switch to parsing filter list body.
-            mode = 'body'
+            # ...then switch to parsing the body.
+            position = 'body'