Issue 3176 - Add metadata to content blocker lists

sitescripts/content_blocker_lists/bin/generate_lists.py

 #!/usr/bin/env python
 # coding: utf-8
 
 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-2015 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/>.
 
+from collections import OrderedDict
 from contextlib import closing
-from datetime import datetime
 import json
 import os
-from StringIO import StringIO
 import subprocess
+import threading
+import time
 import re
 import urllib2
 
 from sitescripts.utils import get_config
 
 config = dict(get_config().items("content_blocker_lists"))
 
 def update_abp2blocklist():
   with open(os.devnull, "w") as devnull:
     abp2blocklist_path = config["abp2blocklist_path"]
     if os.path.isdir(abp2blocklist_path):
         subprocess.check_call(("hg", "pull", "-u", "-R", abp2blocklist_path),
                               stdout=devnull)
     else:
       subprocess.check_call(("hg", "clone", config["abp2blocklist_url"],
                              abp2blocklist_path), stdout=devnull)
     subprocess.check_call(("npm", "install"), cwd=abp2blocklist_path,
                           stdout=devnull)
 
 def download_filter_list(url):
-  filter_list = {}
   with closing(urllib2.urlopen(url)) as response:
-    filter_list["body"] = response.read()
-  filter_list["header"] = parse_filter_list_header(filter_list["body"])
-  filter_list["header"]["url"] = url
-  return filter_list
+    body = response.read()
+  version = re.search(r"^(?:[^[!])|^!\s*Version:\s*(.+)$",
+                      body, re.MULTILINE).group(1)
+  return body, url, version
 
-def parse_filter_list_header(filter_list):
-  field_re = re.compile(r"^!\s*([^:]+):\s*(.+)$")
-  with closing(StringIO(filter_list)) as stream:

[Sebastian Noack, 2015/11/30 13:55:49]

Never mind closing a StringIO. It doesn't do anything that wouldn't happen
anyway. There are no resources that needs to be released associated with a
StringIO. The close method merely exist for compatibility.

Wait.., why do you use a StringIO in the first place? How about simply using a
multi-line regexp?

But wait again.., why do you parse the filter list headers in the first place?
We don't need any of these data in the generated block list.

[kzar, 2015/11/30 15:13:11]

We need the Version field, but otherwise Done.

[Sebastian Noack, 2015/11/30 15:49:19]

Well, you set the version field based on the current timestamp. So there is no
need to parse the source filter lists. Or do I miss something?

[kzar, 2015/11/30 17:06:00]

That's the version for the block list, in the sources section we keep a record
of the version of each filter list.

[Felix Dahlke, 2015/12/01 08:32:56]

You still won't have to parse the header of the files, you can read the version
from the "ABP-Notification-Version" HTTP header in the response.

[kzar, 2015/12/01 12:13:38]

I don't see that header present? 

curl -I https://easylist-downloads.adblockplus.org/easylist_noadult.txt

HTTP/1.1 200 OK
Server: nginx
Date: Tue, 01 Dec 2015 11:24:23 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 1532799
Last-Modified: Tue, 01 Dec 2015 11:20:46 GMT
Connection: close
ETag: "565d828e-17637f"
Strict-Transport-Security: max-age=31536000
Accept-Ranges: bytes

[Felix Dahlke, 2015/12/01 14:04:19]

Ouch, big mixup on my end, we only have this for notifications at this point...
Yes, I'm afraid there's no other way than to parse the file header then.

-    header = {}
-    next(stream)
-    for line in stream:
-      match = field_re.search(line)
-      if match:
-        header[match.group(1)] = match.group(2)
-      else:

[Sebastian Noack, 2015/11/30 13:55:49]

Nit: If you negate the logic you don't need an else-block.

if not match:
  break
header[match.group(1)] = match.group(2)

-        break
-    return header
-
-def generate_metadata(filter_lists, expires="4 days"):

[kzar, 2015/11/27 16:28:11]

It is unclear where the expires value for content blocking lists is supposed to
come from. I've started a discussion about that in the issue, but in the mean
time I've just hard coded it to "4 days".

[Sebastian Noack, 2015/11/30 13:55:49]

The expiration interval should be configured in sitescripts.ini.

[kzar, 2015/11/30 15:13:11]

Done.

-  metadata = {
-    "sources": [],
-    "version": datetime.utcnow().strftime("%Y%m%d%H%M"),
-    "expires": expires
-  }
-  for filter_list in filter_lists:
-    metadata["sources"].append({ k.lower(): filter_list["header"][k]
-                                 for k in ["url", "Version"]})
+def generate_metadata(filter_lists, expires):
+  metadata = OrderedDict((
+    ("version", time.strftime("%Y%m%d%H%M", time.gmtime())),
+    ("expires", expires),
+    ("sources", [])
+  ))
+  for body, url, version in filter_lists:
+    metadata["sources"].append({"url": url, "version": version})
   return metadata
 
-def write_block_list(filter_lists, path):
-  metadata = generate_metadata(filter_lists)

[kzar, 2015/11/27 16:28:11]

I'm doing it this way to avoid having to load the block list into memory, parse
it as JSON again and then mutate it to add the metadata. I realise it's kind of
ugly though, I'm open to either approach.

[Sebastian Noack, 2015/11/30 13:55:49]

We don't have to care too much about memory consumption here. I would be
interested in how much the difference is though, to get an idea whether it's
even worth considering a more complex solution.

[kzar, 2015/11/30 15:13:11]

Done.

-  header = json.dumps(metadata, indent=2).rsplit("}", 1)[0].rstrip()
-  header += ',\n  "rules": '
+def pipe_in(process, filter_lists):
+  try:
+    for body, _, _ in filter_lists:
+      print >>process.stdin, body
+  finally:
+    process.stdin.close()
+
+def write_block_list(filter_lists, path, expires):
+  block_list = generate_metadata(filter_lists, expires)
+  process = subprocess.Popen(("node", "abp2blocklist.js"),
+                             cwd=config["abp2blocklist_path"],
+                             stdin=subprocess.PIPE, stdout=subprocess.PIPE)
+  threading.Thread(target=pipe_in, args=(process, filter_lists)).start()
+  block_list["rules"] = json.load(process.stdout)
+  if process.wait():
+    raise Exception("abp2blocklist returned %s" % process.returncode)
+
   with open(path, "wb") as destination_file:
-    destination_file.write(header)
-    destination_file.flush()

[Sebastian Noack, 2015/11/30 13:55:49]

Any particular reason you flush the file here?

-    process = subprocess.Popen(("node", "abp2blocklist.js"),
-                               cwd=config["abp2blocklist_path"],
-                               stdin=subprocess.PIPE,
-                               stdout=destination_file)
-    try:
-      for filter_list in filter_lists:
-        print >>process.stdin, filter_list["body"]
-    finally:
-      process.stdin.close()
-      process.wait()
-    print >>destination_file, "}"
-
-  if process.returncode:
-    raise Exception("abp2blocklist returned %s" % process.returncode)
+    json.dump(block_list, destination_file, indent=2, separators=(",", ": "))
 
 if __name__ == "__main__":
   update_abp2blocklist()
 
   easylist = download_filter_list(config["easylist_url"])
   exceptionrules = download_filter_list(config["exceptionrules_url"])
 
-  write_block_list([easylist], config["easylist_content_blocker_path"])
+  write_block_list([easylist],
+                   config["easylist_content_blocker_path"],
+                   config["easylist_content_blocker_expires"])
   write_block_list([easylist, exceptionrules],
-                   config["combined_content_blocker_path"])
+                   config["combined_content_blocker_path"],
+                   config["combined_content_blocker_expires"])