Issue 3176 - Add metadata to content blocker lists

sitescripts/content_blocker_lists/bin/generate_lists.py

Index: sitescripts/content_blocker_lists/bin/generate_lists.py
diff --git a/sitescripts/content_blocker_lists/bin/generate_lists.py b/sitescripts/content_blocker_lists/bin/generate_lists.py
index d86a52a95bc4edf0a14a017573c893f1f2c693bf..2e93dc6345c936ea7f0fa0cbb8e9705859b51551 100644
--- a/sitescripts/content_blocker_lists/bin/generate_lists.py
+++ b/sitescripts/content_blocker_lists/bin/generate_lists.py
@@ -16,61 +16,88 @@
 # 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
 import subprocess
+import thread
+import re
 import urllib2
 
 from sitescripts.utils import get_config
 
-def _update_abp2blocklist():
+config = dict(get_config().items("content_blocker_lists"))
+
+def update_abp2blocklist():

[Felix Dahlke, 2015/12/01 08:43:09]

These functions were prefixed with an underscore for a reason, see:
https://adblockplus.org/coding-style#python

[Sebastian Noack, 2015/12/01 10:18:39]

Well, one could argue that this is less a module (to be imported somewhere
else), but more like a stand-alone script providing a CLI as high-level
interface. Therefore these methods wouldn't necessarily have to be considered
private. I probably wouldn't have prefixed them myself in this case. However,
removing the prefixes is a completely unrelated change. So I agree, that it
should be removed from this patch.

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

I would prefer to leave them off, they don't really add anything in this context
and they are kind of ugly. (It's true that it's an unrelated change, but as I'm
changing / creating nearly all the functional definitions in this patch set I
thought it made sense to change this one too.)

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

I would argue that this is still a module - this script can only be run with
`python -m`. It isn't a loadable library, that's true, but neither are our web
modules. Also, It's not unheard of to move code between modules under bin and
reusable modules. I really think we should be consistent here, but it's up to
Sebastian. Let's however please clarify this in the coding style, it's obviously
ambiguous right now.

[Sebastian Noack, 2015/12/07 12:38:24]

Well, technically every piece of code is part of a module in Python. But the
point is that all code that sees these functions is allowed/supposed to call
them. Note that since the actual interface here is the CLI, the only code this
applies to, is this script itself. So there is no context in which these
function would/should be considered private. But if for some reason somebody
imports this module rather than running it as a script, which they shouldn't do
however, they would expect a more low-level API than provided by the CLI.

As for consistency, it seems that it's only code you wrote that overuses
prefixed/private functions in scenarios like this. For example have a look at
ensure_dependencies.py or buildtools in general. 

[Felix Dahlke, 2015/12/08 06:51:24]

Wladimir's newer code in Sitescripts also uses those prefixes for stuff under
bin. Anyway, your reasoning makes sense, so I'm fine with never using those
private prefixes for executable scripts. But please do clarify it in the coding
style.

   with open(os.devnull, "w") as devnull:
-    config = get_config()
-    abp2blocklist_path = config.get("content_blocker_lists",
-                                    "abp2blocklist_path")
+    abp2blocklist_path = config["abp2blocklist_path"]
     if os.path.isdir(abp2blocklist_path):
         subprocess.check_call(("hg", "pull", "-u", "-R", abp2blocklist_path),
                               stdout=devnull)
     else:
