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
@@ skipping 106 matching lines @@
     """Insert metadata comment with version (a.k.a. date)."""
     first_line, rest = _first_and_rest(lines)
     version = Metadata('Version', time.strftime('%Y%m%d%H%M', time.gmtime()))
     return itertools.chain([first_line, version], rest)
 
 
 def _remove_duplicates(lines):
     """Remove duplicate metadata and headers."""
     # Always remove checksum -- a checksum coming from a fragment
     # will not match for the rendered list.
-    seen = {'Checksum'}
+    seen = {'checksum'}
     for i, line in enumerate(lines):
         if line.type == 'metadata':
-            if line.key not in seen:
-                seen.add(line.key)
+            key = line.key.lower()
+            if key not in seen:
+                seen.add(key)
                 yield line
         elif line.type == 'header':
             if i == 0:
                 yield line
         else:
             yield line
 
 
 def _validate(lines):
     """Validate the final list."""
@@ skipping 34 matching lines @@
     _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,
                  _validate]:
         lines = proc(lines)
     return lines
 
 
 def _split_list_for_diff(list_in):
-    filterlist, metadata, keys = (set() for i in range(3))

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

I'm not sure if maybe a slightly more readable version of this line would be:

    filterlist, metadata, keys = set(), set(), set()

or even:

    filterlist = set()
    metadata = set()
    keys = set()

I don't feel too strongly about it so feel free to keep your original version or
choose one of the proposals above as you like.

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

Done.

+    """Split a filter list into metadata, keys, and rules."""

[Vasily Kuznetsov, 2018/08/31 12:11:02]

Nit: strictly speaking this function now returns metadata and rules (the keys
are just part of metadata). The docstring could be updated.

[rhowell, 2018/08/31 21:21:02]

Ah, good point! I'll update that.

+    metadata = {}
+    rules = set()
     for line in parse_filterlist(list_in):

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

When parsing generated filter lists in order to generate diffs, we might want to
make the parser ignore includes/instructions.

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

Theoretically there should be no includes/instructions in the final lists (and
we're not planning to produce diffs of fragments of filter lists), so this is
not really necessary. And even if there were includes, the code below ignores
them, which achieves the same outcome as the parsing ignoring them, so it seems
that things are ok as is.

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

Granted, this is quite an edge case. But in theory if we'll find  "%include" in
the input the diff is generated from, then it seems somewhat more accurate to
just treat it as a filter (like Adblock Plus would) rather than stripping this
line (or perhaps a warning would be in order). But I don't feel too strong.

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

To do this we would need to start parsing fragments of filter lists differently
from rendered filter lists. It does make sense but it also makes things more
complicated and the cases where it's relevant seem to be pretty unrealistic and
edge-casey.

I lean towards keeping the code unified and parsing "%include"s as instructions
everywhere until we have evidence that there are cases where it would be useful
to make the distinction.

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

Yeah, I was working under the assumption that the full lists wouldn't have
%includes in them, and `%include` appears to be the only instruction the parser
knows. Are there any other instructions that might appear, that we might need to
handle? If not, I would agree with Vasily, that it's not necessary to handle
this case.

-        if line.type == 'metadata' and 'Checksum' not in line.to_string():
-            metadata.add(line.to_string())
-            keys.add(line.key)
+        if line.type == 'metadata':
+            metadata[line.key.lower()] = line
         elif line.type == 'filter':
-            filterlist.add(line.to_string())
-    return filterlist, metadata, keys
+            rules.add(line.to_string())
+    return metadata, rules
 
 
 def render_diff(base, latest):
     """Return a diff between two filter lists.
 
     Parameters
     ----------
     base : iterator of str
         The base (old) list that we want to update to latest.
     lastest : iterator  of str
         The latest (most recent) list that we want to update to.
 
     Returns
     -------
     iterable of str
         A diff between two lists (https://issues.adblockplus.org/ticket/6685)
 
     """
-    latest_fl, latest_md, latest_keys = _split_list_for_diff(latest)
-    base_fl, base_md, base_keys = _split_list_for_diff(base)
-
-    new_md = latest_md - base_md
-    removed_keys = base_keys - latest_keys
-    add_fl = latest_fl - base_fl
-    remove_fl = base_fl - latest_fl
+    latest_metadata, latest_rules = _split_list_for_diff(latest)
+    base_metadata, base_rules = _split_list_for_diff(base)
 
     yield '[Adblock Plus Diff]'
-    for item in new_md:
-        yield item
-    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
-        yield '! {}:'.format(key)
-    for item in add_fl:
-        yield '+ {}'.format(item)
-    for item in remove_fl:
-        yield '- {}'.format(item)

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

In the specification, we demand the client to process removed filters first. But
given this implementation, it seems simpler if we'd rather make the diff list
the removed filters first, so that the client can process the diff in one pass.

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

After discussing this on IRC, Vasily agrees. I updated the spec accordingly.

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

👍

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

Done.

+    for key, latest in latest_metadata.items():
+        base = base_metadata.get(key)
+        if not base or base.value != latest.value:
+            yield latest.to_string()
+    for key in set(base_metadata) - set(latest_metadata):
+        yield '! {}:'.format(base_metadata[key].key)
+    for rule in base_rules - latest_rules:
+        yield '- {}'.format(rule)
+    for rule in latest_rules - base_rules:
+        yield '+ {}'.format(rule)