Issue 6685 - Offer incremental filter list downloads

abp/filters/renderer.py

Index: abp/filters/renderer.py
===================================================================
--- a/abp/filters/renderer.py
+++ b/abp/filters/renderer.py
@@ -199,3 +199,41 @@
                  _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)