Issue 6685 - Offer incremental filter list downloads

abp/filters/renderer.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
 # 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/>.
 
 """Combine filter list fragments to produce filter lists."""
 
 from __future__ import unicode_literals
 
 import base64
 import hashlib
 import itertools
 import logging
 import time
 
 from .parser import parse_filterlist, Comment, Metadata
 from .sources import NotFound
 
 __all__ = ['IncludeError', 'MissingHeader', 'render_filterlist']

[Vasily Kuznetsov, 2018/08/17 10:30:54]

render_diff() should probably become a public export here.

[rhowell, 2018/08/20 18:21:27]

Done.

 
 _logger = logging.getLogger(__name__)
 
 
 class IncludeError(Exception):
     """Error in processing include instruction."""
 
     def __init__(self, error, stack):
         stack_str = ' from '.join(map("'{}'".format, reversed(stack)))
         if stack_str:
@@ skipping 152 matching lines @@
         lead to rendering an invalid filter list, so we immediately abort.
 
     """
     _logger.info('Rendering: %s', name)
     lines, default_source = _get_and_parse_fragment(name, sources, top_source)
     lines = _process_includes(sources, default_source, [name], lines)
     for proc in [_process_timestamps, _insert_version, _remove_duplicates,
                  _insert_checksum, _validate]:
         lines = proc(lines)
     return lines
+
+
+def render_diff(base, latest):

[Vasily Kuznetsov, 2018/08/17 10:30:54]

I think it makes more sense to make `base`, `latest` and the return value
iterables of strings. This is what parse_filterlist() expects and this is what
you get when you open a file. This is also the API that other functions dealing
with filterlists take and return. It would also reduce the amount of code here
eliminating the calls to splitlines() and some others.

It would also be good to document the API of this function in the same way as
the other functions are documented.

[rhowell, 2018/08/20 18:21:27]

Done.

+    """Return a diff between two filter lists."""
+    # Collect the special comments
+    diff = ['[Adblock Plus Diff]\n']
+    latest_fl, latest_md, latest_keys = (set() for i in range(3))
+    base_fl, base_md, base_keys = (set() for i in range(3))
+
+    for line in parse_filterlist(latest.splitlines()):
+        if line.type == 'metadata' and 'Checksum' not in line.to_string():

[Vasily Kuznetsov, 2018/08/17 10:30:53]

Note: If this lands after the checksum patch, the 'Checksum' exception should be
removed. If this lands first, it should be removed by that patch.

[rhowell, 2018/08/20 18:21:27]

Are we guaranteed that no filter lists will be encountered that have a checksum?
This line and line 220 make sure that, if we ever see a checksum in either
`base` or `latest`, it will not make it into the diff. But, if we will never see
a checksum, then this check is unnecessary.

[Vasily Kuznetsov, 2018/08/21 14:59:59]

Hm. No, for now we're not guaranteed that there's no Checksum, so you're right,
we should keep this.

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

Isn't python-abp stripping checksums when generating the filter lists now, and
therefore isn't it in fact guaranteed that filter lists delivered by our servers
at least won't include any checksum? Even if not, any reason we'd have to strip
them here? Clients that support diffs, will ignore checksums anyway.

[Sebastian Noack, 2018/08/22 16:03:29]

Moving the discussion from IRC over here:

16:34:47 <•vasily> snoack: the second one I agree with you. For the first one,
at the moment, I think we have other code producing filter lists in production,
so they do have checksums. But we might convert all of this to python-abp
(perhaps together with deployment of the incremental updates) so you would also
be right then... however, we haven't yet done it.
16:36:03 <•vasily> snoack: in general seems like the checksums will be ignored
by diff-aware clients, so yeah, it can be removed.  
17:24:01 <snoack> vasily: How about ignoring checksum metadata in the parser
(rather than when rendering the full list and/or diff)? That way checksums would
be stripped no matter what, without any additional complexity.

+            latest_md.add(line.to_string())
+            latest_keys.add(line.key)
+        elif line.type == 'filter':
+            latest_fl.add(line.to_string())
+
+    # Get the diff between the rest of the lines
+    for line in parse_filterlist(base.splitlines()):

[Vasily Kuznetsov, 2018/08/17 10:30:53]

It would be good to move this repeated piece of code into a small internal
function. This will also make render_diff() more compact and easy to follow.
Maybe something like:

    base_fl, base_md, base_keys = _split_list_for_diff(base)

[rhowell, 2018/08/20 18:21:27]

Done.

+        if line.type == 'metadata' and 'Checksum' not in line.to_string():
+            base_md.add(line.to_string())
+            base_keys.add(line.key)
+        elif line.type == 'filter':
+            base_fl.add(line.to_string())
+    new_md = latest_md - base_md
+    removed_keys = base_keys - latest_keys
+    add_fl = latest_fl - base_fl
+    remove_fl = base_fl - latest_fl
+    for item in new_md:
+        diff.append('{}\n'.format(item))

[Vasily Kuznetsov, 2018/08/17 10:30:53]

If we return an iterable of strings from this function we can just yield in this
and the following loops.

[rhowell, 2018/08/20 18:21:27]

Done.

+    for key in removed_keys:
+        # If a special comment has been removed, enter it with a blank value
+        # so the client will set it back to the default value
+        diff.append('! {}:\n'.format(key))
+    for item in add_fl:
+        diff.append('+ {}\n'.format(item))
+    for item in remove_fl:
+        diff.append('- {}\n'.format(item))
+    return ''.join(diff)