-      abp2blocklist_url = config.get("content_blocker_lists",
-                                     "abp2blocklist_url")
-      subprocess.check_call(("hg", "clone", abp2blocklist_url,
+      subprocess.check_call(("hg", "clone", config["abp2blocklist_url"],
                              abp2blocklist_path), stdout=devnull)
     subprocess.check_call(("npm", "install"), cwd=abp2blocklist_path,
                           stdout=devnull)
 
-def _download(url_key):
-  url = get_config().get("content_blocker_lists", url_key)
-  response = urllib2.urlopen(url)
-  try:
-    return response.read()
-  finally:
-    response.close()
+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

[Sebastian Noack, 2015/12/01 11:01:48]

I think it would be simpler if you just return a tuple like (body, url, version)
here, unpacking it while iterating over the filter lists. In particular as these
really seem to be the only data we need.

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

Done.

+
+def parse_filter_list_header(filter_list):
+  body_start = re.search(r"^[^![]", filter_list, re.MULTILINE).start()
+  field_re = re.compile(r"^!\s*([^:\s]+):\s*(.+)$", re.MULTILINE)
+  return { match.group(1): match.group(2)
+           for match in field_re.finditer(filter_list, 0, body_start) }
+
+def generate_metadata(filter_lists, expires):
+  metadata = OrderedDict((
+    ("version", datetime.utcnow().strftime("%Y%m%d%H%M")),

[Felix Dahlke, 2015/12/01 08:43:08]

FWIW, we're using `time.strftime("%Y%m%d%H%M", time.gmtime())` for this in the
notification handler. But it seems to me those are equivalent.

[Sebastian Noack, 2015/12/01 10:18:39]

I tend to agree, creating a datetime object is unnecessary here. Moreover, I
always have to wrap my head around how datetime handles timezones. I wouldn't
insist though.

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

Done.

+    ("expires", expires),
+    ("sources", [])
+  ))
+  for filter_list in filter_lists:
+    metadata["sources"].append({ k.lower(): filter_list["header"][k]
+                                 for k in ["url", "Version"]})

[Felix Dahlke, 2015/12/01 08:43:08]

Nit: Sebastian convinced me a while ago that tuples are better than lists for
immutable lists like this.

[Sebastian Noack, 2015/12/01 10:18:39]

Well, frankly, I don't think that it matters in the case here. However, when you
assign a sequence to a variable, but changing it isn't intended or supported by
your code, using an immutable type is more appropriate and prevents you from
certain bugs, later. But if the sequence isn't accessible after defining it
anyway, I don't really care.

[Felix Dahlke, 2015/12/01 10:26:29]

Wouldn't insist either.

[Sebastian Noack, 2015/12/01 10:38:09]

For reference, I found a quite interesting answer on SO, on the difference
between tuples and lists, from a more semantic point of view:
https://stackoverflow.com/questions/626759/whats-the-difference-between-list-...

I didn't consider that aspect yet, but I tend to agree. So in our scenario here,
as structure doesn't matter (i.e. adding/removing/changing an item in the
sequence of fields here wouldn't invalidate the whole data structure), a list
would actually be more appropriate than a tuple, in their conclusion.

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

Acknowledged.

+  return metadata
 
-def _convert_filter_list(sources, destination_path_key):
-  config = get_config()
-  destination_path = config.get("content_blocker_lists", destination_path_key)
-  with open(destination_path, "wb") as destination_file:
-    abp2blocklist_path = config.get("content_blocker_lists",
-                                    "abp2blocklist_path")
-    process = subprocess.Popen(("node", "abp2blocklist.js"),
-                               cwd=abp2blocklist_path, stdin=subprocess.PIPE,
-                               stdout=destination_file)
+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)
+  def pipe_in(process):

[Sebastian Noack, 2015/12/01 10:47:14]

Nit: This is inconsistent. You pass in the process but access the filter_lists
via the closure. If you keep it as an inner function I would access all
variables via closure. However, I'd tend to move that function to the top-level.

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

Done.

     try:
-      for source in sources:
-        print >>process.stdin, source
+      for filter_list in filter_lists:
+        print >>process.stdin, filter_list["body"]
     finally:
       process.stdin.close()
       process.wait()
 
+  thread.start_new_thread(pipe_in, (process,))

[Sebastian Noack, 2015/12/01 10:47:14]

Please use the high-level threading module instead. Using the thread module
directly can be considered deprecated as it got prefixed in Python 3.

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

Done.

+  block_list["rules"] = json.load(process.stdout)
+
   if process.returncode:
     raise Exception("abp2blocklist returned %s" % process.returncode)
 
+  with open(path, "wb") as destination_file:
+    json.dump(block_list, destination_file, indent=2, separators=(",", ": "))
+
 if __name__ == "__main__":
-  _update_abp2blocklist()
+  update_abp2blocklist()
 
-  easylist = _download("easylist_url")
-  exceptionrules = _download("exceptionrules_url")
+  easylist = download_filter_list(config["easylist_url"])
+  exceptionrules = download_filter_list(config["exceptionrules_url"])
 
-  _convert_filter_list([easylist], "easylist_content_blocker_path")
-  _convert_filter_list([easylist, exceptionrules],
-                       "combined_content_blocker_path")
+  write_block_list([easylist],

[Felix Dahlke, 2015/12/01 08:43:08]

Nit: "block list" is highly ambiguous, we often use that term for ABP style
filter lists. "content blocker list" is the correct term, but I can see why
you'd want to keep it short here, fair enough.

[Sebastian Noack, 2015/12/01 10:18:39]

Well, after all, the program called here is also called "abp2blocklist".
However, if you want to have a more distinct term, I guess we have to invent
one, because there isn't a name for the format we are generating here (container
holding WebKit content blocker rules plus some metadata) yet.

[Felix Dahlke, 2015/12/01 10:26:29]

Good point, let's really leave it alone.

+                   config["easylist_content_blocker_path"],
+                   config["easylist_content_blocker_expires"])
+  write_block_list([easylist, exceptionrules],
+                   config["combined_content_blocker_path"],
+                   config["combined_content_blocker_expires"])