Issue 395 - Filter hits statistics backend

sitescripts/filterhits/web/query.py

Index: sitescripts/filterhits/web/query.py
diff --git a/sitescripts/filterhits/web/query.py b/sitescripts/filterhits/web/query.py
index b7ee2f05c1a8c3080f06fb67573708eefbf0cb64..aec62265830c06fe4c6238e56b10de432c341544 100644
--- a/sitescripts/filterhits/web/query.py
+++ b/sitescripts/filterhits/web/query.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
@@ -17,42 +17,53 @@
 
 import os, MySQLdb, json
 from urlparse import parse_qsl
+
 from sitescripts.web import url_handler
 from sitescripts.utils import cached, get_config, setupStderr
+from sitescripts.filterhits import common
+from sitescripts.filterhits import db
 
-import sitescripts.filterhits.common as common
-import sitescripts.filterhits.db as db
-
-def query_sql(domain=None, filter=None, skip=0, take=20, order_by="hits DESC", **_):
+def query(domain=None, filter=None, skip=0, take=20, order_by="hits DESC", **_):

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

Any reason why you silently ignore additional keyword arguments?

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

I do that because we're taking the parameters straight from the query string. We
want to ignore any passed parameters that we don't care about here.

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

I see.

+  """
+  Returns the SQL and parameters needed to perform a query of the filterhits data.
+  """
   sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, hits
            FROM geometrical_mean as g
-           LEFT JOIN filters as f ON f.md5=g.filter_md5
+           LEFT JOIN filters as f ON f.sha1=g.filter_sha1
            %s
            ORDER BY %s
-           LIMIT %d, %d;"""
-  where = ["domain LIKE '%%%s%%'" % db.escape(domain) if domain else None,
-           "filter LIKE '%%%s%%'" % db.escape(filter) if filter else None]
-  where = " AND ".join([f for f in where if f])
-  where = "WHERE " + where if where else ""
-  return sql % (where, db.escape(order_by), int(skip), int(take))
+           LIMIT %%s, %%s"""
+
+  where_fields = [(s, "%" + p + "%") for s, p in (("domain", domain),

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

It's best practice to use format string when concatenating more than two string.
Format strings are generally more readable and also faster. Yes, I know that you
have to escape the percent-sign then. ;)

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

I disagree that `"%%%s%%" % p` is easier to read than `"%" + p + "%"`.

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

Fair enough.

+                                                ("filter", filter)) if p]

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

Nit: Seems that the indentation is a little off here.

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

Done.

+  where = " AND ".join([f[0] + " LIKE %s" for f in where_fields])
+  where_sql = "WHERE " + where if where else ""
+
+  order_by, order = order_by.split()

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

Strings aren't proper data structures. So how about turning the order_by
argument into a tuple like ("hists", "DESC"). Or maybe even better, using two
arguments, one for the sort column, and another one to indicate the sort
direction.

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

Done.

+  order = order.upper() if order.upper() in ["ASC", "DESC"] else "ASC"

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

Nit: When a sequence doesn't need to be modified use tuples instead lists.

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

Done.

+  order_by_sql = "`%s` %s" % (MySQLdb.escape_string(order_by), order)
+
+  params = [f[1] for f in where_fields] + [int(skip), int(take)]
+  return [sql % (where_sql, order_by_sql)] + params
 
 @url_handler("/query")
-def query(environ, start_response):
+def query_handler(environ, start_response):
   setupStderr(environ["wsgi.errors"])
   config = get_config()
   params = dict(parse_qsl(environ.get('QUERY_STRING', '')))
 
   try:
-    db.connect(config.get("filterhitstats", "dbuser"),
-               config.get("filterhitstats", "dbpassword"),
-               config.get("filterhitstats", "database"))
-    results = db.query(query_sql(**params), dict_result=True)
-    total = db.query("SELECT FOUND_ROWS();")[0][0]
+    db_connection = db.connect(config.get("filterhitstats", "dbuser"),
+                               config.get("filterhitstats", "dbpassword"),
+                               config.get("filterhitstats", "database"))
+    results = db.query(db_connection, *query(**params), dict_result=True)
+    total = db.query(db_connection, "SELECT FOUND_ROWS()")[0][0]
   except MySQLdb.Error:
     return common.showError("Failed to query database!", start_response,
                             "500 Database error")
   finally:
-    db.disconnect()
+    if db_connection:

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

This will result in a NameError, in case db_connection failed to connect. You
need following pattern here:

try:
  connection = db.connect(...)
  try:
    ...
  finally:
    connection.close()
except MySQLdb.Error:
  ...

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

Done.

+      db_connection.close()
 
   try:
     echo = int(params["echo"])