Issue 395 - Filter hits statistics backend

sitescripts/filterhits/bin/process_logs.py

Index: sitescripts/filterhits/bin/process_logs.py
diff --git a/sitescripts/filterhits/bin/process_logs.py b/sitescripts/filterhits/bin/process_logs.py
index 745524e0f76521770e54188edbbbc7d491ee3fd8..8f9cdcc31b129b09ed762bfde91c12770365080d 100755
--- a/sitescripts/filterhits/bin/process_logs.py
+++ b/sitescripts/filterhits/bin/process_logs.py
@@ -1,7 +1,7 @@
 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
-# Copyright (C) 2006-2014 Eyeo GmbH
+# 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
@@ -25,26 +25,29 @@ last_log_file = None
 
 def log_files(dir):
   """
-   Provides a generator of filter hits log files for the given directory.
-   Works recursively, relative path of log file is returned.
+  Provides a generator of filter hits log files for the given directory.
+  Works recursively, relative path of log file is returned.
   """
   for root, subdirs, files in os.walk(dir):
     for f in files:
-      if f.endswith(".log") and f[0].isdigit():
+      if os.path.splitext(f)[1] == ".log" and f[0].isdigit():
         yield os.path.join(root, f)
 
 def read_data(log_file):
   """
-   Read, parse and return the JSON data for the given log file name.
-   (As a side effect sets the global last_log_file to the log file name.)
+  Read, parse and return the JSON data for the given log file name.
+  (As a side effect sets the global last_log_file to the log file name.)
   """
   global last_log_file
   try:
     with open(log_file, "r") as f:
       # Skip past the date and GET parameters
-      s = ""
-      while s != "\" ":
-        s = s[-1:] + f.read(1)
+      current = last = None
+      while not (last == '"' and current == " "):

[Sebastian Noack, 2015/02/17 14:59:17]

If you use the != instead the == operator in the first place, you don't need to
negate the result:

last != '"' or current != ' '

[kzar, 2015/02/24 18:05:11]

I'm aware of Demorgan's law but I think the intention is much clearer when it's
written this way around.

[Sebastian Noack, 2015/02/26 16:39:25]

I'd rather say, as more logical operations involved, as harder it's to
understand the logic.

[kzar, 2015/02/28 19:39:56]

Well what we're saying is something like "While the last two characters aren't
'" ' then do this...". I think `not (last == '"' and current == " ")` is closer
to that than `last != '"' or current != ' '` but I guess it's subjective.

How about this? It's even closer to the English version and reduces the logical
operations.

[Sebastian Noack, 2015/03/02 10:04:01]

Not sure whether I like it, but fair enough.

+        last, current = current, f.read(1)
+        if not current:
+          sys.exit("Unexpected EOF in log file %s" % log_file)
+
       # Read the JSON
       data = json.load(f)
       # Keep track of the current log file in global variable in case we need to
@@ -53,8 +56,6 @@ def read_data(log_file):
       last_log_file = log_file
   except IOError:
     sys.exit("Could not read log file %s" % log_file)
-  if not common.valid_log_data(data):
-    sys.exit("Invalid data in log file %s." % log_file)
   return data
 
 if __name__ == "__main__":
@@ -75,13 +76,14 @@ if __name__ == "__main__":
                                                        log_files(sys.argv[1])))
 
   try:
-    db.connect(config.get("filterhitstats", "dbuser"),
-               config.get("filterhitstats", "dbpassword"),
-               config.get("filterhitstats", "database"))
-    db.write(sql)
+    db_connection = db.connect(config.get("filterhitstats", "dbuser"),
+                               config.get("filterhitstats", "dbpassword"),
+                               config.get("filterhitstats", "database"))
+    db.write(db_connection, sql)
   except MySQLdb.Error, e:
     sys.exit("Failed to process file %s, all changes rolled back. MySQl error (%d): \"%s\"\n" % (
       last_log_file, e.args[0], e.args[1]
     ))
   finally:
-    db.disconnect()
+    if db_connection:
+      db_connection.close()