Issue 6685 - Offer incremental filter list downloads

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 122 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*(\w+)\s*:\s*(.*)')
+METADATA_REGEXP = re.compile(r'!\s*([\w-]+)\s*:\s*(.*)')
 METADATA_KEYS = {'Homepage', 'Title', 'Expires', 'Checksum', 'Redirect',
-                 'Version'}
+                 'Version', 'Diff-URL', 'Diff-Expires'}

[Sebastian Noack, 2018/08/21 19:42:45]

I would prefer if python-abp would be agnostic of the supported metadata keys.
Otherwise, we have to keep this set in sync with what is implemented in Adblock
Plus, which seems unpractical.

However, note that the regexp above would have to be changed like following so
that it won't match lines with arbitrary URLs: r'!\s*(\w+)\s*:(?!//)\s*(.*)'

[Vasily Kuznetsov, 2018/08/22 13:10:50]

Makes sense and I support it. I actually thought it was already implemented this
way before I started reviewing this change :)

BTW, the regexp should probably be r'!\s*([\w-]+)\s*:(?!//)\s*(.*)' so that we
can still have dashes in metadata field names.

[Sebastian Noack, 2018/08/22 14:31:06]

You are right, well spotted.

[rhowell, 2018/08/27 22:06:26]

It appears this was done to prevent mistaking a comment with a colon for
metadata, like in this test:

def test_parse_nonmeta():
    line = parse_line('! WrongHeader: something')
    assert line.type == 'comment'

It probably doesn't matter too much, since the parser wouldn't be able to parse
invalid metadata anyway. I'll update the test so it doesn't use a colon.

 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
 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_comment(text):
     match = METADATA_REGEXP.match(text)
     if match and match.group(1) in METADATA_KEYS:

[Sebastian Noack, 2018/08/21 19:42:46]

Note that metadata keys are case-insensitive. If we go with my above suggestion,
this won't matter here anymore. But once non-capitalized keys are recognized
this needs to be accounted for in _remove_duplicates() in renderer.py as well.

[Vasily Kuznetsov, 2018/08/22 13:10:50]

Should we then maybe adopt some canonical capitalization scheme (for example
Like-This) and then convert them here to follow it? Otherwise we will have to
remember this in any code that works with metadata keys.

[Sebastian Noack, 2018/08/22 14:31:06]

Initially, I had a similar though, but on the other hand I'd prefer to not
unnecessarily have the output diverge from the input where (easily) possible,
and it seems the only place where we specifically check for a particular
metadata key is where we strip checksums, besides we'd have to account for
case-insensitivity as well when comparing metadata for the diff.

[Vasily Kuznetsov, 2018/08/24 13:03:19]

The output that doesn't diverge from the input unnecessarily seems like a useful
feature. In principle, if we normalize the headers, eventually all "our" filter
lists will have normalized headers and they won't diverge anyway, but this might
not be the case just yet. Anyway, for the time being it's not a big deal and
it's unlikely to result in problems in the future so I don't feel too strongly
about this.

[rhowell, 2018/08/27 22:06:26]

Acknowledged.

         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', text)
     return Header(match.group(1))
 
@@ skipping 115 matching lines @@
         line_text = line_text.decode('utf-8')
 
     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(']'):

[Sebastian Noack, 2018/08/21 19:42:45]

Somewhat unrelated of these changes, but this is inaccurate too. Only the first
line (if encapsulated in brackets) is to be recognized as header. Otherwise it
would be considered a filter.

[Vasily Kuznetsov, 2018/08/22 13:10:50]

Makes sense. I created a separate ticket for it:
https://issues.adblockplus.org/ticket/6877

[rhowell, 2018/08/27 22:06:26]

Acknowledged.

         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.
@@ skipping 11 matching lines @@
     Raises
     ------
     ParseError
         Thrown during iteration for invalid filter list lines.
     TypeError
         If `lines` is not iterable.
 
     """
     for line in lines:
         yield parse_line(line)